Re: [RFC v1 03/19] uprobes: Print warning for missing breakpoint install

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing VMA write lock during uprobe registration exposes page table modifications to races
--

commit c4ab2afca2dc57fb6a2f6789af30c2bec5a5064c
Author: Andi Kleen <ak@kernel.org>

uprobes: Print warning for missing breakpoint install
    
When a uprobe breakpoint installation fails, the error is currently silently
ignored. This commit adds a rate-limited warning to log these failures and
make debugging easier.

> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 4287c4ff4c0fd..941b52c478585 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1630,7 +1630,13 @@ int uprobe_mmap(struct vm_area_struct *vma)
>  		if (!fatal_signal_pending(current) &&
>  		    filter_chain(uprobe, vma->vm_mm)) {
>  			unsigned long vaddr = offset_to_vaddr(vma, uprobe->offset);
> -			install_breakpoint(uprobe, vma, vaddr);
> +			int err = install_breakpoint(uprobe, vma, vaddr);
                        ^^^^

[Severity: High]
This isn't a bug introduced by this patch, but modifying install_breakpoint()
reminded me of a pre-existing issue where it is called from
register_for_each_vma() without isolating the VMA.

In register_for_each_vma(), the code acquires mmap_write_lock(mm) and
dispatches install_breakpoint() or remove_breakpoint():

kernel/events/uprobes.c:register_for_each_vma() {
    ...
		mmap_write_lock(mm);
    ...
		if (is_register) {
			/* consult only the "caller", new consumer. */
			if (consumer_filter(new, mm))
				err = install_breakpoint(uprobe, vma, info->vaddr);
		} else if (mm_flags_test(MMF_HAS_UPROBES, mm)) {
			if (!filter_chain(uprobe, mm))
				err |= remove_breakpoint(uprobe, vma, info->vaddr);
		}
    ...
}

Both install_breakpoint() and remove_breakpoint() ultimately call
uprobe_write() to clear PTEs and collapse PMDs via __uprobe_write() and
collapse_pte_mapped_thp().

However, as per VMA lock invariants, mmap_write_lock alone does not exclude
concurrent per-VMA read lock holders (like page faults) that acquired the lock
prior to the write lock.

Should register_for_each_vma() call vma_start_write(vma) before dispatching
these functions? Failing to drain per-VMA lock holders could allow readers to
concurrently walk the VMA's page tables while uprobe_write() clears them,
potentially leading to use-after-free of PTE pages or stale PMD reads.

> +
> +			if (err)
> +				pr_warn_ratelimited(
> +					"uprobes: probe %pD+0x%llx failed to install (%d)\n",
> +					vma->vm_file,
> +					(unsigned long long)uprobe->offset, err);
>  		}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831150651.1134594-1-ak@kernel.org?part=3