Re: [RFC PATCH] perf: Paper over the hw.target problems

[Mark Rutland]

Hi Alex,

On Thu, Feb 28, 2019 at 04:01:09PM +0200, Alexander Shishkin wrote:
> First, we have a race between perf_event_release_kernel() and
> perf_free_event(), which happens when parent's event is released while the
> child's fork fails (because of a fatal signal, for example), that looks
> like this:
> 
> cpu X                            cpu Y
> -----                            -----
>                                  copy_process() error path
> perf_release(parent)             +->perf_event_free_task()
> +-> lock(child_ctx->mutex)       |  |
> +-> remove_from_context(child)   |  |
> +-> unlock(child_ctx->mutex)     |  |
> |                                |  +-> lock(child_ctx->mutex)
> |                                |  +-> unlock(child_ctx->mutex)
> |                                +-> free_task(child_task)
> +-> put_task_struct(child_task)
> 
> Technically, we're still holding a reference to the task via
> parent->hw.target, that's not stopping free_task(), so we end up poking at
> free'd memory, as is pointed out by KASAN in the syzkaller report (see Link
> below). The straightforward fix is to drop the hw.target reference while
> the task is still around.

I've recently started hitting this on arm64, and had come to the same
conclusion.

> Therein lies the second problem: the users of hw.target (uprobe) assume
> that it's around at ->destroy() callback time, where they use it for
> context. So, in order to not break the uprobe teardown and avoid leaking
> stuff, we need to call ->destroy() at the same time.

I had not spotted that case. That's rather horrid. :/

> This patch fixes the race and the subsequent fallout by doing both these
> things at remove_from_context time.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> Link: https://syzkaller.appspot.com/bug?extid=a24c397a29ad22d86c98
> Reported-by: syzbot+a24c397a29ad22d86c98@xxxxxxxxxxxxxxxxxxxxxxxxx
> ---
>  kernel/events/core.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 36b8320590e8..640695d114f8 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -2105,6 +2105,27 @@ static void perf_remove_from_context(struct perf_event *event, unsigned long fla
>  
>  	event_function_call(event, __perf_remove_from_context, (void *)flags);
>  
> +	/*
> +	 * This is as passable as any hw.target handling out there;
> +	 * hw.target implies task context, therefore, no migration.
> +	 * Which means that we can only get here at the teardown.
> +	 */
> +	if (event->hw.target) {
> +		/*
> +		 * Now, the problem with, say uprobes, is that they
> +		 * use hw.target for context in their ->destroy()
> +		 * callbacks. Supposedly, they may need to poke at
> +		 * its contents, so better call it while we still
> +		 * have the task.
> +		 */
> +		if (event->destroy) {
> +			event->destroy(event);
> +			event->destroy = NULL;
> +		}
> +		put_task_struct(event->hw.target);
> +		event->hw.target = NULL;
> +	}

We also use perf_remove_from_context() in perf_event_open() when we move
events from a SW context to a HW context, so we can't destroy the event
here.

I think we need something more like the below (untested), but I fear
that it's not safe to call perf_event::destroy() in this context.

Thanks,
Mark.

---->8----
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 26d6edab051a..b32f2cac5563 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4532,6 +4532,24 @@ static void put_event(struct perf_event *event)
        _free_event(event);
 }
 
+void perf_event_detach_target(struct perf_event *event)
+{
+       if (!event->hw.target)
+               return;
+
+       /*
+        * The uprobes perf_event::destroy() callback needs the target, so call
+        * that while the target is still valid.
+        */
+       if (event->destroy) {
+               event->destroy(event);
+               event->destroy = NULL;
+       }
+
+       put_task_struct(event->hw.target);
+       event->hw.target = NULL;
+}
+
 /*
  * Kill an event dead; while event:refcount will preserve the event
  * object, it will not preserve its functionality. Once the last 'user'
@@ -4559,6 +4577,7 @@ int perf_event_release_kernel(struct perf_event *event)
        ctx = perf_event_ctx_lock(event);
        WARN_ON_ONCE(ctx->parent_ctx);
        perf_remove_from_context(event, DETACH_GROUP);
+       perf_event_detach_target(event);
 
        raw_spin_lock_irq(&ctx->lock);
        /*
@@ -4614,6 +4633,7 @@ int perf_event_release_kernel(struct perf_event *event)
                                               struct perf_event, child_list);
                if (tmp == child) {
                        perf_remove_from_context(child, DETACH_GROUP);
+                       perf_event_detach_target(child);
                        list_move(&child->child_list, &free_list);
                        /*
                         * This matches the refcount bump in inherit_event();