cleanup: Remove NULL check from unconditional guards

Unconditional guard destructors have dead NULL checks. The lock operation in the constructor would crash before the destructor ever runs with NULL.

- Patches 1-2 prepare guards that legitimately handle NULL.
- Patch 3 adds __nonnull() to guard constructors for compile-time enforcement.
- Patch 4 removes the dead checks.

As compiled by GCC-11 with defconfig on top of the locking/core:

    Total: Before=23889980, After=23834334, chg -0.23%

Changes in v3:

- Audited usages of DEFINE_GUARD(), __DEFINE_UNLOCK_GUARD() and DEFINE_LOCK_GUARD_1() to make sure NULL check removal will work correctly (Peter Zijlstra).
- Moved NULL check into irqdesc_lock unlock expression (Peter Zijlstra).
- Added compiler-enforced nonnull() check for guard constructors.
- Converted nvdimm_bus guard to class.

Changes in v2:

- Expand commit message with detailed reasoning, why the proposed change is correct.
- Rebase on top of locking/core.

v2: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/

See also [1] for relevant discussion.

[1]: https://lore.kernel.org/all/[email protected]/

Dmitry Ilvokhin (4):
nvdimm: Convert nvdimm_bus guard to class
genirq: Move NULL check into irqdesc_lock guard unlock expression
cleanup: Annotate guard constructors with __nonnull()
cleanup: Remove NULL check from unconditional guards

 drivers/nvdimm/nd.h                 | 7 +++++--
 include/linux/cleanup.h             | 8 +++++---
 include/linux/compiler_attributes.h | 6 ++++++
 kernel/irq/internals.h              | 2 +-
 4 files changed, 17 insertions(+), 6 deletions(-)

2.53.0-Meta

The nvdimm_bus guard accepts NULL and skips locking when NULL is passed. Convert from DEFINE_GUARD() to DEFINE_CLASS() + DEFINE_CLASS_IS_GUARD().

This is a preparatory change for making DEFINE_GUARD() constructors __nonnull(). nvdimm_bus legitimately passes NULL, so it must be adjusted to avoid a compile error.

No functional change.

---
 drivers/nvdimm/nd.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index b199eea3260e..18b64559664b 100644
