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

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

[Menglong Dong]

rcu_read_lock() is not needed in fprobe_entry, but rcu_dereference_check()
is used in rhltable_lookup(), which causes suspicious RCU usage warning:

  WARNING: suspicious RCU usage
  6.17.0-rc1-00001-gdfe0d675df82 #1 Tainted: G S
  -----------------------------
  include/linux/rhashtable.h:602 suspicious rcu_dereference_check() usage!
  ......
  stack backtrace:
  CPU: 1 UID: 0 PID: 4652 Comm: ftracetest Tainted: G S
  Tainted: [S]=CPU_OUT_OF_SPEC, [I]=FIRMWARE_WORKAROUND
  Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
  Call Trace:
   <TASK>
   dump_stack_lvl+0x7c/0x90
   lockdep_rcu_suspicious+0x14f/0x1c0
   __rhashtable_lookup+0x1e0/0x260
   ? __pfx_kernel_clone+0x10/0x10
   fprobe_entry+0x9a/0x450
   ? __lock_acquire+0x6b0/0xca0
   ? find_held_lock+0x2b/0x80
   ? __pfx_fprobe_entry+0x10/0x10
   ? __pfx_kernel_clone+0x10/0x10
   ? lock_acquire+0x14c/0x2d0
   ? __might_fault+0x74/0xc0
   function_graph_enter_regs+0x2a0/0x550
   ? __do_sys_clone+0xb5/0x100
   ? __pfx_function_graph_enter_regs+0x10/0x10
   ? _copy_to_user+0x58/0x70
   ? __pfx_kernel_clone+0x10/0x10
   ? __x64_sys_rt_sigprocmask+0x114/0x180
   ? __pfx___x64_sys_rt_sigprocmask+0x10/0x10
   ? __pfx_kernel_clone+0x10/0x10
   ftrace_graph_func+0x87/0xb0

Fix this by using rcu_read_lock() for rhltable_lookup(). Alternatively, we
can use rcu_lock_acquire(&rcu_lock_map) here to obtain better performance.
However, it's not a common usage :/

Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202508281655.54c87330-lkp@intel.com
Fixes: dfe0d675df82 ("tracing: fprobe: use rhltable for fprobe_ip_table")
Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 kernel/trace/fprobe.c | 2 ++
 1 file changed, 2 insertions(+)

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)
-- 
2.51.0

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

[Steven Rostedt]

On Fri, 29 Aug 2025 10:14:36 +0800
Menglong Dong <dongml2@chinatelecom.cn> wrote:

> rcu_read_lock() is not needed in fprobe_entry, but rcu_dereference_check()
> is used in rhltable_lookup(), which causes suspicious RCU usage warning:
> 
>   WARNING: suspicious RCU usage
>   6.17.0-rc1-00001-gdfe0d675df82 #1 Tainted: G S
>   -----------------------------
>   include/linux/rhashtable.h:602 suspicious rcu_dereference_check() usage!
>   ......
>   stack backtrace:
>   CPU: 1 UID: 0 PID: 4652 Comm: ftracetest Tainted: G S
>   Tainted: [S]=CPU_OUT_OF_SPEC, [I]=FIRMWARE_WORKAROUND
>   Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
>   Call Trace:
>    <TASK>
>    dump_stack_lvl+0x7c/0x90
>    lockdep_rcu_suspicious+0x14f/0x1c0
>    __rhashtable_lookup+0x1e0/0x260
>    ? __pfx_kernel_clone+0x10/0x10
>    fprobe_entry+0x9a/0x450
>    ? __lock_acquire+0x6b0/0xca0
>    ? find_held_lock+0x2b/0x80
>    ? __pfx_fprobe_entry+0x10/0x10
>    ? __pfx_kernel_clone+0x10/0x10
>    ? lock_acquire+0x14c/0x2d0
>    ? __might_fault+0x74/0xc0
>    function_graph_enter_regs+0x2a0/0x550
>    ? __do_sys_clone+0xb5/0x100
>    ? __pfx_function_graph_enter_regs+0x10/0x10
>    ? _copy_to_user+0x58/0x70
>    ? __pfx_kernel_clone+0x10/0x10
>    ? __x64_sys_rt_sigprocmask+0x114/0x180
>    ? __pfx___x64_sys_rt_sigprocmask+0x10/0x10
>    ? __pfx_kernel_clone+0x10/0x10
>    ftrace_graph_func+0x87/0xb0
> 
> Fix this by using rcu_read_lock() for rhltable_lookup(). Alternatively, we
> can use rcu_lock_acquire(&rcu_lock_map) here to obtain better performance.
> However, it's not a common usage :/

So this is needed even though it's called under preempt_disable().

Paul, do we need to add an rcu_read_lock() because the code in rht
(rhashtable) requires RCU read lock?

I thought that rcu_read_lock() and preempt_disable() have been merged?

-- Steve


> 
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202508281655.54c87330-lkp@intel.com
> Fixes: dfe0d675df82 ("tracing: fprobe: use rhltable for fprobe_ip_table")
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> ---
>  kernel/trace/fprobe.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> 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)

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

[menglong.dong]

On 2025/8/29 10:23 Steven Rostedt <rostedt@goodmis.org> write:
> On Fri, 29 Aug 2025 10:14:36 +0800
> Menglong Dong <dongml2@chinatelecom.cn> wrote:
> 
> > rcu_read_lock() is not needed in fprobe_entry, but rcu_dereference_check()
> > is used in rhltable_lookup(), which causes suspicious RCU usage warning:
> > 
> >   WARNING: suspicious RCU usage
> >   6.17.0-rc1-00001-gdfe0d675df82 #1 Tainted: G S
> >   -----------------------------
> >   include/linux/rhashtable.h:602 suspicious rcu_dereference_check() usage!
> >   ......
> >   stack backtrace:
> >   CPU: 1 UID: 0 PID: 4652 Comm: ftracetest Tainted: G S
> >   Tainted: [S]=CPU_OUT_OF_SPEC, [I]=FIRMWARE_WORKAROUND
> >   Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
> >   Call Trace:
> >    <TASK>
> >    dump_stack_lvl+0x7c/0x90
> >    lockdep_rcu_suspicious+0x14f/0x1c0
> >    __rhashtable_lookup+0x1e0/0x260
> >    ? __pfx_kernel_clone+0x10/0x10
> >    fprobe_entry+0x9a/0x450
> >    ? __lock_acquire+0x6b0/0xca0
> >    ? find_held_lock+0x2b/0x80
> >    ? __pfx_fprobe_entry+0x10/0x10
> >    ? __pfx_kernel_clone+0x10/0x10
> >    ? lock_acquire+0x14c/0x2d0
> >    ? __might_fault+0x74/0xc0
> >    function_graph_enter_regs+0x2a0/0x550
> >    ? __do_sys_clone+0xb5/0x100
> >    ? __pfx_function_graph_enter_regs+0x10/0x10
> >    ? _copy_to_user+0x58/0x70
> >    ? __pfx_kernel_clone+0x10/0x10
> >    ? __x64_sys_rt_sigprocmask+0x114/0x180
> >    ? __pfx___x64_sys_rt_sigprocmask+0x10/0x10
> >    ? __pfx_kernel_clone+0x10/0x10
> >    ftrace_graph_func+0x87/0xb0
> > 
> > Fix this by using rcu_read_lock() for rhltable_lookup(). Alternatively, we
> > can use rcu_lock_acquire(&rcu_lock_map) here to obtain better performance.
> > However, it's not a common usage :/
> 
> So this is needed even though it's called under preempt_disable().

It is needed when the lock debug configs are enabled.

> 
> Paul, do we need to add an rcu_read_lock() because the code in rht
> (rhashtable) requires RCU read lock?
> 
> I thought that rcu_read_lock() and preempt_disable() have been merged?

