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

[PATCH v3 0/4] 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.  4/4 renders the key with expr_field_str() instead.  The three before
it make that renderer produce something parse_field() accepts.

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

Changes since v2:
 - Rebased onto v7.3-rc4.  v2 was built on 2f0c1cf72f46, before the tracing
   fixes merged, and that turns out to matter -- see the next entry.
 - Reinstated the stacktrace patch, now 3/4.  v2 dropped it after measuring
   that common_stacktrace.stacktrace parsed fine.  That was true of
   2f0c1cf72f46 and is no longer true: a5e70ba87ca8 now refuses the
   modifier unless the field is a real one with FILTER_STACKTRACE, so
   without 3/4 a common_stacktrace key renders into a command that is
   rejected.
 - 1/4 is new: hist_field_print() prints the bucket size with %ld, so a
   size above LONG_MAX reads back negative.  Reported by sashiko-bot.
 - 2/4 uses %lu for the same reason.

Patches 1/4 through 3/4 have no effect on their own -- nothing reaches
expr_field_str() with a bucketed or stacktrace field until 4/4 renders keys
with it -- but each is needed before 4/4, and 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
  hitcount                oops        keys=hitcount

Unpatched, the .log2 and .buckets rows drop their modifier, so the
generated histogram does not bucket the way the one it mirrors does.  Each
oops is a null-ptr-deref at create_field_var_hist+0x771, taken in its own
boot; the three real-field rows run the same loop to completion without
faulting, so the six are the loop reaching the faulting line rather than a
boot failure.

And the bucket size a key can carry, read back from the trigger:

  .buckets=                     unpatched                patched
  0                             rejected                 rejected
  -5                            rejected                 rejected
  18446744073709551616          rejected                 rejected
  9223372036854775807           9223372036854775807      9223372036854775807
  9223372036854775808           -9223372036854775808     9223372036854775808
  18446744073709551615          -1                       18446744073709551615

x86_64 under QEMU, CONFIG_KASAN=y, 4 CPUs, base 704340f1cd0d.  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 (4):
  tracing: Print the bucket size as unsigned
  tracing: Add the bucket size to expr_field_str()
  tracing: Only report the stacktrace modifier on a real field
  tracing: Fix NULL dereference when copying keys for a field variable

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

-- 
2.53.0

[PATCH v3 1/4] tracing: Print the bucket size as unsigned

[Donggeun Yoo]

A key's bucket size reads back negative once it exceeds LONG_MAX:

  # echo 'hist:keys=next_pid.buckets=18446744073709551615' > trigger
  # cat trigger
  hist:keys=next_pid.buckets=-1:vals=hitcount:...

parse_field() takes the size with kstrtoul() and rejects only zero, so the
whole unsigned long range is accepted and stored, but hist_field_print()
renders it with %ld.

Print it with %lu.

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 | 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 8af97fd4ee2d..a3eae5c1344a 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -6200,7 +6200,7 @@ static void hist_field_print(struct seq_file *m, struct hist_field *hist_field)
 		}
 	}
 	if (hist_field->buckets)
-		seq_printf(m, "=%ld", hist_field->buckets);
+		seq_printf(m, "=%lu", hist_field->buckets);
 }
 
 static int event_hist_trigger_print(struct seq_file *m,
-- 
2.53.0

[PATCH v3 2/4] 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 a3eae5c1344a..91a550411e8a 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1754,6 +1754,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, "=%lu", field->buckets);
+
 	return !seq_buf_has_overflowed(s);
 }
 
-- 
2.53.0

[PATCH v3 3/4] tracing: Only report the stacktrace modifier on a real field

[Donggeun Yoo]

