| Message ID | 20230525012105.7936-1-sunliming@kylinos.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tracing/user_events: Prevent same name but different args event | expand |
On Thu, May 25, 2023 at 09:21:05AM +0800, sunliming 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. > Good catch, we do this in dynamic_events, but not in the raw ABI case. > Return EADDRINUSE back to the user process if the same name but different args > event has being registered. > > Signed-off-by: sunliming <sunliming@kylinos.cn> > --- > kernel/trace/trace_events_user.c | 74 ++++++++++++++++--- > .../selftests/user_events/ftrace_test.c | 6 ++ > 2 files changed, 68 insertions(+), 12 deletions(-) > > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c > index b1ecd7677642..aacd22c1e9f8 100644 > --- a/kernel/trace/trace_events_user.c > +++ b/kernel/trace/trace_events_user.c > @@ -1741,6 +1741,46 @@ static int user_event_trace_register(struct user_event *user) > return ret; > } > > +static int compare_ftrace_field(struct ftrace_event_field *field1, > + struct ftrace_event_field *field2) > +{ > + if (field1->offset == field2->offset && > + field1->size == field2->size && > + field1->is_signed == field2->is_signed && > + field1->filter_type == field2->filter_type && > + !strcmp(field1->name, field2->name)) > + return 0; > + else > + return -1; > +} > + > +static int user_event_compare_fields(struct user_event *user1, > + struct user_event *user2) > +{ > + struct ftrace_event_field *field1, *field2; > + > + field1 = list_first_entry(&user1->fields, > + struct ftrace_event_field, link); > + field2 = list_first_entry(&user2->fields, > + struct ftrace_event_field, link); > + > + while (&field1->link != &user1->fields && > + &field2->link != &user2->fields) { > + > + if (compare_ftrace_field(field1, field2)) > + break; > + > + field1 = list_next_entry(field1, link); > + field2 = list_next_entry(field2, link); > + } > + > + if (&field1->link == &user1->fields && > + &field2->link == &user2->fields) > + return 0; > + else > + return -EADDRINUSE; > +} > + We already have a way to compare the fields match via the dynamic_events interface. Please change to use user_fields_match() instead. This makes it so we only have one way to do this check, etc. > /* > * Parses the event name, arguments and flags then registers if successful. > * The name buffer lifetime is owned by this method for success cases only. > @@ -1752,23 +1792,13 @@ static int user_event_parse(struct user_event_group *group, char *name, > { > int ret; > u32 key; > - struct user_event *user; > + struct user_event *user, *tmp_user; > > /* Prevent dyn_event from racing */ > mutex_lock(&event_mutex); > - user = find_user_event(group, name, &key); > + tmp_user = find_user_event(group, name, &key); > mutex_unlock(&event_mutex); > Here we should be able to just do something like: argv = argv_split(GFP_KERNEL, args, &argc); match = user_fields_match(user, argc, argv); argv_free(argv); NOTE: The above is just pseudocode and does not check the return of argv_split, etc. but gives you the idea. > - 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); > - return 0; > - } > - > user = kzalloc(sizeof(*user), GFP_KERNEL_ACCOUNT); > > if (!user) > @@ -1786,6 +1816,26 @@ static int user_event_parse(struct user_event_group *group, char *name, > if (ret) > goto put_user; > > + if (tmp_user) { > + /* > + * Prevent users form using the same name for different fields. This > + * can cause unexpected output for the second event. > + */ > + ret = user_event_compare_fields(user, tmp_user); > + > + if (!ret) { > + *newuser = tmp_user; > + /* > + * Name is allocated by caller, free it since it already exists. > + * Caller only worries about failure cases for freeing. > + */ > + kfree(name); > + } else > + refcount_dec(&tmp_user->refcnt); > + > + goto put_user; > + } > + > ret = user_event_create_print_fmt(user); > > if (ret) > diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c > index 7c99cef94a65..d738efd51daf 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); > > + /* Multipule regitsters to same name but different args should fail */ Typos, should be 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 Thanks, -Beau
diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c index b1ecd7677642..aacd22c1e9f8 100644 --- a/kernel/trace/trace_events_user.c +++ b/kernel/trace/trace_events_user.c @@ -1741,6 +1741,46 @@ static int user_event_trace_register(struct user_event *user) return ret; } +static int compare_ftrace_field(struct ftrace_event_field *field1, + struct ftrace_event_field *field2) +{ + if (field1->offset == field2->offset && + field1->size == field2->size && + field1->is_signed == field2->is_signed && + field1->filter_type == field2->filter_type && + !strcmp(field1->name, field2->name)) + return 0; + else + return -1; +} + +static int user_event_compare_fields(struct user_event *user1, + struct user_event *user2) +{ + struct ftrace_event_field *field1, *field2; + + field1 = list_first_entry(&user1->fields, + struct ftrace_event_field, link); + field2 = list_first_entry(&user2->fields, + struct ftrace_event_field, link); + + while (&field1->link != &user1->fields && + &field2->link != &user2->fields) { + + if (compare_ftrace_field(field1, field2)) + break; + + field1 = list_next_entry(field1, link); + field2 = list_next_entry(field2, link); + } + + if (&field1->link == &user1->fields && + &field2->link == &user2->fields) + return 0; + else + return -EADDRINUSE; +} + /* * Parses the event name, arguments and flags then registers if successful. * The name buffer lifetime is owned by this method for success cases only. @@ -1752,23 +1792,13 @@ static int user_event_parse(struct user_event_group *group, char *name, { int ret; u32 key; - struct user_event *user; + struct user_event *user, *tmp_user; /* Prevent dyn_event from racing */ mutex_lock(&event_mutex); - user = find_user_event(group, name, &key); + tmp_user = find_user_event(group, name, &key); 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); - return 0; - } - user = kzalloc(sizeof(*user), GFP_KERNEL_ACCOUNT); if (!user) @@ -1786,6 +1816,26 @@ static int user_event_parse(struct user_event_group *group, char *name, if (ret) goto put_user; + if (tmp_user) { + /* + * Prevent users form using the same name for different fields. This + * can cause unexpected output for the second event. + */ + ret = user_event_compare_fields(user, tmp_user); + + if (!ret) { + *newuser = tmp_user; + /* + * Name is allocated by caller, free it since it already exists. + * Caller only worries about failure cases for freeing. + */ + kfree(name); + } else + refcount_dec(&tmp_user->refcnt); + + goto put_user; + } + ret = user_event_create_print_fmt(user); if (ret) diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c index 7c99cef94a65..d738efd51daf 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); + /* Multipule regitsters 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);
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@kylinos.cn> --- kernel/trace/trace_events_user.c | 74 ++++++++++++++++--- .../selftests/user_events/ftrace_test.c | 6 ++ 2 files changed, 68 insertions(+), 12 deletions(-)