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

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

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