get_hist_field_flags() returns "stacktrace" for any field carrying
HIST_FIELD_FL_STACKTRACE, including the common_stacktrace pseudo-field,
which has no ftrace_event_field behind it. parse_field() no longer takes
the modifier there:

	if (stack_modifier &&
	    (!field || field->filter_type != FILTER_STACKTRACE)) {
		hist_err(tr, HIST_ERR_BAD_FIELD_MODIFIER, errpos(field_str));

so expr_field_str() renders "common_stacktrace.stacktrace", a spelling
that cannot be parsed back.

Report the modifier only when there is a field to report it for.

hist_field_print(), the other caller of get_hist_field_flags(), excludes
HIST_FIELD_FL_STACKTRACE before it calls, so this is confined to
expr_field_str(), whose only key renderer arrives in the next patch.

Fixes: a5e70ba87ca8 ("tracing: Fix memory corruption from the histogram stacktrace modifier")
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 91a550411e8a..fdd097abb0d6 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1725,7 +1725,7 @@ static const char *get_hist_field_flags(struct hist_field *hist_field)
 		flags_str = "percent";
 	else if (hist_field->flags & HIST_FIELD_FL_GRAPH)
 		flags_str = "graph";
-	else if (hist_field->flags & HIST_FIELD_FL_STACKTRACE)
+	else if (hist_field->flags & HIST_FIELD_FL_STACKTRACE && hist_field->field)
 		flags_str = "stacktrace";
 
 	return flags_str;
-- 
2.53.0

[PATCH v3 4/4] 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.

The three previous patches are prerequisites: without them a .buckets
key and a common_stacktrace key each render into a command that cannot
be parsed back.

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 fdd097abb0d6..11d4cf2544ac 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -3093,7 +3093,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 v3 1/4] tracing: Print the bucket size as unsigned

[sashiko-bot]

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

Pre-existing issues:
- [High] Data truncation and broken sign-extension of 64-bit trace fields on 32-bit architectures.
- [High] Histogram buckets calculate the bucket multiplier using unsigned division, mapping negative signed values to incorrectly shifted intervals.
--

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

tracing: Print the bucket size as unsigned

This commit fixes how the bucket size is printed in hist_field_print().
It changes the format specifier from %ld to %lu to prevent negative output
when the size exceeds LONG_MAX.

> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 963e0d6b61fdf..ecfdc5ddc2c13 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
[ ... ]
> @@ -283,28 +283,28 @@ static u64 hist_field_log2(struct hist_field *hist_field,
>  			   struct tracing_map_elt *elt,
>  			   struct trace_buffer *buffer,
>  			   struct ring_buffer_event *rbe,
>  			   void *event)
>  {
>  	struct hist_field *operand = hist_field->operands[0];
>  
>  	u64 val = hist_fn_call(operand, elt, buffer, rbe, event);
>  
>  	return (u64) ilog2(roundup_pow_of_two(val));

[Severity: High]
This is a pre-existing issue, but does roundup_pow_of_two() in
hist_field_log2() truncate the 64-bit val on 32-bit architectures?

Since roundup_pow_of_two() takes an unsigned long, it appears this will
discard the upper 32 bits before computing the logarithm for any histogram
trigger operating on a 64-bit field on a 32-bit architecture.

>  }
>  
>  static u64 hist_field_bucket(struct hist_field *hist_field,
>  			     struct tracing_map_elt *elt,
>  			     struct trace_buffer *buffer,
>  			     struct ring_buffer_event *rbe,
>  			     void *event)
>  {
>  	struct hist_field *operand = hist_field->operands[0];
>  	unsigned long buckets = hist_field->buckets;
>  
>  	u64 val = hist_fn_call(operand, elt, buffer, rbe, event);
>  
>  	if (WARN_ON_ONCE(!buckets))
>  		return val;
>  
>  	if (val >= LONG_MAX)
>  		val = div64_ul(val, buckets);
>  	else
>  		val = (u64)((unsigned long)val / buckets);

[Severity: High]
This isn't a bug introduced by this patch, but does the unsigned division
in hist_field_bucket() break the bucketing logic for negative numbers?

If the field is signed and negative, its 2's complement representation is
large. Performing unsigned division via div64_ul() or an unsigned cast
without checking hist_field->is_signed seems like it would calculate the
remainder incorrectly. This would happen whenever a histogram is configured
with buckets over a signed field that takes negative values, mapping them
to the wrong bucket intervals.

>  	return val * buckets;
>  }
[ ... ]
> @@ -461,11 +461,11 @@ static u64 hist_field_timestamp(struct hist_field *hist_field,
>  #define DEFINE_HIST_FIELD_FN(type)					\
>  	static u64 hist_field_##type(struct hist_field *hist_field,	\
>  				     struct tracing_map_elt *elt,	\
>  				     struct trace_buffer *buffer,	\
>  				     struct ring_buffer_event *rbe,	\
>  				     void *event)			\
>  {									\
>  	type *addr = (type *)(event + hist_field->field->offset);	\
>  									\
>  	return (u64)(unsigned long)*addr;				\
>  }

