linux-kernel - Re: [PATCH v2 00/12] uprobes: add batched register/unregister APIs and per-CPU RW semaphore

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6b728325-b628-488f-aabf-dbd9afa388fb@paulmck-laptop>
Date: Wed, 3 Jul 2024 07:08:21 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Andrii Nakryiko <andrii.nakryiko@...il.com>,
	Andrii Nakryiko <andrii@...nel.org>,
	linux-trace-kernel@...r.kernel.org, rostedt@...dmis.org,
	mhiramat@...nel.org, oleg@...hat.com, mingo@...hat.com,
	bpf@...r.kernel.org, jolsa@...nel.org, clm@...a.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 00/12] uprobes: add batched register/unregister APIs
 and per-CPU RW semaphore

On Wed, Jul 03, 2024 at 09:50:57AM +0200, Peter Zijlstra wrote:
> On Tue, Jul 02, 2024 at 04:56:53PM -0700, Paul E. McKenney wrote:
> 
> > > Paul, isn't this the RCU flavour you created to deal with
> > > !rcu_is_watching()? The flavour that never should have been created in
> > > favour of just cleaning up the mess instead of making more.
> > 
> > My guess is that you are instead thinking of RCU Tasks Rude, which can
> > be eliminated once all architectures get their entry/exit/deep-idle
> > functions either inlined or marked noinstr.
> 
> Would it make sense to disable it for those architectures that have
> already done this work?

It might well.  Any architectures other than x86 at this point?

But this is still used in common code, so let's see...  In that case,
synchronize_rcu_tasks_rude() becomes a no-op, call_rcu_tasks_rude() can be
a wrapper around something like queue_work(), and rcu_barrier_tasks_rude()
can be a wrapper around something like flush_work().

Except that call_rcu_tasks_rude() and rcu_barrier_tasks_rude() are not
actually used outside of testing, so maybe they can be dropped globally.

Let me see what happens when I do this:

diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
index 7d18b90356fd..5c8492a054f5 100644
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -936,8 +936,8 @@ static struct rcu_torture_ops tasks_rude_ops = {
 	.deferred_free	= rcu_tasks_rude_torture_deferred_free,
 	.sync		= synchronize_rcu_tasks_rude,
 	.exp_sync	= synchronize_rcu_tasks_rude,
-	.call		= call_rcu_tasks_rude,
-	.cb_barrier	= rcu_barrier_tasks_rude,
+	// .call		= call_rcu_tasks_rude,
+	// .cb_barrier	= rcu_barrier_tasks_rude,
 	.gp_kthread_dbg	= show_rcu_tasks_rude_gp_kthread,
 	.get_gp_data	= rcu_tasks_rude_get_gp_data,
 	.cbflood_max	= 50000,

It should be at least mildly amusing...

> > > > I will
> > > > ultimately use it anyway to avoid uprobe taking unnecessary refcount
> > > > and to protect uprobe->consumers iteration and uc->handler() calls,
> > > > which could be sleepable, so would need rcu_read_lock_trace().
> > > 
> > > I don't think you need trace-rcu for that. SRCU would do nicely I think.
> > 
> > From a functional viewpoint, agreed.
> > 
> > However, in the past, the memory-barrier and array-indexing overhead
> > of SRCU has made it a no-go for lightweight probes into fastpath code.
> > And these cases were what motivated RCU Tasks Trace (as opposed to RCU
> > Tasks Rude).
> 
> I'm thinking we're growing too many RCU flavours again :/ I suppose I'll
> have to go read up on rcu/tasks.* and see what's what.

Well, you are in luck.  I am well along with the task of putting together
the 2024 LWN RCU API article, which will include RCU Tasks Trace.  ;-)

And I do sympathize with discomfort with lots of RCU flavors.  After all,
hhad you told me 30 years ago that there would be more than one flavor,
I would have been quite surprised.  Of course, I would also have been
surprised by a great many other things (just how many flavors of locking
and reference counting???), so maybe having three flavors (four if we
cannot drop RCU Tasks RUDE) is not so bad.

Oh, and no one is yet using srcu_down_read() and srcu_up_read(), so
if they stay unused for another year or so...

> > The other rule for RCU Tasks Trace is that although readers are permitted
> > to block, this blocking can be for no longer than a major page fault.
> > If you need longer-term blocking, then you should instead use SRCU.
> 
> I think this would render it unsuitable for uprobes. The whole point of
> having a sleepable handler is to be able to take faults.

???

I said "longer than a major page fault", so it is OK to take a fault,
just not one that potentially blocks forever.  (And those faulting onto
things like NFS filesystems have enough other problems that this would
be in the noise for them, right?)

And yes, RCU Tasks Trace is specialized.  I would expect that unexpected
new uses would get serious scrutiny by those currently using it.

							Thanx, Paul