[PATCH v2] tracing: Compute the log2 histogram bucket in 64 bits

[PATCH v2] tracing: Compute the log2 histogram bucket in 64 bits

[Donggeun Yoo]

The .log2 key modifier files a value under its base-2 order:

  # echo 'hist:keys=bytes_req.log2' > events/kmem/kmalloc/trigger

hist_field_log2() is handed a u64 and computes that order as
ilog2(roundup_pow_of_two(val)). Both helpers take an unsigned long, so
the bucket is wrong wherever the value does not survive the conversion.

The out-of-line roundup_pow_of_two() is

	return 1UL << fls_long(n - 1);

which shifts by BITS_PER_LONG when n is 0, where n - 1 wraps to
ULONG_MAX, and when n is above 2^(BITS_PER_LONG-1). Both are ordinary
keys, because the field fetch hands every value over as a u64. On
x86_64, keying on a u64 field and passing 2^63+1 and U64_MAX puts both
in bucket 0, and with CONFIG_UBSAN_SHIFT=y the shift is reported:

  UBSAN: shift-out-of-bounds in include/linux/log2.h:57:13
  shift exponent 64 is too large for 64-bit type 'long unsigned int'

On a 32-bit kernel the conversion drops the upper half of the value
before any of that runs. Keying on common_timestamp.log2 six seconds
into boot, where the timestamp needs 33 bits, one build of each gives

  x86_64  { common_timestamp: ~ 2^33 }
  i386    { common_timestamp: ~ 2^0  }

hist_field_timestamp() returns the u64 unchanged, so hist_field_log2()
is the only narrowing in that path.

Compute the order from the u64. ilog2() selects __ilog2_u64() for an
eight-byte argument, so ilog2(val - 1) + 1 for val above 1, and 0 below
it, is order_base_2() evaluated without narrowing. It returns what the
old expression returned on every input the old one was defined for.
Where it was not, 0 stays in bucket 0, and the values an unsigned long
could not hold move to the bucket their magnitude asks for.

Cc: stable@vger.kernel.org
Fixes: 4b94f5b7b4a5 ("tracing: Add hist trigger 'log2' modifier")
Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/all/20260914054614.82A9B1F000FF@smtp.kernel.org/
Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
---
Changes since v1
(https://lore.kernel.org/all/20260916002735.808520-1-donggeunyoo.kernel@gmail.com/):
- v1 used order_base_2(), which also takes an unsigned long and so fixed
  only the 64-bit half.  Compute from the u64 instead.
- Added Reported-by/Closes: the 32-bit half is claim (a) of that report,
  which v1 did not fix.

QEMU, v7.3-rc3-82-g238650ef6c7c, CONFIG_UBSAN_SHIFT=y, one kernel per arm
per arch.  A user_events record carries a u64 field; each value is keyed
with .log2 under its own id, so no two share a row.

  value               x86_64 before  i386 before  after, both
  0 1 2 3 4 5 2^31    correct        correct      unchanged
  0x100000000         2^32           2^0          2^32
  0x100000001         2^33           2^0          2^33
  0x123456789abc      2^45           2^31         2^45
  0x8000000000000001  2^0            2^0          2^64
  0xffffffffffffffff  2^0            2^0          2^64
  0xffffffffffffff9c  2^0            2^0          2^64

One UBSAN shift report per arch before, none after.

The i386 column needs "tracing: Fix 64-bit and signed histogram fields on
32-bit kernels" applied; without it the field fetch truncates first and
nothing in that column is attributable to this function.  The
common_timestamp figures above the --- do not need it.

The build-tree prefix was trimmed from the UBSAN path above.

 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..86f5767dbb81 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -289,7 +289,7 @@ static u64 hist_field_log2(struct hist_field *hist_field,
 
 	u64 val = hist_fn_call(operand, elt, buffer, rbe, event);
 
-	return (u64) ilog2(roundup_pow_of_two(val));
+	return val > 1 ? ilog2(val - 1) + 1 : 0;
 }
 
 static u64 hist_field_bucket(struct hist_field *hist_field,
-- 
2.53.0