Re: [PATCH v3 2/2] perf/core: Disable context fast-swap for sigtrap events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] perf/core: non-SMP-atomic local_t race on ctx->nr_no_switch_fast
--

commit 6723e8dad1dd2932a4b047f419d5e1deb0db8c7c
Author: Tengda Wu <wutengda@huaweicloud.com>

perf/core: Disable context fast-swap for sigtrap events

This commit excludes events with attr.sigtrap from the context switch
fast-swap path by incrementing and decrementing ctx->nr_no_switch_fast in
list_add_event() and list_del_event(). This prevents ctx->task from being
swapped to another task while a sigtrap event is attached to the context.

> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 5842552294c19..1d4b16dc190db 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -1067,10 +1067,8 @@ struct perf_event_context {
>  	 * should be avoided.
>  	 *
>  	 * Sum (event->pending_work + events with
> -	 *    (attr->inherit && (attr->sample_type & PERF_SAMPLE_READ)))
> -	 *
> -	 * The SIGTRAP is targeted at ctx->task, as such it won't do changing
> -	 * that until the signal is delivered.
> +	 *    (attr->inherit && (attr->sample_type & PERF_SAMPLE_READ)) +
> +	 *    events with attr->sigtrap)
>  	 */
>  	local_t				nr_no_switch_fast;
>  };
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 7aac14663c80c..6964cd1e6bfec 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -1999,7 +1999,7 @@ list_add_event(struct perf_event *event, struct perf_event_context *ctx)
>  		ctx->nr_user++;
>  	if (event->attr.inherit_stat)
>  		ctx->nr_stat++;
> -	if (has_inherit_and_sample_read(&event->attr))
> +	if (has_inherit_and_sample_read(&event->attr) || event->attr.sigtrap)
>  		local_inc(&ctx->nr_no_switch_fast);
>  
>  	if (event->state > PERF_EVENT_STATE_OFF)
> @@ -2225,7 +2225,7 @@ list_del_event(struct perf_event *event, struct perf_event_context *ctx)
>  		ctx->nr_user--;
>  	if (event->attr.inherit_stat)
>  		ctx->nr_stat--;
> -	if (has_inherit_and_sample_read(&event->attr))
> +	if (has_inherit_and_sample_read(&event->attr) || event->attr.sigtrap)
>  		local_dec(&ctx->nr_no_switch_fast);

[Severity: High]
Can incrementing and decrementing ctx->nr_no_switch_fast here race across
CPUs because nr_no_switch_fast is a local_t rather than an atomic_t?

In struct perf_event_context, nr_no_switch_fast is declared as local_t,
where local_inc() and local_dec() are only CPU-local atomic (for example,
unlocked incq/decq on x86) and do not serialize concurrent read-modify-write
updates across different CPUs.

When installing a disabled sigtrap event into a non-empty task context,
perf_install_in_context() takes the fast path on the caller's CPU without
sending an IPI to the CPU running ctx->task:

kernel/events/core.c:perf_install_in_context() {
	...
	if (__perf_effective_state(event) == PERF_EVENT_STATE_OFF &&
	    ctx->nr_events && !is_cgroup_event(event)) {
		raw_spin_lock_irq(&ctx->lock);
		if (ctx->task == TASK_TOMBSTONE) {
			raw_spin_unlock_irq(&ctx->lock);
			return;
		}
		add_event_to_ctx(event, ctx);
		raw_spin_unlock_irq(&ctx->lock);
		return;
	}
	...
}

This runs add_event_to_ctx() -> list_add_event() ->
local_inc(&ctx->nr_no_switch_fast) on the installer CPU under ctx->lock.

Concurrently, on the CPU where ctx->task is running, an already-active
sigtrap event in the same context can overflow in __perf_event_overflow() or
complete task_work in perf_pending_task(), both of which modify
ctx->nr_no_switch_fast without holding ctx->lock:

kernel/events/core.c:__perf_event_overflow() {
	...
			if (!task_work_add(current, &event->pending_task, notify_mode)) {
				local_inc(&event->ctx->nr_no_switch_fast);
	...
}

kernel/events/core.c:perf_pending_task() {
	...
	if (event->pending_work) {
		event->pending_work = 0;
		perf_sigtrap(event);
		local_dec(&event->ctx->nr_no_switch_fast);
	}
	...
}

Similarly, if ctx becomes inactive (!ctx->is_active) while ctx->task still
has pending task_work in flight, perf_remove_from_context() ->
__perf_remove_from_context() -> list_del_event() can execute
local_dec(&ctx->nr_no_switch_fast) on a remote CPU concurrently with
perf_pending_task() executing local_dec(&event->ctx->nr_no_switch_fast) on
ctx->task's CPU.

Could these concurrent cross-CPU local_inc() and local_dec() operations lose
updates to ctx->nr_no_switch_fast, allowing the counter to drop to zero
while a sigtrap event is still attached and re-enabling the fast-swap path?
Should nr_no_switch_fast be converted from local_t to atomic_t?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793870900.git.wutengda@huaweicloud.com?part=2