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

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

[Felix Hoffmann]

perf_event_open() installs a remove-on-exec event in the target context
while holding the target task's exec_update_lock. It then drops that lock
before linking the event into the opener's perf_event_list.

A concurrent target exec can therefore find the installed event and call
perf_remove_from_owner() before owner_entry has been initialized as a list
node. The resulting list_del() on the zeroed node triggers list corruption
and can panic the kernel.

Keep exec_update_lock held until the event has been published in its
owner's list. This closes the visibility gap while retaining the existing
lock ordering.

Fixes: 2e498d0a74e5 ("perf: Add support for event removal on exec")
Cc: stable@vger.kernel.org
Signed-off-by: Felix Hoffmann <f3lix.dev@gmx.de>
---
Tested on x86-64 with KASAN enabled:

  - on the unmodified current kernel, the unprivileged reproducer triggered
    list_del() corruption in perf_remove_from_owner() and panicked at round
    4,400 as UID 1000
  - with this change, the identical reproducer completed 10,000 rounds and
    8,395,975 successful perf_event_open() calls without a BUG, warning,
    KASAN report or panic
  - kernel/events/core.o builds with W=1 and the KASAN-enabled x86-64 kernel
    builds successfully

The reproducer and complete console logs are available privately on request.
 kernel/events/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index f5f2261f1fe6f..f4f292f7b47e2 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -14414,15 +14414,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.
-- 
2.43.0

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

[sashiko-bot]

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