On Mon, May 29, 2023 at 06:23:19PM +0900, Masami Hiramatsu wrote:
> On Mon, 29 May 2023 11:21:00 +0800
> sunliming <sunliming@xxxxxxxxxx> wrote:
>
> > User processes register name_args for events. If the same name but different
> > args event are registered. The trace outputs of second event are printed
> > as the first event. This is incorrect.
> >
> > Return EADDRINUSE back to the user process if the same name but different args
> > event has being registered.
> >
> > Signed-off-by: sunliming <sunliming@xxxxxxxxxx>
>
> Looks good to me.
>
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
>
> Thank you,
>
This also looks good to me, thank you for your patch!
Acked-by: Beau Belgrave <beaub@xxxxxxxxxxxxxxxxxxx>
Thanks,
-Beau
> > ---
> > kernel/trace/trace_events_user.c | 36 +++++++++++++++----
> > .../selftests/user_events/ftrace_test.c | 6 ++++
> > 2 files changed, 36 insertions(+), 6 deletions(-)
> >
> > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> > index b1ecd7677642..e90161294698 100644
> > --- a/kernel/trace/trace_events_user.c
> > +++ b/kernel/trace/trace_events_user.c
> > @@ -1753,6 +1753,8 @@ static int user_event_parse(struct user_event_group *group, char *name,
> > int ret;
> > u32 key;
> > struct user_event *user;
> > + int argc = 0;
> > + char **argv;
> >
> > /* Prevent dyn_event from racing */
> > mutex_lock(&event_mutex);
> > @@ -1760,13 +1762,35 @@ static int user_event_parse(struct user_event_group *group, char *name,
> > mutex_unlock(&event_mutex);
> >
> > if (user) {
> > - *newuser = user;
> > - /*
> > - * Name is allocated by caller, free it since it already exists.
> > - * Caller only worries about failure cases for freeing.
> > - */
> > - kfree(name);
> > + if (args) {
> > + argv = argv_split(GFP_KERNEL, args, &argc);
> > + if (!argv) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> > +
> > + ret = user_fields_match(user, argc, (const char **)argv);
> > + argv_free(argv);
> > +
> > + } else
> > + ret = list_empty(&user->fields);
> > +
> > + if (ret) {
> > + *newuser = user;
> > + /*
> > + * Name is allocated by caller, free it since it already exists.
> > + * Caller only worries about failure cases for freeing.
> > + */
> > + kfree(name);
> > + } else {
> > + ret = -EADDRINUSE;
> > + goto error;
> > + }
> > +
> > return 0;
> > +error:
> > + refcount_dec(&user->refcnt);
> > + return ret;
> > }
> >
> > user = kzalloc(sizeof(*user), GFP_KERNEL_ACCOUNT);
> > diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c
> > index 7c99cef94a65..6e8c4b47281c 100644
> > --- a/tools/testing/selftests/user_events/ftrace_test.c
> > +++ b/tools/testing/selftests/user_events/ftrace_test.c
> > @@ -228,6 +228,12 @@ TEST_F(user, register_events) {
> > ASSERT_EQ(0, ioctl(self->data_fd, DIAG_IOCSREG, ®));
> > ASSERT_EQ(0, reg.write_index);
> >
> > + /* Multiple registers to same name but different args should fail */
> > + reg.enable_bit = 29;
> > + reg.name_args = (__u64)"__test_event u32 field1;";
> > + ASSERT_EQ(-1, ioctl(self->data_fd, DIAG_IOCSREG, ®));
> > + ASSERT_EQ(EADDRINUSE, errno);
> > +
> > /* Ensure disabled */
> > self->enable_fd = open(enable_file, O_RDWR);
> > ASSERT_NE(-1, self->enable_fd);
> > --
> > 2.25.1
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
