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
... 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.
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?
Thanks,
-- Marco
