> 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
