[PATCH v2 0/2] tracing: Fix NULL dereference when copying keys for a field variable

[PATCH v2 0/2] tracing: Fix NULL dereference when copying keys for a field variable

[Donggeun Yoo]

A hist trigger with an onmatch() action copies the key list of the
compatible histogram it finds on the matched event.  Reading each key's
name straight out of key_field->field->name faults on any pseudo-field
key.  2/2 renders the key with expr_field_str() instead; 1/2 teaches that
renderer to emit a bucket count, which 2/2 needs before it can use it.

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

Changes since v1:
 - Split the single patch in two.
 - Dropped the third change, which gated the ".stacktrace" arm of
   get_hist_field_flags() on hist_field->field.  It fixes nothing:
   hist_field_print() excludes FL_STACKTRACE before it ever calls
   get_hist_field_flags(), and parse_field() takes the "stacktrace"
   modifier on any field.  Measured with and without it: identical
   generated command on all nine key kinds below.
 - 1/2 is not separable from 2/2.  .buckets is refused on a value, so no
   expression can reach expr_field_str() with a bucketed field, and 2/2
   alone turns a working .buckets key into a refused one.  In this order
   no bisection point regresses.

The command create_field_var_hist() generates, for each kind of key the
copied histogram can carry:

  WK key                  unpatched   patched
  pid                     keys=pid    keys=pid
  pid.log2                keys=pid    keys=pid.log2
  pid.buckets=10          keys=pid    keys=pid.buckets=10
  common_cpu              oops        keys=common_cpu
  common_comm             oops        keys=common_comm
  common_timestamp        oops        keys=common_timestamp
  common_timestamp.usecs  oops        keys=common_timestamp.usecs
  common_stacktrace       oops        keys=common_stacktrace.stacktrace
  hitcount                oops        keys=hitcount

x86_64 under QEMU, CONFIG_KASAN=y, 4 CPUs, base 2f0c1cf72f46.  A compatible
histogram on sched_waking keyed on WK, an onmatch() target on sched_switch
keyed on SK, my_synth($wakeup_lat,prio) forcing a field variable.

Donggeun Yoo (2):
  tracing: Add the bucket size to expr_field_str()
  tracing: Fix NULL dereference when copying keys for a field variable

 kernel/trace/trace_events_hist.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.53.0

[PATCH v2 1/2] tracing: Add the bucket size to expr_field_str()

[Donggeun Yoo]

expr_field_str() renders a HIST_FIELD_FL_BUCKET field as "pid.buckets",
dropping the count, and parse_field() rejects that spelling.

get_hist_field_flags() returns the bare string "buckets" and keeps the
count in hist_field->buckets. hist_field_print() appends it,
expr_field_str() never did. Nothing reaches it today, since .buckets is
refused on a value and no expression can carry one, but the next patch
renders histogram keys with expr_field_str(), where a .buckets key is
legal.

Append the count, as hist_field_print() does.

Cc: stable@vger.kernel.org
Fixes: de9a48a360b7 ("tracing: Add linear buckets to histogram logic")
Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
Assisted-by: Claude:claude-fable-5
---
 kernel/trace/trace_events_hist.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 963e0d6b61fd..9d7ce01fda36 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1755,6 +1755,9 @@ static bool expr_field_str(struct hist_field *field, struct seq_buf *s)
 			seq_buf_printf(s, ".%s", flags_str);
 	}
 
+	if (field->buckets)
+		seq_buf_printf(s, "=%ld", field->buckets);
+
 	return !seq_buf_has_overflowed(s);
 }
 
-- 
2.53.0

[PATCH v2 2/2] tracing: Fix NULL dereference when copying keys for a field variable

[Donggeun Yoo]

