[PATCH] tracing: Unregister field variable histograms when the command fails

[PATCH] tracing: Unregister field variable histograms when the command fails

[Donggeun Yoo]

An action that names a field on another event needs that field as a
variable, so create_field_var_hist() registers a second hist trigger on
that event to provide it, and records it in
target_hist_data->field_var_hists[].

If a later part of the same command fails, create_actions() returns an
error and event_hist_trigger_parse() jumps to out_free. The write fails
with -EINVAL and sched_switch gets no trigger, but:

  # echo 'hist:keys=pid:ts0=common_timestamp.usecs' >> \
      events/sched/sched_waking/trigger
  # echo 'my_synth u64 lat; int a; int b' >> synthetic_events
  # echo 'hist:keys=next_pid:wakeup_lat=common_timestamp.usecs-$ts0:\
      onmatch(sched.sched_waking).my_synth($wakeup_lat,prio,nosuchfld)'\
      >> events/sched/sched_switch/trigger
  # cat events/sched/sched_waking/trigger
  hist:keys=pid:vals=hitcount:ts0=common_timestamp.usecs:sort=hitcount:size=2048:clock=global [active]
  hist:keys=pid:vals=hitcount:synthetic_prio=prio:sort=hitcount:size=2048 [active]

The second histogram is the one created for 'prio'. It is still active and
still counting.

out_free reaches destroy_field_var_hists(), which frees the
field_var_hists[] entries, but never unregister_field_var_hists(), which is
what removes the triggers and runs only from event_hist_trigger_free(). The
entry is freed and the trigger is left with nothing tracking it.

Unregister the field variable histograms in out_free as well, and record
each one as soon as its trigger is registered rather than after the lookup
that follows it, so no exit can leave a registered trigger untracked. The
entry then belongs to field_var_hists[] and is released by
destroy_field_var_hists(), so freeing it on that exit would be a double
free.

Cc: stable@vger.kernel.org
Fixes: 02205a6752f2 ("tracing: Add support for 'field variables'")
Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/all/20260913204302.DB2551F000FF@smtp.kernel.org/
Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
Assisted-by: Claude:claude-fable-5
---
Tested under QEMU on 704340f1cd0d, x86_64, KASAN + PROVE_LOCKING, 2 vCPU,
unfixed arm first.

  command                                      before     after
  valid onmatch on another event's field       works      works
  removing that trigger                        works      works
  onmatch with a trailing bad field            orphan     clean
  the same bad command, five more times        orphan     clean
  two field variables, then a bad field        2 orphans  clean
  '!hist' that matches nothing                 clean      clean

orphan = an extra [active] histogram left on the matched event.

ftracetest test.d/trigger: identical per-test verdicts on both arms.
trigger-synthetic-event-dynstring.tc fails on both because busybox execs
its ping applet as /proc/self/exe, so the test's grep cannot match.

The exit that reports a missing synthetic variable is not reachable
through onmatch, so no run covers it.

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

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 8af97fd4ee2d..90e6e3990c25 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -3134,20 +3134,18 @@ create_field_var_hist(struct hist_trigger_data *target_hist_data,
 
 	kfree(cmd);
 
+	n = target_hist_data->n_field_var_hists;
+	target_hist_data->field_var_hists[n] = var_hist;
+	target_hist_data->n_field_var_hists++;
+
 	/* If we can't find the variable, something went wrong */
 	event_var = find_synthetic_field_var(target_hist_data, subsys_name,
 					     event_name, field_name);
 	if (IS_ERR_OR_NULL(event_var)) {
-		kfree(var_hist->cmd);
-		kfree(var_hist);
 		hist_err(tr, HIST_ERR_SYNTH_VAR_NOT_FOUND, errpos(field_name));
 		return ERR_PTR(-EINVAL);
 	}
 
-	n = target_hist_data->n_field_var_hists;
-	target_hist_data->field_var_hists[n] = var_hist;
-	target_hist_data->n_field_var_hists++;
-
 	return event_var;
 }
 
@@ -6980,6 +6978,8 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops,
 
 	trigger_data_free(trigger_data);
 
+	unregister_field_var_hists(hist_data);
+
 	destroy_hist_data(hist_data);
 	goto out;
 }
-- 
2.53.0

Re: [PATCH] tracing: Unregister field variable histograms when the command fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Error unwinding in event_hist_trigger_parse() causes Use-After-Free and Double-Free of hist_data, and leaves dangling trigger_data on the global named_triggers list.
- [High] Use-after-free of the compatible hist_data when removing a target histogram that uses field variables.
--

commit a635652ad2d37c0cc5b35a037b2c68349bec678b
Author: Donggeun Yoo <donggeunyoo.kernel@gmail.com>

