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
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 <redacted>
Assisted-by: Claude:claude-fable-5
---
kernel/trace/trace_events_hist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 <redacted>
Assisted-by: Claude:claude-fable-5
---
kernel/trace/trace_events_hist.c | 3 +++
1 file changed, 3 insertions(+)
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 <redacted>
Assisted-by: Claude:claude-fable-5
---
kernel/trace/trace_events_hist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 <redacted>
Assisted-by: Claude:claude-fable-5
---
kernel/trace/trace_events_hist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 [off-list ref]
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.
[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.
[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.
[ ... ]
From: Tom Zanussi <zanussi@kernel.org> Date: 2026-09-14 20:01:56
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>
On Mon, 14 Sep 2026 05:46:14 +0000, sashiko-bot@kernel.org wrote:
[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.
Yes, and the two other issues in this mail are real as well. The 32-bit
truncation in DEFINE_HIST_FIELD_FN() and the one in hist_field_log2() are
posted:
https://lore.kernel.org/all/20260917015532.103081-1-donggeunyoo.kernel@gmail.com/https://lore.kernel.org/all/20260917023834.216893-1-donggeunyoo.kernel@gmail.com/
This one I have not posted. When I went to write it, it turned out to need
more change than either of those, and I would rather hear what people think
of the approach first.
What it looks like today. The largest multiple of ten that fits in a u64 is
2^64 - 6, so with .buckets=10 over a signed field the groups below zero are
-6..-1, then -16..-7, then -26..-17: none of the boundaries fall on a
multiple of ten and the group next to zero holds six values. At the end of
the range the two ends meet. S64_MAX and S64_MIN are 9223372036854775807
and 9223372036854775808 unsigned, and both divide down to
9223372036854775800, so one group holds both.
One detail in the report is off. hist_field->is_signed is 0 on a .buckets
key even over a signed field. create_hist_field() takes the modifier
branch, copies size and type from operands[0] and stops. hist_debug on
"keys=arg.buckets=10" over an s32 field prints
type: s32 ... is_signed: 0
The member that does carry it is hist_field->field->is_signed, which is
already what create_tracing_map_fields() passes to tracing_map_cmp_num() to
pick the sort comparator - so the tree orders such a key signed today while
grouping and printing it unsigned.
The rendering half of that is posted separately:
https://lore.kernel.org/all/20260917045918.370993-1-donggeunyoo.kernel@gmail.com/
What I have for the grouping:
- Take the signedness from hist_field->field->is_signed, so the grouping,
the sort and the rendering all come from one place.
- For a negative value, round toward negative infinity instead of dividing
the two's complement. Boundaries then stay on multiples of the size on
both sides of zero, so .buckets=10 groups -10..-1 and 0..9.
- Clamp the lowest group at S64_MIN. The boundary below it is not
representable in the u64 a key is stored in, so that group is short,
the same way .buckets already has a short group at the top of an
unsigned range. Its printed end has to come from the true boundary
rather than start + size - 1, or it overlaps the group above it. With
.buckets=10 it prints as
{ arg: ~ -9223372036854775808--9223372036854775801 } hitcount: 2
and the next group starts at -9223372036854775800. With a size that
divides 2^63 nothing is short.
- Print the range signed, which cannot be done on its own: rendering
today's grouping signed would name a range that does not contain
S64_MIN.
Three things I would like an opinion on.
Should .buckets interpret signedness at all? histogram.rst says "in general
the semantics of a given field aren't interpreted when applying a modifier
to it", which reads against this. On the other side, the sort comparator is
already chosen from field->is_signed, so the tree does interpret it, just
not for grouping.
Is the short group at S64_MIN acceptable? I do not see a way to avoid it
that keeps the boundaries on multiples of the size, and anchoring the grid
at S64_MIN instead puts zero inside a group.
Is this a fix or a change? It alters what .buckets prints for any signed
field holding negative values. No ftrace selftest uses .buckets, but it is
still visible output, so I am not sure a Fixes: tag is the right framing.
I have it written and measured against three arms in QEMU if an RFC posting
would be more useful than this description.
Thanks,
Donggeun