lkml.org 
[lkml]   [2016]   [Jan]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] perf: Synchronously cleanup child events
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;



\
 
 \ /
  Last update: 2016-01-18 16:21    [W:0.091 / U:0.496 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site