1. Patchew
  2. linux
  3. [PATCH 0/2] tracing/perf: Prevent double unregister of perf and tracepoint probes
  4. View patch

[PATCH 2/2] tracing: Prevent double unregister of tracepoint probes

Aditya Chillara posted 2 patches 7 months ago
There is a newer version of this series
[PATCH 2/2] tracing: Prevent double unregister of tracepoint probes
Posted by Aditya Chillara 7 months ago 
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.

Signed-off-by: Aditya Chillara <quic_achillar@quicinc.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++;
+			}
 		}
 	}
 
+	/* If there is nothing to delete, do not allow */
+	if (nr_del - nr_tp_stub_del == 0)
+		return ERR_PTR(-ENOENT);
+
 	/*
 	 * If probe is NULL, then nr_probes = nr_del = 0, and then the
 	 * entire entry will be removed.

-- 
2.34.1
Re: [PATCH 2/2] tracing: Prevent double unregister of tracepoint probes
Posted by Steven Rostedt 7 months ago 
[ Added Mathieu who is the author of the tracepoint code ]

On Wed, 9 Jul 2025 11:11:10 +0530
Aditya Chillara <quic_achillar@quicinc.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@quicinc.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.
>
Re: [PATCH 2/2] tracing: Prevent double unregister of tracepoint probes
Posted by Steven Rostedt 7 months ago 
On Wed, 9 Jul 2025 10:40:17 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> 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++;

That probably should be:

			 else if (old[nr_probes].func == tp_stub_func)
				nr_tp_stub_del++;

-- Steve
Re: [PATCH 2/2] tracing: Prevent double unregister of tracepoint probes
Posted by Mathieu Desnoyers 7 months ago 
On 2025-07-09 10:40, Steven Rostedt wrote:
> [ Added Mathieu who is the author of the tracepoint code ]
> 
> On Wed, 9 Jul 2025 11:11:10 +0530
> Aditya Chillara <quic_achillar@quicinc.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.
> 

Agreed.

>>
>> Signed-off-by: Aditya Chillara <quic_achillar@quicinc.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;
> 

Indeed func_remove() already returns ERR_PTR(-ENOENT) when
old is NULL at the beginning of the function, so its intent
is indeed to catch this kind of scenario. I agree with Steven's
recommended changes.

Thanks,

Mathieu

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


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.