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@xxxxxxxxxx>
> Closes: https://lore.kernel.org/linux-trace-kernel/20260907092944.3950E1F00A3D@xxxxxxxxxxxxxxx/
> Fixes: 7ab0fc61ce73 ("tracing: Move histogram trigger variables from stack to per CPU structure")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@xxxxxxxxx>
> ---
>  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@xxxxxxxxxx>


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