On Thu, 9 Apr 2026 10:56:28 +0800 Pengpeng Hou <[email protected]> wrote:
Hi Pengpeng, Again, subject should be: tracing: Bound histogram expression strings with seq_buf > expr_str() allocates a fixed MAX_FILTER_STR_VAL buffer and then builds > expression names with a series of raw strcat() appends. Nested operands, > constants and field flags can push the rendered string past that fixed > limit before the name is attached to the hist field. > > Build the expression strings with seq_buf and propagate failures back to > the expression parser when the rendered name would exceed > MAX_FILTER_STR_VAL. > > Signed-off-by: Pengpeng Hou <[email protected]> > --- > Changes since v1: > - replace the previous bounded append helper and manual length tracking > with seq_buf as suggested by Steven Rostedt > - keep the -E2BIG propagation back into parse_unary() and parse_expr() > > kernel/trace/trace_events_hist.c | 93 ++++++++++++++++++++++---------- > 1 file changed, 64 insertions(+), 29 deletions(-) > > diff --git a/kernel/trace/trace_events_hist.c > b/kernel/trace/trace_events_hist.c > index 73ea180cad55..09aaedb92993 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -8,6 +8,7 @@ > #include <linux/module.h> > #include <linux/kallsyms.h> > #include <linux/security.h> > +#include <linux/seq_buf.h> > #include <linux/mutex.h> > #include <linux/slab.h> > #include <linux/stacktrace.h> > @@ -1738,85 +1739,104 @@ static const char *get_hist_field_flags(struct > hist_field *hist_field) > return flags_str; > } > > -static void expr_field_str(struct hist_field *field, char *expr) > +static bool expr_field_str(struct hist_field *field, struct seq_buf *s) > { > + const char *field_name; > + > if (field->flags & HIST_FIELD_FL_VAR_REF) > - strcat(expr, "$"); > - else if (field->flags & HIST_FIELD_FL_CONST) { > - char str[HIST_CONST_DIGITS_MAX]; > + seq_buf_putc(s, '$'); > + else if (field->flags & HIST_FIELD_FL_CONST) > + seq_buf_printf(s, "%llu", field->constant); > > - snprintf(str, HIST_CONST_DIGITS_MAX, "%llu", field->constant); > - strcat(expr, str); > - } > + field_name = hist_field_name(field, 0); > + if (!field_name) > + return false; > > - strcat(expr, hist_field_name(field, 0)); > + seq_buf_puts(s, field_name); > > if (field->flags && !(field->flags & HIST_FIELD_FL_VAR_REF)) { > const char *flags_str = get_hist_field_flags(field); > > - if (flags_str) { > - strcat(expr, "."); > - strcat(expr, flags_str); > - } > + if (flags_str) > + seq_buf_printf(s, ".%s", flags_str); > } > + > + return !seq_buf_has_overflowed(s); > } > > static char *expr_str(struct hist_field *field, unsigned int level) > { > char *expr; > + struct seq_buf s; > + int ret = -E2BIG; > > if (level > 1) > - return NULL; > + return ERR_PTR(-EINVAL); > > expr = kzalloc(MAX_FILTER_STR_VAL, GFP_KERNEL); > if (!expr) > - return NULL; > + return ERR_PTR(-ENOMEM); A patch should do one thing at a time. This patch appears to be doing two things: fixing the bound strings, returning errors instead of NULL. Please split this up into two patches. One that changes the return values from NULL to ERR_PTR() and the other to use seq_buf. Thanks, -- Steve
