[PATCH] tracing/user_events: Don't free an event that a dynamic event still references

[PATCH] tracing/user_events: Don't free an event that a dynamic event still references

[David Carlier]

An eprobe takes a reference on the call of the event it attaches to,
and kprobe, uprobe, fprobe and synth events all refuse to go away while
trace_event_dyn_busy() says such a reference exists. user_events never
checks it, so a user event can be freed under an eprobe:

  echo 'u:test u32 a' >> dynamic_events
  echo 'e:ep user_events/test a=$a' >> dynamic_events
  echo '-:user_events/test' >> dynamic_events
  echo '-:eprobes/ep' >> dynamic_events

The last write drops the reference on the freed call.

Return -EBUSY from destroy_user_event() in that case.

Fixes: 7f5a08c79df3 ("user_events: Add minimal support for trace_event into ftrace")
Cc: stable@vger.kernel.org
Signed-off-by: David Carlier <devnexen@gmail.com>
---
A non-persistent event whose last fd closes while an eprobe
holds it now stays registered after the eprobe is gone. Can follow
up with a retry on the eprobe put if wanted.
 kernel/trace/trace_events_user.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
index f658c3a77aa7..49b857a7f1d7 100644
--- a/kernel/trace/trace_events_user.c
+++ b/kernel/trace/trace_events_user.c
@@ -1506,6 +1506,10 @@ static int destroy_user_event(struct user_event *user)
 
 	lockdep_assert_held(&event_mutex);
 
+	/* If there is a reference to a dynamic event */
+	if (trace_event_dyn_busy(&user->call))
+		return -EBUSY;
+
 	/*
 	 * Detach the fields before removing the call. Removing the event
 	 * frees the field list memory (trace_destroy_fields() is run on
-- 
2.55.0

Re: [PATCH] tracing/user_events: Don't free an event that a dynamic event still references

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tracing/user_events: permanent leak of non-persistent event in destroy_user_event()
--

commit 426e76d7362e855d5b85cce6bb2e395443a0a474
Author: David Carlier <devnexen@gmail.com>

tracing/user_events: Don't free an event that a dynamic event still references

This commit prevents destroy_user_event() from freeing a user_event while a
dynamic event still holds a reference on user->call by checking
trace_event_dyn_busy(&user->call) and returning -EBUSY.

> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> index f658c3a77aa7a..49b857a7f1d78 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
> @@ -1506,6 +1506,10 @@ static int destroy_user_event(struct user_event *user)
>  
>  	lockdep_assert_held(&event_mutex);
>  
> +	/* If there is a reference to a dynamic event */
> +	if (trace_event_dyn_busy(&user->call))
> +		return -EBUSY;

[Severity: High]
Does returning -EBUSY here cause non-persistent user_event instances to leak
and become permanently undeletable if their last reference is dropped while
user->call.refcnt is non-zero?

When a non-persistent event is created via DIAG_IOCSREG without
USER_EVENT_REG_PERSIST, user->refcnt tracks active user and probe references.
If a dynamic event consumer holds a reference on user->call (such as an
eprobe via find_and_get_event(), a histogram action via
trace_get_event_file(), or an enable_event trigger during teardown in
event_enable_trigger_free() before trigger_kthread runs
enable_trigger_private_data_free()) when the last user->refcnt reference is
dropped in user_event_put(), delayed_destroy_user_event() runs:

kernel/trace/trace_events_user.c:delayed_destroy_user_event() {
	...
	if (!refcount_dec_and_test(&user->refcnt))
		goto out;

	if (destroy_user_event(user)) {
		...
		pr_warn("user_events: Unable to delete event\n");
		refcount_set(&user->refcnt, 1);
	}
out:
	mutex_unlock(&event_mutex);
}

Because destroy_user_event() now returns -EBUSY, delayed_destroy_user_event()
resets user->refcnt to 1 without setting USER_EVENT_REG_PERSIST in
user->reg_flags, and with no owner holding that reference.

Once the eprobe or trigger later drops its reference on user->call, how can
this user_event ever be freed?

1. Manual deletion via dynamic_events in user_event_free(), via DIAG_IOCSDEL
in delete_user_event(), or via dyn_events_release_all() ->
user_event_is_busy() all check user_event_last_ref():

kernel/trace/trace_events_user.c:user_event_last_ref() {
	int last = 0;

	if (user->reg_flags & USER_EVENT_REG_PERSIST)
		last = 1;

	return refcount_read(&user->refcnt) == last;
}

Because USER_EVENT_REG_PERSIST is not set, user_event_last_ref() requires
user->refcnt to be 0, so manual deletion permanently fails with -EBUSY.

2. Re-opening and closing the event increments user->refcnt from 1 to 2 and
decrements it back to 1 in user_event_put(), never reaching 0 to re-queue
delayed_destroy_user_event().

As a result, the struct user_event, its fields and validators, its tracefs
entry, and a current_user_events quota slot appear to be leaked until reboot.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031210420.501187-1-devnexen@gmail.com?part=1