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

[Tom Zanussi <zanussi@kernel.org>]

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>