Re: [PATCH v2] tracing/user_events: Don't destroy fields when event removal fails

[Beau Belgrave]

On Fri, Sep 04, 2026 at 07:52:23PM +0800, Henry Martin wrote:
> destroy_user_event() destroys the event's fields before attempting to
> remove the trace event call. If user_event_set_call_visible() fails,
> e.g. because the event is still enabled and trace_remove_event_call()
> returns -EBUSY, the event is left registered with an irreversibly
> destroyed field list. Any subsequent interaction with the event then
> operates on an empty field list while it is still fully visible in
> tracefs.
> 
> Move the field destruction after the call removal, and splice the
> field list back onto the event when the removal fails so the event
> remains in a consistent state.
> 
> Fixes: 7f5a08c79df35 ("user_events: Add minimal support for trace_event into ftrace")
> Signed-off-by: Henry Martin <bsdhenrymartin@xxxxxxxxx>
> ---
> v2:
>  - Restore the comment on detaching the fields before removal: removing
>    the event frees the field list memory, which is allocated and owned
>    by user_events (Beau).
>  - Comment why the fields are spliced back onto the event when removal
>    fails: the event stays registered and the fields must be recovered
>    (Beau).
> 

This looks good to me.

Reviewed-by: Beau Belgrave <beaub@xxxxxxxxxxxxxxxxxxx>

Thanks,
-Beau

>  kernel/trace/trace_events_user.c | 26 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> index 93cda2f6f2692..f658c3a77aa7a 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
> @@ -1122,10 +1122,9 @@ static void user_event_destroy_validators(struct user_event *user)
>  	}
>  }
>  
> -static void user_event_destroy_fields(struct user_event *user)
> +static void user_event_destroy_fields(struct list_head *head)
>  {
>  	struct ftrace_event_field *field, *next;
> -	struct list_head *head = &user->fields;
>  
>  	list_for_each_entry_safe(field, next, head, link) {
>  		list_del(&field->link);
> @@ -1502,17 +1501,32 @@ static int user_event_set_call_visible(struct user_event *user, bool visible)
>  
>  static int destroy_user_event(struct user_event *user)
>  {
> +	LIST_HEAD(fields);
>  	int ret = 0;
>  
>  	lockdep_assert_held(&event_mutex);
>  
> -	/* Must destroy fields before call removal */
> -	user_event_destroy_fields(user);
> +	/*
> +	 * Detach the fields before removing the call. Removing the event
> +	 * frees the field list memory (trace_destroy_fields() is run on
> +	 * successful removal and kmem_cache_free()s the fields), but the
> +	 * fields here are allocated and owned by user_events. Destroy
> +	 * them separately once removal has succeeded.
> +	 */
> +	list_splice_init(&user->fields, &fields);
>  
>  	ret = user_event_set_call_visible(user, false);
>  
> -	if (ret)
> +	if (ret) {
> +		/*
> +		 * Removal failed and the event stays registered, recover
> +		 * the fields so it is left in a consistent state.
> +		 */
> +		list_splice(&fields, &user->fields);
>  		return ret;
> +	}
> +
> +	user_event_destroy_fields(&fields);
>  
>  	dyn_event_remove(&user->devent);
>  	hash_del(&user->node);
> @@ -2212,7 +2226,7 @@ static int user_event_parse(struct user_event_group *group, char *name,
>  put_user_lock:
>  	mutex_unlock(&event_mutex);
>  put_user:
> -	user_event_destroy_fields(user);
> +	user_event_destroy_fields(&user->fields);
>  	user_event_destroy_validators(user);
>  	kfree(user->call.print_fmt);
>  
> -- 
> 2.43.0