Re: [PATCH bpf-next 7/9] fork: Let the new task point to its mm before uprobes can find it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] fork: clear p->mm under task_lock() before mmput() in bad_fork_cleanup_mm
--

commit ec1e952207350f8b07e6038358df3fabd0496801
Author: Alexei Starovoitov <ast@kernel.org>

fork: Let the new task point to its mm before uprobes can find it

This commit sets tsk->mm under task_lock() at the end of dup_mmap() before
uprobe_end_dup_mmap() releases dup_mmap_sem, so that uprobe filters on
inherited perf events can match the child's mm. It also clears tsk->mm under
task_lock() if dup_mm() fails after dup_mmap() succeeds.

> diff --git a/kernel/fork.c b/kernel/fork.c
> index 2e125623cdbc3..a1042e370bdb8 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1537,7 +1537,7 @@ static struct mm_struct *dup_mm(struct task_struct *tsk,
>  	if (!mm_init(mm, tsk))
>  		goto fail_nomem;
>  
> -	err = dup_mmap(mm, oldmm);
> +	err = dup_mmap(tsk, mm, oldmm);
>  	if (err)
>  		goto free_pt;
>  
> @@ -1550,6 +1550,13 @@ static struct mm_struct *dup_mm(struct task_struct *tsk,
>  	return mm;
>  
>  free_pt:
> +	/*
> +	 * dup_mmap() has set it if it succeeded. get_task_mm() must not find
> +	 * it after the mmput() below.
> +	 */
> +	task_lock(tsk);
> +	tsk->mm = NULL;
> +	task_unlock(tsk);
>  	/* don't put binfmt in mmput, we haven't got module yet */
>  	mm->binfmt = NULL;
>  	mm_init_owner(mm, NULL);

[Severity: High]
dup_mm() clears tsk->mm under task_lock(tsk) before calling mmput(mm) so
that get_task_mm() cannot find a freed mm when dup_mm() fails. However,
what happens if copy_process() fails after copy_mm() has already succeeded?

In kernel/fork.c:copy_process(), perf_event_init_task() runs before
copy_mm() and links inherited perf events (with event->hw.target = p) onto
parent_event->child_list and uprobe filter->perf_events:

kernel/fork.c:copy_process() {
	...
	retval = perf_event_init_task(p, clone_flags);
	if (retval)
		goto bad_fork_sched_cancel_fork;
	...
	retval = copy_mm(clone_flags, p);
	if (retval)
		goto bad_fork_cleanup_signal;

If a later step in copy_process() fails (for example, copy_namespaces(),
copy_io(), copy_thread(), alloc_pid(), cgroup_can_fork(), or
fatal_signal_pending(current) after waiting in dup_userfaultfd_complete()),
execution jumps to bad_fork_cleanup_mm:

bad_fork_cleanup_mm:
	sched_cache_fork_cleanup(p);
	if (p->mm) {
		mm_clear_owner(p->mm, p);
		mmput(p->mm);
	}
	...
bad_fork_cleanup_security:
	security_task_free(p);
bad_fork_cleanup_audit:
	audit_free(p);
bad_fork_cleanup_perf:
	perf_event_free_task(p);

Here mmput(p->mm) drops the reference and frees the mm, but p->mm is left
non-null while teardown continues through exit_fs(p), exit_files(p), and
security_task_free(p) before perf_event_free_task(p) detaches the inherited
events.

During this window, can a concurrent caller of get_task_mm(p) (such as
perf_event_set_addr_filter() -> perf_event_for_each_child() ->
perf_event_addr_filters_apply() in kernel/events/core.c) or bpf helpers like
bpf_find_vma() and bpf_iter_task_vma_new() in kernel/bpf/task_iter.c (for
instance from security_task_free(p)) observe the non-null p->mm under
task_lock(p) and call mmget() on the already-freed mm_struct?

Similarly, when perf_event_free_task(p) -> uprobe_perf_close() runs (or a
concurrent uprobe_perf_filter() runs), trace_uprobe_filter_remove() and
__uprobe_perf_filter() in kernel/trace/trace_uprobe.c read
event->hw.target->mm and compare that dangling pointer against active
mm_structs.

Should bad_fork_cleanup_mm in copy_process() also clear p->mm under
task_lock(p) before calling mmput()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022015712.1297331-1-alexei.starovoitov@gmail.com?part=7