Maybe we can do some adjustment do rcu_read_lock_held_common()
like this:

diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index c912b594ba98..280fa4d2fc79 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -114,6 +114,10 @@ static bool rcu_read_lock_held_common(bool *ret)
                *ret = false;
                return true;
        }
+       if (!preemptible()) {
+               *ret = true;
+               return true;
+       }
        return false;
 }
 
@@ -123,7 +127,7 @@ int rcu_read_lock_sched_held(void)
 
        if (rcu_read_lock_held_common(&ret))
                return ret;
-       return lock_is_held(&rcu_sched_lock_map) || !preemptible();
+       return lock_is_held(&rcu_sched_lock_map);
 }
 EXPORT_SYMBOL(rcu_read_lock_sched_held);
 #endif

I think it's a bad idea, as !preemptiable() has different semantic
with rcu_read_lock() :(

Thanks!
Menglong Dong

> 
> -- Steve
> 
> 
> > 
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> > Closes: https://lore.kernel.org/oe-lkp/202508281655.54c87330-lkp@intel.com
> > Fixes: dfe0d675df82 ("tracing: fprobe: use rhltable for fprobe_ip_table")
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> >  kernel/trace/fprobe.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > 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)
> 
> 
> 

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

[Paul E. McKenney]

On Fri, Aug 29, 2025 at 10:49:46AM +0800, menglong.dong@linux.dev wrote:
> On 2025/8/29 10:23 Steven Rostedt <rostedt@goodmis.org> write:
> > On Fri, 29 Aug 2025 10:14:36 +0800
> > Menglong Dong <dongml2@chinatelecom.cn> wrote:
> > 
> > > rcu_read_lock() is not needed in fprobe_entry, but rcu_dereference_check()
> > > is used in rhltable_lookup(), which causes suspicious RCU usage warning:
> > > 
> > >   WARNING: suspicious RCU usage
> > >   6.17.0-rc1-00001-gdfe0d675df82 #1 Tainted: G S
> > >   -----------------------------
> > >   include/linux/rhashtable.h:602 suspicious rcu_dereference_check() usage!
> > >   ......
> > >   stack backtrace:
> > >   CPU: 1 UID: 0 PID: 4652 Comm: ftracetest Tainted: G S
> > >   Tainted: [S]=CPU_OUT_OF_SPEC, [I]=FIRMWARE_WORKAROUND
> > >   Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
> > >   Call Trace:
> > >    <TASK>
> > >    dump_stack_lvl+0x7c/0x90
> > >    lockdep_rcu_suspicious+0x14f/0x1c0
> > >    __rhashtable_lookup+0x1e0/0x260
> > >    ? __pfx_kernel_clone+0x10/0x10
> > >    fprobe_entry+0x9a/0x450
> > >    ? __lock_acquire+0x6b0/0xca0
> > >    ? find_held_lock+0x2b/0x80
> > >    ? __pfx_fprobe_entry+0x10/0x10
> > >    ? __pfx_kernel_clone+0x10/0x10
> > >    ? lock_acquire+0x14c/0x2d0
> > >    ? __might_fault+0x74/0xc0
> > >    function_graph_enter_regs+0x2a0/0x550
> > >    ? __do_sys_clone+0xb5/0x100
> > >    ? __pfx_function_graph_enter_regs+0x10/0x10
> > >    ? _copy_to_user+0x58/0x70
> > >    ? __pfx_kernel_clone+0x10/0x10
> > >    ? __x64_sys_rt_sigprocmask+0x114/0x180
> > >    ? __pfx___x64_sys_rt_sigprocmask+0x10/0x10
> > >    ? __pfx_kernel_clone+0x10/0x10
> > >    ftrace_graph_func+0x87/0xb0
> > > 
> > > Fix this by using rcu_read_lock() for rhltable_lookup(). Alternatively, we
> > > can use rcu_lock_acquire(&rcu_lock_map) here to obtain better performance.
> > > However, it's not a common usage :/
> > 
> > So this is needed even though it's called under preempt_disable().
> 
> It is needed when the lock debug configs are enabled.
> 
> > 
> > Paul, do we need to add an rcu_read_lock() because the code in rht
> > (rhashtable) requires RCU read lock?
> > 
> > I thought that rcu_read_lock() and preempt_disable() have been merged?
> 
> Maybe we can do some adjustment do rcu_read_lock_held_common()
> like this:
> 
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index c912b594ba98..280fa4d2fc79 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -114,6 +114,10 @@ static bool rcu_read_lock_held_common(bool *ret)
>                 *ret = false;
>                 return true;
>         }
> +       if (!preemptible()) {
> +               *ret = true;
> +               return true;
> +       }
>         return false;
>  }
>  
> @@ -123,7 +127,7 @@ int rcu_read_lock_sched_held(void)
>  
>         if (rcu_read_lock_held_common(&ret))
>                 return ret;
> -       return lock_is_held(&rcu_sched_lock_map) || !preemptible();
> +       return lock_is_held(&rcu_sched_lock_map);
>  }
>  EXPORT_SYMBOL(rcu_read_lock_sched_held);
>  #endif
> 
> I think it's a bad idea, as !preemptiable() has different semantic
> with rcu_read_lock() :(

Especially given the definition of preemptible() within kernels built
with CONFIG_PREEMPT_COUNT=n...

							Thanx, Paul

> Thanks!
> Menglong Dong
> 
> > 
> > -- Steve
> > 
> > 
> > > 
> > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > Closes: https://lore.kernel.org/oe-lkp/202508281655.54c87330-lkp@intel.com
> > > Fixes: dfe0d675df82 ("tracing: fprobe: use rhltable for fprobe_ip_table")
> > > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > > ---
> > >  kernel/trace/fprobe.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > 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)
> > 
> > 
> > 
> 
> 
> 
> 

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

[Paul E. McKenney]

On Thu, Aug 28, 2025 at 10:23:57PM -0400, Steven Rostedt wrote:
> On Fri, 29 Aug 2025 10:14:36 +0800
> Menglong Dong <dongml2@chinatelecom.cn> wrote:
> 
> > rcu_read_lock() is not needed in fprobe_entry, but rcu_dereference_check()
> > is used in rhltable_lookup(), which causes suspicious RCU usage warning:
> > 
> >   WARNING: suspicious RCU usage
> >   6.17.0-rc1-00001-gdfe0d675df82 #1 Tainted: G S
> >   -----------------------------
> >   include/linux/rhashtable.h:602 suspicious rcu_dereference_check() usage!
> >   ......
> >   stack backtrace:
> >   CPU: 1 UID: 0 PID: 4652 Comm: ftracetest Tainted: G S
> >   Tainted: [S]=CPU_OUT_OF_SPEC, [I]=FIRMWARE_WORKAROUND
> >   Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
> >   Call Trace:
> >    <TASK>
> >    dump_stack_lvl+0x7c/0x90
> >    lockdep_rcu_suspicious+0x14f/0x1c0
> >    __rhashtable_lookup+0x1e0/0x260
> >    ? __pfx_kernel_clone+0x10/0x10
> >    fprobe_entry+0x9a/0x450
> >    ? __lock_acquire+0x6b0/0xca0
> >    ? find_held_lock+0x2b/0x80
> >    ? __pfx_fprobe_entry+0x10/0x10
> >    ? __pfx_kernel_clone+0x10/0x10
> >    ? lock_acquire+0x14c/0x2d0
> >    ? __might_fault+0x74/0xc0
> >    function_graph_enter_regs+0x2a0/0x550
> >    ? __do_sys_clone+0xb5/0x100
> >    ? __pfx_function_graph_enter_regs+0x10/0x10
> >    ? _copy_to_user+0x58/0x70
> >    ? __pfx_kernel_clone+0x10/0x10
> >    ? __x64_sys_rt_sigprocmask+0x114/0x180
> >    ? __pfx___x64_sys_rt_sigprocmask+0x10/0x10
> >    ? __pfx_kernel_clone+0x10/0x10
> >    ftrace_graph_func+0x87/0xb0
> > 
> > Fix this by using rcu_read_lock() for rhltable_lookup(). Alternatively, we
> > can use rcu_lock_acquire(&rcu_lock_map) here to obtain better performance.
> > However, it's not a common usage :/
> 
> So this is needed even though it's called under preempt_disable().
> 
> Paul, do we need to add an rcu_read_lock() because the code in rht
> (rhashtable) requires RCU read lock?
> 
> I thought that rcu_read_lock() and preempt_disable() have been merged?

