Re: [PATCH] perf/core: Prevent duplicate sigtrap task work enqueue

[Tengda Wu]

Hi,

After applying the following patch, we found another warning that
is unrelated to the current fix patch, but both are related to
perf sigtrap. I will explain it together in the v3 version.

Thanks,
Tengda


On 2026/10/31 19:45, Tengda Wu wrote:
> The sigtrap handling in __perf_event_overflow() checks
> event->pending_work before calling task_work_add(), but only sets
> pending_work after the task work has been queued.
> 
> This leaves a window where an NMI can interrupt task_work_add()
> and re-enter __perf_event_overflow() for the same event. Since
> pending_work is still zero, the nested invocation can enqueue
> the same pending_task again.
> 
> The re-entrant call path is:
> 
>   __perf_event_overflow()
>     task_work_add()
>       kasan_record_aux_stack()
>         <NMI>
>           __perf_event_overflow()
>             task_work_add()
> 
> The nested invocation can enqueue pending_task before the
> outer invocation does. When the outer invocation resumes, it
> inserts the same task_work entry again, potentially creating
> a cycle in the task work list.
> 
> This causes perf_pending_task() to be executed repeatedly
> and leads to unbalanced event reference counting.
> 
> Use cmpxchg() to atomically claim pending_work before calling
> task_work_add(). This ensures that only one invocation can
> enqueue the pending_task, even when interrupted by an NMI.
> 
> If task_work_add() fails, clear the pending_work claim so that
> a subsequent overflow can retry.
> 
> Also use READ_ONCE() when checking pending_work in the
> exclude_kernel warning path to avoid an unsynchronized read.
> 
> Fixes: c5d93d23a260 ("perf: Enqueue SIGTRAP always via task_work.")
> Signed-off-by: Tengda Wu <wutengda@xxxxxxxxxxxxxxx>
> ---
>  kernel/events/core.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 457d75ea7142..f1e4e153c49a 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -10925,16 +10925,18 @@ static int __perf_event_overflow(struct perf_event *event,
>  
>  		notify_mode = in_nmi() ? TWA_NMI_CURRENT : TWA_RESUME;
>  
> -		if (!event->pending_work &&
> -		    !task_work_add(current, &event->pending_task, notify_mode)) {
> -			event->pending_work = pending_id;
> -			local_inc(&event->ctx->nr_no_switch_fast);
> -			WARN_ON_ONCE(!atomic_long_inc_not_zero(&event->refcount));
> -
> -			event->pending_addr = 0;
> -			if (valid_sample && (data->sample_flags & PERF_SAMPLE_ADDR))
> -				event->pending_addr = data->addr;
> -
> +		if (cmpxchg(&event->pending_work, 0, pending_id) == 0) {
> +			if (!task_work_add(current, &event->pending_task, notify_mode)) {
> +				event->pending_work = pending_id;
> +				local_inc(&event->ctx->nr_no_switch_fast);
> +				WARN_ON_ONCE(!atomic_long_inc_not_zero(&event->refcount));
> +
> +				event->pending_addr = 0;
> +				if (valid_sample && (data->sample_flags & PERF_SAMPLE_ADDR))
> +					event->pending_addr = data->addr;
> +			} else {
> +				cmpxchg(&event->pending_work, pending_id, 0);
> +			}
>  		} else if (event->attr.exclude_kernel && valid_sample) {
>  			/*
>  			 * Should not be able to return to user space without
> @@ -10948,7 +10950,7 @@ static int __perf_event_overflow(struct perf_event *event,
>  			 *     To approximate progress (with false negatives),
>  			 *     check 32-bit hash of the current IP.
>  			 */
> -			WARN_ON_ONCE(event->pending_work != pending_id);
> +			WARN_ON_ONCE(READ_ONCE(event->pending_work) != pending_id);
>  		}
>  	}
>