On Thu, 9 Apr 2026 10:19:43 +0800
Pengpeng Hou <[email protected]> wrote:
Hi Pengpeng,
Note, the tracing subsystem uses capital letters in the subject:
Subject: tracing: Bound synthetic-field strings with seq_buf
> 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 <[email protected]>
> ---
> Changes since v2:
> https://lore.kernel.org/all/[email protected]/
>
> - switch the synthetic name and generated command construction to seq_buf
> as suggested by Steven Rostedt
> - keep MAX_FILTER_STR_VAL as the tracing-side limit and return -E2BIG on
> overflow
>
> kernel/trace/trace_events_hist.c | 44 ++++++++++++++++++++++----------
> 1 file changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c
> b/kernel/trace/trace_events_hist.c
> index 73ea180cad55..7c3873719beb 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>
> @@ -2962,14 +2963,21 @@ 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);
Should have a comment here specifying what the seq_buf_str() is doing:
/* 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);
>
> @@ -3014,6 +3022,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;
> char *saved_filter;
> char *cmd;
> int ret;
> @@ -3046,41 +3055,48 @@ create_field_var_hist(struct hist_trigger_data
> *target_hist_data,
> /* See if a synthetic field variable has already been created */
> event_var = find_synthetic_field_var(target_hist_data, subsys_name,
> event_name, field_name);
> - if (!IS_ERR_OR_NULL(event_var))
> + if (IS_ERR(event_var))
> + return event_var;
> + if (event_var)
> return event_var;
Note, the above is equivalent to:
if (event_var)
return event_var;
And since it is a separate issue than the bounding of the string, it
should be a separate patch.
>
> var_hist = kzalloc_obj(*var_hist);
> if (!var_hist)
> return ERR_PTR(-ENOMEM);
>
> + saved_filter = find_trigger_filter(hist_data, file);
Why did you move this up here?
> +
> cmd = kzalloc(MAX_FILTER_STR_VAL, GFP_KERNEL);
> if (!cmd) {
> kfree(var_hist);
> 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);
It makes more sense to define saved_filter next to where it is used.
> - if (saved_filter) {
> - strcat(cmd, " if ");
> - strcat(cmd, saved_filter);
> + if (saved_filter)
> + seq_buf_printf(&s, " if %s", saved_filter);
> +
> + 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);
-- Steve
