2023-05-26 10:19:35

by sunliming

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

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 <[email protected]>
---
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;
+
+ 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, &reg));
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, &reg));
+ ASSERT_EQ(EADDRINUSE, errno);
+
/* Ensure disabled */
self->enable_fd = open(enable_file, O_RDWR);
ASSERT_NE(-1, self->enable_fd);
--
2.25.1

2023-05-26 16:56:02

by Beau Belgrave

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

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 <[email protected]>
> ---
> 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, &reg));
> 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, &reg));
> + ASSERT_EQ(EADDRINUSE, errno);
> +
> /* Ensure disabled */
> self->enable_fd = open(enable_file, O_RDWR);
> ASSERT_NE(-1, self->enable_fd);
> --
> 2.25.1