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

[Google]

On Thu, 30 Jul 2026 12:30:10 +0200
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> On Thu, Jul 30, 2026 at 10:45:39AM +0900, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> > 
> > find_active_uprobe_speculative() is called from find_active_uprobe_rcu(),
> > which is invoked by handle_swbp() and handle_syscall_uprobe().
> > Both callers already hold RCU Tasks Trace (via rcu_read_lock_trace() or
> > guard(rcu_tasks_trace)()).
> > 
> > Calling guard(rcu)() inside find_active_uprobe_speculative() is redundant
> > because the caller already guarantees RCU Tasks Trace protection.
> > Furthermore, standard RCU (rcu_read_lock()) does not protect uprobes_tree
> > against asynchronous uprobe destruction (call_rcu_tasks_trace()).
> > 
> > Remove the redundant guard(rcu)() from find_active_uprobe_speculative() and
> > replace it with lockdep_assert(rcu_read_lock_trace_held()) and a comment
> > documenting that callers must hold RCU Tasks Trace protection, matching the
> > convention used by find_active_uprobe_rcu() and find_uprobe_rcu().
> > 
> > Fixes: e0925f2dc4de ("uprobes: add speculative lockless VMA-to-inode-to-uprobe resolution")
> > Assisted-by: Antigravity:gemini-3.6-flash
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> > ---
> >  kernel/events/uprobes.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index 4084e926e284..b4e253d43b30 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -2424,6 +2424,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;
> > @@ -2433,7 +2434,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;
> 
> Seems fine, but it this really worthy of Fixes? I mean, its just a micro
> optimization, no?

Yeah, I agree this is a kind of cleanup. But Sashiko pointed that
this guard(rcu)() is for protecting vma/vm_file access.
Maybe what we need is to add a comment that this guard(rcu)() is
for protecting vma during this operation.
(mmap_lock_speculate_try_begin() only checks a sequence number, not locking.)

Thank you,

-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>