Re: [PATCH] perf/core: Fix double put of the system-wide perf_ctx_data reference

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] perf/core: memory leak of perf_ctx_data due to TOCTOU race in attach_global_ctx_data()
- [Medium] perf/core: data race on cd->global leading to undefined behavior
--

commit 6c40093668f0aa28205d435b7cecaca312682e03
Author: vineash <vineash30640@gmail.com>

perf/core: Fix double put of the system-wide perf_ctx_data reference

The reference on a task's perf_ctx_data that belongs to the system-wide
events (cd->global) can be put twice. __detach_global_ctx_data() puts it
when the last system-wide event goes away, and perf_event_exit_task()
puts it again when the task exits.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index a34ff4cb410da..63122c3b872ef 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -5498,8 +5498,9 @@ attach_global_ctx_data(struct kmem_cache *ctx_cache)
>  				continue;
>  			cd = rcu_dereference(p->perf_ctx_data);
>  			if (cd && !cd->global) {
> -				cd->global = 1;
> -				if (!refcount_inc_not_zero(&cd->refcount))
> +				if (refcount_inc_not_zero(&cd->refcount))
> +					cd->global = 1;

[Severity: High]
Does this introduce a TOCTOU race condition that can leak the perf_ctx_data
reference?

Earlier in attach_global_ctx_data(), the code checks if the task is exiting
without holding locks that exclude do_exit():

    if (p->flags & PF_EXITING)
        continue;

Could a task concurrently set PF_EXITING and call perf_event_exit_task(),
which then calls detach_task_global_ctx_data()?

If so, the exiting task would observe cd->global == 0 via xchg(), miss the
cleanup, and return. Then this code continues, increments cd->refcount, and
sets cd->global = 1. Because the exiting task has already bypassed the
cleanup phase, it will never drop this reference. Once the task is unhashed,
__detach_global_ctx_data() will not find it, resulting in a permanent
memory leak of the perf_ctx_data structure.

A local user with perf_event_paranoid=2 might trigger this by racing
CPU-wide perf event creation with task exits.

[Severity: Medium]
Does this plain store to cd->global race with the atomic xchg() operation in
detach_task_global_ctx_data() from an exiting task?

Because perf_event_exit_task() executes without holding global_ctx_data_rwsem,
these accesses appear unsynchronized, which violates the Linux Kernel Memory
Model and can lead to undefined behavior.

> +				else
>  					cd = NULL;
>  			}
>  			if (!cd) {

[ ... ]

> @@ -5569,19 +5570,33 @@ detach_task_ctx_data(struct task_struct *p)
>  		perf_free_ctx_data_rcu(cd);
>  }
>  
> +/*
> + * Drop the reference owned by the system-wide events, if @p still holds one.
> + *
> + * Both __detach_global_ctx_data() and perf_event_exit_task() may try to drop
> + * it concurrently; claiming ->global with xchg() makes sure only one of them
> + * does, otherwise the refcount is decremented twice.
> + */
> +static void detach_task_global_ctx_data(struct task_struct *p)
> +{
> +	struct perf_ctx_data *cd;
> +
> +	scoped_guard (rcu) {
> +		cd = rcu_dereference(p->perf_ctx_data);
> +		if (!cd || !xchg(&cd->global, 0))
> +			return;
> +	}
> +
> +	detach_task_ctx_data(p);
> +}

[ ... ]

> @@ -9459,8 +9474,8 @@ perf_event_alloc_task_data(struct task_struct *child,
>  		}
>  
>  		if (!cd->global) {
> -			cd->global = 1;
>  			refcount_inc(&cd->refcount);
> +			cd->global = 1;

[Severity: Medium]
Similar to the concern in attach_global_ctx_data(), does this plain store
to cd->global race with the atomic xchg() operation from an exiting task in
detach_task_global_ctx_data()?

>  		}
>  	}
>  

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179105852659.1844849.16381123619267169417@gmail.com?part=1