| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <615da75d-cb2f-4e7e-9e11-6b19f03fea6c@paulmck-laptop>
Date: Mon, 1 Sep 2025 08:00:15 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Masami Hiramatsu <mhiramat@...nel.org>
Cc: Steven Rostedt <rostedt@...dmis.org>,
Menglong Dong <dongml2@...natelecom.cn>,
mathieu.desnoyers@...icios.com, linux-kernel@...r.kernel.org,
linux-trace-kernel@...r.kernel.org,
kernel test robot <oliver.sang@...el.com>, tgraf@...g.ch,
herbert@...dor.apana.org.au, linux-crypto@...r.kernel.org
Subject: Re: [PATCH] tracing: fprobe: fix suspicious rcu usage in fprobe_entry
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@...nel.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@...natelecom.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@...el.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@...natelecom.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@...nel.org>
Powered by blists - more mailing lists