| Message ID | 20250206181711.1902989-16-elver@google.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Compiler-Based Capability- and Locking-Analysis | expand |
On Thu, Feb 06, 2025 at 07:10:09PM +0100, Marco Elver wrote: > Improve the existing annotations to properly support Clang's capability > analysis. > > The old annotations distinguished between RCU, RCU_BH, and RCU_SCHED. > However, it does not make sense to acquire rcu_read_lock_bh() after > rcu_read_lock() - annotate the _bh() and _sched() variants to also > acquire 'RCU', so that Clang (and also Sparse) can warn about it. You lost me on this one. What breaks if rcu_read_lock_bh() is invoked while rcu_read_lock() is in effect? Thanx, Paul > The above change also simplified introducing annotations, where it would > not matter if RCU, RCU_BH, or RCU_SCHED is acquired: through the > introduction of __rcu_guarded, we can use Clang's capability analysis to > warn if a pointer is dereferenced without any of the RCU locks held, or > updated without the appropriate helpers. > > The primitives rcu_assign_pointer() and friends are wrapped with > capability_unsafe(), which enforces using them to update RCU-protected > pointers marked with __rcu_guarded. > > Signed-off-by: Marco Elver <elver@google.com> > --- > .../dev-tools/capability-analysis.rst | 2 +- > include/linux/cleanup.h | 4 + > include/linux/rcupdate.h | 73 +++++++++++++------ > lib/test_capability-analysis.c | 68 +++++++++++++++++ > 4 files changed, 123 insertions(+), 24 deletions(-) > > diff --git a/Documentation/dev-tools/capability-analysis.rst b/Documentation/dev-tools/capability-analysis.rst > index a34dfe7b0b09..73dd28a23b11 100644 > --- a/Documentation/dev-tools/capability-analysis.rst > +++ b/Documentation/dev-tools/capability-analysis.rst > @@ -86,7 +86,7 @@ Supported Kernel Primitives > > Currently the following synchronization primitives are supported: > `raw_spinlock_t`, `spinlock_t`, `rwlock_t`, `mutex`, `seqlock_t`, > -`bit_spinlock`. > +`bit_spinlock`, RCU. > > For capabilities with an initialization function (e.g., `spin_lock_init()`), > calling this function on the capability instance before initializing any > diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h > index 93a166549add..7d70d308357a 100644 > --- a/include/linux/cleanup.h > +++ b/include/linux/cleanup.h > @@ -404,6 +404,10 @@ static inline class_##_name##_t class_##_name##_constructor(void) \ > return _t; \ > } > > +#define DECLARE_LOCK_GUARD_0_ATTRS(_name, _lock, _unlock) \ > +static inline class_##_name##_t class_##_name##_constructor(void) _lock;\ > +static inline void class_##_name##_destructor(class_##_name##_t *_T) _unlock > + > #define DEFINE_LOCK_GUARD_1(_name, _type, _lock, _unlock, ...) \ > __DEFINE_CLASS_IS_CONDITIONAL(_name, false); \ > __DEFINE_UNLOCK_GUARD(_name, _type, _unlock, __VA_ARGS__) \ > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > index 48e5c03df1dd..ee68095ba9f0 100644 > --- a/include/linux/rcupdate.h > +++ b/include/linux/rcupdate.h > @@ -31,6 +31,16 @@ > #include <asm/processor.h> > #include <linux/context_tracking_irq.h> > > +token_capability(RCU); > +token_capability_instance(RCU, RCU_SCHED); > +token_capability_instance(RCU, RCU_BH); > + > +/* > + * A convenience macro that can be used for RCU-protected globals or struct > + * members; adds type qualifier __rcu, and also enforces __var_guarded_by(RCU). > + */ > +#define __rcu_guarded __rcu __var_guarded_by(RCU) > + > #define ULONG_CMP_GE(a, b) (ULONG_MAX / 2 >= (a) - (b)) > #define ULONG_CMP_LT(a, b) (ULONG_MAX / 2 < (a) - (b)) > > @@ -431,7 +441,8 @@ static inline void rcu_preempt_sleep_check(void) { } > > // See RCU_LOCKDEP_WARN() for an explanation of the double call to > // debug_lockdep_rcu_enabled(). > -static inline bool lockdep_assert_rcu_helper(bool c) > +static inline bool lockdep_assert_rcu_helper(bool c, const struct __capability_RCU *cap) > + __asserts_shared_cap(RCU) __asserts_shared_cap(cap) > { > return debug_lockdep_rcu_enabled() && > (c || !rcu_is_watching() || !rcu_lockdep_current_cpu_online()) && > @@ -444,7 +455,7 @@ static inline bool lockdep_assert_rcu_helper(bool c) > * Splats if lockdep is enabled and there is no rcu_read_lock() in effect. > */ > #define lockdep_assert_in_rcu_read_lock() \ > - WARN_ON_ONCE(lockdep_assert_rcu_helper(!lock_is_held(&rcu_lock_map))) > + WARN_ON_ONCE(lockdep_assert_rcu_helper(!lock_is_held(&rcu_lock_map), RCU)) > > /** > * lockdep_assert_in_rcu_read_lock_bh - WARN if not protected by rcu_read_lock_bh() > @@ -454,7 +465,7 @@ static inline bool lockdep_assert_rcu_helper(bool c) > * actual rcu_read_lock_bh() is required. > */ > #define lockdep_assert_in_rcu_read_lock_bh() \ > - WARN_ON_ONCE(lockdep_assert_rcu_helper(!lock_is_held(&rcu_bh_lock_map))) > + WARN_ON_ONCE(lockdep_assert_rcu_helper(!lock_is_held(&rcu_bh_lock_map), RCU_BH)) > > /** > * lockdep_assert_in_rcu_read_lock_sched - WARN if not protected by rcu_read_lock_sched() > @@ -464,7 +475,7 @@ static inline bool lockdep_assert_rcu_helper(bool c) > * instead an actual rcu_read_lock_sched() is required. > */ > #define lockdep_assert_in_rcu_read_lock_sched() \ > - WARN_ON_ONCE(lockdep_assert_rcu_helper(!lock_is_held(&rcu_sched_lock_map))) > + WARN_ON_ONCE(lockdep_assert_rcu_helper(!lock_is_held(&rcu_sched_lock_map), RCU_SCHED)) > > /** > * lockdep_assert_in_rcu_reader - WARN if not within some type of RCU reader > @@ -482,17 +493,17 @@ static inline bool lockdep_assert_rcu_helper(bool c) > WARN_ON_ONCE(lockdep_assert_rcu_helper(!lock_is_held(&rcu_lock_map) && \ > !lock_is_held(&rcu_bh_lock_map) && \ > !lock_is_held(&rcu_sched_lock_map) && \ > - preemptible())) > + preemptible(), RCU)) > > #else /* #ifdef CONFIG_PROVE_RCU */ > > #define RCU_LOCKDEP_WARN(c, s) do { } while (0 && (c)) > #define rcu_sleep_check() do { } while (0) > > -#define lockdep_assert_in_rcu_read_lock() do { } while (0) > -#define lockdep_assert_in_rcu_read_lock_bh() do { } while (0) > -#define lockdep_assert_in_rcu_read_lock_sched() do { } while (0) > -#define lockdep_assert_in_rcu_reader() do { } while (0) > +#define lockdep_assert_in_rcu_read_lock() __assert_shared_cap(RCU) > +#define lockdep_assert_in_rcu_read_lock_bh() __assert_shared_cap(RCU_BH) > +#define lockdep_assert_in_rcu_read_lock_sched() __assert_shared_cap(RCU_SCHED) > +#define lockdep_assert_in_rcu_reader() __assert_shared_cap(RCU) > > #endif /* #else #ifdef CONFIG_PROVE_RCU */ > > @@ -512,11 +523,11 @@ static inline bool lockdep_assert_rcu_helper(bool c) > #endif /* #else #ifdef __CHECKER__ */ > > #define __unrcu_pointer(p, local) \ > -({ \ > +capability_unsafe( \ > typeof(*p) *local = (typeof(*p) *__force)(p); \ > rcu_check_sparse(p, __rcu); \ > ((typeof(*p) __force __kernel *)(local)); \ > -}) > +) > /** > * unrcu_pointer - mark a pointer as not being RCU protected > * @p: pointer needing to lose its __rcu property > @@ -592,7 +603,7 @@ static inline bool lockdep_assert_rcu_helper(bool c) > * other macros that it invokes. > */ > #define rcu_assign_pointer(p, v) \ > -do { \ > +capability_unsafe( \ > uintptr_t _r_a_p__v = (uintptr_t)(v); \ > rcu_check_sparse(p, __rcu); \ > \ > @@ -600,7 +611,7 @@ do { \ > WRITE_ONCE((p), (typeof(p))(_r_a_p__v)); \ > else \ > smp_store_release(&p, RCU_INITIALIZER((typeof(p))_r_a_p__v)); \ > -} while (0) > +) > > /** > * rcu_replace_pointer() - replace an RCU pointer, returning its old value > @@ -843,9 +854,10 @@ do { \ > * only when acquiring spinlocks that are subject to priority inheritance. > */ > static __always_inline void rcu_read_lock(void) > + __acquires_shared(RCU) > { > __rcu_read_lock(); > - __acquire(RCU); > + __acquire_shared(RCU); > rcu_lock_acquire(&rcu_lock_map); > RCU_LOCKDEP_WARN(!rcu_is_watching(), > "rcu_read_lock() used illegally while idle"); > @@ -874,11 +886,12 @@ static __always_inline void rcu_read_lock(void) > * See rcu_read_lock() for more information. > */ > static inline void rcu_read_unlock(void) > + __releases_shared(RCU) > { > RCU_LOCKDEP_WARN(!rcu_is_watching(), > "rcu_read_unlock() used illegally while idle"); > rcu_lock_release(&rcu_lock_map); /* Keep acq info for rls diags. */ > - __release(RCU); > + __release_shared(RCU); > __rcu_read_unlock(); > } > > @@ -897,9 +910,11 @@ static inline void rcu_read_unlock(void) > * was invoked from some other task. > */ > static inline void rcu_read_lock_bh(void) > + __acquires_shared(RCU) __acquires_shared(RCU_BH) > { > local_bh_disable(); > - __acquire(RCU_BH); > + __acquire_shared(RCU); > + __acquire_shared(RCU_BH); > rcu_lock_acquire(&rcu_bh_lock_map); > RCU_LOCKDEP_WARN(!rcu_is_watching(), > "rcu_read_lock_bh() used illegally while idle"); > @@ -911,11 +926,13 @@ static inline void rcu_read_lock_bh(void) > * See rcu_read_lock_bh() for more information. > */ > static inline void rcu_read_unlock_bh(void) > + __releases_shared(RCU) __releases_shared(RCU_BH) > { > RCU_LOCKDEP_WARN(!rcu_is_watching(), > "rcu_read_unlock_bh() used illegally while idle"); > rcu_lock_release(&rcu_bh_lock_map); > - __release(RCU_BH); > + __release_shared(RCU_BH); > + __release_shared(RCU); > local_bh_enable(); > } > > @@ -935,9 +952,11 @@ static inline void rcu_read_unlock_bh(void) > * rcu_read_lock_sched() was invoked from an NMI handler. > */ > static inline void rcu_read_lock_sched(void) > + __acquires_shared(RCU) __acquires_shared(RCU_SCHED) > { > preempt_disable(); > - __acquire(RCU_SCHED); > + __acquire_shared(RCU); > + __acquire_shared(RCU_SCHED); > rcu_lock_acquire(&rcu_sched_lock_map); > RCU_LOCKDEP_WARN(!rcu_is_watching(), > "rcu_read_lock_sched() used illegally while idle"); > @@ -945,9 +964,11 @@ static inline void rcu_read_lock_sched(void) > > /* Used by lockdep and tracing: cannot be traced, cannot call lockdep. */ > static inline notrace void rcu_read_lock_sched_notrace(void) > + __acquires_shared(RCU) __acquires_shared(RCU_SCHED) > { > preempt_disable_notrace(); > - __acquire(RCU_SCHED); > + __acquire_shared(RCU); > + __acquire_shared(RCU_SCHED); > } > > /** > @@ -956,18 +977,22 @@ static inline notrace void rcu_read_lock_sched_notrace(void) > * See rcu_read_lock_sched() for more information. > */ > static inline void rcu_read_unlock_sched(void) > + __releases_shared(RCU) __releases_shared(RCU_SCHED) > { > RCU_LOCKDEP_WARN(!rcu_is_watching(), > "rcu_read_unlock_sched() used illegally while idle"); > rcu_lock_release(&rcu_sched_lock_map); > - __release(RCU_SCHED); > + __release_shared(RCU_SCHED); > + __release_shared(RCU); > preempt_enable(); > } > > /* Used by lockdep and tracing: cannot be traced, cannot call lockdep. */ > static inline notrace void rcu_read_unlock_sched_notrace(void) > + __releases_shared(RCU) __releases_shared(RCU_SCHED) > { > - __release(RCU_SCHED); > + __release_shared(RCU_SCHED); > + __release_shared(RCU); > preempt_enable_notrace(); > } > > @@ -1010,10 +1035,10 @@ static inline notrace void rcu_read_unlock_sched_notrace(void) > * ordering guarantees for either the CPU or the compiler. > */ > #define RCU_INIT_POINTER(p, v) \ > - do { \ > + capability_unsafe( \ > rcu_check_sparse(p, __rcu); \ > WRITE_ONCE(p, RCU_INITIALIZER(v)); \ > - } while (0) > + ) > > /** > * RCU_POINTER_INITIALIZER() - statically initialize an RCU protected pointer > @@ -1172,4 +1197,6 @@ DEFINE_LOCK_GUARD_0(rcu, > } while (0), > rcu_read_unlock()) > > +DECLARE_LOCK_GUARD_0_ATTRS(rcu, __acquires_shared(RCU), __releases_shared(RCU)); > + > #endif /* __LINUX_RCUPDATE_H */ > diff --git a/lib/test_capability-analysis.c b/lib/test_capability-analysis.c > index fc8dcad2a994..f5a1dda6ca38 100644 > --- a/lib/test_capability-analysis.c > +++ b/lib/test_capability-analysis.c > @@ -7,6 +7,7 @@ > #include <linux/bit_spinlock.h> > #include <linux/build_bug.h> > #include <linux/mutex.h> > +#include <linux/rcupdate.h> > #include <linux/seqlock.h> > #include <linux/spinlock.h> > > @@ -277,3 +278,70 @@ static void __used test_bit_spin_lock(struct test_bit_spinlock_data *d) > bit_spin_unlock(3, &d->bits); > } > } > + > +/* > + * Test that we can mark a variable guarded by RCU, and we can dereference and > + * write to the pointer with RCU's primitives. > + */ > +struct test_rcu_data { > + long __rcu_guarded *data; > +}; > + > +static void __used test_rcu_guarded_reader(struct test_rcu_data *d) > +{ > + rcu_read_lock(); > + (void)rcu_dereference(d->data); > + rcu_read_unlock(); > + > + rcu_read_lock_bh(); > + (void)rcu_dereference(d->data); > + rcu_read_unlock_bh(); > + > + rcu_read_lock_sched(); > + (void)rcu_dereference(d->data); > + rcu_read_unlock_sched(); > +} > + > +static void __used test_rcu_guard(struct test_rcu_data *d) > +{ > + guard(rcu)(); > + (void)rcu_dereference(d->data); > +} > + > +static void __used test_rcu_guarded_updater(struct test_rcu_data *d) > +{ > + rcu_assign_pointer(d->data, NULL); > + RCU_INIT_POINTER(d->data, NULL); > + (void)unrcu_pointer(d->data); > +} > + > +static void wants_rcu_held(void) __must_hold_shared(RCU) { } > +static void wants_rcu_held_bh(void) __must_hold_shared(RCU_BH) { } > +static void wants_rcu_held_sched(void) __must_hold_shared(RCU_SCHED) { } > + > +static void __used test_rcu_lock_variants(void) > +{ > + rcu_read_lock(); > + wants_rcu_held(); > + rcu_read_unlock(); > + > + rcu_read_lock_bh(); > + wants_rcu_held_bh(); > + rcu_read_unlock_bh(); > + > + rcu_read_lock_sched(); > + wants_rcu_held_sched(); > + rcu_read_unlock_sched(); > +} > + > +static void __used test_rcu_assert_variants(void) > +{ > + lockdep_assert_in_rcu_read_lock(); > + wants_rcu_held(); > + > + lockdep_assert_in_rcu_read_lock_bh(); > + wants_rcu_held_bh(); > + > + lockdep_assert_in_rcu_read_lock_sched(); > + wants_rcu_held_sched(); > +} > -- > 2.48.1.502.g6dc24dfdaf-goog >
On Thu, 20 Feb 2025 at 23:00, Paul E. McKenney <paulmck@kernel.org> wrote: > > On Thu, Feb 06, 2025 at 07:10:09PM +0100, Marco Elver wrote: > > Improve the existing annotations to properly support Clang's capability > > analysis. > > > > The old annotations distinguished between RCU, RCU_BH, and RCU_SCHED. > > However, it does not make sense to acquire rcu_read_lock_bh() after > > rcu_read_lock() - annotate the _bh() and _sched() variants to also > > acquire 'RCU', so that Clang (and also Sparse) can warn about it. > > You lost me on this one. What breaks if rcu_read_lock_bh() is invoked > while rcu_read_lock() is in effect? I thought something like this does not make sense: rcu_read_lock_bh(); .. rcu_read_lock(); .. rcu_read_unlock(); .. rcu_read_unlock_bh(); However, the inverse may well be something we might find somewhere in the kernel? Another problem was that if we want to indicate that "RCU" read lock is held, then we should just be able to write "__must_hold_shared(RCU)", and it shouldn't matter if rcu_read_lock() or rcu_read_lock_bh() was used. Previously each of them acquired their own capability "RCU" and "RCU_BH" respectively. But rather, we're dealing with one acquiring a superset of the other, and expressing that is also what I attempted to solve. Let me rethink this... Thanks, -- Marco
On Thu, Feb 20, 2025 at 11:11:04PM +0100, Marco Elver wrote: > On Thu, 20 Feb 2025 at 23:00, Paul E. McKenney <paulmck@kernel.org> wrote: > > > > On Thu, Feb 06, 2025 at 07:10:09PM +0100, Marco Elver wrote: > > > Improve the existing annotations to properly support Clang's capability > > > analysis. > > > > > > The old annotations distinguished between RCU, RCU_BH, and RCU_SCHED. > > > However, it does not make sense to acquire rcu_read_lock_bh() after > > > rcu_read_lock() - annotate the _bh() and _sched() variants to also > > > acquire 'RCU', so that Clang (and also Sparse) can warn about it. > > > > You lost me on this one. What breaks if rcu_read_lock_bh() is invoked > > while rcu_read_lock() is in effect? > > I thought something like this does not make sense: > > rcu_read_lock_bh(); > .. > rcu_read_lock(); > .. > rcu_read_unlock(); > .. > rcu_read_unlock_bh(); If you have the choice, it is often better to do the rcu_read_lock() first and the rcu_read_lock_bh() second. > However, the inverse may well be something we might find somewhere in > the kernel? Suppose that one function walks an RCU-protected list, calling some function from some other subsystem on each element. Suppose that each element has another RCU protected list. It would be good if the two subsystems could just choose their desired flavor of RCU reader, without having to know about each other. > Another problem was that if we want to indicate that "RCU" read lock > is held, then we should just be able to write > "__must_hold_shared(RCU)", and it shouldn't matter if rcu_read_lock() > or rcu_read_lock_bh() was used. Previously each of them acquired their > own capability "RCU" and "RCU_BH" respectively. But rather, we're > dealing with one acquiring a superset of the other, and expressing > that is also what I attempted to solve. > Let me rethink this... Would it work to have just one sort of RCU reader, relying on a separate BH-disable capability for the additional semantics of rcu_read_lock_bh()? Thanx, Paul
On Thu, 20 Feb 2025 at 23:36, Paul E. McKenney <paulmck@kernel.org> wrote: [...] > Suppose that one function walks an RCU-protected list, calling some > function from some other subsystem on each element. Suppose that each > element has another RCU protected list. > > It would be good if the two subsystems could just choose their desired > flavor of RCU reader, without having to know about each other. That's what I figured might be the case - thanks for clarifying. > > Another problem was that if we want to indicate that "RCU" read lock > > is held, then we should just be able to write > > "__must_hold_shared(RCU)", and it shouldn't matter if rcu_read_lock() > > or rcu_read_lock_bh() was used. Previously each of them acquired their > > own capability "RCU" and "RCU_BH" respectively. But rather, we're > > dealing with one acquiring a superset of the other, and expressing > > that is also what I attempted to solve. > > Let me rethink this... > > Would it work to have just one sort of RCU reader, relying on a separate > BH-disable capability for the additional semantics of rcu_read_lock_bh()? 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. Thanks, -- Marco
On Fri, Feb 21, 2025 at 01:16:00AM +0100, Marco Elver wrote: > On Thu, 20 Feb 2025 at 23:36, Paul E. McKenney <paulmck@kernel.org> wrote: > [...] > > Suppose that one function walks an RCU-protected list, calling some > > function from some other subsystem on each element. Suppose that each > > element has another RCU protected list. > > > > It would be good if the two subsystems could just choose their desired > > flavor of RCU reader, without having to know about each other. > > That's what I figured might be the case - thanks for clarifying. > > > > Another problem was that if we want to indicate that "RCU" read lock > > > is held, then we should just be able to write > > > "__must_hold_shared(RCU)", and it shouldn't matter if rcu_read_lock() > > > or rcu_read_lock_bh() was used. Previously each of them acquired their > > > own capability "RCU" and "RCU_BH" respectively. But rather, we're > > > dealing with one acquiring a superset of the other, and expressing > > > that is also what I attempted to solve. > > > Let me rethink this... > > > > Would it work to have just one sort of RCU reader, relying on a separate > > BH-disable capability for the additional semantics of rcu_read_lock_bh()? > > 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? Thanx, Paul
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
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
On Fri, Feb 21, 2025 at 10:08:06AM -0800, Paul E. McKenney wrote: > > ... 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. Right, so I read the clang documentation for this feature the other day, and my take away was that this was all really primitive and lots of work will need to go into making this more capable before we can cover much of the more interesting things we do in the kernel. Notably the whole guarded_by member annotations, which are very cool in concept, are very primitive in practise and will need much extensions. To that effect, and because this is basically a static analysis pass with no codegen implications, I would suggest that we keep the whole feature limited to the very latest clang version for now and don't bother supporting older versions at all.
On Fri, 21 Feb 2025 at 19:52, Peter Zijlstra <peterz@infradead.org> wrote: > > On Fri, Feb 21, 2025 at 10:08:06AM -0800, Paul E. McKenney wrote: > > > > ... 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. > > Right, so I read the clang documentation for this feature the other day, > and my take away was that this was all really primitive and lots of work > will need to go into making this more capable before we can cover much > of the more interesting things we do in the kernel. > > Notably the whole guarded_by member annotations, which are very cool in > concept, are very primitive in practise and will need much extensions. I have one extension in flight: https://github.com/llvm/llvm-project/pull/127396 - it'll improve coverage for pointer passing of guarded_by members. Anything else you see as urgent? Re-entrant locks support a deal breaker? But yes, a lot of complex locking patterns will not easily be expressible right away. > To that effect, and because this is basically a static analysis pass > with no codegen implications, I would suggest that we keep the whole > feature limited to the very latest clang version for now and don't > bother supporting older versions at all. Along those lines, in an upcoming v2, I'm planning to bump it up to Clang 20+ because that version introduced a reasonable way to ignore warnings in not-yet-annotated headers: https://git.kernel.org/pub/scm/linux/kernel/git/melver/linux.git/commit/?h=cap-analysis/dev&id=2432a39eae8197f5058c578430bd1906c18480c3
On Fri, Feb 21, 2025 at 08:46:45PM +0100, Marco Elver wrote:
> Anything else you see as urgent? Re-entrant locks support a deal breaker?
Most actual locks are not recursive -- RCU being the big exception here.
As to this being deal breakers, I don't think so. We should just start
with the bits we can do and chip away at stuff. Raise the LLVM version
requirement every time new stuff gets added.
diff --git a/Documentation/dev-tools/capability-analysis.rst b/Documentation/dev-tools/capability-analysis.rst index a34dfe7b0b09..73dd28a23b11 100644 --- a/Documentation/dev-tools/capability-analysis.rst +++ b/Documentation/dev-tools/capability-analysis.rst @@ -86,7 +86,7 @@ Supported Kernel Primitives Currently the following synchronization primitives are supported: `raw_spinlock_t`, `spinlock_t`, `rwlock_t`, `mutex`, `seqlock_t`, -`bit_spinlock`. +`bit_spinlock`, RCU. For capabilities with an initialization function (e.g., `spin_lock_init()`), calling this function on the capability instance before initializing any diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h index 93a166549add..7d70d308357a 100644 --- a/include/linux/cleanup.h +++ b/include/linux/cleanup.h @@ -404,6 +404,10 @@ static inline class_##_name##_t class_##_name##_constructor(void) \ return _t; \ } +#define DECLARE_LOCK_GUARD_0_ATTRS(_name, _lock, _unlock) \ +static inline class_##_name##_t class_##_name##_constructor(void) _lock;\ +static inline void class_##_name##_destructor(class_##_name##_t *_T) _unlock + #define DEFINE_LOCK_GUARD_1(_name, _type, _lock, _unlock, ...) \ __DEFINE_CLASS_IS_CONDITIONAL(_name, false); \ __DEFINE_UNLOCK_GUARD(_name, _type, _unlock, __VA_ARGS__) \ diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h index 48e5c03df1dd..ee68095ba9f0 100644 --- a/include/linux/rcupdate.h +++ b/include/linux/rcupdate.h @@ -31,6 +31,16 @@ #include <asm/processor.h> #include <linux/context_tracking_irq.h> +token_capability(RCU); +token_capability_instance(RCU, RCU_SCHED); +token_capability_instance(RCU, RCU_BH); + +/* + * A convenience macro that can be used for RCU-protected globals or struct + * members; adds type qualifier __rcu, and also enforces __var_guarded_by(RCU). + */ +#define __rcu_guarded __rcu __var_guarded_by(RCU) + #define ULONG_CMP_GE(a, b) (ULONG_MAX / 2 >= (a) - (b)) #define ULONG_CMP_LT(a, b) (ULONG_MAX / 2 < (a) - (b)) @@ -431,7 +441,8 @@ static inline void rcu_preempt_sleep_check(void) { } // See RCU_LOCKDEP_WARN() for an explanation of the double call to // debug_lockdep_rcu_enabled(). -static inline bool lockdep_assert_rcu_helper(bool c) +static inline bool lockdep_assert_rcu_helper(bool c, const struct __capability_RCU *cap) + __asserts_shared_cap(RCU) __asserts_shared_cap(cap) { return debug_lockdep_rcu_enabled() && (c || !rcu_is_watching() || !rcu_lockdep_current_cpu_online()) && @@ -444,7 +455,7 @@ static inline bool lockdep_assert_rcu_helper(bool c) * Splats if lockdep is enabled and there is no rcu_read_lock() in effect. */ #define lockdep_assert_in_rcu_read_lock() \ - WARN_ON_ONCE(lockdep_assert_rcu_helper(!lock_is_held(&rcu_lock_map))) + WARN_ON_ONCE(lockdep_assert_rcu_helper(!lock_is_held(&rcu_lock_map), RCU)) /** * lockdep_assert_in_rcu_read_lock_bh - WARN if not protected by rcu_read_lock_bh() @@ -454,7 +465,7 @@ static inline bool lockdep_assert_rcu_helper(bool c) * actual rcu_read_lock_bh() is required. */ #define lockdep_assert_in_rcu_read_lock_bh() \ - WARN_ON_ONCE(lockdep_assert_rcu_helper(!lock_is_held(&rcu_bh_lock_map))) + WARN_ON_ONCE(lockdep_assert_rcu_helper(!lock_is_held(&rcu_bh_lock_map), RCU_BH)) /** * lockdep_assert_in_rcu_read_lock_sched - WARN if not protected by rcu_read_lock_sched() @@ -464,7 +475,7 @@ static inline bool lockdep_assert_rcu_helper(bool c) * instead an actual rcu_read_lock_sched() is required. */ #define lockdep_assert_in_rcu_read_lock_sched() \ - WARN_ON_ONCE(lockdep_assert_rcu_helper(!lock_is_held(&rcu_sched_lock_map))) + WARN_ON_ONCE(lockdep_assert_rcu_helper(!lock_is_held(&rcu_sched_lock_map), RCU_SCHED)) /** * lockdep_assert_in_rcu_reader - WARN if not within some type of RCU reader @@ -482,17 +493,17 @@ static inline bool lockdep_assert_rcu_helper(bool c) WARN_ON_ONCE(lockdep_assert_rcu_helper(!lock_is_held(&rcu_lock_map) && \ !lock_is_held(&rcu_bh_lock_map) && \ !lock_is_held(&rcu_sched_lock_map) && \ - preemptible())) + preemptible(), RCU)) #else /* #ifdef CONFIG_PROVE_RCU */ #define RCU_LOCKDEP_WARN(c, s) do { } while (0 && (c)) #define rcu_sleep_check() do { } while (0) -#define lockdep_assert_in_rcu_read_lock() do { } while (0) -#define lockdep_assert_in_rcu_read_lock_bh() do { } while (0) -#define lockdep_assert_in_rcu_read_lock_sched() do { } while (0) -#define lockdep_assert_in_rcu_reader() do { } while (0) +#define lockdep_assert_in_rcu_read_lock() __assert_shared_cap(RCU) +#define lockdep_assert_in_rcu_read_lock_bh() __assert_shared_cap(RCU_BH) +#define lockdep_assert_in_rcu_read_lock_sched() __assert_shared_cap(RCU_SCHED) +#define lockdep_assert_in_rcu_reader() __assert_shared_cap(RCU) #endif /* #else #ifdef CONFIG_PROVE_RCU */ @@ -512,11 +523,11 @@ static inline bool lockdep_assert_rcu_helper(bool c) #endif /* #else #ifdef __CHECKER__ */ #define __unrcu_pointer(p, local) \ -({ \ +capability_unsafe( \ typeof(*p) *local = (typeof(*p) *__force)(p); \ rcu_check_sparse(p, __rcu); \ ((typeof(*p) __force __kernel *)(local)); \ -}) +) /** * unrcu_pointer - mark a pointer as not being RCU protected * @p: pointer needing to lose its __rcu property @@ -592,7 +603,7 @@ static inline bool lockdep_assert_rcu_helper(bool c) * other macros that it invokes. */ #define rcu_assign_pointer(p, v) \ -do { \ +capability_unsafe( \ uintptr_t _r_a_p__v = (uintptr_t)(v); \ rcu_check_sparse(p, __rcu); \ \ @@ -600,7 +611,7 @@ do { \ WRITE_ONCE((p), (typeof(p))(_r_a_p__v)); \ else \ smp_store_release(&p, RCU_INITIALIZER((typeof(p))_r_a_p__v)); \ -} while (0) +) /** * rcu_replace_pointer() - replace an RCU pointer, returning its old value @@ -843,9 +854,10 @@ do { \ * only when acquiring spinlocks that are subject to priority inheritance. */ static __always_inline void rcu_read_lock(void) + __acquires_shared(RCU) { __rcu_read_lock(); - __acquire(RCU); + __acquire_shared(RCU); rcu_lock_acquire(&rcu_lock_map); RCU_LOCKDEP_WARN(!rcu_is_watching(), "rcu_read_lock() used illegally while idle"); @@ -874,11 +886,12 @@ static __always_inline void rcu_read_lock(void) * See rcu_read_lock() for more information. */ static inline void rcu_read_unlock(void) + __releases_shared(RCU) { RCU_LOCKDEP_WARN(!rcu_is_watching(), "rcu_read_unlock() used illegally while idle"); rcu_lock_release(&rcu_lock_map); /* Keep acq info for rls diags. */ - __release(RCU); + __release_shared(RCU); __rcu_read_unlock(); } @@ -897,9 +910,11 @@ static inline void rcu_read_unlock(void) * was invoked from some other task. */ static inline void rcu_read_lock_bh(void) + __acquires_shared(RCU) __acquires_shared(RCU_BH) { local_bh_disable(); - __acquire(RCU_BH); + __acquire_shared(RCU); + __acquire_shared(RCU_BH); rcu_lock_acquire(&rcu_bh_lock_map); RCU_LOCKDEP_WARN(!rcu_is_watching(), "rcu_read_lock_bh() used illegally while idle"); @@ -911,11 +926,13 @@ static inline void rcu_read_lock_bh(void) * See rcu_read_lock_bh() for more information. */ static inline void rcu_read_unlock_bh(void) + __releases_shared(RCU) __releases_shared(RCU_BH) { RCU_LOCKDEP_WARN(!rcu_is_watching(), "rcu_read_unlock_bh() used illegally while idle"); rcu_lock_release(&rcu_bh_lock_map); - __release(RCU_BH); + __release_shared(RCU_BH); + __release_shared(RCU); local_bh_enable(); } @@ -935,9 +952,11 @@ static inline void rcu_read_unlock_bh(void) * rcu_read_lock_sched() was invoked from an NMI handler. */ static inline void rcu_read_lock_sched(void) + __acquires_shared(RCU) __acquires_shared(RCU_SCHED) { preempt_disable(); - __acquire(RCU_SCHED); + __acquire_shared(RCU); + __acquire_shared(RCU_SCHED); rcu_lock_acquire(&rcu_sched_lock_map); RCU_LOCKDEP_WARN(!rcu_is_watching(), "rcu_read_lock_sched() used illegally while idle"); @@ -945,9 +964,11 @@ static inline void rcu_read_lock_sched(void) /* Used by lockdep and tracing: cannot be traced, cannot call lockdep. */ static inline notrace void rcu_read_lock_sched_notrace(void) + __acquires_shared(RCU) __acquires_shared(RCU_SCHED) { preempt_disable_notrace(); - __acquire(RCU_SCHED); + __acquire_shared(RCU); + __acquire_shared(RCU_SCHED); } /** @@ -956,18 +977,22 @@ static inline notrace void rcu_read_lock_sched_notrace(void) * See rcu_read_lock_sched() for more information. */ static inline void rcu_read_unlock_sched(void) + __releases_shared(RCU) __releases_shared(RCU_SCHED) { RCU_LOCKDEP_WARN(!rcu_is_watching(), "rcu_read_unlock_sched() used illegally while idle"); rcu_lock_release(&rcu_sched_lock_map); - __release(RCU_SCHED); + __release_shared(RCU_SCHED); + __release_shared(RCU); preempt_enable(); } /* Used by lockdep and tracing: cannot be traced, cannot call lockdep. */ static inline notrace void rcu_read_unlock_sched_notrace(void) + __releases_shared(RCU) __releases_shared(RCU_SCHED) { - __release(RCU_SCHED); + __release_shared(RCU_SCHED); + __release_shared(RCU); preempt_enable_notrace(); } @@ -1010,10 +1035,10 @@ static inline notrace void rcu_read_unlock_sched_notrace(void) * ordering guarantees for either the CPU or the compiler. */ #define RCU_INIT_POINTER(p, v) \ - do { \ + capability_unsafe( \ rcu_check_sparse(p, __rcu); \ WRITE_ONCE(p, RCU_INITIALIZER(v)); \ - } while (0) + ) /** * RCU_POINTER_INITIALIZER() - statically initialize an RCU protected pointer @@ -1172,4 +1197,6 @@ DEFINE_LOCK_GUARD_0(rcu, } while (0), rcu_read_unlock()) +DECLARE_LOCK_GUARD_0_ATTRS(rcu, __acquires_shared(RCU), __releases_shared(RCU)); + #endif /* __LINUX_RCUPDATE_H */ diff --git a/lib/test_capability-analysis.c b/lib/test_capability-analysis.c index fc8dcad2a994..f5a1dda6ca38 100644 --- a/lib/test_capability-analysis.c +++ b/lib/test_capability-analysis.c @@ -7,6 +7,7 @@ #include <linux/bit_spinlock.h> #include <linux/build_bug.h> #include <linux/mutex.h> +#include <linux/rcupdate.h> #include <linux/seqlock.h> #include <linux/spinlock.h> @@ -277,3 +278,70 @@ static void __used test_bit_spin_lock(struct test_bit_spinlock_data *d) bit_spin_unlock(3, &d->bits); } } + +/* + * Test that we can mark a variable guarded by RCU, and we can dereference and + * write to the pointer with RCU's primitives. + */ +struct test_rcu_data { + long __rcu_guarded *data; +}; + +static void __used test_rcu_guarded_reader(struct test_rcu_data *d) +{ + rcu_read_lock(); + (void)rcu_dereference(d->data); + rcu_read_unlock(); + + rcu_read_lock_bh(); + (void)rcu_dereference(d->data); + rcu_read_unlock_bh(); + + rcu_read_lock_sched(); + (void)rcu_dereference(d->data); + rcu_read_unlock_sched(); +} + +static void __used test_rcu_guard(struct test_rcu_data *d) +{ + guard(rcu)(); + (void)rcu_dereference(d->data); +} + +static void __used test_rcu_guarded_updater(struct test_rcu_data *d) +{ + rcu_assign_pointer(d->data, NULL); + RCU_INIT_POINTER(d->data, NULL); + (void)unrcu_pointer(d->data); +} + +static void wants_rcu_held(void) __must_hold_shared(RCU) { } +static void wants_rcu_held_bh(void) __must_hold_shared(RCU_BH) { } +static void wants_rcu_held_sched(void) __must_hold_shared(RCU_SCHED) { } + +static void __used test_rcu_lock_variants(void) +{ + rcu_read_lock(); + wants_rcu_held(); + rcu_read_unlock(); + + rcu_read_lock_bh(); + wants_rcu_held_bh(); + rcu_read_unlock_bh(); + + rcu_read_lock_sched(); + wants_rcu_held_sched(); + rcu_read_unlock_sched(); +} + +static void __used test_rcu_assert_variants(void) +{ + lockdep_assert_in_rcu_read_lock(); + wants_rcu_held(); + + lockdep_assert_in_rcu_read_lock_bh(); + wants_rcu_held_bh(); + + lockdep_assert_in_rcu_read_lock_sched(); + wants_rcu_held_sched(); +}
Improve the existing annotations to properly support Clang's capability analysis. The old annotations distinguished between RCU, RCU_BH, and RCU_SCHED. However, it does not make sense to acquire rcu_read_lock_bh() after rcu_read_lock() - annotate the _bh() and _sched() variants to also acquire 'RCU', so that Clang (and also Sparse) can warn about it. The above change also simplified introducing annotations, where it would not matter if RCU, RCU_BH, or RCU_SCHED is acquired: through the introduction of __rcu_guarded, we can use Clang's capability analysis to warn if a pointer is dereferenced without any of the RCU locks held, or updated without the appropriate helpers. The primitives rcu_assign_pointer() and friends are wrapped with capability_unsafe(), which enforces using them to update RCU-protected pointers marked with __rcu_guarded. Signed-off-by: Marco Elver <elver@google.com> --- .../dev-tools/capability-analysis.rst | 2 +- include/linux/cleanup.h | 4 + include/linux/rcupdate.h | 73 +++++++++++++------ lib/test_capability-analysis.c | 68 +++++++++++++++++ 4 files changed, 123 insertions(+), 24 deletions(-)