Re: [PATCH] uprobes: Remove redundant guard(rcu) in find_active_uprobe_speculative()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing `guard(rcu)()` from `find_active_uprobe_speculative()` eliminates standard RCU protection, exposing lockless VMA and file accesses to Use-After-Free and page faults.
--

--- Patch [1]: [PATCH] uprobes: Remove redundant guard(rcu) in find_active_uprobe_speculative() ---
commit 008639a39e6344f6f057b7d120c62792b7cd6ddf
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

uprobes: Remove redundant guard(rcu) in find_active_uprobe_speculative()

This commit removes the guard(rcu)() from find_active_uprobe_speculative()
and adds a lockdep assertion for RCU Tasks Trace, assuming Tasks Trace RCU
protection provided by the caller is sufficient.

> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 07f69dd3093d5..52833db53b25c 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -2416,6 +2416,7 @@ static int is_trap_at_addr(struct mm_struct *mm, unsigned long vaddr)
>  	return is_trap_insn(&opcode);
>  }
>  
> +/* assumes being inside RCU Tasks Trace protected region */
>  static struct uprobe *find_active_uprobe_speculative(unsigned long bp_vaddr)
>  {
>  	struct mm_struct *mm = current->mm;
> @@ -2425,7 +2426,7 @@ static struct uprobe *find_active_uprobe_speculative(unsigned long bp_vaddr)
>  	loff_t offset;
>  	unsigned int seq;
>  
> -	guard(rcu)();
> +	lockdep_assert(rcu_read_lock_trace_held());
>  
>  	if (!mmap_lock_speculate_try_begin(mm, &seq))
>  		return NULL;

[Severity: High]
Does removing the standard RCU protection here expose the subsequent VMA and
file accesses to use-after-free and page faults?

The VMA and file objects are allocated from SLAB_TYPESAFE_BY_RCU caches,
meaning their underlying slab pages can be returned to the page allocator
after a standard RCU grace period. RCU Tasks Trace does not block standard
RCU grace periods.

The subsequent call to vma_lookup() briefly takes standard RCU to safely walk
the Maple Tree, but drops it before returning the VMA pointer. If the thread
is preempted after this point, and another thread unmaps the VMA and closes
the file, a standard RCU grace period could complete, freeing the VMA and file
slab pages.

When the thread wakes up and dereferences vma->vm_file or vm_file->f_inode on
a freed page, could it cause a fatal page fault? The code relies on the VMA
and file not being freed from under it, which appears to require standard RCU
protection.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178537593930.153254.1296986092000402239.stgit@devnote2?part=1