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_args() 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 v5:

- Renamed __nonnull() attribute to __nonnull_args() to fix User Mode Linux build failures (kernel test robot).
- Dropped Acked-by tag from Miguel, since I renamed the attribute.

Changes in v4:

- Re-worded commit message paragraph about nonnull "compiler-enforced verification" (Miguel Ojeda).
- Fixed GCC documentation link for nonnull() attribute (Miguel Ojeda).
- Placed nonnull() before nonstring() in include/linux/compiler_attributes.h (Miguel Ojeda).
- Picked up tags, where appropriate.

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.

v4: https://lore.kernel.org/all/[email protected]/
v3: https://lore.kernel.org/all/[email protected]/
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 | 9 +++++++++
 kernel/irq/internals.h              | 2 +-
 4 files changed, 20 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_args(). 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_args() to unconditional guard constructors so the compiler warns when NULL is statically known to be passed:

- DEFINE_GUARD(): re-declare the constructor with __nonnull_args().
- __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).

Define the __nonnull_args() macro in compiler_attributes.h, following the existing convention for attribute wrappers. Deliberately not named '__nonnull', to avoid clashing with glibc's __nonnull() when kernel and userspace headers are combined (User Mode Linux for example).

---
Miguel, I dropped your Acked-by due to the rename. Went with __nonnull_args() (over __knonnull()). Happy to restore your tag if that spelling works for you.

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

index ea95ca4bc11c..4e60d519713c 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_args() _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_args()					\
+class_##_name##_t class_##_name##_constructor(_type *l)			\
 	__no_context_analysis						\
 {									\
 	class_##_name##_t _t = { .lock = l }, *_T = &_t;		\

index c16d4199bf92..cffe09387ea6 100644
@@ -230,6 +230,15 @@
  */
 #define   noinline                      __attribute__((__noinline__))
 
+/*
+ * Note: deliberately not named '__nonnull', to avoid clashing with glibc's
+ * __nonnull() when kernel and userspace headers are combined.
+ *
+ *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Attributes.html#index-nonnull
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#nonnull
+ */
+#define __nonnull_args(x...)		__attribute__((__nonnull__(x)))
+
 /*
  * Optional: only supported since gcc >= 8
  * Optional: not supported by clang

2.53.0-Meta

I am fine with either, but thanks for the caution! :)

I assume _args() is meant as "the nonnull for the arguments, not the return"?

Cheers,
Miguel

Thanks for confirmation, Miguel.

Yes, that is exactly what I had in mind.

Sparse doesn't like an empty __nonnull_args() at all.

./include/linux/spinlock.h:608:1: error: an expression is expected before ')' ./include/linux/spinlock.h:619:1: error: an expression is expected before ')' ./include/linux/spinlock.h:631:1: error: an expression is expected before ')'

Shouldn't we specify the arguments which are non-NULL? __nonnull_args(1)?

regards,
dan carpenter

Thanks for the report, Dan.

That is interesting, it seems sparse trips over the empty-parens form the guard macro expands to, nonnull(), but is fine with __attribute__((nonnull)) without explicit arguments, that a few places in-tree already use. GCC and clang accept both, so this probably deserves a fix in sparse too.

Meanwhile, spelling the argument out explicitly is a simple and reasonable fix (and arguably clearer). Patch below.

From 50f7e7eaad1d33773cee9a278625fdb4b451f53b Mon Sep 17 00:00:00 2001
From: Dmitry Ilvokhin <[email protected]>
Date: Fri, 5 Jun 2026 03:06:22 -0700
Subject: [PATCH] cleanup: Specify nonnull argument index

The guard constructors were annotated with an empty __nonnull_args(), relying on __nonnull__() marking every pointer parameter as non-NULL. Sparse cannot parse the empty argument list.

Both constructors take the lock pointer as their first parameter, so specify the index explicitly: __nonnull_args(1).

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

index 65416938e318..b1b5698cbf1b 100644
@@ -397,7 +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_args() _type class_##_name##_constructor(_type _T); \
+	static __always_inline __nonnull_args(1) _type class_##_name##_constructor(_type _T); \
 	DEFINE_CLASS(_name, _type, _unlock, ({ _lock; _T; }), _type _T); \
 	DEFINE_CLASS_IS_GUARD(_name)
 

@@ -498,7 +498,7 @@ 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 __nonnull_args()					\
+static __always_inline __nonnull_args(1)				\
 class_##_name##_t class_##_name##_constructor(_type *l)			\
 	__no_context_analysis						\
 {									\

2.53.0-Meta

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 4e60d519713c..65416938e318 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_args() _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