@@ -632,8 +632,11 @@ u64 nd_region_interleave_set_cookie(struct nd_region *nd_region,
 u64 nd_region_interleave_set_altcookie(struct nd_region *nd_region);
 void nvdimm_bus_lock(struct device *dev);
 void nvdimm_bus_unlock(struct device *dev);
-DEFINE_GUARD(nvdimm_bus, struct device *,
-	     if (_T) nvdimm_bus_lock(_T), if (_T) nvdimm_bus_unlock(_T));
+DEFINE_CLASS(nvdimm_bus, struct device *,
+	     if (_T) nvdimm_bus_unlock(_T),
+	     ({ if (_T) nvdimm_bus_lock(_T); _T; }),
+	     struct device *_T);
+DEFINE_CLASS_IS_GUARD(nvdimm_bus);
 
 bool is_nvdimm_bus_locked(struct device *dev);
 void nvdimm_check_and_set_ro(struct gendisk *disk);

2.53.0-Meta

irqdesc_lock uses __DEFINE_UNLOCK_GUARD() directly with a custom constructor that can set .lock to NULL.

In preparation for removing the NULL check from __DEFINE_UNLOCK_GUARD(), move the NULL check into the irqdesc_lock unlock expression, making the NULL handling explicit at the call site.

No functional change.

---
 kernel/irq/internals.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 9412e57056f5..347cb333b9fe 100644
@@ -171,7 +171,7 @@ void __irq_put_desc_unlock(struct irq_desc *desc, unsigned long flags, bool bus)
 
 __DEFINE_CLASS_IS_CONDITIONAL(irqdesc_lock, true);
 __DEFINE_UNLOCK_GUARD(irqdesc_lock, struct irq_desc,
-		      __irq_put_desc_unlock(_T->lock, _T->flags, _T->bus),
+		      if (_T->lock) __irq_put_desc_unlock(_T->lock, _T->flags, _T->bus),
 		      unsigned long flags; bool bus);
 
 static inline class_irqdesc_lock_t class_irqdesc_lock_constructor(unsigned int irq, bool bus,

2.53.0-Meta

Add __nonnull() to unconditional guard constructors so the compiler verifies at each call site that NULL is never passed:

- DEFINE_GUARD(): re-declare the constructor with __nonnull().
- __DEFINE_LOCK_GUARD_1(): annotate the constructor directly.

DEFINE_LOCK_GUARD_0() needs no annotation: its constructor takes no pointer arguments (.lock is hardcoded to (void *)1).

This provides automated, compiler-enforced verification that no unconditional guard constructor receives NULL.

Define the __nonnull() macro in compiler_attributes.h, following the existing convention for attribute wrappers.

---
 include/linux/cleanup.h             | 4 +++-
 include/linux/compiler_attributes.h | 6 ++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

index ea95ca4bc11c..8f8d588b5595 100644
@@ -397,6 +397,7 @@ static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
 	__DEFINE_GUARD_LOCK_PTR(_name, _T)
 
 #define DEFINE_GUARD(_name, _type, _lock, _unlock) \
+	static __always_inline __nonnull() _type class_##_name##_constructor(_type _T); \
 	DEFINE_CLASS(_name, _type, if (_T) { _unlock; }, ({ _lock; _T; }), _type _T); \
 	DEFINE_CLASS_IS_GUARD(_name)
 

@@ -497,7 +498,8 @@ static __always_inline void class_##_name##_destructor(class_##_name##_t *_T) \
 __DEFINE_GUARD_LOCK_PTR(_name, &_T->lock)
 
 #define __DEFINE_LOCK_GUARD_1(_name, _type, ...)			\
-static __always_inline class_##_name##_t class_##_name##_constructor(_type *l) \
+static __always_inline __nonnull()					\
+class_##_name##_t class_##_name##_constructor(_type *l)			\
 	__no_context_analysis						\
 {									\
 	class_##_name##_t _t = { .lock = l }, *_T = &_t;		\

index c16d4199bf92..85f08d6137a2 100644
@@ -176,6 +176,12 @@
  */
 #define __mode(x)                       __attribute__((__mode__(x)))
 
+/*
+ *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-nonnull-function-attribute
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#nonnull
+ */
+#define __nonnull(x...)			__attribute__((__nonnull__(x)))
+
 /*
  * Optional: only supported since gcc >= 7
  *

2.53.0-Meta

I wouldn't say "verify", since the compiler does a best-effort here with the information it has statically.

In other words, the attribute does not prevent NULL pointers to be passed.

Hmm... It appears GCC has changed the docs in commit 6e3c137f5dbb ("doc: Merge function, variable, type, and statement attribute sections [PR88472]"), dropping the per-kind attribute pages.

So the right link would need to be now:

https://gcc.gnu.org/onlinedocs/gcc/Common-Attributes.html#index-nonnull

I will need to send a patch to fix the other links.

I think this link goes to _Nonnull -- the GNU one is instead:

https://clang.llvm.org/docs/AttributeReference.html#id10

(I don't love the numeric IDs, though, since they break, so I think it is fine either way -- the _Nonnull is fairly close to the one we want and I hope that one doesn't break)

This is indeed available for a long time, and we already use it elsewhere in the kernel tree (which would be nice to clean up separately).

If you don't mind, please place it before __nonstring__ (the file is meant to be sorted by the actual attribute name -- there are a few instances where this is not the case anymore, which I will eventually clean up)

Thanks!

Cheers,
Miguel

Fair enough.

I'll re-word this paragraph as "Add __nonnull() to unconditional guard constructors so the compiler warns when NULL is statically known to be passed" and drop the "compiler-enforced verification" paragraph.

Fixed locally. Thanks!

I don't quite like numeric IDs either. There is only one #id reference in include/linux/compiler_attributes.h and link is already dead. I'll keep current link since it gives at least some clue what to look for on the page.

Thanks, fixed locally.

Sounds good, thanks!

Cheers,
Miguel

The unconditional guard destructors check whether the lock pointer is NULL before unlocking. This check is dead code because unconditional guards guarantee a non-NULL lock pointer at destructor time.

DEFINE_GUARD() runs the lock operation unconditionally in the constructor. If the pointer were NULL, the lock operation (e.g. mutex_lock(NULL)) would crash before the constructor returns. The destructor never runs with a NULL pointer. All DEFINE_GUARD() users dereference the pointer in their lock. Verified by auditing every instance found by: git grep -n -A 1 'DEFINE_GUARD('. The only exception is xe_pm_runtime_release_only, whose constructor is a noop, but it has no callers.

__DEFINE_UNLOCK_GUARD() has only a few usages outside of include/linux/cleanup.h: tty_port_tty (NULL-checks in its tty_kref_put() call), irqdesc_lock (fixed earlier) and two guards in kernel/sched/sched.h (dereference the pointer unconditionally in their lock constructors).

DEFINE_LOCK_GUARD_1() sets .lock from its argument and runs the lock operation in the constructor. Same reasoning applies. All DEFINE_LOCK_GUARD_1() users dereference the pointer in their lock. Also, verified by auditing every match of: git grep -n 'DEFINE_LOCK_GUARD_1('.

DEFINE_LOCK_GUARD_0() hardcodes .lock = (void *)1 in the constructor, so it is never NULL by construction.

Conditional (_try) variants: DEFINE_GUARD_COND() and DEFINE_LOCK_GUARD_1_COND() use EXTEND_CLASS_COND(), whose wrapper destructor returns early when the lock was not acquired, before reaching the base destructor since commit 2deccd5c862a ("cleanup: Optimize guards"):

    if (_cond) return; class_##_name##_destructor(_T);

As compiled by GCC-11 with defconfig on top of the locking/core:

    Total: Before=23889980, After=23834334, chg -0.23%

---
 include/linux/cleanup.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 8f8d588b5595..1f6d1a97617a 100644
@@ -398,7 +398,7 @@ static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
 
 #define DEFINE_GUARD(_name, _type, _lock, _unlock) \
 	static __always_inline __nonnull() _type class_##_name##_constructor(_type _T); \
-	DEFINE_CLASS(_name, _type, if (_T) { _unlock; }, ({ _lock; _T; }), _type _T); \
+	DEFINE_CLASS(_name, _type, _unlock, ({ _lock; _T; }), _type _T); \
 	DEFINE_CLASS_IS_GUARD(_name)
 
 #define DEFINE_GUARD_COND_4(_name, _ext, _lock, _cond) \

@@ -492,7 +492,7 @@ typedef struct {							\
 static __always_inline void class_##_name##_destructor(class_##_name##_t *_T) \
 	__no_context_analysis						\
 {									\
-	if (_T->lock) { _unlock; }					\
+	_unlock;							\
 }									\
 									\
 __DEFINE_GUARD_LOCK_PTR(_name, &_T->lock)

2.53.0-Meta