From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
To: Beau Belgrave <beaub@linux.microsoft.com>
Cc: rostedt@goodmis.org, mathieu.desnoyers@efficios.com,
linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org,
dcook@linux.microsoft.com
Subject: Re: [PATCH 1/2] tracing/user_events: Fix non-spaced field matching
Date: Sat, 20 Apr 2024 21:50:52 +0900 [thread overview]
Message-ID: <20240420215052.1f806c8acb5f99ec2f63945c@kernel.org> (raw)
In-Reply-To: <20240419211334.GA7774-beaub@linux.microsoft.com>
On Fri, 19 Apr 2024 14:13:34 -0700
Beau Belgrave <beaub@linux.microsoft.com> wrote:
> On Fri, Apr 19, 2024 at 11:33:05AM +0900, Masami Hiramatsu wrote:
> > On Tue, 16 Apr 2024 22:41:01 +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
> >
> > Sounds good to me. I just have some nits.
> >
> > >
> > > 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 | 88 +++++++++++++++++++++++++++++++-
> > > 1 file changed, 87 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> > > index 70d428c394b6..9184d3962b2a 100644
> > > --- a/kernel/trace/trace_events_user.c
> > > +++ b/kernel/trace/trace_events_user.c
> > > @@ -1989,6 +1989,92 @@ 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 *fix_semis_no_space(char *args, int count)
> >
> > nit: This name does not represent what it does. 'insert_space_after_semis()'
> > is more self-described.
> >
>
> Sure, will fix in a v2.
>
> > > +{
> > > + char *fixed, *pos;
> > > + char c, last;
> > > + int len;
> > > +
> > > + len = strlen(args) + count;
> > > + fixed = kmalloc(len + 1, GFP_KERNEL);
> > > +
> > > + if (!fixed)
> > > + return NULL;
> > > +
> > > + pos = fixed;
> > > + last = '\0';
> > > +
> > > + while (len > 0) {
> > > + c = *args++;
> > > +
> > > + if (last == ';' && !isspace(c)) {
> > > + *pos++ = ' ';
> > > + len--;
> > > + }
> > > +
> > > + if (len > 0) {
> > > + *pos++ = c;
> > > + len--;
> > > + }
> > > +
> > > + last = c;
> > > + }
> >
> > nit: This loop can be simpler, because we are sure fixed has enough length;
> >
> > /* insert a space after ';' if there is no space. */
> > while(*args) {
> > *pos = *args++;
> > if (*pos++ == ';' && !isspace(*args))
> > *pos++ = ' ';
> > }
> >
>
> I was worried that if count_semis_no_space() ever had different logic
> (maybe after this commit) that it could cause an overflow if the count
> was wrong, etc.
>
> I don't have an issue making it shorter, but I was trying to be more on
> the safe side, since this isn't a fast path (event register).
OK, anyway current code looks correct. But note that I don't think
"pos++; len--;" is safer, since it is not atomic. This pattern
easily loose "len--;" in my experience. So please carefully use it ;)
>
> > > +
> > > + /*
> > > + * len is the length of the copy excluding the null.
> > > + * This ensures we always have room for a null.
> > > + */
> > > + *pos = '\0';
> > > +
> > > + return fixed;
> > > +}
> > > +
> > > +static char **user_event_argv_split(char *args, int *argc)
> > > +{
> > > + /* Count how many ';' without a trailing space */
> > > + int count = count_semis_no_space(args);
> > > +
> > > + if (count) {
> >
> > nit: it is better to exit fast, so
> >
> > if (!count)
> > return argv_split(GFP_KERNEL, args, argc);
> >
> > ...
>
> Sure, will fix in a v2.
>
> >
> > Thank you,
> >
> > OT: BTW, can this also simplify synthetic events?
> >
>
> I'm not sure, I'll check when I have some time. I want to get this fix
> in sooner rather than later.
Ah, nevermind. Synthetic event parses the field by strsep(';') first
and argv_split(). So it does not have this issue.
Thank you,
>
> Thanks,
> -Beau
>
> > > + /* We must fixup 'field;field' to 'field; field' */
> > > + char *fixed = fix_semis_no_space(args, count);
> > > + char **split;
> > > +
> > > + 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;
> > > + }
> > > +
> > > + /* No fixup is required */
> > > + return argv_split(GFP_KERNEL, args, argc);
> > > +}
> > > +
> > > /*
> > > * 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 +2098,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;
> > > --
> > > 2.34.1
> > >
> >
> >
> > --
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
next prev parent reply other threads:[~2024-04-20 12:50 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-16 22:41 [PATCH 0/2] tracing/user_events: Fix non-spaced field matching Beau Belgrave
2024-04-16 22:41 ` [PATCH 1/2] " Beau Belgrave
2024-04-19 2:33 ` Masami Hiramatsu
2024-04-19 21:13 ` Beau Belgrave
2024-04-20 12:50 ` Masami Hiramatsu [this message]
2024-04-22 21:55 ` Beau Belgrave
2024-04-22 23:50 ` Masami Hiramatsu
2024-04-16 22:41 ` [PATCH 2/2] selftests/user_events: Add non-spacing separator check Beau Belgrave
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240420215052.1f806c8acb5f99ec2f63945c@kernel.org \
--to=mhiramat@kernel.org \
--cc=beaub@linux.microsoft.com \
--cc=dcook@linux.microsoft.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-trace-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=rostedt@goodmis.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.