From: Beau Belgrave <beaub@linux.microsoft.com>
To: Masami Hiramatsu <mhiramat@kernel.org>
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: Mon, 22 Apr 2024 14:55:25 -0700 [thread overview]
Message-ID: <20240422215525.GA414-beaub@linux.microsoft.com> (raw)
In-Reply-To: <20240420215052.1f806c8acb5f99ec2f63945c@kernel.org>
On Sat, Apr 20, 2024 at 09:50:52PM +0900, Masami Hiramatsu wrote:
> 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:
*SNIP*
> > > 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 ;)
>
I'll stick with your loop. Perhaps others will chime in on the v2 and
state a stronger opinion.
You scared me with the atomic comment, I went back and looked at all the
paths for this. In the user_events IOCTL the buffer is copied from user
to kernel, so it cannot change (and no other threads access it). I also
checked trace_parse_run_command() which is the same. So at least in this
context the non-atomic part is OK.
> >
> > > > +
> > > > + /*
> > > > + * 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.
>
Ok, seems unrelated. Thanks for checking.
Thanks,
-Beau
> Thank you,
>
> >
> > Thanks,
> > -Beau
> >
*SNIP*
> > > Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
next prev parent reply other threads:[~2024-04-22 21:55 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
2024-04-22 21:55 ` Beau Belgrave [this message]
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=20240422215525.GA414-beaub@linux.microsoft.com \
--to=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=mhiramat@kernel.org \
--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.