Yes, preempt_disable() does indeed start an RCU read-side critical section,
just as surely as rcu_read_lock() does.

However, this is a lockdep check inside of __rhashtable_lookup():

	rht_dereference_rcu(ht->tbl, ht)

Which is defined as:

	rcu_dereference_check(p, lockdep_rht_mutex_is_held(ht));

This is explicitly telling lockdep that rcu_read_lock() is OK and
holding ht->mutex is OK, but nothing else is.

So an alternative way to fix this is to declare it to be a false positive,
and then avoid that false positive by adding a check that preemption
is disabled.  Adding the rhashtable maintainers for their perspective.

							Thanx, Paul

> -- Steve
> 
> 
> > 
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> > Closes: https://lore.kernel.org/oe-lkp/202508281655.54c87330-lkp@intel.com
> > Fixes: dfe0d675df82 ("tracing: fprobe: use rhltable for fprobe_ip_table")
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> >  kernel/trace/fprobe.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > 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)
> 

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

[Masami Hiramatsu]

On Fri, 29 Aug 2025 04:11:02 -0700
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> On Thu, Aug 28, 2025 at 10:23:57PM -0400, Steven Rostedt wrote:
> > On Fri, 29 Aug 2025 10:14:36 +0800
> > Menglong Dong <dongml2@chinatelecom.cn> wrote:
> > 
> > > rcu_read_lock() is not needed in fprobe_entry, but rcu_dereference_check()
> > > is used in rhltable_lookup(), which causes suspicious RCU usage warning:
> > > 
> > >   WARNING: suspicious RCU usage
> > >   6.17.0-rc1-00001-gdfe0d675df82 #1 Tainted: G S
> > >   -----------------------------
> > >   include/linux/rhashtable.h:602 suspicious rcu_dereference_check() usage!
> > >   ......
> > >   stack backtrace:
> > >   CPU: 1 UID: 0 PID: 4652 Comm: ftracetest Tainted: G S
> > >   Tainted: [S]=CPU_OUT_OF_SPEC, [I]=FIRMWARE_WORKAROUND
> > >   Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
> > >   Call Trace:
> > >    <TASK>
> > >    dump_stack_lvl+0x7c/0x90
> > >    lockdep_rcu_suspicious+0x14f/0x1c0
> > >    __rhashtable_lookup+0x1e0/0x260
> > >    ? __pfx_kernel_clone+0x10/0x10
> > >    fprobe_entry+0x9a/0x450
> > >    ? __lock_acquire+0x6b0/0xca0
> > >    ? find_held_lock+0x2b/0x80
> > >    ? __pfx_fprobe_entry+0x10/0x10
> > >    ? __pfx_kernel_clone+0x10/0x10
> > >    ? lock_acquire+0x14c/0x2d0
> > >    ? __might_fault+0x74/0xc0
> > >    function_graph_enter_regs+0x2a0/0x550
> > >    ? __do_sys_clone+0xb5/0x100
> > >    ? __pfx_function_graph_enter_regs+0x10/0x10
> > >    ? _copy_to_user+0x58/0x70
> > >    ? __pfx_kernel_clone+0x10/0x10
> > >    ? __x64_sys_rt_sigprocmask+0x114/0x180
> > >    ? __pfx___x64_sys_rt_sigprocmask+0x10/0x10
> > >    ? __pfx_kernel_clone+0x10/0x10
> > >    ftrace_graph_func+0x87/0xb0
> > > 
> > > Fix this by using rcu_read_lock() for rhltable_lookup(). Alternatively, we
> > > can use rcu_lock_acquire(&rcu_lock_map) here to obtain better performance.
> > > However, it's not a common usage :/
> > 
> > So this is needed even though it's called under preempt_disable().
> > 
> > Paul, do we need to add an rcu_read_lock() because the code in rht
> > (rhashtable) requires RCU read lock?
> > 
> > I thought that rcu_read_lock() and preempt_disable() have been merged?
> 
> Yes, preempt_disable() does indeed start an RCU read-side critical section,
> just as surely as rcu_read_lock() does.
> 
> However, this is a lockdep check inside of __rhashtable_lookup():
> 
> 	rht_dereference_rcu(ht->tbl, ht)
> 
> Which is defined as:
> 
> 	rcu_dereference_check(p, lockdep_rht_mutex_is_held(ht));
> 
> This is explicitly telling lockdep that rcu_read_lock() is OK and
> holding ht->mutex is OK, but nothing else is.

That is similar to the kprobes, which also allows accessing in
rcu critical section or under mutex.

> 
> So an alternative way to fix this is to declare it to be a false positive,
> and then avoid that false positive by adding a check that preemption
> is disabled.  Adding the rhashtable maintainers for their perspective.

What about changing it alloing it with preempt disabled flag?

Thank you,

> 
> 							Thanx, Paul
> 
> > -- Steve
> > 
> > 
> > > 
> > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > Closes: https://lore.kernel.org/oe-lkp/202508281655.54c87330-lkp@intel.com
> > > Fixes: dfe0d675df82 ("tracing: fprobe: use rhltable for fprobe_ip_table")
> > > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > > ---
> > >  kernel/trace/fprobe.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > 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)
> > 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[Paul E. McKenney]