tracing: Unregister field variable histograms when the command fails

An action that names a field on another event needs that field as a
variable, so create_field_var_hist() registers a second hist trigger on
that event to provide it, and records it in
target_hist_data->field_var_hists[].

If a later part of the same command fails, create_actions() returns an
error and event_hist_trigger_parse() jumps to out_free.

Closes: https://lore.kernel.org/all/20260913204302.DB2551F000FF@smtp.kernel.org/

> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 963e0d6b61fdf..8e6c699b75c2a 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -3138,20 +3138,18 @@ create_field_var_hist(struct hist_trigger_data *target_hist_data,
>  
>  	kfree(cmd);
>  
> +	n = target_hist_data->n_field_var_hists;
> +	target_hist_data->field_var_hists[n] = var_hist;
> +	target_hist_data->n_field_var_hists++;
> +
>  	/* If we can't find the variable, something went wrong */
>  	event_var = find_synthetic_field_var(target_hist_data, subsys_name,
>  					     event_name, field_name);
>  	if (IS_ERR_OR_NULL(event_var)) {
> -		kfree(var_hist->cmd);
> -		kfree(var_hist);
>  		hist_err(tr, HIST_ERR_SYNTH_VAR_NOT_FOUND, errpos(field_name));
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> -	n = target_hist_data->n_field_var_hists;
> -	target_hist_data->field_var_hists[n] = var_hist;
> -	target_hist_data->n_field_var_hists++;
> -
>  	return event_var;
>  }

[Severity: High]
This is a pre-existing issue, but can this lead to a use-after-free of the
compatible histogram?

Earlier in create_field_var_hist(), find_compatible_hist() finds an existing
histogram to copy its keys and filter, and stores it in var_hist->hist_data:

kernel/trace/trace_events_hist.c:create_field_var_hist() {
    ...
	hist_data = find_compatible_hist(target_hist_data, file);
    ...
	var_hist->hist_data = hist_data;
    ...
}

Since the original compatible histogram has no variables attached to it,
check_var_refs() will return false, allowing a user to safely remove it while
the target histogram is still active.

When the compatible histogram is removed, its hist_data is freed via
destroy_hist_data().

Later, when the target histogram is removed, unregister_field_var_hists()
is called:

kernel/trace/trace_events_hist.c:unregister_field_var_hists() {
    ...
		file = hist_data->field_var_hists[i]->hist_data->event_file;
    ...
}

Would this dereference the freed compatible hist_data, resulting in a
use-after-free?

> @@ -6976,6 +6974,8 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops,
>  
>  	trigger_data_free(trigger_data);
>  
> +	unregister_field_var_hists(hist_data);
> +
>  	destroy_hist_data(hist_data);
>  	goto out;
>  }

[Severity: High]
This is a pre-existing issue, but can this error path lead to a double-free and
use-after-free if named trigger registration fails?

If a named trigger is successfully linked to existing state in
hist_register_trigger(), save_named_trigger() is called which adds
trigger_data to the global named_triggers list, and the local hist_data
object is freed:

kernel/trace/trace_events_hist.c:hist_register_trigger() {
    ...
	if (named_data) {
		remove_hist_vars(hist_data);
		destroy_hist_data(hist_data);
	}
    ...
}

If a subsequent initialization step fails in hist_register_trigger() (such as
tracing_set_clock() failing), it returns an error without unwinding the list
addition.

When this happens, event_hist_trigger_parse() jumps to out_free and calls
trigger_data_free(trigger_data). Does this leave a dangling pointer in the
global named_triggers list?

Additionally, this path unconditionally calls destroy_hist_data(hist_data). If
hist_data was already freed by hist_register_trigger(), could this result in a
double-free?

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

Re: [PATCH] tracing: Unregister field variable histograms when the command fails

[Donggeun Yoo]

> - [High] Error unwinding in event_hist_trigger_parse() causes Use-After-Free
>   and Double-Free of hist_data, and leaves dangling trigger_data on the
>   global named_triggers list.

This cannot happen on this base. tracing_set_clock() runs before
cmd_ops->init(), not after -- 6ede78d0563a ("tracing: Set the trace clock
before registering the histogram trigger") swapped them -- and the only code
after init() is the if (named_data) destroy block, which cannot fail. Once
hist_register_trigger() succeeds, out_free is unreachable anyway:
hist_trigger_enable() failure goes to out_unreg, which 92383cef6679 ("tracing:
Undo the registration when enabling the histogram trigger fails") made skip
the free.

> - [High] Use-after-free of the compatible hist_data when removing a target
>   histogram that uses field variables.

Real, and reproduced under KASAN. Fix sent:
https://lore.kernel.org/all/20260914234754.3676905-1-donggeunyoo.kernel@gmail.com/