Re: [PATCH v5] tracing: Bound synthetic-field strings with seq_buf

[Steven Rostedt <rostedt@goodmis.org>]

I should have also mentioned. Please, new versions of a patch should always
start a new thread. Don't reply to the previous version. That makes it very
difficult to find which is the latest version.

On Thu, 23 Apr 2026 23:33:00 +0800
Pengpeng Hou <pengpeng@iscas.ac.cn> wrote:

> The synthetic field helpers build a prefixed synthetic variable name and
> a generated hist command in fixed MAX_FILTER_STR_VAL buffers. The
> current code appends those strings with raw strcat(), so long key lists,
> field names, or saved filters can run past the end of the staging
> buffers.
> 
> Build both strings with seq_buf and propagate -E2BIG if either the
> synthetic variable name or the generated command exceeds
> MAX_FILTER_STR_VAL. This keeps the existing tracing-side limit while
> using the helper intended for bounded command construction.
> 
> Fixes: 02205a6752f2 ("tracing: Add support for 'field variables'")
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> Changes since v4:

To keep a link to the previous version, use lore links to access it (the
message ID of the previous version attached to
"https://lore.kernel.org/all/$message-id")

 Changes since v4: https://lore.kernel.org/all/20260417223001.1-tracing-synth-v4-pengpeng@iscas.ac.cn/


> - add the requested blank lines around seq_buf_str() comments
> - add the seq_buf_str() comment for the generated command buffer too
> - keep saved_filter scoped next to its point of use
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 0dbbf6cca9bc..87429567417f 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>
> @@ -2968,14 +2969,24 @@ find_synthetic_field_var(struct hist_trigger_data *target_hist_data,
>  			 char *system, char *event_name, char *field_name)
>  {
>  	struct hist_field *event_var;
> +	struct seq_buf s;
>  	char *synthetic_name;
>  
>  	synthetic_name = kzalloc(MAX_FILTER_STR_VAL, GFP_KERNEL);
>  	if (!synthetic_name)
>  		return ERR_PTR(-ENOMEM);
>  
> -	strcpy(synthetic_name, "synthetic_");
> -	strcat(synthetic_name, field_name);
> +	seq_buf_init(&s, synthetic_name, MAX_FILTER_STR_VAL);

> +	seq_buf_puts(&s, "synthetic_");
> +	seq_buf_puts(&s, field_name);

Hmm, the above is probably simpler with:

	seq_buf_printf(&s, "synthetic_%s", field_name);

> +
> +	/* Terminate synthetic_name with a NUL. */
> +	seq_buf_str(&s);
> +
> +	if (seq_buf_has_overflowed(&s)) {
> +		kfree(synthetic_name);
> +		return ERR_PTR(-E2BIG);
> +	}
>  
>  	event_var = find_event_var(target_hist_data, system, event_name, synthetic_name);
>  
> @@ -3020,7 +3031,7 @@ create_field_var_hist(struct hist_trigger_data *target_hist_data,
>  	struct trace_event_file *file;
>  	struct hist_field *key_field;
>  	struct hist_field *event_var;

> -	char *saved_filter;

Don't remove this for an anonymous block.

> +	struct seq_buf s;
>  	char *cmd;
>  	int ret;
>  
> @@ -3065,28 +3076,37 @@ create_field_var_hist(struct hist_trigger_data *target_hist_data,
>  		return ERR_PTR(-ENOMEM);
>  	}
>  
> +	seq_buf_init(&s, cmd, MAX_FILTER_STR_VAL);
> +
>  	/* Use the same keys as the compatible histogram */
> -	strcat(cmd, "keys=");
> +	seq_buf_puts(&s, "keys=");
>  
>  	for_each_hist_key_field(i, hist_data) {
>  		key_field = hist_data->fields[i];
>  		if (!first)
> -			strcat(cmd, ",");
> -		strcat(cmd, key_field->field->name);
> +			seq_buf_putc(&s, ',');
> +		seq_buf_puts(&s, key_field->field->name);
>  		first = false;
>  	}
>  
>  	/* Create the synthetic field variable specification */
> -	strcat(cmd, ":synthetic_");
> -	strcat(cmd, field_name);
> -	strcat(cmd, "=");
> -	strcat(cmd, field_name);
> +	seq_buf_printf(&s, ":synthetic_%s=%s", field_name, field_name);
>  
>  	/* Use the same filter as the compatible histogram */
> -	saved_filter = find_trigger_filter(hist_data, file);
> -	if (saved_filter) {
> -		strcat(cmd, " if ");
> -		strcat(cmd, saved_filter);
> +	{

We don't add anonymous blocks in the kernel (with the exception of needing
to be around #ifdefs).

-- Steve

> +		char *saved_filter = find_trigger_filter(hist_data, file);
> +
> +		if (saved_filter)
> +			seq_buf_printf(&s, " if %s", saved_filter);
> +	}
> +
> +	/* Terminate cmd with a NUL. */
> +	seq_buf_str(&s);
> +
> +	if (seq_buf_has_overflowed(&s)) {
> +		kfree(cmd);
> +		kfree(var_hist);
> +		return ERR_PTR(-E2BIG);
>  	}
>  
>  	var_hist->cmd = kstrdup(cmd, GFP_KERNEL);