On Mon, Sep 01, 2025 at 05:06:55PM +0900, Masami Hiramatsu wrote:
> On Fri, 29 Aug 2025 04:11:02 -0700
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> > On Thu, Aug 28, 2025 at 10:23:57PM -0400, Steven Rostedt wrote:
> > > On Fri, 29 Aug 2025 10:14:36 +0800
> > > Menglong Dong <dongml2@chinatelecom.cn> wrote:
> > > 
> > > > rcu_read_lock() is not needed in fprobe_entry, but rcu_dereference_check()
> > > > is used in rhltable_lookup(), which causes suspicious RCU usage warning:
> > > > 
> > > >   WARNING: suspicious RCU usage
> > > >   6.17.0-rc1-00001-gdfe0d675df82 #1 Tainted: G S
> > > >   -----------------------------
> > > >   include/linux/rhashtable.h:602 suspicious rcu_dereference_check() usage!
> > > >   ......
> > > >   stack backtrace:
> > > >   CPU: 1 UID: 0 PID: 4652 Comm: ftracetest Tainted: G S
> > > >   Tainted: [S]=CPU_OUT_OF_SPEC, [I]=FIRMWARE_WORKAROUND
> > > >   Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
> > > >   Call Trace:
> > > >    <TASK>
> > > >    dump_stack_lvl+0x7c/0x90
> > > >    lockdep_rcu_suspicious+0x14f/0x1c0
> > > >    __rhashtable_lookup+0x1e0/0x260
> > > >    ? __pfx_kernel_clone+0x10/0x10
> > > >    fprobe_entry+0x9a/0x450
> > > >    ? __lock_acquire+0x6b0/0xca0
> > > >    ? find_held_lock+0x2b/0x80
> > > >    ? __pfx_fprobe_entry+0x10/0x10
> > > >    ? __pfx_kernel_clone+0x10/0x10
> > > >    ? lock_acquire+0x14c/0x2d0
> > > >    ? __might_fault+0x74/0xc0
> > > >    function_graph_enter_regs+0x2a0/0x550
> > > >    ? __do_sys_clone+0xb5/0x100
> > > >    ? __pfx_function_graph_enter_regs+0x10/0x10
> > > >    ? _copy_to_user+0x58/0x70
> > > >    ? __pfx_kernel_clone+0x10/0x10
> > > >    ? __x64_sys_rt_sigprocmask+0x114/0x180
> > > >    ? __pfx___x64_sys_rt_sigprocmask+0x10/0x10
> > > >    ? __pfx_kernel_clone+0x10/0x10
> > > >    ftrace_graph_func+0x87/0xb0
> > > > 
> > > > Fix this by using rcu_read_lock() for rhltable_lookup(). Alternatively, we
> > > > can use rcu_lock_acquire(&rcu_lock_map) here to obtain better performance.
> > > > However, it's not a common usage :/
> > > 
> > > So this is needed even though it's called under preempt_disable().
> > > 
> > > Paul, do we need to add an rcu_read_lock() because the code in rht
> > > (rhashtable) requires RCU read lock?
> > > 
> > > I thought that rcu_read_lock() and preempt_disable() have been merged?
> > 
> > Yes, preempt_disable() does indeed start an RCU read-side critical section,
> > just as surely as rcu_read_lock() does.
> > 
> > However, this is a lockdep check inside of __rhashtable_lookup():
> > 
> > 	rht_dereference_rcu(ht->tbl, ht)
> > 
> > Which is defined as:
> > 
> > 	rcu_dereference_check(p, lockdep_rht_mutex_is_held(ht));
> > 
> > This is explicitly telling lockdep that rcu_read_lock() is OK and
> > holding ht->mutex is OK, but nothing else is.
> 
> That is similar to the kprobes, which also allows accessing in
> rcu critical section or under mutex.
> 
> > So an alternative way to fix this is to declare it to be a false positive,
> > and then avoid that false positive by adding a check that preemption
> > is disabled.  Adding the rhashtable maintainers for their perspective.
> 
> What about changing it alloing it with preempt disabled flag?

I am not sure that "it" that you are proposing changing.  ;-)

However, another option for the the above rcu_dereference_check() to
become something like this:

	rcu_dereference_check(p, lockdep_rht_mutex_is_held(ht) ||
				 rcu_read_lock_any_held());

This would be happy with any RCU reader, including rcu_read_lock(),
preempt_disable(), local_irq_disable(), local_bh_disable(), and various
handler contexts.  One downside is that this would *always* be happy in
a kernel built with CONFIG_PREEMPT_{NONE,VOLUNTARY}=y.

If this is happening often enough, it would be easy for me to create an
rcu_dereference_all_check() that allows all forms of vanilla RCU readers
(but not, for example, SRCU readers), but with only two use cases,
it is not clear to me that this is an overall win.

Or am I missing a turn in here somewhere?

							Thanx, Paul

> Thank you,
> 
> > 
> > 							Thanx, Paul
> > 
> > > -- Steve
> > > 
> > > 
> > > > 
> > > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > Closes: https://lore.kernel.org/oe-lkp/202508281655.54c87330-lkp@intel.com
> > > > Fixes: dfe0d675df82 ("tracing: fprobe: use rhltable for fprobe_ip_table")
> > > > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > > > ---
> > > >  kernel/trace/fprobe.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > 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)
> > > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[Masami Hiramatsu]

On Mon, 1 Sep 2025 08:00:15 -0700
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> On Mon, Sep 01, 2025 at 05:06:55PM +0900, Masami Hiramatsu wrote:
> > On Fri, 29 Aug 2025 04:11:02 -0700
> > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > 
> > > On Thu, Aug 28, 2025 at 10:23:57PM -0400, Steven Rostedt wrote:
> > > > On Fri, 29 Aug 2025 10:14:36 +0800
> > > > Menglong Dong <dongml2@chinatelecom.cn> wrote:
> > > > 
> > > > > rcu_read_lock() is not needed in fprobe_entry, but rcu_dereference_check()
> > > > > is used in rhltable_lookup(), which causes suspicious RCU usage warning:
> > > > > 
> > > > >   WARNING: suspicious RCU usage
> > > > >   6.17.0-rc1-00001-gdfe0d675df82 #1 Tainted: G S
> > > > >   -----------------------------
> > > > >   include/linux/rhashtable.h:602 suspicious rcu_dereference_check() usage!
> > > > >   ......
> > > > >   stack backtrace:
> > > > >   CPU: 1 UID: 0 PID: 4652 Comm: ftracetest Tainted: G S
> > > > >   Tainted: [S]=CPU_OUT_OF_SPEC, [I]=FIRMWARE_WORKAROUND
> > > > >   Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
> > > > >   Call Trace:
> > > > >    <TASK>
> > > > >    dump_stack_lvl+0x7c/0x90
> > > > >    lockdep_rcu_suspicious+0x14f/0x1c0
> > > > >    __rhashtable_lookup+0x1e0/0x260
> > > > >    ? __pfx_kernel_clone+0x10/0x10
> > > > >    fprobe_entry+0x9a/0x450
> > > > >    ? __lock_acquire+0x6b0/0xca0
> > > > >    ? find_held_lock+0x2b/0x80
> > > > >    ? __pfx_fprobe_entry+0x10/0x10
> > > > >    ? __pfx_kernel_clone+0x10/0x10
> > > > >    ? lock_acquire+0x14c/0x2d0
> > > > >    ? __might_fault+0x74/0xc0
> > > > >    function_graph_enter_regs+0x2a0/0x550
> > > > >    ? __do_sys_clone+0xb5/0x100
> > > > >    ? __pfx_function_graph_enter_regs+0x10/0x10
> > > > >    ? _copy_to_user+0x58/0x70
> > > > >    ? __pfx_kernel_clone+0x10/0x10
> > > > >    ? __x64_sys_rt_sigprocmask+0x114/0x180
> > > > >    ? __pfx___x64_sys_rt_sigprocmask+0x10/0x10
> > > > >    ? __pfx_kernel_clone+0x10/0x10
> > > > >    ftrace_graph_func+0x87/0xb0
> > > > > 
> > > > > Fix this by using rcu_read_lock() for rhltable_lookup(). Alternatively, we
> > > > > can use rcu_lock_acquire(&rcu_lock_map) here to obtain better performance.
> > > > > However, it's not a common usage :/
> > > > 
> > > > So this is needed even though it's called under preempt_disable().
> > > > 
> > > > Paul, do we need to add an rcu_read_lock() because the code in rht
> > > > (rhashtable) requires RCU read lock?
> > > > 
> > > > I thought that rcu_read_lock() and preempt_disable() have been merged?
> > > 
> > > Yes, preempt_disable() does indeed start an RCU read-side critical section,
> > > just as surely as rcu_read_lock() does.
> > > 
> > > However, this is a lockdep check inside of __rhashtable_lookup():
> > > 
> > > 	rht_dereference_rcu(ht->tbl, ht)
> > > 
> > > Which is defined as:
> > > 
> > > 	rcu_dereference_check(p, lockdep_rht_mutex_is_held(ht));
> > > 
> > > This is explicitly telling lockdep that rcu_read_lock() is OK and
> > > holding ht->mutex is OK, but nothing else is.
> > 
> > That is similar to the kprobes, which also allows accessing in
> > rcu critical section or under mutex.
> > 
> > > So an alternative way to fix this is to declare it to be a false positive,
> > > and then avoid that false positive by adding a check that preemption
> > > is disabled.  Adding the rhashtable maintainers for their perspective.
> > 
> > What about changing it alloing it with preempt disabled flag?
> 
> I am not sure that "it" that you are proposing changing.  ;-)

