linux-kernel - Re: [PATCH RFC 15/24] rcu: Support Clang's capability analysis

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: <aa50d616-fdbb-4c68-86ff-82bb57aaa26a@paulmck-laptop>
Date: Fri, 21 Feb 2025 10:08:06 -0800
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Marco Elver <elver@...gle.com>
Cc: Alexander Potapenko <glider@...gle.com>,
	Bart Van Assche <bvanassche@....org>,
	Bill Wendling <morbo@...gle.com>, Boqun Feng <boqun.feng@...il.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	Frederic Weisbecker <frederic@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Ingo Molnar <mingo@...nel.org>, Jann Horn <jannh@...gle.com>,
	Joel Fernandes <joel@...lfernandes.org>,
	Jonathan Corbet <corbet@....net>,
	Josh Triplett <josh@...htriplett.org>,
	Justin Stitt <justinstitt@...gle.com>, Kees Cook <kees@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Miguel Ojeda <ojeda@...nel.org>,
	Nathan Chancellor <nathan@...nel.org>,
	Neeraj Upadhyay <neeraj.upadhyay@...nel.org>,
	Nick Desaulniers <ndesaulniers@...gle.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Uladzislau Rezki <urezki@...il.com>,
	Waiman Long <longman@...hat.com>, Will Deacon <will@...nel.org>,
	kasan-dev@...glegroups.com, linux-kernel@...r.kernel.org,
	llvm@...ts.linux.dev, rcu@...r.kernel.org,
	linux-crypto@...r.kernel.org
Subject: Re: [PATCH RFC 15/24] rcu: Support Clang's capability analysis

On Fri, Feb 21, 2025 at 06:10:02PM +0100, Marco Elver wrote:
> On Thu, Feb 20, 2025 at 05:26PM -0800, Paul E. McKenney wrote:
> [...]
> > > That's what I've tried with this patch (rcu_read_lock_bh() also
> > > acquires "RCU", on top of "RCU_BH"). I need to add a re-entrancy test,
> > > and make sure it doesn't complain about that. At a later stage we
> > > might also want to add more general "BH" and "IRQ" capabilities to
> > > denote they're disabled when held, but that'd overcomplicate the first
> > > version of this series.
> > 
> > Fair enough!  Then would it work to just do "RCU" now, and ad the "BH"
> > and "IRQ" when those capabilities are added?
> 
> I tried if this kind of re-entrant locking works - a test like this:
> 
>  | --- a/lib/test_capability-analysis.c
>  | +++ b/lib/test_capability-analysis.c
>  | @@ -370,6 +370,15 @@ static void __used test_rcu_guarded_reader(struct test_rcu_data *d)
>  |  	rcu_read_unlock_sched();
>  |  }
>  |  
>  | +static void __used test_rcu_reentrancy(struct test_rcu_data *d)
>  | +{
>  | +	rcu_read_lock();
>  | +	rcu_read_lock_bh();
>  | +	(void)rcu_dereference(d->data);
>  | +	rcu_read_unlock_bh();
>  | +	rcu_read_unlock();
>  | +}
> 
> 
>  | $ make lib/test_capability-analysis.o
>  |   DESCEND objtool
>  |   CC      arch/x86/kernel/asm-offsets.s
>  |   INSTALL libsubcmd_headers
>  |   CALL    scripts/checksyscalls.sh
>  |   CC      lib/test_capability-analysis.o
>  | lib/test_capability-analysis.c:376:2: error: acquiring __capability_RCU 'RCU' that is already held [-Werror,-Wthread-safety-analysis]
>  |   376 |         rcu_read_lock_bh();
>  |       |         ^
>  | lib/test_capability-analysis.c:375:2: note: __capability_RCU acquired here
>  |   375 |         rcu_read_lock();
>  |       |         ^
>  | lib/test_capability-analysis.c:379:2: error: releasing __capability_RCU 'RCU' that was not held [-Werror,-Wthread-safety-analysis]
>  |   379 |         rcu_read_unlock();
>  |       |         ^
>  | lib/test_capability-analysis.c:378:2: note: __capability_RCU released here
>  |   378 |         rcu_read_unlock_bh();
>  |       |         ^
>  | 2 errors generated.
>  | make[3]: *** [scripts/Makefile.build:207: lib/test_capability-analysis.o] Error 1
>  | make[2]: *** [scripts/Makefile.build:465: lib] Error 2

I was hoping!  Ah well...  ;-)

> ... unfortunately even for shared locks, the compiler does not like
> re-entrancy yet. It's not yet supported, and to fix that I'd have to go
> and implement that in Clang first before coming back to this.

This would be needed for some types of reader-writer locks, and also for
reference counting, so here is hoping that such support is forthcoming
sooner rather than later.

> I see 2 options for now:
> 
>   a. Accepting the limitation that doing a rcu_read_lock() (and
>      variants) while the RCU read lock is already held in the same function
>      will result in a false positive warning (like above). Cases like that
>      will need to disable the analysis for that piece of code.
> 
>   b. Make the compiler not warn about unbalanced rcu_read_lock/unlock(),
>      but instead just help enforce a rcu_read_lock() was issued somewhere
>      in the function before an RCU-guarded access.
> 
> Option (b) is obviously weaker than (a), but avoids the false positives
> while accepting more false negatives.
> 
> For all the code that I have already tested this on I observed no false
> positives, so I'd go with (a), but I'm also fine with the weaker
> checking for now until the compiler gains re-entrancy support.
> 
> Preferences?

Whichever one provides the best checking without false positives.
Which sounds to me like (a) unless and until false positives crop up,
in which case (b).  Which looks to be where you were going anyway.  ;-)

							Thanx, Paul