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

[Peter Zijlstra]

On Fri, Jan 03, 2025 at 09:54:09AM +0530, Ravi Bangoria wrote:
> Hi Peter,
> 
> Sorry for the delay. Was on vacation.

Yeah, me too :-)

> Both of these are incorrect. They just reduce the race window, doesn't
> actually solve the race. Anyway, I could spot few other races:
> 
> 1) A race between event creation and perf_pmu_unregister(). Any event
>    create code path (perf_event_open(), perf_event_create_kernel_counter()
>    and inherit_event()) allocates event with perf_event_alloc() which adds
>    an event to the pmu->events list. However, the event is still immature,
>    for ex, event->ctx is still NULL. In the mean time, perf_pmu_unregister()
>    finds this event and tries to detach it.
> 
>    perf_event_open()                              perf_pmu_unregister()
>      event = perf_event_alloc()                     pmu_detach_event(event)
>        list_add(&event->pmu_list, &pmu->events);      perf_event_ctx_lock(event)
>        /*                                               perf_event_ctx_lock_nested(ctx)
>         * event->ctx is NULL.                             ctx = READ_ONCE(event->ctx); /* event->ctx is NULL */
>         */                                                if (!refcount_inc_not_zero(&ctx->refcount)) { /* Crash */
>      perf_install_in_context(ctx, event);

Ah, that puts the lie to the guard(srcu) comment there, doesn't it :/

So the intent was for that SRCU section to cover the creation, so that
perf_pmu_unregister() can take out the pmu to avoid creating more
events, then srcu-sync to wait on all in-progress creation and then go
detach everything.

I suppose the simplest thing here is to grow that SRCU section.

> 2) A race with perf_event_release_kernel(). perf_event_release_kernel()
>    prepares a separate "free_list" of all children events under ctx->mutex
>    and event->child_mutex. However, the "free_list" uses the same
>    "event->child_list" for entries. OTOH, perf_pmu_unregister() ultimately
>    calls __perf_remove_from_context() with DETACH_CHILD, which checks if
>    the event being removed is a child event, and if so, it will try to
>    detach the child from parent using list_del_init(&event->child_list);
>    i.e. two code path doing list_del on the same list entry.
> 
>    perf_event_release_kernel()                                        perf_pmu_unregister()
>      /* Move children events to free_list */                            ...
>      list_for_each_entry_safe(child, tmp, &free_list, child_list) {       perf_remove_from_context() /* with DETACH_CHILD */
>        ...                                                                  __perf_remove_from_context()
>        list_del(&child->child_list);                                          perf_child_detach()
>                                                                                 list_del_init(&event->child_list);

Bah, I had figured it was taken care of, because perf_event_exit_event()
has a similar race. I'll try and figure out what to do there.

> 3) A WARN(), not a race. perf_pmu_unregister() increments event->refcount
>    before detaching the event. If perf_pmu_unregister() picks up a child
>    event, perf_event_exit_event() called through perf_pmu_unregister()
>    will try to free it. Since event->refcount would be 2, free_event()
>    will trigger a WARN().
> 
>    perf_pmu_unregister()
>      event = pmu_get_event() /* event->refcount => 2 */
>        ...
>          perf_event_exit_event()
> 	   if (parent_event) { /* true, because `event` is a child */
> 	     free_event(event);
> 	       if (WARN(atomic_long_cmpxchg(&event->refcount, 1, 0) != 1,
>                         "unexpected event refcount: %ld; ptr=%p\n",
>                         atomic_long_read(&event->refcount), event))

I'll make that something like:

	if (revoke)
		put_event(event);
	else
		free_event(event);

or so.

> 4) A race with perf_event_set_bpf_prog(). perf_event_set_bpf_prog() might
>    be in process of setting event->prog, where as perf_pmu_unregister(),
>    which internally calls perf_event_free_bpf_prog(), will clear the
>    event->prog pointer.
> 
>    perf_pmu_unregister()                perf_event_set_bpf_prog()
>      ...                                  perf_event_set_bpf_handler()
>        perf_event_free_bpf_prog()           event->prog = prog;
>          event->prog = NULL;
> 
> I've yet to inspect other code paths, so there might be more races.

Weird, that should be serialized by perf_event_ctx_lock(), both
__pmu_detach_event() and _perf_ioctl() are called under that.

Thanks for going over this!