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 <[email protected]> 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/[email protected]?part=7
