[PATCH] tracing: hist: free var refs regardless of how often they are referenced

[PATCH] tracing: hist: free var refs regardless of how often they are referenced

[Donggeun Yoo]

Using the same variable three or more times in one hist trigger leaks the
variable reference and its strings when the trigger is removed.

commit 656fe2ba85e8 ("tracing: Use hist trigger's var_ref array to destroy
var_refs") made a trigger's var_refs[] array the only owner of a var ref:
destroy_hist_field() returns early for HIST_FIELD_FL_VAR_REF, so the field
expressions never destroy one. One entry, freed once, no count needed.

commit 8bcebc77e85f ("tracing: Fix histogram code when expression has same
var as value") then made repeated references share one object and added a
count of them. Only the increment side exists, since those expressions
still return early and never drop a reference, so __destroy_hist_field()
sees how many references were created rather than how many are left. It
frees when the decremented count is 0 or 1, so two references work and
three or more leak.

Sharing kept one array entry per object, and create_var_ref() searches and
appends within a single trigger, so nothing outside it holds the object.
Removing a trigger whose variables are still referenced is already refused
by check_var_refs() with -EBUSY. Drop the count and free unconditionally.

Fixes: 8bcebc77e85f ("tracing: Fix histogram code when expression has same var as value")
Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
---
Reproduced under QEMU (x86_64) with CONFIG_DEBUG_KMEMLEAK. Two triggers
differing only in a third reference to the same variable, each installed
and removed 200 times:

  hist:keys=next_pid:delta=common_timestamp-$start,start2=$start:
      onmatch(sched.sched_waking).trace(first,$start2,common_timestamp,next_pid,$delta)

  ... plus delta2=common_timestamp-$start

  two references    0 unreferenced objects,   0 bytes
  three references  620 / 666 objects, 62000 / 66600 bytes over two runs

With this patch both are 0. kmemleak points at the var ref itself and at
the strings init_var_ref() attaches to it:

  unreferenced object (size 192):
    create_hist_field+0x39/0x390
    create_var_ref+0x96/0x100
    parse_atom+0x4ad/0x910
  unreferenced object (size 8):
    hex dump: 73 74 61 72 74 00 00 00   start...
    kstrdup+0x37/0x70
    init_var_ref+0x88/0x110

tools/testing/selftests/ftrace test.d/trigger: 45 tests, results identical
before and after, including the ones covering variable references --
field variable support, fully-qualified variable reference support,
inter-event combined, onmatch, onmax, onmatch-onmax and trace action all
pass. Three tests fail identically with and without the patch (onchange
action, and trace action with a dynamic string param); I did not chase
those down.

checkpatch --strict is clean and an x86_64 W=1 build of the file adds no
warnings.

 kernel/trace/trace_events_hist.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 963e0d6b61fd..f90680b33a37 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -169,7 +169,6 @@ struct hist_field {
 	struct hist_field		*operands[HIST_FIELD_OPERANDS_MAX];
 	struct hist_trigger_data	*hist_data;
 	enum hist_field_fn		fn_num;
-	unsigned int			ref;
 	unsigned int			size;
 	unsigned int			offset;
 	unsigned int                    is_signed;
@@ -1913,16 +1912,8 @@ static int contains_operator(char *str, char **sep)
 	return field_op;
 }
 
-static void get_hist_field(struct hist_field *hist_field)
-{
-	hist_field->ref++;
-}
-
 static void __destroy_hist_field(struct hist_field *hist_field)
 {
-	if (--hist_field->ref > 1)
-		return;
-
 	kfree(hist_field->var.name);
 	kfree(hist_field->name);
 
@@ -1969,8 +1960,6 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data,
 	if (!hist_field)
 		return NULL;
 
-	hist_field->ref = 1;
-
 	hist_field->hist_data = hist_data;
 
 	if (flags & HIST_FIELD_FL_EXPR || flags & HIST_FIELD_FL_ALIAS)
@@ -2223,10 +2212,8 @@ static struct hist_field *create_var_ref(struct hist_trigger_data *hist_data,
 	for (i = 0; i < hist_data->n_var_refs; i++) {
 		ref_field = hist_data->var_refs[i];
 		if (ref_field->var.idx == var_field->var.idx &&
-		    ref_field->var.hist_data == var_field->hist_data) {
-			get_hist_field(ref_field);
+		    ref_field->var.hist_data == var_field->hist_data)
 			return ref_field;
-		}
 	}
 	/* Sanity check to avoid out-of-bound write on 'hist_data->var_refs' */
 	if (hist_data->n_var_refs >= TRACING_MAP_VARS_MAX)
@@ -3276,7 +3263,6 @@ static struct hist_field *create_var(struct hist_trigger_data *hist_data,
 		goto out;
 	}
 
-	var->ref = 1;
 	var->flags = HIST_FIELD_FL_VAR;
 	var->var.idx = idx;
 	var->var.hist_data = var->hist_data = hist_data;

base-commit: 1fc5a74b108fc90951890ec513ac81869f5eaff1
-- 
2.53.0

Re: [PATCH] tracing: hist: free var refs regardless of how often they are referenced

[sashiko-bot]

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

Pre-existing issues:
- [Medium] Memory leak of a newly created `VAR_REF` hist_field when `init_var_ref()` fails in `create_var_ref()`.
--

commit 7e5deb418198a666953cde5c792cb003b22199c2
Author: Donggeun Yoo <donggeunyoo.kernel@gmail.com>

tracing: hist: free var refs regardless of how often they are referenced

This commit addresses a memory leak that occurs when the same variable is
referenced three or more times in a single hist trigger. It fixes this by
dropping the reference count and unconditionally freeing the object when the
trigger is removed.

> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 963e0d6b61fdf..f90680b33a373 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
[ ... ]
> @@ -2223,10 +2212,8 @@ static struct hist_field *create_var_ref(struct hist_trigger_data *hist_data,
>  	for (i = 0; i < hist_data->n_var_refs; i++) {
>  		ref_field = hist_data->var_refs[i];
>  		if (ref_field->var.idx == var_field->var.idx &&
> -		    ref_field->var.hist_data == var_field->hist_data) {
> -			get_hist_field(ref_field);
> +		    ref_field->var.hist_data == var_field->hist_data)
>  			return ref_field;
> -		}
>  	}
>  	/* Sanity check to avoid out-of-bound write on 'hist_data->var_refs' */
>  	if (hist_data->n_var_refs >= TRACING_MAP_VARS_MAX)

[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at
create_var_ref() in kernel/trace/trace_events_hist.c, does this code leak
a newly created hist_field when init_var_ref() fails?

The function create_var_ref() allocates ref_field with
HIST_FIELD_FL_VAR_REF set. If init_var_ref() fails, the error path calls
destroy_hist_field():

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

However, destroy_hist_field() has an early return for fields with
HIST_FIELD_FL_VAR_REF set, expecting them to be cleaned up via
hist_data->var_refs[]:

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

Since ref_field has not yet been added to the hist_data->var_refs[]
array at this point, it is bypassed by destroy_hist_field() and never
freed, leading to a memory leak.

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

Re: [PATCH] tracing: hist: free var refs regardless of how often they are referenced

[Donggeun Yoo]

Sashiko wrote:
> - [Medium] Memory leak of a newly created `VAR_REF` hist_field when
> `init_var_ref()` fails in `create_var_ref()`.

Confirmed, and it predates this patch.

The call that leaks was correct when 067fe038e70f ("tracing: Add variable
reference handling to hist triggers") added it, since destroy_hist_field()
freed var refs unconditionally then. It became a leak with 656fe2ba85e8
("tracing: Use hist trigger's var_ref array to destroy var_refs"), which
made destroy_hist_field() return early for HIST_FIELD_FL_VAR_REF while
create_var_ref() still reaches var_refs[] only after init_var_ref() has
succeeded.

Fix posted separately:
https://lore.kernel.org/linux-trace-kernel/20260906133352.3815019-1-donggeunyoo.kernel@gmail.com/