linux-kernel - Re: [PATCH] rcu: mollify sparse with RCU guard

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: <055111792727869a98c1fa693014e0b6f5d256ea.camel@sipsolutions.net>
Date: Mon, 25 Mar 2024 17:41:22 +0100
From: Johannes Berg <johannes@...solutions.net>
To: Boqun Feng <boqun.feng@...il.com>
Cc: rcu@...r.kernel.org, linux-kernel@...r.kernel.org, "Paul E. McKenney"
 <paulmck@...nel.org>, Frederic Weisbecker <frederic@...nel.org>, Josh
 Triplett <josh@...htriplett.org>, Peter Zijlstra <peterz@...radead.org>,
 Dan Carpenter <dan.carpenter@...aro.org>
Subject: Re: [PATCH] rcu: mollify sparse with RCU guard

On Mon, 2024-03-25 at 09:35 -0700, Boqun Feng wrote: 
> > -DEFINE_LOCK_GUARD_0(rcu, rcu_read_lock(), rcu_read_unlock())
> > +DEFINE_LOCK_GUARD_0(rcu, do { rcu_read_lock(); __release(RCU); } while(0), rcu_read_unlock())
> >  
> 
> Hmm.. not a big fan of this. __release(RCU) following a rcu_read_lock()
> is really confusing. Maybe we can introduce a _rcu_read_lock():
> 
> 	void _rcu_read_lock(bool guard) {
> 		__rcu_read_lock();
> 		// Skip sparse annotation in "guard(rcu)()" to work
> 		// around sparse's lack of support of cleanup.
> 		if (!guard)
> 			__acquire(RCU);
> 		rcu_lock_acquire(...);
> 		...
> 	}
> 
> and normal rcu_read_lock() is just a _rcu_read_lock(false), RCU guard is
> a _rcu_read_lock(true)?

Not sure I see any value in that, that's pretty much equivalent but
seems IMHO less specific, where here we know we really want this only in
this case. I don't see any other case where we'd want to ever "call"
_rcu_read_lock(true).

Also __acquire()/__release() are just empty macros without __CHECKER__.
So not sure the indirection really is warranted for this special case.

I can add a comment in there, I guess, something like

 /* sparse doesn't actually "call" cleanup functions */

perhaps. That reminds me I forgot to CC Dan ...

> But before that how does it looks if we don't fix this entirely? ;-)

Well basically every time you write

void myfunc(void)
{
  guard(rcu)();
  ...
}

sparse will complain about mismatched locks, which is _really_ annoying
for e.g. networking where there's (a) a kind of "no new warnings" rule,
and (b) sparse is actually important for all the endian annotations etc.

Which right now means that we can't use all this new machinery, which is
a shame.

johannes