Re: [PATCH v2 24/24] perf: Make perf_pmu_unregister() useable

[Peter Zijlstra]

On Mon, Feb 10, 2025 at 12:12:40PM +0530, Ravi Bangoria wrote:
> > @@ -13467,7 +13665,13 @@ perf_event_exit_event(struct perf_event
> >  		 * Kick perf_poll() for is_event_hup();
> >  		 */
> >  		perf_event_wakeup(parent_event);
> > -		free_event(event);
> > +		/*
> > +		 * pmu_detach_event() will have an extra refcount.
> > +		 */
> > +		if (revoke)
> > +			put_event(event);
> > +		else
> > +			free_event(event);
> >  		put_event(parent_event);
> >  		return;
> >  	}
> 
> There is a race between do_exit() and perf_pmu_unregister():
> 
> Assume: a Parent process with 'event1' and a Child process with an
> inherited 'event2'.
> 
>                   CPU 1                                                       CPU 2
> 
>       Child process exiting ...
>    1  do_exit()
>    2    perf_event_exit_task()
>    3      perf_event_exit_task_context()
>    4        mutex_lock(&child_ctx->mutex);
>    5        list_for_each_entry_safe(&child_ctx->event_list)
>    6        /* picks event2. event2->refcount is 1 */
>    7                                                             perf_pmu_unregister()
>    8                                                               pmu_detach_events()
>    9                                                                 pmu_get_event(pmu) /* Picks event2 */
>   10                                                                   atomic_long_inc_not_zero(&event->refcount) /* event2 */
>   11                                                                   /* event2->refcount becomes 2 */
>   12                                                                 pmu_detach_event() /* event2 */
>   13                                                                   perf_event_ctx_lock(); /* event2 */
>   14        /* event2->refcount became 2 (by CPU 2) */                 .
>   15      perf_event_exit_event() /* event2 */                         . /* CPU 1 is holding ctx->mutex of
>   16        if (parent_event) { /* true */                             .    child process. Waiting ... */
>   17          if (revoke) /* false */                                  .
>   18          else                                                     .
>   19              free_event();  /* event2 */                          .
>   20                  WARN()                                           .
> 
>                       ^^^^^^
> 
> This race manifests as:
> 
>   unexpected event refcount: 2; ptr=00000000c6ca2049
>   WARNING: CPU: 57 PID: 9729 at kernel/events/core.c:5439 free_event+0x48/0x60
>   RIP: 0010:free_event+0x48/0x60
>   Call Trace:
>    ? free_event+0x48/0x60
>    perf_event_exit_event.isra.0+0x60/0xf0
>    perf_event_exit_task+0x1e1/0x280
>    do_exit+0x327/0xb00
> 
> The race continues... now, with the stale child2->parent pointer:
> 
>                   CPU 1                                                       CPU 2
> 
>   15      perf_event_exit_event() /* event2 */                         . /* CPU 1 is holding ctx->mutex of
>   16        if (parent_event) { /* true */                             .    child process. Waiting ... */
>   17          if (revoke) /* false */                                  .
>   18          else                                                     .
>   19              free_event();  /* event2 */                          .
>   20                  WARN()                                           .
>   21          put_event(parent_event); /* event1 */                    .
>   22          /* event1->refcount becomes 1 */                         .
>   23        }                                                          .
>   24        +- mutex_unlock(&child_ctx->mutex);                        . /* Acquired child's ctx->mutex */
>   25                                                                   __pmu_detach_event() /* event2 */
>   26                                                                     perf_event_exit_event() /* event2 */
>   27                                                                       if (parent_event) { /* true, BUT FALSE POSITIVE. */
>   28                                                                         if (revoke) /* true */
>   29                                                                             put_event(); /* event2 */
>   30                                                                             /* event2->refcount becomes 1 */
>   31                                                                         put_event(parent_event); /* event1 */
>   32                                                                         /* event1->refcount becomes 0 */
> 
>                                                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> This can manifest as some random bug. I guess, perf_event_exit_event()
> should also check for (event->attach_state & PERF_ATTACH_CHILD) when
> event->parent != NULL.

Does this work?

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2303,6 +2303,7 @@ static void perf_child_detach(struct per
 
 	sync_child_event(event);
 	list_del_init(&event->child_list);
+	event->parent = NULL;
 }
 
 static bool is_orphaned_event(struct perf_event *event)