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 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.

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 | 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 warns when NULL is statically known to be 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).

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..10a1410eb3e2 100644
@@ -230,6 +230,12 @@
  */
 #define   noinline                      __attribute__((__noinline__))
 
+/*
+ *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Attributes.html#index-nonnull
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#nonnull
+ */
+#define __nonnull(x...)			__attribute__((__nonnull__(x)))
+
 /*
  * Optional: only supported since gcc >= 8
  * Optional: not supported by clang

2.53.0-Meta

The build robot found something to hate in this one. I think you're on Cc there. It looks to me like clang-23 is confused somehow, but who knows.

Thanks for the heads-up, Peter. I got the report and will look into it.

Seems like clang is not confused here, I was able to reproduce the problem with GCC 11 as well.

There is a conflict with glibc's own __nonnull macro

    https://elixir.bootlin.com/glibc/glibc-2.43/source/misc/sys/cdefs.h#L560-L562

which doesn't match the one from include/linux/compiler_attributes.h. They usually don't collide, except for User Mode Linux builds, which include both kernel and userspace headers.

Options are:

1. Drop the __nonnull() macro from include/linux/compiler_attributes.h and use __attribute__((__nonnull__())) directly in include/linux/cleanup.h. This is a bit unfortunate, since __nonnull() seems like a useful shortcut, but seems like the simplest solution.

2. Rename __nonnull() to __nonnull_args() to avoid the conflict. A returns_nonnull attribute is supported by compilers, so the name fits, but it diverges from the existing naming convention in include/linux/compiler_attributes.h.

3. #define __nonnull(params) __attribute__((__nonnull__ params)). This keeps the name and is compatible with both kernel and glibc usage. Current call sites use __nonnull() with no arguments, which works identically. Future callers with specific parameter numbers would use __nonnull((1, 2)) with double parens, matching glibc's convention. I don't like this option, listed it here for the sake of completeness.

I am leaning towards option 1 so far.

:(

What about other similar names? i.e. a variation of your option 2, e.g. just nonnull (we also have others like that, i.e. no underscore, e.g. noinline), or ___nonnull (triple underscore, but may be confusing), or a suffix/prefix letter, e.g. __knonnull (for kernel nonnull)...

i.e. it would be nice to have a "standard" spelling for ourselves, and also replace the existing __attribute__((nonnull))s we have elsewhere in the tree.

Cheers,
Miguel

Yes, a different name might work as well. The main question is which name to pick.

Plain nonnull() is a bit dangerous. It might collide with existing identifiers (now or in the future) and is not great for a kernel-wide macro. I think we got away with plain noinline, because it was there forever. There are also at least 24 nonnull attribute usages in the kernel that need to be converted atomically. This can be done, but it increases the scope of the patchset, which I'd rather avoid.

___nonnull() with triple underscore might be a bit confusing, I agree.

__knonnull() might work. I like __nonnull_args() better: it gives the reader a hint about the semantics, while __knonnull() is just a collision-avoidance trick without conveying any additional meaning. That being said, I can totally do __knonnull().

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