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

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

[Aditya Chillara]

Double perf_trace_event_unreg is allowed causing perf_refcount to go
negative. total_ref_count also goes negative because the return value
of tracepoint_probe_unregister is ignored.

Once total_ref_count is negative, the next call to perf_trace_event_reg
will register perf_probe but will not allocate perf_trace_buf and sets
it to NULL instead.

The subsequent trace_##call() will mem abort in perf_trace_buf_alloc
because memset will be called on the NULL perf_trace_buf.

tracepoint_remove_func returns error after the warning:

[  111.552262][ T8418] WARNING: CPU: 4 PID: 8418 at kernel/tracepoint.c:405 tracepoint_probe_unregister+0xb0/0x468
.
.
[  111.552831][ T8418] CPU: 4 UID: 1779017056 PID: 8418 Comm: trinity-c19 Tainted: G        W  OE 6.12.23-android16-5-maybe-dirty-debug #1   2362fed3965ed146e5e20aa901c0fae1990ed57d
.
.
[  111.552881][ T8418] Call trace:
[  111.552882][ T8418]  tracepoint_probe_unregister+0xb0/0x468
[  111.552885][ T8418]  trace_event_reg+0x5c/0xa4
[  111.552887][ T8418]  perf_trace_event_unreg+0x58/0xd4
[  111.552889][ T8418]  perf_trace_destroy+0x5c/0x94
[  111.552891][ T8418]  tp_perf_event_destroy+0x10/0x20
[  111.552895][ T8418]  __free_event+0x90/0x154
[  111.552896][ T8418]  perf_event_alloc+0x478/0x684
[  111.552898][ T8418]  __arm64_sys_perf_event_open+0x278/0x7d8
[  111.552900][ T8418]  invoke_syscall+0x58/0x10c

and then mem abort on trace_##call():

[  189.790824][    C4] CPU: 4 UID: 8388678 PID: 8422 Comm: trinity-c23 Tainted: G        W  OE      6.12.23-android16-5-maybe-dirty-debug #1 2362fed3965ed146e5e20aa901c0fae1990ed57d
.
.
[  189.790862][    C4] Call trace:
[  189.790863][    C4]  perf_trace_buf_alloc+0xb4/0x104
.
.
[  189.790894][    C4]  trace_preempt_off+0x138/0x140
[  189.790895][    C4]  preempt_count_add+0xa8/0x13c
[  189.790897][    C4]  copy_page_from_iter_atomic+0xa4/0x7e4
[  189.790900][    C4]  generic_perform_write+0x170/0x350
[  189.790904][    C4]  f2fs_file_write_iter+0x268/0xb64
[  189.790908][    C4]  vfs_write+0x340/0x3ac
[  189.790910][    C4]  ksys_write+0x78/0xe8
[  189.790911][    C4]  __arm64_sys_write+0x1c/0x2c
[  189.790912][    C4]  invoke_syscall+0x58/0x10c
.
.
[  189.797921][    C4] ---[ end trace 0000000000000000 ]---
[  189.797923][    C4] Kernel panic - not syncing: Oops: Fatal exception
in interrupt

Fix the issue by preventing double remove in tracepoint_remove_func
and gracefully handling the error in perf_trace_event_unreg.

Signed-off-by: Aditya Chillara <quic_achillar@quicinc.com>
---
---
Aditya Chillara (2):
      tracing/perf: Prevent double unregister of perf probes
      tracing: Prevent double unregister of tracepoint probes

 kernel/trace/trace_event_perf.c |  8 ++++++--
 kernel/trace/trace_events.c     |  3 +--
 kernel/tracepoint.c             | 11 +++++++++--
 3 files changed, 16 insertions(+), 6 deletions(-)
---
base-commit: 70575e77839f4c5337ce2653b39b86bb365a870e
change-id: 20250704-fix-double-perf-probe-unregister-ab98912f521b

Best regards,
-- 
Aditya Chillara <quic_achillar@quicinc.com>

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

[Aditya Chillara]

Double perf_trace_event_unreg is allowed causing perf_refcount to go
negative. total_ref_count also goes negative because the return value
of tracepoint_probe_unregister is ignored.

Once total_ref_count is negative, the next call to perf_trace_event_reg
will register perf_probe but will not allocate perf_trace_buf and sets
it to NULL instead.

The subsequent trace_##call() will mem abort in perf_trace_buf_alloc
because memset will be called on the NULL perf_trace_buf.

Gracefully handle the error in perf_trace_event_unreg to prevent
double unregister.

