Re: [PATCH bpf-next 5/9] uprobes: Don't sleep on mmap_lock under dup_mmap_sem

[bot+bpf-ci]

> uprobes: Don't sleep on mmap_lock under dup_mmap_sem

TL;DR: Question, possibly not a bug: uprobe_dup_mmap() edits oldmm page tables
inside dup_mmap() before vma_start_write(), so it may race with per-VMA read
lockers (page fault, MADV_DONTNEED); the review suspects it is safe.

> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c

[ ... ]

>  void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm)
>  {
> +	struct uprobe_walk *walk;
> +	struct map_info *info;
> +
> +	list_for_each_entry(walk, &uprobe_walks, list) {
> +		/*
> +		 * fork() that is being killed fails in copy_process().
> +		 * There will be no child. Leave oldmm to the walk.
> +		 */
> +		if (fatal_signal_pending(current))
> +			break;
> +		for (info = walk->busy; info; info = info->next)
> +			if (info->mm == oldmm)
> +				register_one_mm(walk, info, true);
> +	}
> +

A subsystem pattern flags this call as potentially concerning, so this may
not be a bug.

uprobe_dup_mmap() is called from dup_mmap() with mmap_write_lock_killable()
held on oldmm, and before the copy loop reaches vma_start_write_killable()
on each vma:

dup_mmap()
    mmap_write_lock_killable(oldmm)
    uprobe_dup_mmap(oldmm, mm)
        register_one_mm(walk, info, true)
            install_breakpoint() / remove_breakpoint()
                set_swbp() / set_orig_insn()
                    uprobe_write()
                        get_user_pages_remote()
                        folio_walk_start() + __uprobe_write()
                        collapse_pte_mapped_thp()   (unregister path)
                            try_collapse_pte_mapped_thp()
                                pmdp_collapse_flush() + pte_free_defer()
    ...
    vma_start_write_killable(mpnt)

The guide for the VMA locking code says to report functions that hold
mmap_write_lock and access page tables or PTE pages before calling
vma_start_write(vma), because other threads of the forking process may
still hold per-VMA read locks on the same vma, for example in a page fault
or in MADV_DONTNEED with CONFIG_PT_RECLAIM.

Can uprobe_write() and collapse_pte_mapped_thp() run against oldmm here
while another thread of the forking process holds a per-VMA read lock on
the same vma?

For context, the existing register_for_each_vma() -> register_one_mm()
walk already did this with only mmap_write_lock held.  What this patch adds
is a second caller of that code inside dup_mmap(), which did no page table
access on oldmm before vma_start_write().

On the other side, uprobe_write() takes the page table lock through
folio_walk_start() and retries when fw.page does not match the expected
page, and try_collapse_pte_mapped_thp() re-checks pmd_same() under the
locks before pmdp_collapse_flush().  khugepaged's retract_page_tables()
also retracts page tables without mmap_lock at all.  So it may be that
this access is safe without the per-VMA write lock.

Could you confirm whether the vma_start_write() rule is meant to apply to
the uprobe_write() path called from uprobe_dup_mmap()?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39710454342