| Message ID | 20240423162338.292-2-beaub@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bd125a084091396f3e796bb3dc009940d9771811 |
| Headers | show |
| Series | tracing/user_events: Fix non-spaced field matching | expand |
On Tue, 23 Apr 2024 16:23:37 +0000 Beau Belgrave <beaub@linux.microsoft.com> wrote: > When the ABI was updated to prevent same name w/different args, it > missed an important corner case when fields don't end with a space. > Typically, space is used for fields to help separate them, like > "u8 field1; u8 field2". If no spaces are used, like > "u8 field1;u8 field2", then the parsing works for the first time. > However, the match check fails on a subsequent register, leading to > confusion. > > This is because the match check uses argv_split() and assumes that all > fields will be split upon the space. When spaces are used, we get back > { "u8", "field1;" }, without spaces we get back { "u8", "field1;u8" }. > This causes a mismatch, and the user program gets back -EADDRINUSE. > > Add a method to detect this case before calling argv_split(). If found > force a space after the field separator character ';'. This ensures all > cases work properly for matching. > > With this fix, the following are all treated as matching: > u8 field1;u8 field2 > u8 field1; u8 field2 > u8 field1;\tu8 field2 > u8 field1;\nu8 field2 I'm curious, what happens if you have: "u8 field1; u8 field2;" ? Do you care? As you will then create "u8 field1; u8 field2; " but I'm guessing the extra whitespace at the end doesn't affect anything. > > Fixes: ba470eebc2f6 ("tracing/user_events: Prevent same name but different args event") > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com> > --- > kernel/trace/trace_events_user.c | 76 +++++++++++++++++++++++++++++++- > 1 file changed, 75 insertions(+), 1 deletion(-) > > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c > index 70d428c394b6..82b191f33a28 100644 > --- a/kernel/trace/trace_events_user.c > +++ b/kernel/trace/trace_events_user.c > @@ -1989,6 +1989,80 @@ static int user_event_set_tp_name(struct user_event *user) > return 0; > } > > +/* > + * Counts how many ';' without a trailing space are in the args. > + */ > +static int count_semis_no_space(char *args) > +{ > + int count = 0; > + > + while ((args = strchr(args, ';'))) { > + args++; > + > + if (!isspace(*args)) > + count++; This will count that "..;" This is most likely not an issue, but since I didn't see this case anywhere, I figured I bring it up just to confirm that it's not an issue. -- Steve > + } > + > + return count; > +} > + > +/* > + * Copies the arguments while ensuring all ';' have a trailing space. > + */ > +static char *insert_space_after_semis(char *args, int count) > +{ > + char *fixed, *pos; > + int len; > + > + len = strlen(args) + count; > + fixed = kmalloc(len + 1, GFP_KERNEL); > + > + if (!fixed) > + return NULL; > + > + pos = fixed; > + > + /* Insert a space after ';' if there is no trailing space. */ > + while (*args) { > + *pos = *args++; > + > + if (*pos++ == ';' && !isspace(*args)) > + *pos++ = ' '; > + } > + > + *pos = '\0'; > + > + return fixed; > +} > +
On Thu, May 02, 2024 at 05:16:34PM -0400, Steven Rostedt wrote: > On Tue, 23 Apr 2024 16:23:37 +0000 > Beau Belgrave <beaub@linux.microsoft.com> wrote: > > > When the ABI was updated to prevent same name w/different args, it > > missed an important corner case when fields don't end with a space. > > Typically, space is used for fields to help separate them, like > > "u8 field1; u8 field2". If no spaces are used, like > > "u8 field1;u8 field2", then the parsing works for the first time. > > However, the match check fails on a subsequent register, leading to > > confusion. > > > > This is because the match check uses argv_split() and assumes that all > > fields will be split upon the space. When spaces are used, we get back > > { "u8", "field1;" }, without spaces we get back { "u8", "field1;u8" }. > > This causes a mismatch, and the user program gets back -EADDRINUSE. > > > > Add a method to detect this case before calling argv_split(). If found > > force a space after the field separator character ';'. This ensures all > > cases work properly for matching. > > > > With this fix, the following are all treated as matching: > > u8 field1;u8 field2 > > u8 field1; u8 field2 > > u8 field1;\tu8 field2 > > u8 field1;\nu8 field2 > > I'm curious, what happens if you have: "u8 field1; u8 field2;" ? > You'll get an extra whitespace during the copy, assuming it was really: "u8 field1;u8 field2" If it had spaces, this code wouldn't run. > Do you care? As you will then create "u8 field1; u8 field2; " > > but I'm guessing the extra whitespace at the end doesn't affect anything. > Right, you get an extra byte allocated, but the argv_split() with ignore it. The compare will work correctly (I've verified this just now to double check). IE these all match: "Test u8 a; u8 b; " "Test u8 a; u8 b;" "Test u8 a; u8 b" > > > > > Fixes: ba470eebc2f6 ("tracing/user_events: Prevent same name but different args event") > > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com> > > --- > > kernel/trace/trace_events_user.c | 76 +++++++++++++++++++++++++++++++- > > 1 file changed, 75 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c > > index 70d428c394b6..82b191f33a28 100644 > > --- a/kernel/trace/trace_events_user.c > > +++ b/kernel/trace/trace_events_user.c > > @@ -1989,6 +1989,80 @@ static int user_event_set_tp_name(struct user_event *user) > > return 0; > > } > > > > +/* > > + * Counts how many ';' without a trailing space are in the args. > > + */ > > +static int count_semis_no_space(char *args) > > +{ > > + int count = 0; > > + > > + while ((args = strchr(args, ';'))) { > > + args++; > > + > > + if (!isspace(*args)) > > + count++; > > This will count that "..;" > > This is most likely not an issue, but since I didn't see this case > anywhere, I figured I bring it up just to confirm that it's not an issue. > It's not an issue on the matching/logic. However, you do get an extra byte alloc (which doesn't bother me in this edge case). Thanks, -Beau > -- Steve > > > > + } > > + > > + return count; > > +} > > + > > +/* > > + * Copies the arguments while ensuring all ';' have a trailing space. > > + */ > > +static char *insert_space_after_semis(char *args, int count) > > +{ > > + char *fixed, *pos; > > + int len; > > + > > + len = strlen(args) + count; > > + fixed = kmalloc(len + 1, GFP_KERNEL); > > + > > + if (!fixed) > > + return NULL; > > + > > + pos = fixed; > > + > > + /* Insert a space after ';' if there is no trailing space. */ > > + while (*args) { > > + *pos = *args++; > > + > > + if (*pos++ == ';' && !isspace(*args)) > > + *pos++ = ' '; > > + } > > + > > + *pos = '\0'; > > + > > + return fixed; > > +} > > +
On Thu, 2 May 2024 15:58:53 -0700 Beau Belgrave <beaub@linux.microsoft.com> wrote: > It's not an issue on the matching/logic. However, you do get an extra > byte alloc (which doesn't bother me in this edge case). Figured as much, but since there was no mention of it, I decided to bring it up. I'll take this as-is then. -- Steve
diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c index 70d428c394b6..82b191f33a28 100644 --- a/kernel/trace/trace_events_user.c +++ b/kernel/trace/trace_events_user.c @@ -1989,6 +1989,80 @@ static int user_event_set_tp_name(struct user_event *user) return 0; } +/* + * Counts how many ';' without a trailing space are in the args. + */ +static int count_semis_no_space(char *args) +{ + int count = 0; + + while ((args = strchr(args, ';'))) { + args++; + + if (!isspace(*args)) + count++; + } + + return count; +} + +/* + * Copies the arguments while ensuring all ';' have a trailing space. + */ +static char *insert_space_after_semis(char *args, int count) +{ + char *fixed, *pos; + int len; + + len = strlen(args) + count; + fixed = kmalloc(len + 1, GFP_KERNEL); + + if (!fixed) + return NULL; + + pos = fixed; + + /* Insert a space after ';' if there is no trailing space. */ + while (*args) { + *pos = *args++; + + if (*pos++ == ';' && !isspace(*args)) + *pos++ = ' '; + } + + *pos = '\0'; + + return fixed; +} + +static char **user_event_argv_split(char *args, int *argc) +{ + char **split; + char *fixed; + int count; + + /* Count how many ';' without a trailing space */ + count = count_semis_no_space(args); + + /* No fixup is required */ + if (!count) + return argv_split(GFP_KERNEL, args, argc); + + /* We must fixup 'field;field' to 'field; field' */ + fixed = insert_space_after_semis(args, count); + + if (!fixed) + return NULL; + + /* We do a normal split afterwards */ + split = argv_split(GFP_KERNEL, fixed, argc); + + /* We can free since argv_split makes a copy */ + kfree(fixed); + + return split; +} + /* * Parses the event name, arguments and flags then registers if successful. * The name buffer lifetime is owned by this method for success cases only. @@ -2012,7 +2086,7 @@ static int user_event_parse(struct user_event_group *group, char *name, return -EPERM; if (args) { - argv = argv_split(GFP_KERNEL, args, &argc); + argv = user_event_argv_split(args, &argc); if (!argv) return -ENOMEM;
When the ABI was updated to prevent same name w/different args, it missed an important corner case when fields don't end with a space. Typically, space is used for fields to help separate them, like "u8 field1; u8 field2". If no spaces are used, like "u8 field1;u8 field2", then the parsing works for the first time. However, the match check fails on a subsequent register, leading to confusion. This is because the match check uses argv_split() and assumes that all fields will be split upon the space. When spaces are used, we get back { "u8", "field1;" }, without spaces we get back { "u8", "field1;u8" }. This causes a mismatch, and the user program gets back -EADDRINUSE. Add a method to detect this case before calling argv_split(). If found force a space after the field separator character ';'. This ensures all cases work properly for matching. With this fix, the following are all treated as matching: u8 field1;u8 field2 u8 field1; u8 field2 u8 field1;\tu8 field2 u8 field1;\nu8 field2 Fixes: ba470eebc2f6 ("tracing/user_events: Prevent same name but different args event") Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com> --- kernel/trace/trace_events_user.c | 76 +++++++++++++++++++++++++++++++- 1 file changed, 75 insertions(+), 1 deletion(-)