Re: [PATCH] tracing: hist: free the field rejected for a bad modifier

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Preexisting memory leak of VAR_REF hist_field objects when a variable is referenced 3 or more times.
- [High] Preexisting memory leak of VAR_REF hist_field in create_var_ref error path.
--

commit 899dd39af79bb308de7335dd48b6656e713c7a62
Author: Donggeun Yoo <donggeunyoo.kernel@gmail.com>

tracing: hist: free the field rejected for a bad modifier

Writing a hist trigger whose value or variable carries a modifier that is
not allowed there leaks the fields that were built for it. This patch fixes
the leak by using destroy_hist_field() to free the rejected fields when
a bad modifier is encountered.

> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 893bd8b0e48a8..6b5572ef2d4ba 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -4331,6 +4331,7 @@ static int __create_val_field(struct hist_trigger_data *hist_data,
>  	return ret;
>   err:
>  	hist_err(file->tr, HIST_ERR_BAD_FIELD_MODIFIER, errpos(field_str));
> +	destroy_hist_field(hist_field, 0);
>  	return -EINVAL;
>  }

[Severity: High]
This is a pre-existing issue, and was not introduced by the patch under review,
but since the commit message mentions that destroy_hist_field() returns
early for HIST_FIELD_FL_VAR_REF, does this behavior cause a memory leak in
the error path of create_var_ref()?

Looking at create_var_ref() in kernel/trace/trace_events_hist.c, if
init_var_ref() fails:

		if (init_var_ref(ref_field, var_field, system, event_name)) {
			destroy_hist_field(ref_field, 0);
			return NULL;
		}

destroy_hist_field() explicitly skips freeing fields with the
HIST_FIELD_FL_VAR_REF flag:

	if (hist_field->flags & HIST_FIELD_FL_VAR_REF)
		return; /* var refs will be destroyed separately */

Since the ref_field failed initialization and hasn't been added to the
global hist_data->var_refs array yet, will this object be permanently
leaked rather than freed during global trigger teardown?

[Severity: High]
This is also a pre-existing issue, and was not introduced by the patch under
review, but is there a permanent memory leak of VAR_REF hist_field objects
when a variable is referenced 3 or more times?

For example, when a trigger is created referencing the same variable
multiple times, or fails during parsing after reaching 3 references:
hist:keys=pid:val1=$x:val2=$x:val3=$x

destroy_hist_fields() calls __destroy_hist_field() exactly once per unique
variable in hist_data->var_refs:

		__destroy_hist_field(hist_data->var_refs[i]);
		hist_data->var_refs[i] = NULL;

However, in __destroy_hist_field(), there is a refcount check:

	if (--hist_field->ref > 1)
		return;

If a variable is used 3 or more times, its initial refcount would be at
least 3. When __destroy_hist_field() decrements it by one, the refcount
remains 2 or higher, causing an early return without freeing the memory.

Since destroy_hist_fields() sets the pointer to NULL immediately after,
could this permanently leak the VAR_REF object because no other code drops
references for these objects?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907034948.240387-1-donggeunyoo.kernel@gmail.com?part=1