Sorry, Ii meant the rcu_dereference_check().

> 
> However, another option for the the above rcu_dereference_check() to
> become something like this:
> 
> 	rcu_dereference_check(p, lockdep_rht_mutex_is_held(ht) ||
> 				 rcu_read_lock_any_held());
> 
> This would be happy with any RCU reader, including rcu_read_lock(),
> preempt_disable(), local_irq_disable(), local_bh_disable(), and various
> handler contexts.  One downside is that this would *always* be happy in
> a kernel built with CONFIG_PREEMPT_{NONE,VOLUNTARY}=y.

Ah, indeed. This means that we lose the ability to explicitly check
whether the rcu pointer is in a critical section on that kernel.

> 
> If this is happening often enough, it would be easy for me to create an
> rcu_dereference_all_check() that allows all forms of vanilla RCU readers
> (but not, for example, SRCU readers), but with only two use cases,
> it is not clear to me that this is an overall win.

OK, I think this discussion is important for the patch from Menglong [1]

[1] https://lore.kernel.org/all/20250829021436.19982-1-dongml2@chinatelecom.cn/

because this does not make an rcu critical section while using `head`
but it works because fprobe_entry() runs under preempt_disable().

Is it better to use `guard(rcu)()` instead of rcu_read_lock() so that
it explicitly secure the `head` usage? I just wonder if there is any
downside to extend rcu_read_lock() area (still in the same
preempt_disable section).

Thank you,

> 
> Or am I missing a turn in here somewhere?
> 
> 							Thanx, Paul
> 
> > Thank you,
> > 
> > > 
> > > 							Thanx, Paul
> > > 
> > > > -- Steve
> > > > 
> > > > 
> > > > > 
> > > > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > > Closes: https://lore.kernel.org/oe-lkp/202508281655.54c87330-lkp@intel.com
> > > > > Fixes: dfe0d675df82 ("tracing: fprobe: use rhltable for fprobe_ip_table")
> > > > > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > > > > ---
> > > > >  kernel/trace/fprobe.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > > 
> > > > > 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)
> > > > 
> > 
> > 
> > -- 
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[Paul E. McKenney]

On Tue, Sep 02, 2025 at 03:59:53PM +0900, Masami Hiramatsu wrote:
> On Mon, 1 Sep 2025 08:00:15 -0700
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> > On Mon, Sep 01, 2025 at 05:06:55PM +0900, Masami Hiramatsu wrote:
> > > On Fri, 29 Aug 2025 04:11:02 -0700
> > > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > 
> > > > On Thu, Aug 28, 2025 at 10:23:57PM -0400, Steven Rostedt wrote:
> > > > > On Fri, 29 Aug 2025 10:14:36 +0800
> > > > > Menglong Dong <dongml2@chinatelecom.cn> wrote:
> > > > > 
> > > > > > rcu_read_lock() is not needed in fprobe_entry, but rcu_dereference_check()
> > > > > > is used in rhltable_lookup(), which causes suspicious RCU usage warning:
> > > > > > 
> > > > > >   WARNING: suspicious RCU usage
> > > > > >   6.17.0-rc1-00001-gdfe0d675df82 #1 Tainted: G S
> > > > > >   -----------------------------
> > > > > >   include/linux/rhashtable.h:602 suspicious rcu_dereference_check() usage!
> > > > > >   ......
> > > > > >   stack backtrace:
> > > > > >   CPU: 1 UID: 0 PID: 4652 Comm: ftracetest Tainted: G S
> > > > > >   Tainted: [S]=CPU_OUT_OF_SPEC, [I]=FIRMWARE_WORKAROUND
> > > > > >   Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
> > > > > >   Call Trace:
> > > > > >    <TASK>
> > > > > >    dump_stack_lvl+0x7c/0x90
> > > > > >    lockdep_rcu_suspicious+0x14f/0x1c0
> > > > > >    __rhashtable_lookup+0x1e0/0x260
> > > > > >    ? __pfx_kernel_clone+0x10/0x10
> > > > > >    fprobe_entry+0x9a/0x450
> > > > > >    ? __lock_acquire+0x6b0/0xca0
> > > > > >    ? find_held_lock+0x2b/0x80
> > > > > >    ? __pfx_fprobe_entry+0x10/0x10
> > > > > >    ? __pfx_kernel_clone+0x10/0x10
> > > > > >    ? lock_acquire+0x14c/0x2d0
> > > > > >    ? __might_fault+0x74/0xc0
> > > > > >    function_graph_enter_regs+0x2a0/0x550
> > > > > >    ? __do_sys_clone+0xb5/0x100
> > > > > >    ? __pfx_function_graph_enter_regs+0x10/0x10
> > > > > >    ? _copy_to_user+0x58/0x70
> > > > > >    ? __pfx_kernel_clone+0x10/0x10
> > > > > >    ? __x64_sys_rt_sigprocmask+0x114/0x180
> > > > > >    ? __pfx___x64_sys_rt_sigprocmask+0x10/0x10
> > > > > >    ? __pfx_kernel_clone+0x10/0x10
> > > > > >    ftrace_graph_func+0x87/0xb0
> > > > > > 
> > > > > > Fix this by using rcu_read_lock() for rhltable_lookup(). Alternatively, we
> > > > > > can use rcu_lock_acquire(&rcu_lock_map) here to obtain better performance.
> > > > > > However, it's not a common usage :/
> > > > > 
> > > > > So this is needed even though it's called under preempt_disable().
> > > > > 
> > > > > Paul, do we need to add an rcu_read_lock() because the code in rht
> > > > > (rhashtable) requires RCU read lock?
> > > > > 
> > > > > I thought that rcu_read_lock() and preempt_disable() have been merged?
> > > > 
> > > > Yes, preempt_disable() does indeed start an RCU read-side critical section,
> > > > just as surely as rcu_read_lock() does.
> > > > 
> > > > However, this is a lockdep check inside of __rhashtable_lookup():
> > > > 
> > > > 	rht_dereference_rcu(ht->tbl, ht)
> > > > 
> > > > Which is defined as:
> > > > 
> > > > 	rcu_dereference_check(p, lockdep_rht_mutex_is_held(ht));
> > > > 
> > > > This is explicitly telling lockdep that rcu_read_lock() is OK and
> > > > holding ht->mutex is OK, but nothing else is.
> > > 
> > > That is similar to the kprobes, which also allows accessing in
> > > rcu critical section or under mutex.
> > > 
> > > > So an alternative way to fix this is to declare it to be a false positive,
> > > > and then avoid that false positive by adding a check that preemption
> > > > is disabled.  Adding the rhashtable maintainers for their perspective.
> > > 
> > > What about changing it alloing it with preempt disabled flag?
> > 
> > I am not sure that "it" that you are proposing changing.  ;-)
> 
> Sorry, Ii meant the rcu_dereference_check().
> 
> > 
> > However, another option for the the above rcu_dereference_check() to
> > become something like this:
> > 
> > 	rcu_dereference_check(p, lockdep_rht_mutex_is_held(ht) ||
> > 				 rcu_read_lock_any_held());
> > 
> > This would be happy with any RCU reader, including rcu_read_lock(),
> > preempt_disable(), local_irq_disable(), local_bh_disable(), and various
> > handler contexts.  One downside is that this would *always* be happy in
> > a kernel built with CONFIG_PREEMPT_{NONE,VOLUNTARY}=y.
> 
> Ah, indeed. This means that we lose the ability to explicitly check
> whether the rcu pointer is in a critical section on that kernel.

