Re: [PATCH] tracing/user_events: Prevent same name but different args event

Beau Belgrave
2023-05-25T17:02:21Z

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, &reg));
> 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, &reg));
> + 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