[PATCH 0/2] tracing: hist: two named trigger error paths that free a published trigger

[PATCH 0/2] tracing: hist: two named trigger error paths that free a published trigger

[Donggeun Yoo]

Both of these were pointed out on the list against an unrelated hist
patch [1], and both are the same shape as commit 6f86bdeab633 ("tracing:
Fix bad hist from corrupting named_triggers list"): a trigger that is on
the global named_triggers list gets freed without being taken off it, and
the next lookup by name reads the freed object.

Link: https://lore.kernel.org/linux-trace-kernel/20260907092944.3950E1F00A3D@smtp.kernel.org/ [1]

Patch 1 is the init path. event_hist_trigger_named_init() saves the
trigger under the name before it takes the reference that can fail, and
does not undo the save.

Patch 2 is the enable path, the one case that fix did not reach.
hist_trigger_enable() removes the trigger from file->triggers on
failure, which is the only handle hist_unregister_trigger() has, so the
->free() that unlinks it never runs. Leaving it on the list lets the
existing unregister do the whole unwind, which also stops out_free from
touching a hist_data that hist_register_trigger() already destroyed.

The two are independent and can be taken separately.

Reproduced on x86_64 under KASAN_INLINE by making the two failures fire
from a debug knob, since neither is reachable without an allocation
failure or an event that refuses to register. Same kernel and initramfs
either way, one boot per case:

                                        v7.3-rc2        patched
  control, no injected failure          clean           clean
  1) nested init fails                  KASAN           clean
  2) enable fails, new named trigger    KASAN           clean
  3) enable fails, shared histogram     KASAN + panic   clean

Case 3 reports in remove_hist_vars() rather than find_named_trigger(),
from the out_free fall-through described in patch 2.

After each injected failure the same name is written again and is
accepted on the patched kernel, so the trigger really did leave the list;
on v7.3-rc2 case 2 rejects it.

selftests/ftrace test.d/trigger before and after: 45 results, identical
item by item (32 passed, 3 failed, 2 unresolved, 8 unsupported). The
failures and the unresolved results are there without the patches too.

Donggeun Yoo (2):
  tracing: hist: take the reference before publishing the named trigger
  tracing: hist: undo the registration when enabling the trigger fails

 kernel/trace/trace_events_hist.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)


base-commit: df2908090cda368b01ff43709f51890076c56157
-- 
2.53.0

[PATCH 1/2] tracing: hist: take the reference before publishing the named trigger

[Donggeun Yoo]

event_hist_trigger_named_init() puts the trigger on the global
named_triggers list and only then takes the reference on the trigger it
shares its histogram with:

	data->ref++;

	save_named_trigger(data->named_data->name, data);

	ret = event_hist_trigger_init(data->named_data);
	if (ret < 0) {
		kfree(data->cmd_ops);
		data->cmd_ops = &trigger_hist_cmd;
	}

	return ret;

event_hist_trigger_init() fails when alloc_hist_pad() cannot allocate, and
nothing takes the trigger back off the list on the way out.
event_hist_trigger_parse() frees it, and the next lookup by name reads the
freed object:

 BUG: KASAN: slab-use-after-free in find_named_trigger+0xac/0xc0
 Read of size 8 at addr ffff888009346860 by task init/1
  find_named_trigger+0xac/0xc0
  hist_register_trigger+0xc1/0xa00
  event_hist_trigger_parse+0x3146/0x6af0
  event_trigger_write+0xce/0x160
 Freed by task 67:
  kfree+0x154/0x420
  trigger_kthread_fn+0xfd/0x160

Do the reference first and publish once it has succeeded, so that nothing
which can fail runs after the trigger becomes findable.

Reported-by: Sashiko AI <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-trace-kernel/20260907092944.3950E1F00A3D@smtp.kernel.org/
Fixes: 7ab0fc61ce73 ("tracing: Move histogram trigger variables from stack to per CPU structure")
Cc: stable@vger.kernel.org
Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
---
 kernel/trace/trace_events_hist.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 963e0d6b61fd..c6c04926bdf0 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -6383,17 +6383,18 @@ static int event_hist_trigger_named_init(struct event_trigger_data *data)
 {
 	int ret;
 
-	data->ref++;
-
-	save_named_trigger(data->named_data->name, data);
-
 	ret = event_hist_trigger_init(data->named_data);
 	if (ret < 0) {
 		kfree(data->cmd_ops);
 		data->cmd_ops = &trigger_hist_cmd;
+		return ret;
 	}
 
-	return ret;
+	data->ref++;
+
+	save_named_trigger(data->named_data->name, data);
+
+	return 0;
 }
 
 static void event_hist_trigger_named_free(struct event_trigger_data *data)

base-commit: df2908090cda368b01ff43709f51890076c56157
-- 
2.53.0

Re: [PATCH 1/2] tracing: hist: take the reference before publishing the named trigger

