On Tue, 08 Dec 2020 11:34:41 -0600 Tom Zanussi <zanu...@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? -- Steve