Re: [PATCH v2] tracing/hist: bound expression strings with seq_buf

[Steven Rostedt]

On Thu, 9 Apr 2026 10:56:28 +0800
Pengpeng Hou <pengpeng@xxxxxxxxxxx> 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 <pengpeng@xxxxxxxxxxx>
> ---
> 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