diff mbox series

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

Message ID 20230529032100.286534-1-sunliming@kylinos.cn (mailing list archive)
State Accepted
Commit ba470eebc2f6c2f704872955a715b9555328e7d0
Headers show
Series [V3] tracing/user_events: Prevent same name but different args event | expand

Commit Message

sunliming May 29, 2023, 3:21 a.m. UTC 
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              | 36 +++++++++++++++----
 .../selftests/user_events/ftrace_test.c       |  6 ++++
 2 files changed, 36 insertions(+), 6 deletions(-)

Comments

Masami Hiramatsu (Google) May 29, 2023, 9:23 a.m. UTC | #1 
On Mon, 29 May 2023 11:21:00 +0800
sunliming <sunliming@kylinos.cn> 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@kylinos.cn>

Looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

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, &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
>
Beau Belgrave May 30, 2023, 6:10 p.m. UTC | #2 
On Mon, May 29, 2023 at 06:23:19PM +0900, Masami Hiramatsu wrote:
> On Mon, 29 May 2023 11:21:00 +0800
> sunliming <sunliming@kylinos.cn> 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@kylinos.cn>
> 
> Looks good to me.
> 
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Thank you,
> 

This also looks good to me, thank you for your patch!

Acked-by: Beau Belgrave <beaub@linux.microsoft.com>

Thanks,
-Beau

> > ---
> >  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, &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
> > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
diff mbox series

Patch

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