Re: [PATCH v2] perf/core: Close owner-list publication race with remove-on-exec

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] perf/core: unpinning ctx before owner_entry link allows fork/swap exec race
--

commit 32e21b1610ca28e8fe922d31bdbd5f0a4a31607c
Author: Daehyeon Ko <4ncienth@gmail.com>

perf/core: Close owner-list publication race with remove-on-exec

This commit keeps the target task's exec_update_lock held in perf_event_open()
until event->owner_entry is added to current->perf_event_list, preventing a
concurrent target exec from running perf_remove_from_owner() on an unlinked
owner_entry.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 7846d70be57f6..2fdd8d71dcd55 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -14246,15 +14246,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: Critical]
Can the same owner_entry race still happen if the target task forks and the
child execs before list_add_tail() runs?

In SYSCALL_DEFINE5(perf_event_open) in kernel/events/core.c, ctx is unpinned
and ctx->mutex is unlocked before event->owner_entry is linked onto
current->perf_event_list:

	event->owner = current;

	perf_install_in_context(ctx, event, event->cpu);
	perf_unpin_context(ctx);

	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);

Holding task->signal->exec_update_lock only blocks execve() on task's own
signal_struct. It does not prevent task from calling fork(), which runs
copy_process() -> perf_event_init_task() -> perf_event_init_context() without
taking task->signal->exec_update_lock and allocates a new signal_struct with a
separate unlocked exec_update_lock for the child.

When an event is opened with attr.remove_on_exec = 1 and attr.inherit = 1,
perf_event_init_context() acquires ctx->mutex, inherits the event into
child_ctx, and marks child_ctx as a clone of ctx. Because perf_unpin_context()
already dropped ctx->pin_count to 0, context_equiv() allows the context swap
optimization:

kernel/events/core.c:context_equiv() {
	/* Pinning disables the swap optimization */
	if (ctx1->pin_count || ctx2->pin_count)
		return 0;
}

When switching from task to the newly forked child,
perf_event_context_sched_out() can then swap the unpinned original ctx onto the
child task:

kernel/events/core.c:perf_event_context_sched_out() {
			WRITE_ONCE(ctx->task, next);
			WRITE_ONCE(next_ctx->task, task);

			perf_ctx_sched_task_cb(ctx, task, false);

			perf_ctx_enable(ctx, 0);

			RCU_INIT_POINTER(task->perf_event_ctxp, next_ctx);
			RCU_INIT_POINTER(next->perf_event_ctxp, ctx);
}

At that point, the original ctx containing the parent event (with
event->owner == current and event->owner_entry still {NULL, NULL}) belongs to
the child process. If the child immediately calls execve() under its own
child->signal->exec_update_lock, begin_new_exec() -> perf_event_exec() ->
perf_event_remove_on_exec() iterates the swapped ctx->event_list:

kernel/events/core.c:perf_event_remove_on_exec() {
	list_for_each_entry_safe(event, next, &ctx->event_list, event_entry) {
		if (!event->attr.remove_on_exec)
			continue;

		if (!is_kernel_event(event))
			perf_remove_from_owner(event);
}

Wouldn't this allow perf_remove_from_owner(event) to run before the opener
executes list_add_tail(&event->owner_entry, &current->perf_event_list), hitting
the same list_del_init() corruption or use-after-free on
current->perf_event_list?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017150747.1119197-1-4ncienth@gmail.com?part=1