[Severity: High]
This is a pre-existing issue, but will casting to unsigned long in the
DEFINE_HIST_FIELD_FN macro corrupt 64-bit and signed trace fields on 32-bit
platforms?

It looks like this cast truncates 64-bit trace values to 32 bits, silently
discarding the upper 32 bits. Additionally, for signed types (s32, s16, s8),
it appears to break sign extension, causing negative values to become large
positive 32-bit integers. This would corrupt values for any histogram
trigger operating on a 64-bit or signed field on a 32-bit architecture.

[ ... ]
> @@ -6203,7 +6203,7 @@ static void hist_field_print(struct seq_file *m, struct hist_field *hist_field)
>  		}
>  	}
>  	if (hist_field->buckets)
> -		seq_printf(m, "=%ld", hist_field->buckets);
> +		seq_printf(m, "=%lu", hist_field->buckets);
>  }
>  
>  static int event_hist_trigger_print(struct seq_file *m,

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

Re: [PATCH v3 0/4] tracing: Fix NULL dereference when copying keys for a field variable

[Tom Zanussi]

Hi,

On Mon, 2026-09-14 at 14:34 +0900, Donggeun Yoo wrote:
> 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.  4/4 renders the key with expr_field_str() instead.  The three before
> it make that renderer produce something parse_field() accepts.
> 
> Link: https://lore.kernel.org/linux-trace-kernel/20260913203129.941270-1-donggeunyoo.kernel@gmail.com/
> 
> Changes since v2:
>  - Rebased onto v7.3-rc4.  v2 was built on 2f0c1cf72f46, before the tracing
>    fixes merged, and that turns out to matter -- see the next entry.
>  - Reinstated the stacktrace patch, now 3/4.  v2 dropped it after measuring
>    that common_stacktrace.stacktrace parsed fine.  That was true of
>    2f0c1cf72f46 and is no longer true: a5e70ba87ca8 now refuses the
>    modifier unless the field is a real one with FILTER_STACKTRACE, so
>    without 3/4 a common_stacktrace key renders into a command that is
>    rejected.
>  - 1/4 is new: hist_field_print() prints the bucket size with %ld, so a
>    size above LONG_MAX reads back negative.  Reported by sashiko-bot.
>  - 2/4 uses %lu for the same reason.
> 
> Patches 1/4 through 3/4 have no effect on their own -- nothing reaches
> expr_field_str() with a bucketed or stacktrace field until 4/4 renders keys
> with it -- but each is needed before 4/4, and 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
>   hitcount                oops        keys=hitcount
> 
> Unpatched, the .log2 and .buckets rows drop their modifier, so the
> generated histogram does not bucket the way the one it mirrors does.  Each
> oops is a null-ptr-deref at create_field_var_hist+0x771, taken in its own
> boot; the three real-field rows run the same loop to completion without
> faulting, so the six are the loop reaching the faulting line rather than a
> boot failure.
> 
> And the bucket size a key can carry, read back from the trigger:
> 
>   .buckets=                     unpatched                patched
>   0                             rejected                 rejected
>   -5                            rejected                 rejected
>   18446744073709551616          rejected                 rejected
>   9223372036854775807           9223372036854775807      9223372036854775807
>   9223372036854775808           -9223372036854775808     9223372036854775808
>   18446744073709551615          -1                       18446744073709551615
> 
> x86_64 under QEMU, CONFIG_KASAN=y, 4 CPUs, base 704340f1cd0d.  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 (4):
>   tracing: Print the bucket size as unsigned
>   tracing: Add the bucket size to expr_field_str()
>   tracing: Only report the stacktrace modifier on a real field
>   tracing: Fix NULL dereference when copying keys for a field variable
> 
>  kernel/trace/trace_events_hist.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 

These look fine to me, thanks. For the set,

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