[Donggeun Yoo]

On Mon, Sep 07, 2026 at 12:59:33PM +0000, sashiko-bot@kernel.org wrote:
> [Severity: High]
> This isn't a bug introduced by this patch, but does this fix leave another
> identical use-after-free and memory leak reachable from tracefs?
>
> If trigger registration fails in hist_register_trigger() after this .init()
> function succeeds, the error propagates up without undoing it:

Yes, and that one is already fixed. tracing_set_clock() is the only step
in hist_register_trigger() that can fail after cmd_ops->init(), and this:

  https://lore.kernel.org/linux-trace-kernel/20260907091415.554535-1-donggeunyoo.kernel@gmail.com/

moves it above the init, so nothing which can fail runs after the trigger
is published. This series is based on v7.3-rc2, without that patch.

> [Severity: Medium]
> This is a pre-existing issue, but does ignoring the return value of
> save_named_trigger() break histogram sharing?

Yes. On a kstrdup() failure the trigger keeps running with data->name
NULL and off the list, so find_named_trigger() cannot see it, a later
trigger of the same name builds its own map instead of sharing, and
pause_named_trigger() misses it.

Memory safety is not affected. trigger_data_alloc() runs
INIT_LIST_HEAD(&trigger_data->named_list), so the unconditional
del_named_trigger() in event_hist_trigger_named_free() deletes a
self-linked entry and kfree()s a NULL name.

Not addressed by this series.

Re: [PATCH 1/2] tracing: hist: take the reference before publishing the named trigger

[Tom Zanussi]

On Mon, 2026-09-07 at 21:44 +0900, Donggeun Yoo wrote:
> event_hist_trigger_named_init() puts the trigger on the global
> named_triggers list and only then takes the reference on the trigger it
> shares its histogram with:
> 
> 	data->ref++;
> 
> 	save_named_trigger(data->named_data->name, data);
> 
> 	ret = event_hist_trigger_init(data->named_data);
> 	if (ret < 0) {
> 		kfree(data->cmd_ops);
> 		data->cmd_ops = &trigger_hist_cmd;
> 	}
> 
> 	return ret;
> 
> event_hist_trigger_init() fails when alloc_hist_pad() cannot allocate, and
> nothing takes the trigger back off the list on the way out.
> event_hist_trigger_parse() frees it, and the next lookup by name reads the
> freed object:
> 
>  BUG: KASAN: slab-use-after-free in find_named_trigger+0xac/0xc0
>  Read of size 8 at addr ffff888009346860 by task init/1
>   find_named_trigger+0xac/0xc0
>   hist_register_trigger+0xc1/0xa00
>   event_hist_trigger_parse+0x3146/0x6af0
>   event_trigger_write+0xce/0x160
>  Freed by task 67:
>   kfree+0x154/0x420
>   trigger_kthread_fn+0xfd/0x160
> 
> Do the reference first and publish once it has succeeded, so that nothing
> which can fail runs after the trigger becomes findable.
> 
> Reported-by: Sashiko AI <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/linux-trace-kernel/20260907092944.3950E1F00A3D@smtp.kernel.org/
> Fixes: 7ab0fc61ce73 ("tracing: Move histogram trigger variables from stack to per CPU structure")
> Cc: stable@vger.kernel.org
> Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
> ---
>  kernel/trace/trace_events_hist.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 963e0d6b61fd..c6c04926bdf0 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -6383,17 +6383,18 @@ static int event_hist_trigger_named_init(struct event_trigger_data *data)
>  {
>  	int ret;
>  
> -	data->ref++;
> -
> -	save_named_trigger(data->named_data->name, data);
> -
>  	ret = event_hist_trigger_init(data->named_data);
>  	if (ret < 0) {
>  		kfree(data->cmd_ops);
>  		data->cmd_ops = &trigger_hist_cmd;
> +		return ret;
>  	}
>  
> -	return ret;
> +	data->ref++;
> +
> +	save_named_trigger(data->named_data->name, data);
> +
> +	return 0;
>  }
>  

Makes sense. Note that save_named_trigger() can also fail with -ENOMEM,
but maybe that's a separate patch..

Acked-by: Tom Zanussi <zanussi@kernel.org>


>  static void event_hist_trigger_named_free(struct event_trigger_data *data)
> 
> base-commit: df2908090cda368b01ff43709f51890076c56157

[PATCH 2/2] tracing: hist: undo the registration when enabling the trigger fails

[Donggeun Yoo]

