Re: [PATCH] perf: Synchronously cleanup child events
From: Peter Zijlstra
Date: Mon Jan 18 2016 - 09:44:21 EST
On Mon, Jan 18, 2016 at 02:37:06PM +0200, Alexander Shishkin wrote:
> Or how about this instead:
In principle better since it doesn't increase the lock hold times etc.,
but buggy I think:
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 8eb3fee429..cd9f1ac537 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3786,8 +3786,9 @@ int perf_event_release_kernel(struct perf_event *event)
>
> event->owner = NULL;
>
> +retry:
> /*
> - * event::child_mutex nests inside ctx::lock, so move children
> + * event::child_mutex nests inside ctx::mutex, so move children
> * to a safe place first and avoid inversion
> */
> mutex_lock(&event->child_mutex);
> @@ -3818,8 +3819,13 @@ int perf_event_release_kernel(struct perf_event *event)
> put_event(event);
> }
>
> - /* Must be the last reference */
> + /* Must be the last reference, .. */
> put_event(event);
> +
> + /* .. unless we raced with inherit_event(), in which case, repeat */
Nothing prevents @event from being freed here, which would make:
> + if (atomic_long_inc_not_zero(&event->refcount))
a use-after-free.
You'll have to do something like:
static bool put_event_last(struct perf_event *event, long value)
{
if (atomic_long_cmpxchg(&event->refcount, 1, 0)) {
__put_event(event); /* normal put_event() body */
return true;
}
return false;
}
with which you can do:
if (!put_event_last(event))
goto retry;