| Message ID | 20250206181711.1902989-8-elver@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Compiler-Based Capability- and Locking-Analysis | expand |
On 2/6/25 10:10 AM, Marco Elver wrote: > @@ -243,15 +243,18 @@ const volatile void * __must_check_fn(const volatile void *val) > #define DEFINE_CLASS(_name, _type, _exit, _init, _init_args...) \ > typedef _type class_##_name##_t; \ > static inline void class_##_name##_destructor(_type *p) \ > + __no_capability_analysis \ > { _type _T = *p; _exit; } \ > static inline _type class_##_name##_constructor(_init_args) \ > + __no_capability_analysis \ > { _type t = _init; return t; } guard() uses the constructor and destructor functions defined by DEFINE_GUARD(). The DEFINE_GUARD() implementation uses DEFINE_CLASS(). Here is an example that I found in <linux/mutex.h>: DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T)) For this example, how is the compiler told that mutex _T is held around the code protected by guard()? Thanks, Bart.
On Thu, 6 Feb 2025 at 22:29, Bart Van Assche <bvanassche@acm.org> wrote: > > On 2/6/25 10:10 AM, Marco Elver wrote: > > @@ -243,15 +243,18 @@ const volatile void * __must_check_fn(const volatile void *val) > > #define DEFINE_CLASS(_name, _type, _exit, _init, _init_args...) \ > > typedef _type class_##_name##_t; \ > > static inline void class_##_name##_destructor(_type *p) \ > > + __no_capability_analysis \ > > { _type _T = *p; _exit; } \ > > static inline _type class_##_name##_constructor(_init_args) \ > > + __no_capability_analysis \ > > { _type t = _init; return t; } > > guard() uses the constructor and destructor functions defined by > DEFINE_GUARD(). The DEFINE_GUARD() implementation uses DEFINE_CLASS(). > Here is an example that I found in <linux/mutex.h>: > > DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T)) > > For this example, how is the compiler told that mutex _T is held around > the code protected by guard()? DEFINE_GUARD is the generic variant usable for more than just locking primitives. DEFINE_LOCK_GUARD_X is a specialization of DEFINE_GUARD intended for locking primitives, all of which should be capability-enabled. So I added automatic support for DEFINE_LOCK_GUARD_1 (keeping in mind the limitations as described in the commit message). All later patches that introduce support for a locking primitive that had been using DEFINE_GUARD are switched over to DEFINE_LOCK_GUARD. There's no additional runtime cost (_T is just a struct containing _T->lock). For example, the change for mutex [1] switches it to use DEFINE_LOCK_GUARD_1. [1] https://lore.kernel.org/all/20250206181711.1902989-12-elver@google.com/ (For every primitive added I have added tests in test_capability-analysis.c, including testing that the scoped guard() helpers work and do not produce false positives.) The RCU patch [15/24] also makes it work for LOCK_GUARD_0, by simply adding an optional helper macro to declare the attributes for lock and unlock. There's no need for additional variants of DEFINE_LOCK_GUARD_X. Should the need arise to add add annotations for DEFINE_GUARD, we can introduce DECLARE_GUARD_ATTRS(), similar to DECLARE_LOCK_GUARD_0_ATTRS() introduced in [15/24]. But it's omitted because DEFINE_GUARD() can be replaced by DEFINE_LOCK_GUARD for locking primitives. In general I wanted to keep the current interface for defining guards untouched, and keeping it simpler.
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h index ec00e3f7af2b..93a166549add 100644 --- a/include/linux/cleanup.h +++ b/include/linux/cleanup.h @@ -223,7 +223,7 @@ const volatile void * __must_check_fn(const volatile void *val) * @exit is an expression using '_T' -- similar to FREE above. * @init is an expression in @init_args resulting in @type * - * EXTEND_CLASS(name, ext, init, init_args...): + * EXTEND_CLASS(name, ext, ctor_attrs, init, init_args...): * extends class @name to @name@ext with the new constructor * * CLASS(name, var)(args...): @@ -243,15 +243,18 @@ const volatile void * __must_check_fn(const volatile void *val) #define DEFINE_CLASS(_name, _type, _exit, _init, _init_args...) \ typedef _type class_##_name##_t; \ static inline void class_##_name##_destructor(_type *p) \ + __no_capability_analysis \ { _type _T = *p; _exit; } \ static inline _type class_##_name##_constructor(_init_args) \ + __no_capability_analysis \ { _type t = _init; return t; } -#define EXTEND_CLASS(_name, ext, _init, _init_args...) \ +#define EXTEND_CLASS(_name, ext, ctor_attrs, _init, _init_args...) \ typedef class_##_name##_t class_##_name##ext##_t; \ static inline void class_##_name##ext##_destructor(class_##_name##_t *p)\ { class_##_name##_destructor(p); } \ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \ + __no_capability_analysis ctor_attrs \ { class_##_name##_t t = _init; return t; } #define CLASS(_name, var) \ @@ -299,7 +302,7 @@ static __maybe_unused const bool class_##_name##_is_conditional = _is_cond #define DEFINE_GUARD_COND(_name, _ext, _condlock) \ __DEFINE_CLASS_IS_CONDITIONAL(_name##_ext, true); \ - EXTEND_CLASS(_name, _ext, \ + EXTEND_CLASS(_name, _ext,, \ ({ void *_t = _T; if (_T && !(_condlock)) _t = NULL; _t; }), \ class_##_name##_t _T) \ static inline void * class_##_name##_ext##_lock_ptr(class_##_name##_t *_T) \ @@ -371,6 +374,7 @@ typedef struct { \ } class_##_name##_t; \ \ static inline void class_##_name##_destructor(class_##_name##_t *_T) \ + __no_capability_analysis \ { \ if (_T->lock) { _unlock; } \ } \ @@ -383,6 +387,7 @@ static inline void *class_##_name##_lock_ptr(class_##_name##_t *_T) \ #define __DEFINE_LOCK_GUARD_1(_name, _type, _lock) \ static inline class_##_name##_t class_##_name##_constructor(_type *l) \ + __no_capability_analysis __asserts_cap(l) \ { \ class_##_name##_t _t = { .lock = l }, *_T = &_t; \ _lock; \ @@ -391,6 +396,7 @@ static inline class_##_name##_t class_##_name##_constructor(_type *l) \ #define __DEFINE_LOCK_GUARD_0(_name, _lock) \ static inline class_##_name##_t class_##_name##_constructor(void) \ + __no_capability_analysis \ { \ class_##_name##_t _t = { .lock = (void*)1 }, \ *_T __maybe_unused = &_t; \ @@ -410,7 +416,7 @@ __DEFINE_LOCK_GUARD_0(_name, _lock) #define DEFINE_LOCK_GUARD_1_COND(_name, _ext, _condlock) \ __DEFINE_CLASS_IS_CONDITIONAL(_name##_ext, true); \ - EXTEND_CLASS(_name, _ext, \ + EXTEND_CLASS(_name, _ext, __asserts_cap(l), \ ({ class_##_name##_t _t = { .lock = l }, *_T = &_t;\ if (_T->lock && !(_condlock)) _T->lock = NULL; \ _t; }), \
Due to the scoped cleanup helpers used for lock guards wrapping acquire/release around their own constructors/destructors that store pointers to the passed locks in a separate struct, we currently cannot accurately annotate *destructors* which lock was released. While it's possible to annotate the constructor to say which lock was acquired, that alone would result in false positives claiming the lock was not released on function return. Instead, to avoid false positives, we can claim that the constructor "asserts" that the taken lock is held. This will ensure we can still benefit from the analysis where scoped guards are used to protect access to guarded variables, while avoiding false positives. The only downside are false negatives where we might accidentally lock the same lock again: raw_spin_lock(&my_lock); ... guard(raw_spinlock)(&my_lock); // no warning Arguably, lockdep will immediately catch issues like this. While Clang's analysis supports scoped guards in C++ [1], there's no way to apply this to C right now. Better support for Linux's scoped guard design could be added in future if deemed critical. [1] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#scoped-capability Signed-off-by: Marco Elver <elver@google.com> --- include/linux/cleanup.h | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)