On Fri, May 26, 2023 at 06:03:36PM +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.
>
> Return EADDRINUSE back to the user process if the same name but different args
> event has being registered.
>
> Signed-off-by: sunliming <sunliming@xxxxxxxxxx>
> ---
> kernel/trace/trace_events_user.c | 34 +++++++++++++++----
> .../selftests/user_events/ftrace_test.c | 6 ++++
> 2 files changed, 33 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> index b1ecd7677642..bd455052ccd0 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,31 @@ 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);
> - return 0;
> + if (args) {
> + argv = argv_split(GFP_KERNEL, args, &argc);
> + if (!argv)
> + return -ENOMEM;
This out of memory case needs a refcount_dec(), otherwise we leak a
refcount here and the event won't ever be able to be deleted afterwards.
I would suggest having an error label for both the mismatch and out of
memory case, which makes a single spot to do the refcount_dec().
IE:
ret = -ENOMEM;
goto error;
Thanks,
-Beau
> +
> + 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);
> + ret = 0;
> + } else {
> + refcount_dec(&user->refcnt);
> + ret = -EADDRINUSE;
> + }
> +
> + 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
