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]

Date: Mon, 25 Mar 2024 10:33:52 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Johannes Berg <johannes@...solutions.net>
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, Mar 25, 2024 at 05:41:22PM +0100, Johannes Berg wrote:
> 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.
> 

Fair enough.

> I can add a comment in there, I guess, something like
> 
>  /* sparse doesn't actually "call" cleanup functions */
> 

Yeah, that's helpful.

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

Indeed.

Regards,
Boqun

> johannes