It is a usability/bug-detection design tradeoff, and as such, the RCU
user's choice.  RCU is simply an arms supplier on this one.  ;-)

> > If this is happening often enough, it would be easy for me to create an
> > rcu_dereference_all_check() that allows all forms of vanilla RCU readers
> > (but not, for example, SRCU readers), but with only two use cases,
> > it is not clear to me that this is an overall win.
> 
> OK, I think this discussion is important for the patch from Menglong [1]
> 
> [1] https://lore.kernel.org/all/20250829021436.19982-1-dongml2@chinatelecom.cn/
> 
> because this does not make an rcu critical section while using `head`
> but it works because fprobe_entry() runs under preempt_disable().

Agreed, and that appears to be what initiated this dicussion.

> Is it better to use `guard(rcu)()` instead of rcu_read_lock() so that
> it explicitly secure the `head` usage? I just wonder if there is any
> downside to extend rcu_read_lock() area (still in the same
> preempt_disable section).

Another option is `scoped_guard(rcu)`, for example, as used in
ftrace_find_callable_addr() in arch/loongarch/kernel/ftrace_dyn.c.
That way you keep the RAII usability while keeping the RCU read-side
critical section small.

							Thanx, Paul

> Thank you,
> 
> > 
> > Or am I missing a turn in here somewhere?
> > 
> > 							Thanx, Paul
> > 
> > > Thank you,
> > > 
> > > > 
> > > > 							Thanx, Paul
> > > > 
> > > > > -- Steve
> > > > > 
> > > > > 
> > > > > > 
> > > > > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > > > Closes: https://lore.kernel.org/oe-lkp/202508281655.54c87330-lkp@intel.com
> > > > > > Fixes: dfe0d675df82 ("tracing: fprobe: use rhltable for fprobe_ip_table")
> > > > > > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > > > > > ---
> > > > > >  kernel/trace/fprobe.c | 2 ++
> > > > > >  1 file changed, 2 insertions(+)
> > > > > > 
> > > > > > 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)
> > > > > 
> > > 
> > > 
> > > -- 
> > > Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[Herbert Xu]

On Mon, Sep 01, 2025 at 08:00:15AM -0700, Paul E. McKenney wrote:
>
> If this is happening often enough, it would be easy for me to create an
> rcu_dereference_all_check() that allows all forms of vanilla RCU readers
> (but not, for example, SRCU readers), but with only two use cases,
> it is not clear to me that this is an overall win.

Hi Paul:

Please create such a helper.  Because the alternative is for me
to do something like this in rhashtable:

#define rht_dereference_rcu(p, ht) \
	rcu_dereference_check(p, lockdep_rht_mutex_is_held(ht) || \
				 rcu_read_lock_any_held())

This really makes no sense because rcu_read_lock_any_held is an
internal RCU implementation detail and has nothing to do with
rhashtable.

rhashtable is just a middle-man like RCU.  The actual context
(be it vanilla, bh or sched RCU) used is entirely up to the user.

Actually what puzzles me is why can't we just get rid of the
bh and sched variants of rcu_dereference? After all, there is
only one synchronize_rcu/call_rcu and it supports all three
variants.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH] rhashtable: Use rcu_dereference_all and rcu_dereference_all_check

[Herbert Xu]

On Mon, Sep 01, 2025 at 08:00:15AM -0700, Paul E. McKenney wrote:
>
> However, another option for the the above rcu_dereference_check() to
> become something like this:
> 
> 	rcu_dereference_check(p, lockdep_rht_mutex_is_held(ht) ||
> 				 rcu_read_lock_any_held());
> 
> This would be happy with any RCU reader, including rcu_read_lock(),
> preempt_disable(), local_irq_disable(), local_bh_disable(), and various
> handler contexts.  One downside is that this would *always* be happy in
> a kernel built with CONFIG_PREEMPT_{NONE,VOLUNTARY}=y.

Why is that a problem? Assuming that it is always safe to perform
an rcu_dereference_all() on such a kernel, not warning would appear
to be the right thing to do.

> If this is happening often enough, it would be easy for me to create an
> rcu_dereference_all_check() that allows all forms of vanilla RCU readers
> (but not, for example, SRCU readers), but with only two use cases,
> it is not clear to me that this is an overall win.
> 
> Or am I missing a turn in here somewhere?

As I said rhashtable is just a middle-man, while it does some
RCU actions internally, in the end the user is free to choose
the RCU variant (vanilla, bh, sched).  So I'm faced with the
choice of using only rcu_dereference_all in rhashtable code,
or splitting the rhashtable API into three variants just like
RCU.  I'd rather not do this especially because it appears RCU
itself has essentially morphed into a single variant because
call_rcu etc. always waits for all three variants.

So how about this patch?

---8<---
Add rcu_dereference_all and rcu_dereference_all_check so that
library code such as rhashtable can be used with any RCU variant.

As it stands rcu_dereference is used within rashtable, which
creates false-positive warnings if the user calls it from another
RCU variant context, such as preempt_disable().

Use the rcu_dereference_all and rcu_dereference_all_check calls
in rhashtable to suppress these warnings.

Also replace the rcu_dereference_raw calls in the list iterators
with rcu_dereference_all to uncover buggy calls.

Reported-by: Menglong Dong <dongml2@chinatelecom.cn>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 120536f4c6eb..b4261a0498f0 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -713,6 +713,23 @@ do {									      \
 				(c) || rcu_read_lock_sched_held(), \
 				__rcu)
 
+/**
+ * rcu_dereference_all_check() - rcu_dereference_all with debug checking
+ * @p: The pointer to read, prior to dereferencing
+ * @c: The conditions under which the dereference will take place
+ *
+ * This is the RCU-all counterpart to rcu_dereference_check().
+ * However, please note that starting in v5.0 kernels, vanilla RCU grace
+ * periods wait for preempt_disable() regions of code in addition to
+ * regions of code demarked by rcu_read_lock() and rcu_read_unlock().
+ * This means that synchronize_rcu(), call_rcu, and friends all take not
+ * only rcu_read_lock() but also rcu_read_lock_*() into account.
+ */
+#define rcu_dereference_all_check(p, c) \
+	__rcu_dereference_check((p), __UNIQUE_ID(rcu), \
+				(c) || rcu_read_lock_any_held(), \
+				__rcu)
+
 /*
  * The tracing infrastructure traces RCU (we want that), but unfortunately
  * some of the RCU checks causes tracing to lock up the system.
@@ -767,6 +784,14 @@ do {									      \
  */
 #define rcu_dereference_sched(p) rcu_dereference_sched_check(p, 0)
 
