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, > --- > 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>