Signed-off-by: Aditya Chillara <quic_achillar@quicinc.com>
---
 kernel/trace/trace_event_perf.c | 8 ++++++--
 kernel/trace/trace_events.c     | 3 +--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index 61e3a2620fa3c9417ac23cf5a18aeb86e7393dcc..247db88accd88eb0acf3692ea593d576519ce8b1 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -154,12 +154,16 @@ static int perf_trace_event_reg(struct trace_event_call *tp_event,
 static void perf_trace_event_unreg(struct perf_event *p_event)
 {
 	struct trace_event_call *tp_event = p_event->tp_event;
-	int i;
+	int i, ret;
 
 	if (--tp_event->perf_refcount > 0)
 		return;
 
-	tp_event->class->reg(tp_event, TRACE_REG_PERF_UNREGISTER, NULL);
+	ret = tp_event->class->reg(tp_event, TRACE_REG_PERF_UNREGISTER, NULL);
+	if (ret) {
+		++tp_event->perf_refcount;
+		return;
+	}
 
 	/*
 	 * Ensure our callback won't be called anymore. The buffers
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 0356cae0cf74e79075f607bc841df05568688baa..50e0e08b29aa6617a04b191419ad1e587adf69fe 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -530,10 +530,9 @@ int trace_event_reg(struct trace_event_call *call,
 						 call->class->perf_probe,
 						 call);
 	case TRACE_REG_PERF_UNREGISTER:
-		tracepoint_probe_unregister(call->tp,
+		return tracepoint_probe_unregister(call->tp,
 					    call->class->perf_probe,
 					    call);
-		return 0;
 	case TRACE_REG_PERF_OPEN:
 	case TRACE_REG_PERF_CLOSE:
 	case TRACE_REG_PERF_ADD:

-- 
2.34.1

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

[Steven Rostedt]

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

> Double perf_trace_event_unreg is allowed causing perf_refcount to go
> negative. total_ref_count also goes negative because the return value
> of tracepoint_probe_unregister is ignored.
> 
> Once total_ref_count is negative, the next call to perf_trace_event_reg
> will register perf_probe but will not allocate perf_trace_buf and sets
> it to NULL instead.
> 
> The subsequent trace_##call() will mem abort in perf_trace_buf_alloc
> because memset will be called on the NULL perf_trace_buf.
> 
> Gracefully handle the error in perf_trace_event_unreg to prevent
> double unregister.
> 
> Signed-off-by: Aditya Chillara <quic_achillar@quicinc.com>
> ---
>  kernel/trace/trace_event_perf.c | 8 ++++++--
>  kernel/trace/trace_events.c     | 3 +--
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> index 61e3a2620fa3c9417ac23cf5a18aeb86e7393dcc..247db88accd88eb0acf3692ea593d576519ce8b1 100644
> --- a/kernel/trace/trace_event_perf.c
> +++ b/kernel/trace/trace_event_perf.c
> @@ -154,12 +154,16 @@ static int perf_trace_event_reg(struct trace_event_call *tp_event,
>  static void perf_trace_event_unreg(struct perf_event *p_event)
>  {
>  	struct trace_event_call *tp_event = p_event->tp_event;
> -	int i;
> +	int i, ret;
>  
>  	if (--tp_event->perf_refcount > 0)
>  		return;
>  
> -	tp_event->class->reg(tp_event, TRACE_REG_PERF_UNREGISTER, NULL);
> +	ret = tp_event->class->reg(tp_event, TRACE_REG_PERF_UNREGISTER, NULL);

The only time unreg() fails is when it doesn't find a tracepoint to
unregister.

There should be no reason to check the return value of unregister if
you have your accounting correct. Thus I think you are fixing a symptom
of a bug elsewhere.

-- Steve


> +	if (ret) {
> +		++tp_event->perf_refcount;
> +		return;
> +	}
>  

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

[Aditya Chillara]

On 7/9/2025 7:53 PM, Steven Rostedt wrote:
> On Wed, 9 Jul 2025 11:11:09 +0530
> Aditya Chillara <quic_achillar@quicinc.com> wrote:
> 
>> Double perf_trace_event_unreg is allowed causing perf_refcount to go
>> negative. total_ref_count also goes negative because the return value
>> of tracepoint_probe_unregister is ignored.
>>
>> Once total_ref_count is negative, the next call to perf_trace_event_reg
>> will register perf_probe but will not allocate perf_trace_buf and sets
>> it to NULL instead.
>>
>> The subsequent trace_##call() will mem abort in perf_trace_buf_alloc
>> because memset will be called on the NULL perf_trace_buf.
>>
>> Gracefully handle the error in perf_trace_event_unreg to prevent
>> double unregister.
>>
>> Signed-off-by: Aditya Chillara <quic_achillar@quicinc.com>
>> ---
>>  kernel/trace/trace_event_perf.c | 8 ++++++--
>>  kernel/trace/trace_events.c     | 3 +--
>>  2 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
>> index 61e3a2620fa3c9417ac23cf5a18aeb86e7393dcc..247db88accd88eb0acf3692ea593d576519ce8b1 100644
>> --- a/kernel/trace/trace_event_perf.c
>> +++ b/kernel/trace/trace_event_perf.c
>> @@ -154,12 +154,16 @@ static int perf_trace_event_reg(struct trace_event_call *tp_event,
>>  static void perf_trace_event_unreg(struct perf_event *p_event)
>>  {
>>  	struct trace_event_call *tp_event = p_event->tp_event;
>> -	int i;
>> +	int i, ret;
>>  
>>  	if (--tp_event->perf_refcount > 0)
>>  		return;
>>  
>> -	tp_event->class->reg(tp_event, TRACE_REG_PERF_UNREGISTER, NULL);
>> +	ret = tp_event->class->reg(tp_event, TRACE_REG_PERF_UNREGISTER, NULL);
> 
> The only time unreg() fails is when it doesn't find a tracepoint to
> unregister.
> 
> There should be no reason to check the return value of unregister if
> you have your accounting correct. Thus I think you are fixing a symptom
> of a bug elsewhere.

The exact problem was introduced by:
https://github.com/torvalds/linux/commit/7ef5aa081f989ecfecc1df02068a80aebbd3ec31
(perf/core: Simplify the perf_event_alloc() error path)
where __free_event was calling event->destroy() even though it would
have been called by perf_try_init_event in case it failed.

This was fixed by:
https://github.com/torvalds/linux/commit/da02f54e81db2f7bf6af9d1d0cfc5b41ec6d0dcb
(perf/core: Clean up perf_try_init_event())

This patch prevents from crashing even if that happens, and there
will be a warning anyway to notice the double unregister.

> 
> -- Steve
> 
> 
>> +	if (ret) {
>> +		++tp_event->perf_refcount;
>> +		return;
>> +	}
>>  

Best Regards,
Aditya

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

[Steven Rostedt]

On Wed, 9 Jul 2025 22:20:00 +0530
Aditya Chillara <quic_achillar@quicinc.com> wrote:

> The exact problem was introduced by:
> https://github.com/torvalds/linux/commit/7ef5aa081f989ecfecc1df02068a80aebbd3ec31
> (perf/core: Simplify the perf_event_alloc() error path)
> where __free_event was calling event->destroy() even though it would
> have been called by perf_try_init_event in case it failed.

Then I rather have it trigger a WARN_ON() and disable that event
permanently until reboot. It's a bug, no need to continue using the
event when it's in an a bad state.

-- Steve

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

[Aditya Chillara]

On 7/9/2025 10:48 PM, Steven Rostedt wrote:
>> The exact problem was introduced by:
>> https://github.com/torvalds/linux/commit/7ef5aa081f989ecfecc1df02068a80aebbd3ec31
>> (perf/core: Simplify the perf_event_alloc() error path)
>> where __free_event was calling event->destroy() even though it would
>> have been called by perf_try_init_event in case it failed.
> Then I rather have it trigger a WARN_ON() and disable that event
> permanently until reboot. It's a bug, no need to continue using the
> event when it's in an a bad state.

Acknowledged, will update in v3.

Best Regards,
Aditya

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

[Aditya Chillara]

[ Added Peter for 7ef5aa081f98 and kernel/events/core.c ]

On 7/9/2025 10:48 PM, Steven Rostedt wrote:
> On Wed, 9 Jul 2025 22:20:00 +0530
> Aditya Chillara <quic_achillar@quicinc.com> wrote:
> 
>> The exact problem was introduced by:
>> https://github.com/torvalds/linux/commit/7ef5aa081f989ecfecc1df02068a80aebbd3ec31
>> (perf/core: Simplify the perf_event_alloc() error path)
>> where __free_event was calling event->destroy() even though it would
>> have been called by perf_try_init_event in case it failed.
> 
> Then I rather have it trigger a WARN_ON() and disable that event
> permanently until reboot. It's a bug, no need to continue using the
> event when it's in an a bad state.

perf_trace_event_unreg is called only in event->destroy(), and this is
called in event free path; either after the event is removed from its
context, or before the event is installed in a context. I believe there
is no need to explicitly disable the perf event here because it must
have been disabled already. Please let me know if I'm missing anything.

Best Regards,
Aditya

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

[Aditya Chillara]

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

[Steven Rostedt]

[ 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

[Steven Rostedt]

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

[Mathieu Desnoyers]

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