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

Tom Zanussi
2026-05-01T17:10:41Z

Hi Pengpeng,

Looks fine to me, just a couple minor nits below..

On Thu, 2026-04-30 at 12:33 +0800, Pengpeng Hou 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 v5: https://lore.kernel.org/all/20260424070104.1-tracing-synth-v5-pengpeng@iscas.ac.cn/
> - start a new thread for the new patch revision
> - use a lore link for the previous version in the changelog
> - simplify the synthetic-name construction with seq_buf_printf()
> - keep saved_filter as a normal local variable and avoid an anonymous block
>
>  kernel/trace/trace_events_hist.c | 41 ++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 0dbbf6cca9bc..aa8e7f043ac0 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,23 @@ 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;

Can you move this down a line, to maintain the reverse Christmas tree
declarations?

>
>   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_printf(&s, "synthetic_%s", field_name);
> +
> + /* Terminate synthetic_name with a NUL. */
> + seq_buf_str(&s);
> +

This doesn't hurt, but is it really needed? I think seq_buf_printf()
already null-terminates.

> + 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,6 +3030,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;
> + struct seq_buf s;

Same here.

>   char *saved_filter;
>   char *cmd;
>   int ret;
> @@ -3065,28 +3076,34 @@ 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);
> + if (saved_filter)
> + seq_buf_printf(&s, " if %s", saved_filter);
> +
> + /* Terminate cmd with a NUL. */
> + seq_buf_str(&s);
> +

And here.

> + if (seq_buf_has_overflowed(&s)) {
> + kfree(cmd);
> + kfree(var_hist);
> + return ERR_PTR(-E2BIG);
>   }
>
>   var_hist->cmd = kstrdup(cmd, GFP_KERNEL);

Anyway,

Reviewed-by: Tom Zanussi <zanussi@kernel.org>

Thanks,

Tom