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 <[email protected]> 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/[email protected]?part=1
