On 2025/9/2 22:57 Steven Rostedt <rost...@goodmis.org> write:
> On Tue, 2 Sep 2025 17:17:03 +0800
> Herbert Xu <herb...@gondor.apana.org.au> wrote:
>
> > Menglong Dong <dong...@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
>
>