In event_hist_trigger_parse() where it needs to create actions like
"onmatch", it calls:

  event_hist_trigger_parse() {
    create_actions() {
      action_create() {
        trace_action_create() {
          trace_action_create_field_var() {
            create_field_var_hist()

Where create_field_var_hist() does a loop on the hist_data
representing the keys. The issue is, if the keys uses one of the
pseudo field types (like common_cpu), the hist_data field element
will have NULL for its field member causing a NULL pointer
dereference when accessing the key_field->field->name.

  # echo 'hist:keys=common_cpu:ts0=common_timestamp.usecs' > \
      events/sched/sched_waking/trigger
  # echo 'my_synth u64 lat; int prio' > synthetic_events
  # echo 'hist:keys=common_cpu:wakeup_lat=common_timestamp.usecs-$ts0:\
      onmatch(sched.sched_waking).my_synth($wakeup_lat,prio)' > \
      events/sched/sched_switch/trigger

  KASAN: null-ptr-deref in range [0x0000000000000010-0x0000000000000017]
  RIP: 0010:create_field_var_hist+0x771/0x1380
  Call Trace:
   trace_action_create_field_var+0x360/0x600
   trace_action_create+0x393/0xe80
   event_hist_trigger_parse+0x3e47/0x69e0
   trigger_process_regex+0x1a6/0x250
   event_trigger_write+0xce/0x160

Instead of accessing it directly, use the proper handler
expr_field_str() to get the name.

A .buckets key needs the previous patch to render, so apply that first.

Cc: stable@vger.kernel.org
Fixes: 02205a6752f2 ("tracing: Add support for 'field variables'")
Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
Assisted-by: Claude:claude-fable-5
---
 kernel/trace/trace_events_hist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 9d7ce01fda36..b43b83d7c6f3 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -3097,7 +3097,7 @@ create_field_var_hist(struct hist_trigger_data *target_hist_data,
 		key_field = hist_data->fields[i];
 		if (!first)
 			seq_buf_putc(&s, ',');
-		seq_buf_puts(&s, key_field->field->name);
+		expr_field_str(key_field, &s);
 		first = false;
 	}
 
-- 
2.53.0

Re: [PATCH v2 2/2] tracing: Fix NULL dereference when copying keys for a field variable

[sashiko-bot]

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

Pre-existing issues:
- [High] Leaked synthetic field variable histogram triggers on error paths in `create_field_var_hist()` and `event_hist_trigger_parse()`.
--

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

tracing: Fix NULL dereference when copying keys for a field variable

This commit fixes a NULL pointer dereference in create_field_var_hist()
by using expr_field_str() to retrieve the field name instead of accessing
the name directly. This prevents crashes when handling pseudo field types
like common_cpu.

Cc: stable@vger.kernel.org
Fixes: 02205a6752f2 ("tracing: Add support for 'field variables'")

> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 9d7ce01fda360..b43b83d7c6f39 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -3097,7 +3097,7 @@ create_field_var_hist(struct hist_trigger_data *target_hist_data,
>  		key_field = hist_data->fields[i];
>  		if (!first)
>  			seq_buf_putc(&s, ',');
> -		seq_buf_puts(&s, key_field->field->name);
> +		expr_field_str(key_field, &s);
>  		first = false;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but does create_field_var_hist() leak
synthetic field variable histogram triggers on error paths?

Later in create_field_var_hist(), if the histogram trigger is successfully
registered but find_synthetic_field_var() subsequently fails:

	ret = event_hist_trigger_parse(&trigger_hist_cmd, file,
				       "", "hist", cmd);
	if (ret) {
		...
	}

	kfree(cmd);

	/* 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);
	}

It appears the tracking reference is dropped without unregistering the
newly created sub-trigger.

Similarly, in event_hist_trigger_parse(), if a subsequent action fails:

		ret = create_actions(hist_data);
		if (ret)
			goto out_free;

The out_free path destroys the tracking structures via destroy_hist_data()
but doesn't appear to call unregister_field_var_hists() to unregister them:

 out_free:
	remove_hist_vars(hist_data);

	trigger_data_free(trigger_data);

	destroy_hist_data(hist_data);
	goto out;

Could this allow userspace to trigger a memory leak or memory exhaustion
by repeatedly writing malformed histogram triggers with invalid actions?

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

Re: [PATCH v2 1/2] tracing: Add the bucket size to expr_field_str()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Incorrect format specifier `%ld` for `unsigned long` field `buckets`.
--

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

tracing: Add the bucket size to expr_field_str()

This commit updates expr_field_str() to append the bucket size count to the
rendered string. This matches the behavior of hist_field_print() and allows
legal histogram keys with .buckets to be handled correctly.

> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 963e0d6b61fdf..9d7ce01fda360 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1755,6 +1755,9 @@ static bool expr_field_str(struct hist_field *field, struct seq_buf *s)
>  			seq_buf_printf(s, ".%s", flags_str);
>  	}
>  
> +	if (field->buckets)
> +		seq_buf_printf(s, "=%ld", field->buckets);

[Severity: Low]
Since field->buckets is an unsigned long, could formatting it with %ld
cause it to print as a negative number if the highest bit is set?

If a user configures a histogram trigger specifying a .buckets size that
exceeds the maximum signed long value, it appears the field will incorrectly
print with a negative bucket size (e.g., =-9223372036854775808) when the
configuration is read back through tracefs.

Would %lu be more appropriate here?

> +
>  	return !seq_buf_has_overflowed(s);
>  }

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