Commit 6f86bdeab633 ("tracing: Fix bad hist from corrupting named_triggers
list") described how a trigger that is registered but not on file->triggers
ends up freed while still on the global named_triggers list, and moved the
registration down so that hist_trigger_enable() follows it immediately. One
path still gets there. hist_trigger_enable() adds the trigger and takes it
straight back out when the event cannot be enabled:

	list_add_tail_rcu(&data->list, &file->triggers);

	update_cond_flag(file);

	if (trace_event_trigger_enable_disable(file, 1) < 0) {
		list_del_rcu(&data->list);
		update_cond_flag(file);
		ret--;
	}

so the list walk in hist_unregister_trigger() matches nothing, test stays
NULL, and the ->free() that would call del_named_trigger() is skipped.
out_unreg falls through to out_free, which frees the trigger anyway:

 BUG: KASAN: slab-use-after-free in find_named_trigger+0xac/0xc0
 Read of size 8 at addr ffff8880091d3160 by task init/1
  find_named_trigger+0xac/0xc0
  hist_register_trigger+0xc1/0xa00
  event_hist_trigger_parse+0x3146/0x6af0
  event_trigger_write+0xce/0x160
 Freed by task 69:
  kfree+0x154/0x420
  trigger_kthread_fn+0xfd/0x160

Leave the trigger where hist_unregister_trigger() can find it and let that
undo the registration, which is the only code that knows all of what
cmd_ops->init() took: the named list entry, the hist_pad reference, the
reference on the trigger a named histogram is shared with, and the copied
cmd_ops. It also pairs the failed trace_event_trigger_enable_disable(),
whose sm_ref and buffered event reference are otherwise left behind.

Since ->free() releases trigger_data and, for a trigger that does not share
its histogram, hist_data with it, out_unreg can no longer fall through to
out_free. For a trigger that does share, hist_register_trigger() has
already destroyed the caller's hist_data, so the fall-through was reading
freed memory there as well.

Move the enable_timestamps check in hist_unregister_trigger() above the
->free() call for the same reason: hist_data does not outlive it once the
trigger being removed is the one that owns it.

Reported-by: Sashiko AI <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-trace-kernel/20260907092944.3950E1F00A3D@smtp.kernel.org/
Fixes: 067fe038e70f ("tracing: Add variable reference handling to hist triggers")
Cc: stable@vger.kernel.org
Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
---
 kernel/trace/trace_events_hist.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index c6c04926bdf0..1de224a5a2bb 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -6679,11 +6679,12 @@ static int hist_trigger_enable(struct event_trigger_data *data,
 
 	update_cond_flag(file);
 
-	if (trace_event_trigger_enable_disable(file, 1) < 0) {
-		list_del_rcu(&data->list);
-		update_cond_flag(file);
+	/*
+	 * On failure the caller undoes the registration, and
+	 * hist_unregister_trigger() can only find the trigger here.
+	 */
+	if (trace_event_trigger_enable_disable(file, 1) < 0)
 		ret--;
-	}
 
 	return ret;
 }
@@ -6761,13 +6762,13 @@ static void hist_unregister_trigger(char *glob,
 		}
 	}
 
-	if (test && test->cmd_ops->free)
-		test->cmd_ops->free(test);
-
 	if (hist_data->enable_timestamps) {
 		if (!hist_data->remove || test)
 			tracing_set_filter_buffering(file->tr, false);
 	}
+
+	if (test && test->cmd_ops->free)
+		test->cmd_ops->free(test);
 }
 
 static bool hist_file_check_refs(struct trace_event_file *file)
@@ -6972,6 +6973,8 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops,
 	return ret;
  out_unreg:
 	event_trigger_unregister(cmd_ops, file, glob+1, trigger_data);
+	/* The unregister frees trigger_data, skip out_free */
+	goto out;
  out_free:
 	remove_hist_vars(hist_data);
 
-- 
2.53.0

Re: [PATCH 2/2] tracing: hist: undo the registration when enabling the trigger fails

[Donggeun Yoo]

On Mon, Sep 07, 2026 at 01:01:05PM +0000, sashiko-bot@kernel.org wrote:
> [Severity: High]
> This is a pre-existing issue, but missing cleanup in the error path before
> this label can lead to a use-after-free on the global named_triggers list.
>
> When hist_register_trigger() fails after successfully executing
> data->cmd_ops->init(data), it returns a negative error code without undoing
> the initialization. For instance, if tracing_set_clock() fails due to an
> invalid clock name:

Same one as on patch 1, answered there:

  https://lore.kernel.org/linux-trace-kernel/20260907125934.5C6261F00A3E@smtp.kernel.org/

It is fixed by a separate patch that moves tracing_set_clock() above the
init, which this series is not based on.

Re: [PATCH 0/2] tracing: hist: two named trigger error paths that free a published trigger

[Donggeun Yoo]

The Sashiko AI reviewer reported these bugs before I sent the patches, in
its review of "tracing: hist: set the trace clock before registering the
trigger". Both patches should carry:

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/linux-trace-kernel/20260907092944.3950E1F00A3D@smtp.kernel.org/

No change to the patches themselves.

Thanks,
Donggeun