linux-kernel - Re: [PATCH 2/2] tracing: Prevent double unregister of tracepoint probes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250709104017.69190334@batman.local.home>
Date: Wed, 9 Jul 2025 10:40:17 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Aditya Chillara <quic_achillar@...cinc.com>
Cc: Ingo Molnar <mingo@...hat.com>, <linux-kernel@...r.kernel.org>, Mathieu
 Desnoyers <mathieu.desnoyers@...icios.com>
Subject: Re: [PATCH 2/2] tracing: Prevent double unregister of tracepoint
 probes

[ Added Mathieu who is the author of the tracepoint code ]

On Wed, 9 Jul 2025 11:11:10 +0530
Aditya Chillara <quic_achillar@...cinc.com> wrote:

> Prevent tracepoint_probe_unregister from being executed multiple times
> for the same probe, which can cause issues with perf due to the lack
> of error handling.
> 
> Return an error if the probe is not present in the list of probes.

This patch even shows that the first patch is fixing a symptom.

Yes, I agree with this patch (with some cleanups below), but there
should be no reason for perf to be ever calling unreg() if it doesn't
have a tracepoint registered. Something else got screwed up in the mean
time.

> 
> Signed-off-by: Aditya Chillara <quic_achillar@...cinc.com>
> ---
>  kernel/tracepoint.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index ef42c1a1192053cc05b45ccb61358a4996453add..e6eee7e44a9d6f4f19114fbcf8fd9e5c85075324 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -232,7 +232,7 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
>  static void *func_remove(struct tracepoint_func **funcs,
>  		struct tracepoint_func *tp_func)
>  {
> -	int nr_probes = 0, nr_del = 0, i;
> +	int nr_probes = 0, nr_del = 0, nr_tp_stub_del = 0, i;
>  	struct tracepoint_func *old, *new;
>  
>  	old = *funcs;
> @@ -246,11 +246,18 @@ static void *func_remove(struct tracepoint_func **funcs,
>  		for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
>  			if ((old[nr_probes].func == tp_func->func &&
>  			     old[nr_probes].data == tp_func->data) ||
> -			    old[nr_probes].func == tp_stub_func)
> +			    old[nr_probes].func == tp_stub_func) {
> +				if (old[nr_probes].func == tp_stub_func)
> +					nr_tp_stub_del++;
>  				nr_del++;
> +			}

I would make this a bit cleaner by:

 			if ((old[nr_probes].func == tp_func->func &&
 			     old[nr_probes].data == tp_func->data))
				nr_del++;

			 if (old[nr_probes].func == tp_stub_func)
				nr_tp_stub_del++;
>  		}
>  	}
>  
> +	/* If there is nothing to delete, do not allow */
> +	if (nr_del - nr_tp_stub_del == 0)
> +		return ERR_PTR(-ENOENT);

	if (!nr_del)
		return ERR_PTR(-ENOENT);

	nr_del += nr_tp_stub_del;

-- Steve

> +
>  	/*
>  	 * If probe is NULL, then nr_probes = nr_del = 0, and then the
>  	 * entire entry will be removed.
>