Re: [PATCH] perf: Hold exec lock until owner-list publication

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] perf: incomplete fix in perf_event_open() races on fork and
  context swap
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] perf: ABBA deadlock in perf_remove_from_owner() [1]
--

commit edd7543b84d380d1d23aa263c8f4c123c4e044c9
Author: Felix Hoffmann <f3lix.dev@gmx.de>

perf: Hold exec lock until owner-list publication

This commit keeps task->signal->exec_update_lock held in perf_event_open()
until the new event is added to current->perf_event_list, preventing a
concurrent target exec from calling perf_remove_from_owner() before
event->owner_entry is initialized.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 614f9f0e65005..315edee9bf3c3 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -14428,15 +14428,15 @@ SYSCALL_DEFINE5(perf_event_open,
>  
>  	mutex_unlock(&ctx->mutex);
>  
> +	mutex_lock(&current->perf_event_mutex);
> +	list_add_tail(&event->owner_entry, &current->perf_event_list);
> +	mutex_unlock(&current->perf_event_mutex);
> +
>  	if (task) {
>  		up_read(&task->signal->exec_update_lock);
>  		put_task_struct(task);
>  	}
>  
> -	mutex_lock(&current->perf_event_mutex);
> -	list_add_tail(&event->owner_entry, &current->perf_event_list);
> -	mutex_unlock(&current->perf_event_mutex);
> -
>  	/*
>  	 * File reference in group guarantees that group_leader has been
>  	 * kept alive until we place the new event on the sibling_list.

[Severity: High]
Can perf_remove_from_owner() still reach an uninitialized
event->owner_entry if the target task forks and swaps contexts before
perf_event_open() acquires current->perf_event_mutex?

Holding task->signal->exec_update_lock blocks execve() on the target task,
but does not block fork().  When an event has both inherit and
remove_on_exec set, the target task can fork right after perf_unpin_context()
and mutex_unlock(&ctx->mutex), swap ctx into the child during context
switch via perf_event_context_sched_out(), and let the child exec under its
own signal->exec_update_lock:

CPU0 (opener)                     CPU1 (target task -> child)
perf_event_open()
  event->owner = current
  perf_install_in_context(ctx, event, ...)
  perf_unpin_context(ctx)
  mutex_unlock(&ctx->mutex)
                                  copy_process()
                                    perf_event_init_context()
                                  perf_event_context_sched_out()
                                    /* swaps ctx into child */
                                  execve() [in child]
                                    ...
                                      perf_event_remove_on_exec()
                                        perf_remove_from_owner(event)
                                          list_del_init(&event->owner_entry)
                                            <-- NULL dereference
                                          event->owner = NULL
  mutex_lock(&current->perf_event_mutex)
  list_add_tail(&event->owner_entry,
                &current->perf_event_list)

In addition to the NULL pointer dereference in list_del_init() on the
zeroed owner_entry, perf_remove_from_owner() clears event->owner before
list_add_tail() runs, so perf_event_release_kernel() later skips unlinking
the event and leaves a freed event on current->perf_event_list.

[Severity: High]
This problem wasn't introduced by this patch, but can
perf_remove_from_owner() and perf_event_task_enable() cause an ABBA
deadlock [1] between ctx->mutex and perf_event_mutex?

When Task A opens a remove_on_exec event on Task B and calls
prctl(PR_TASK_PERF_EVENTS_ENABLE) while Task B calls execve(), the two
paths take perf_event_mutex and ctx->mutex in opposite orders:

CPU0 (Task A)                     CPU1 (Task B)
----                              ----
lock(&Task_A->perf_event_mutex)
                                  lock(&Task_B_ctx->mutex)
lock(&Task_B_ctx->mutex)
                                  lock(&Task_A->perf_event_mutex)

 *** DEADLOCK ***

CPU0: perf_event_task_enable() -> perf_event_ctx_lock()
CPU1: perf_event_exec() -> perf_event_remove_on_exec() ->
      perf_remove_from_owner()

The SINGLE_DEPTH_NESTING annotation on mutex_lock_nested() in
perf_remove_from_owner() places owner->perf_event_mutex in subclass 1,
which also hides this cycle from lockdep.

[1] https://sashiko.dev/#/bug/linux-342ff44b-cc35-4e1e-a35b-b682a94d40dd

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113221220.162338-1-f3lix.dev@gmx.de?part=1