+/**
+ * rcu_dereference_all() - fetch RCU-all-protected pointer for dereferencing
+ * @p: The pointer to read, prior to dereferencing
+ *
+ * Makes rcu_dereference_check() do the dirty work.
+ */
+#define rcu_dereference_all(p) rcu_dereference_all_check(p, 0)
+
 /**
  * rcu_pointer_handoff() - Hand off a pointer from RCU to other mechanism
  * @p: The pointer to hand off
diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index 6c85b28ea30b..e9b5ac26b42d 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -272,13 +272,13 @@ struct rhash_lock_head __rcu **rht_bucket_nested_insert(
 	rcu_dereference_protected(p, lockdep_rht_mutex_is_held(ht))
 
 #define rht_dereference_rcu(p, ht) \
-	rcu_dereference_check(p, lockdep_rht_mutex_is_held(ht))
+	rcu_dereference_all_check(p, lockdep_rht_mutex_is_held(ht))
 
 #define rht_dereference_bucket(p, tbl, hash) \
 	rcu_dereference_protected(p, lockdep_rht_bucket_is_held(tbl, hash))
 
 #define rht_dereference_bucket_rcu(p, tbl, hash) \
-	rcu_dereference_check(p, lockdep_rht_bucket_is_held(tbl, hash))
+	rcu_dereference_all_check(p, lockdep_rht_bucket_is_held(tbl, hash))
 
 #define rht_entry(tpos, pos, member) \
 	({ tpos = container_of(pos, typeof(*tpos), member); 1; })
@@ -373,7 +373,7 @@ static inline struct rhash_head *__rht_ptr(
 static inline struct rhash_head *rht_ptr_rcu(
 	struct rhash_lock_head __rcu *const *bkt)
 {
-	return __rht_ptr(rcu_dereference(*bkt), bkt);
+	return __rht_ptr(rcu_dereference_all(*bkt), bkt);
 }
 
 static inline struct rhash_head *rht_ptr(
@@ -497,7 +497,7 @@ static inline void rht_assign_unlock(struct bucket_table *tbl,
 	for (({barrier(); }),						\
 	     pos = head;						\
 	     !rht_is_a_nulls(pos);					\
-	     pos = rcu_dereference_raw(pos->next))
+	     pos = rcu_dereference_all(pos->next))
 
 /**
  * rht_for_each_rcu - iterate over rcu hash chain
@@ -513,7 +513,7 @@ static inline void rht_assign_unlock(struct bucket_table *tbl,
 	for (({barrier(); }),					\
 	     pos = rht_ptr_rcu(rht_bucket(tbl, hash));		\
 	     !rht_is_a_nulls(pos);				\
-	     pos = rcu_dereference_raw(pos->next))
+	     pos = rcu_dereference_all(pos->next))
 
 /**
  * rht_for_each_entry_rcu_from - iterated over rcu hash chain from given head
@@ -560,7 +560,7 @@ static inline void rht_assign_unlock(struct bucket_table *tbl,
  * list returned by rhltable_lookup.
  */
 #define rhl_for_each_rcu(pos, list)					\
-	for (pos = list; pos; pos = rcu_dereference_raw(pos->next))
+	for (pos = list; pos; pos = rcu_dereference_all(pos->next))
 
 /**
  * rhl_for_each_entry_rcu - iterate over rcu hash table list of given type
@@ -574,7 +574,7 @@ static inline void rht_assign_unlock(struct bucket_table *tbl,
  */
 #define rhl_for_each_entry_rcu(tpos, pos, list, member)			\
 	for (pos = list; pos && rht_entry(tpos, pos, member);		\
-	     pos = rcu_dereference_raw(pos->next))
+	     pos = rcu_dereference_all(pos->next))
 
 static inline int rhashtable_compare(struct rhashtable_compare_arg *arg,
 				     const void *obj)

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Herbert Xu]

Paul E. McKenney <paulmck@kernel.org> wrote:
>
> Yes, preempt_disable() does indeed start an RCU read-side critical section,
> just as surely as rcu_read_lock() does.
> 
> However, this is a lockdep check inside of __rhashtable_lookup():
> 
>        rht_dereference_rcu(ht->tbl, ht)
> 
> Which is defined as:
> 
>        rcu_dereference_check(p, lockdep_rht_mutex_is_held(ht));
> 
> This is explicitly telling lockdep that rcu_read_lock() is OK and
> holding ht->mutex is OK, but nothing else is.

I think that's a deficiency in rcu_dereference_check.

Yes I could certainly add a preemption check to rht_dereference_rcu,
but that makes zero sense because this is an implementation detail
of RCU and there is no reason why this logic should be added to
rhashtable.

rhashtable never relies on the fact that turning off preemption
creates is safe for RCU reads, so it makes no sense to add this
logic to rht_dereference_rcu since RCU could conceivably (even
if it is unlikely) be changed on day so that turning off preemption
is no longer safe for RCU reads.

My preference would be to add the preemption test to
rcu_dereference_check, or if that is not possible for some reason,
create a new RCU helper that includes the preemption test.

Of course just adding RCU read locks as this patch does is also
fine.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Masami Hiramatsu]

On Fri, 29 Aug 2025 10:14:36 +0800
Menglong Dong <dongml2@chinatelecom.cn> wrote:

> rcu_read_lock() is not needed in fprobe_entry, but rcu_dereference_check()
> is used in rhltable_lookup(), which causes suspicious RCU usage warning:
> 
>   WARNING: suspicious RCU usage
>   6.17.0-rc1-00001-gdfe0d675df82 #1 Tainted: G S
>   -----------------------------
>   include/linux/rhashtable.h:602 suspicious rcu_dereference_check() usage!
>   ......
>   stack backtrace:
>   CPU: 1 UID: 0 PID: 4652 Comm: ftracetest Tainted: G S
>   Tainted: [S]=CPU_OUT_OF_SPEC, [I]=FIRMWARE_WORKAROUND
>   Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
>   Call Trace:
>    <TASK>
>    dump_stack_lvl+0x7c/0x90
>    lockdep_rcu_suspicious+0x14f/0x1c0
>    __rhashtable_lookup+0x1e0/0x260
>    ? __pfx_kernel_clone+0x10/0x10
>    fprobe_entry+0x9a/0x450
>    ? __lock_acquire+0x6b0/0xca0
>    ? find_held_lock+0x2b/0x80
>    ? __pfx_fprobe_entry+0x10/0x10
>    ? __pfx_kernel_clone+0x10/0x10
>    ? lock_acquire+0x14c/0x2d0
>    ? __might_fault+0x74/0xc0
>    function_graph_enter_regs+0x2a0/0x550
>    ? __do_sys_clone+0xb5/0x100
>    ? __pfx_function_graph_enter_regs+0x10/0x10
>    ? _copy_to_user+0x58/0x70
>    ? __pfx_kernel_clone+0x10/0x10
>    ? __x64_sys_rt_sigprocmask+0x114/0x180
>    ? __pfx___x64_sys_rt_sigprocmask+0x10/0x10
>    ? __pfx_kernel_clone+0x10/0x10
>    ftrace_graph_func+0x87/0xb0
> 
> Fix this by using rcu_read_lock() for rhltable_lookup(). Alternatively, we
> can use rcu_lock_acquire(&rcu_lock_map) here to obtain better performance.

Yeah, that kind of trick may not good. 

> However, it's not a common usage :/
> 
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202508281655.54c87330-lkp@intel.com
> Fixes: dfe0d675df82 ("tracing: fprobe: use rhltable for fprobe_ip_table")
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>

OK, I agree this fixes the problem. Let me pick it.

> ---
>  kernel/trace/fprobe.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> 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)
> -- 
> 2.51.0
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[Herbert Xu]

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.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[menglong.dong]

On 2025/9/2 17:17 Herbert Xu <herbert@gondor.apana.org.au> write:
> 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.

Yeah, I understand. The rcu_read_lock() here is totally used to
suppress the suspicious rcu usage warning, not for the protection.
So I used it just for the rhltable_lookup() to reduce the impact.
Maybe I should add some comment for it.

This is the easiest way to suppress the warning, but not the  best,
as it can introduce addition overhead when PREEMPT is enabled.

As Masami said, maybe we can use guard(rcu)() here to obtain
better code readability.

It seems that it's hard to think of a way to suppress the warning
without holding the rcu_read_lock :/

Thanks!
Menglong Dong

> 
> Thanks,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> 
> 

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

[Herbert Xu]

On Tue, Sep 02, 2025 at 05:50:32PM +0800, menglong.dong@linux.dev wrote:
> On 2025/9/2 17:17 Herbert Xu <herbert@gondor.apana.org.au> write:
> > 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.
> 
> Yeah, I understand. The rcu_read_lock() here is totally used to
> suppress the suspicious rcu usage warning, not for the protection.
> So I used it just for the rhltable_lookup() to reduce the impact.
> Maybe I should add some comment for it.

