Messages in this thread | | | Date | Wed, 9 Dec 2020 22:51:14 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH v3 3/5] tracing: Update synth command errors |
| |
On Tue, 8 Dec 2020 12:53:40 -0500 Steven Rostedt <rostedt@goodmis.org> 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?
This makes sense. Anyway, what I considered were - synthetic_events interface doesn't provide syntax error reports - synthetic_events interface is not self-reproducive*.
*) I meant
$ cat synthetic_events > saved_events $ cat saved_events > synthetic_events
should work. But this does *NOT* mean
$ cat user-input > synthetic_events $ cat synthetic_events > saved_events $ diff user-input saved_events # no diff
So input and output can be different, but the output can be input again.
Thank you,
-- Masami Hiramatsu <mhiramat@kernel.org>
| |