Re: [PATCH] tracing: fprobe: fix suspicious rcu usage in fprobe_entry

[menglong.dong@linux.dev]

On 2025/9/2 22:57 Steven Rostedt <rostedt@goodmis.org> write:
> On Tue, 2 Sep 2025 17:17:03 +0800
> Herbert Xu <herbert@gondor.apana.org.au> wrote:
> 
> > Menglong Dong <dongml2@chinatelecom.cn> wrote:
> > >
> > > diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> > > index fb127fa95f21..fece0f849c1c 100644
> > > --- a/kernel/trace/fprobe.c
> > > +++ b/kernel/trace/fprobe.c
> > > @@ -269,7 +269,9 @@ static int fprobe_entry(struct ftrace_graph_ent *trace, struct fgraph_ops *gops,
> > >        if (WARN_ON_ONCE(!fregs))
> > >                return 0;
> > > 
> > > +       rcu_read_lock();
> > >        head = rhltable_lookup(&fprobe_ip_table, &func, fprobe_rht_params);
> > > +       rcu_read_unlock();
> > >        reserved_words = 0;
> > >        rhl_for_each_entry_rcu(node, pos, head, hlist) {
> > >                if (node->addr != func)  
> > 
> > Actually this isn't quite right.  I know that it is a false-positive
> > so that it's actually safe, but if you're going to mark it with
> > rcu_read_lock, it should cover both the lookup as well as the
> > dereference which happens in the loop rhl_for_each_entry_rcu.
> > 
> 
> I disagree. It's a false positive as RCU is actually enabled here
> because preemption is disabled. Now we are spreading the internals of
> rhltable into the fprobe code.
> 
> We should just wrap it as is with a comment saying that currently RCU
> checking doesn't have a good way to know preemption is disabled in all
> config settings.
> 
> That is, I don't want rcu disabled here where people will think it's
> actually needed when it isn't. Wrapping the call with rcu_read_lock()
> with a comment that says it's to quiet a false positive is enough, as
> then we are not misrepresenting the code.
> 
> Maybe instead have:
> 
> /*
>  * fprobes calls rhltable_lookup() from a preempt_disabled location.
>  * This is equivalent to rcu_read_lock(). But rcu_deferefernce_check()
>  * will trigger a false positive when PREEMPT_COUNT is not defined.
>  * Quiet the check.
>  */
> #ifndef CONFIG_PREEMPT_COUNT
> # define quiet_rcu_lock_check() rcu_read_lock()
> # define quiet_rcu_unlock_check() rcu_read_unlock()
> #else
> # define quiet_rcu_lock_check()
> # define_quiet_rcu_unlock_check()
> #endif
> 
> And then have:
> 
>        quiet_rcu_read_lock_check();
>        head = rhltable_lookup(&fprobe_ip_table, &func, fprobe_rht_params);
>        quiet_rcu_read_unlock_check();

That's a good idea. But I think it doesn't work for PREEMPT_COUNT
case, unless we do some modification to
rcu_read_lock_held()/rcu_read_lock_held_common().

I'm not sure if is it possible to define them as:

# define quiet_rcu_lock_check() rcu_lock_acquire(&rcu_lock_map)
# define quiet_rcu_unlock_check() rcu_lock_release(&rcu_lock_map)

Thanks!
Menglong
> 
> -- Steve
> 
>