My point is that after a lookup you will be doing some sort of a
dereference on the RCU pointer.  That would cause exactly the same
splat that rhltable_lookup itself generated.

For example, rhl_for_each_entry_rcu should have created the same
warning, but it doesn't because for some reason it is using
rcu_dereference_raw.  I'll need to dig up the history of this
to see if there is a good reason for it to not warn.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Menglong Dong]

On 2025/9/3 12:22 Herbert Xu <herbert@gondor.apana.org.au> write:
> On Tue, Sep 02, 2025 at 05:50:32PM +0800, menglong.dong@linux.dev wrote:
> > On 2025/9/2 17:17 Herbert Xu <herbert@gondor.apana.org.au> write:
> > > 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.
> > 
> > Yeah, I understand. The rcu_read_lock() here is totally used to
> > suppress the suspicious rcu usage warning, not for the protection.
> > So I used it just for the rhltable_lookup() to reduce the impact.
> > Maybe I should add some comment for it.
> 
> My point is that after a lookup you will be doing some sort of a
> dereference on the RCU pointer.  That would cause exactly the same
> splat that rhltable_lookup itself generated.
> 
> For example, rhl_for_each_entry_rcu should have created the same
> warning, but it doesn't because for some reason it is using
> rcu_dereference_raw.  I'll need to dig up the history of this
> to see if there is a good reason for it to not warn.

Yeah, I understand what you mean. I noticed this, and that's why
I added the rcu_read_lock() for rhashtable_lookup() only.

Maybe it is to obtain better performance? Just guess ;)
And hlist_for_each_entry_rcu() also uses rcu_dereference_raw().

Thanks!
Menglong Dong
> 
> Cheers,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> 

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

[Masami Hiramatsu]

On Thu, 04 Sep 2025 11:37:35 +0800
Menglong Dong <menglong.dong@linux.dev> wrote:

> On 2025/9/3 12:22 Herbert Xu <herbert@gondor.apana.org.au> write:
> > On Tue, Sep 02, 2025 at 05:50:32PM +0800, menglong.dong@linux.dev wrote:
> > > On 2025/9/2 17:17 Herbert Xu <herbert@gondor.apana.org.au> write:
> > > > 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.
> > > 
> > > Yeah, I understand. The rcu_read_lock() here is totally used to
> > > suppress the suspicious rcu usage warning, not for the protection.
> > > So I used it just for the rhltable_lookup() to reduce the impact.
> > > Maybe I should add some comment for it.
> > 
> > My point is that after a lookup you will be doing some sort of a
> > dereference on the RCU pointer.  That would cause exactly the same
> > splat that rhltable_lookup itself generated.
> > 
> > For example, rhl_for_each_entry_rcu should have created the same
> > warning, but it doesn't because for some reason it is using
> > rcu_dereference_raw.  I'll need to dig up the history of this
> > to see if there is a good reason for it to not warn.
> 
> Yeah, I understand what you mean. I noticed this, and that's why
> I added the rcu_read_lock() for rhashtable_lookup() only.
> 
> Maybe it is to obtain better performance? Just guess ;)
> And hlist_for_each_entry_rcu() also uses rcu_dereference_raw().

Hi Menglong, if you update the patch to use guard(rcu)() because
head is used repeatedly in fprobe_entry(), I can replace it.

Thank you,

> 
> Thanks!
> Menglong Dong
> > 
> > Cheers,
> > -- 
> > Email: Herbert Xu <herbert@gondor.apana.org.au>
> > Home Page: http://gondor.apana.org.au/~herbert/
> > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> > 
> 
> 
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[Menglong Dong]

On 2025/9/4 12:29 Masami Hiramatsu <mhiramat@kernel.org> write:
> On Thu, 04 Sep 2025 11:37:35 +0800
> Menglong Dong <menglong.dong@linux.dev> wrote:
> 
> > On 2025/9/3 12:22 Herbert Xu <herbert@gondor.apana.org.au> write:
> > > On Tue, Sep 02, 2025 at 05:50:32PM +0800, menglong.dong@linux.dev wrote:
> > > > On 2025/9/2 17:17 Herbert Xu <herbert@gondor.apana.org.au> write:
> > > > > 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.
> > > > 
> > > > Yeah, I understand. The rcu_read_lock() here is totally used to
> > > > suppress the suspicious rcu usage warning, not for the protection.
> > > > So I used it just for the rhltable_lookup() to reduce the impact.
> > > > Maybe I should add some comment for it.
> > > 
> > > My point is that after a lookup you will be doing some sort of a
> > > dereference on the RCU pointer.  That would cause exactly the same
> > > splat that rhltable_lookup itself generated.
> > > 
> > > For example, rhl_for_each_entry_rcu should have created the same
> > > warning, but it doesn't because for some reason it is using
> > > rcu_dereference_raw.  I'll need to dig up the history of this
> > > to see if there is a good reason for it to not warn.
> > 
> > Yeah, I understand what you mean. I noticed this, and that's why
> > I added the rcu_read_lock() for rhashtable_lookup() only.
> > 
> > Maybe it is to obtain better performance? Just guess ;)
> > And hlist_for_each_entry_rcu() also uses rcu_dereference_raw().
> 
> Hi Menglong, if you update the patch to use guard(rcu)() because
> head is used repeatedly in fprobe_entry(), I can replace it.

Of course, with pleasure. I can send a new version of this
patch with guard(rcu)() instead.

> 
> Thank you,
> 
> > 
> > Thanks!
> > Menglong Dong
> > > 
> > > Cheers,
> > > -- 
> > > Email: Herbert Xu <herbert@gondor.apana.org.au>
> > > Home Page: http://gondor.apana.org.au/~herbert/
> > > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> > > 
> > 
> > 
> > 
> > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 

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

[Herbert Xu]

On Thu, Sep 04, 2025 at 11:37:35AM +0800, Menglong Dong wrote:
>
> Yeah, I understand what you mean. I noticed this, and that's why
> I added the rcu_read_lock() for rhashtable_lookup() only.
> 
> Maybe it is to obtain better performance? Just guess ;)
> And hlist_for_each_entry_rcu() also uses rcu_dereference_raw().

I did some digging and this appears to be the source of the
rcu_dereference_raw call:

commit 3120438ad68601f341e61e7cb1323b0e1a6ca367
Author: Paul E. McKenney <paulmck@kernel.org>
Date:   Mon Feb 22 17:04:48 2010 -0800

    rcu: Disable lockdep checking in RCU list-traversal primitives

    The theory is that use of bare rcu_dereference() is more prone
    to error than use of the RCU list-traversal primitives.
    Therefore, disable lockdep RCU read-side critical-section
    checking in these primitives for the time being.  Once all of
    the rcu_dereference() uses have been dealt with, it may be time
    to re-enable lockdep checking for the RCU list-traversal
    primitives.

So I don't think there is a good reason to use rcu_dereference_raw
here at all.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Steven Rostedt]

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();

-- Steve

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

[Herbert Xu]

On Tue, Sep 02, 2025 at 10:57:57AM -0400, Steven Rostedt wrote:
>
> And then have:
> 
>        quiet_rcu_read_lock_check();
>        head = rhltable_lookup(&fprobe_ip_table, &func, fprobe_rht_params);
>        quiet_rcu_read_unlock_check();

The thing is that rhl_for_each_entry_rcu which is called right after
your unlock above should have created the same warning as
rhltable_lookup.  The fact that it doesn't appears to be a bug:
it's using rcu_dereference_raw and I don't see why that's safe
at all.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[menglong.dong]

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
> 
>