Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <d44d45dfd9d540b7dae7d645ce05a3f7bb79c03c.camel@kernel.org>
Subject: Re: [PATCH v3 3/5] tracing: Update synth command errors
From:   Tom Zanussi <zanussi@kernel.org>
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     axelrasmussen@google.com, mhiramat@kernel.org,
        linux-kernel@vger.kernel.org
Date:   Tue, 08 Dec 2020 12:32:08 -0600
In-Reply-To: <20201208125340.407150f2@gandalf.local.home>
References: <cover.1603723933.git.zanussi@kernel.org>
         <8671adc7ce95ff1d5c7b037d371467e96f7f2914.1603723933.git.zanussi@kernel.org>
         <20201207201304.627bfe48@oasis.local.home>
         <44b9e471f0d3b77ab0a2bf11024e2e72c1f1a80d.camel@kernel.org>
         <20201208125340.407150f2@gandalf.local.home>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Steve,

On Tue, 2020-12-08 at 12:53 -0500, Steven Rostedt wrote:
> On Tue, 08 Dec 2020 11:34:41 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > Unfortunately, you're correct, if you have a script that creates a
> > synthetic event without semicolons, this patchset will break it, as
> > I
> > myself found out and fixed in patch 4 ([PATCH v3 4/5]
> > selftests/ftrace:
> > Add synthetic event field separators) [4].
> > 
> > So whereas before this would work, even though it shouldn't have in
> > the
> > first place:
> > 
> >   # echo 'wakeup_latency  u64 lat pid_t pid char comm[16]' >
> > synthetic_events
> > 
> > it now has to be:
> > 
> >   # echo 'wakeup_latency  u64 lat; pid_t pid; char comm[16]' >
> > synthetic_events
> > 
> > So yeah, this patchset fixes a set of parsing bugs for things that
> > shouldn't have been accepted as valid, but shouldn't break things
> > that
> > are obviously valid.
> > 
> > If it's too late to fix them, though, I guess we'll just have to
> > live
> > with them, or some other option?
> 
> 
> I would suggest allowing the old interface work (with no new
> features, for
> backward compatibility), but new things like "char comm[16]" we
> require
> semicolons.
> 
> One method to do this is to add to the start of reading the string,
> and
> checking if it has semicolons. If it does not, we create a new string
> with
> them, but make sure that the string does not include new changes.
> 
> 	strncpy_from_user(buffer, user_buff, sizeof(buffer));
> 
> 	if (!strstr(buffer, ";")) {
> 		if (!audit_old_buffer(buffer))
> 			goto error;
> 		insert_colons(buffer);
> 	}
> 
> 
> That is, if the buffer does not have semicolons, then check if it is
> a
> valid "old format", and if not, we error out. Otherwise, we insert
> the
> colons into the buffer, and process that as if the user put in
> colons:
> 
> That is:
> 
> 	echo 'wakeup_latency u64 lat pid_t pid' > synthetic_events
> 
> would change the buffer to:
> 
> 	"wakeup_latency u64 lat; pid_t pid;"
> 
> And then put it through the normal processing. I think its OK that if
> the
> user were to cat out the synthetic events, it would see the
> semicolons even
> if it did not add them. As I don't think that will break userspace.
> 
> Does that make sense?
> 

Yeah, that should work, I'll try adding that.

Thanks,

Tom

> -- Steve