[PATCH v2] cleanup: Remove NULL check from unconditional guards

[PATCH v2] cleanup: Remove NULL check from unconditional guards

[Dmitry Ilvokhin]

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() and DEFINE_LOCK_GUARD_1() both run the lock operation
in the constructor before returning. 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.

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 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=23833993, chg -0.23%

Signed-off-by: Dmitry Ilvokhin <d@ilvokhin.com>
---
Changes in v2:

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

v1: https://lore.kernel.org/all/20260427165037.205337-1-d@ilvokhin.com/

See also [1] for relevant discussion.

[1]: https://lore.kernel.org/all/afCS4d4YccQFtvpi@shell.ilvokhin.com/

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

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index ea95ca4bc11c..1410effa8780 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -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) \
-	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) \
@@ -491,7 +491,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

Re: [PATCH v2] cleanup: Remove NULL check from unconditional guards

[Peter Zijlstra]

On Tue, May 12, 2026 at 07:15:10AM +0000, Dmitry Ilvokhin wrote:
> 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() and DEFINE_LOCK_GUARD_1() both run the lock operation
> in the constructor before returning. 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.
> 
> 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 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=23833993, chg -0.23%
> 
> Signed-off-by: Dmitry Ilvokhin <d@ilvokhin.com>
> ---
> Changes in v2:
> 
> - Expand commit message with detailed reasoning, why the proposed
>   change is correct.
> - Rebase on top of locking/core.
> 
> v1: https://lore.kernel.org/all/20260427165037.205337-1-d@ilvokhin.com/
> 
> See also [1] for relevant discussion.
> 
> [1]: https://lore.kernel.org/all/afCS4d4YccQFtvpi@shell.ilvokhin.com/
> 
>  include/linux/cleanup.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index ea95ca4bc11c..1410effa8780 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -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) \
> -	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) \
> @@ -491,7 +491,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)

What about class_irqdesc_lock_constructor() ? AFAICT
__irq_get_desc_lock() can return NULL.

Re: [PATCH v2] cleanup: Remove NULL check from unconditional guards

[Dmitry Ilvokhin]

On Tue, May 12, 2026 at 02:45:57PM +0200, Peter Zijlstra wrote:
> 
> What about class_irqdesc_lock_constructor() ? AFAICT
> __irq_get_desc_lock() can return NULL.

Thanks for taking a look, Peter. Yes, that is actually a very good
catch.

For some reason I naively thought __DEFINE_UNLOCK_GUARD() wouldn't be
used outside of include/linux/cleanup.h, but this is obviously wrong
assumption. There are cases, where DEFINE_LOCK_GUARD_1() doesn't fit
callers needs, so __DEFINE_UNLOCK_GUARD() is used directly.

- kernel/irq/internals.h: the case you pointed out. Can be fixed by
  moving the NULL check into the irqdesc_lock unlock expression
  directly.

- include/linux/tty_port.h: similar use case. NULL check can be moved
  into tty_port_tty as well, similar to previous case.

- kernel/sched/sched.h: lock and lock2 shouldn't be NULL at destructor
  time, since they are dereferenced unconditionally at constructor.

Below is example how this will look like. Does this look reasonable to
you, or would you prefer a different approach?

From 835d6c13caf8169db7bd049c9c549d5988fd0a82 Mon Sep 17 00:00:00 2001
From: Dmitry Ilvokhin <d@ilvokhin.com>
Date: Tue, 12 May 2026 06:59:04 -0700
Subject: [PATCH] cleanup: Move NULL check into conditional guard unlock
 expressions

irqdesc_lock and tty_port_tty use __DEFINE_UNLOCK_GUARD() directly
with custom constructors that can set .lock to NULL. Move the NULL
check from the generic __DEFINE_UNLOCK_GUARD() destructor into their
_unlock expressions, making the NULL handling explicit at the call
site.

Signed-off-by: Dmitry Ilvokhin <d@ilvokhin.com>
---
 include/linux/tty_port.h | 2 +-
 kernel/irq/internals.h   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/tty_port.h b/include/linux/tty_port.h
index d2a7882c0b58..cbbff6aabdad 100644
--- a/include/linux/tty_port.h
+++ b/include/linux/tty_port.h
@@ -286,7 +286,7 @@ static inline void tty_port_tty_vhangup(struct tty_port *port)
 #ifdef CONFIG_TTY
 void tty_kref_put(struct tty_struct *tty);
 __DEFINE_CLASS_IS_CONDITIONAL(tty_port_tty, true);
-__DEFINE_UNLOCK_GUARD(tty_port_tty, struct tty_struct, tty_kref_put(_T->lock));
+__DEFINE_UNLOCK_GUARD(tty_port_tty, struct tty_struct, if (_T->lock) tty_kref_put(_T->lock));
 static inline class_tty_port_tty_t class_tty_port_tty_constructor(struct tty_port *tport)
 {
 	class_tty_port_tty_t _t = {
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 9412e57056f5..347cb333b9fe 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -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

Re: [PATCH v2] cleanup: Remove NULL check from unconditional guards

[Peter Zijlstra]

On Tue, May 12, 2026 at 02:37:25PM +0000, Dmitry Ilvokhin wrote:
> On Tue, May 12, 2026 at 02:45:57PM +0200, Peter Zijlstra wrote:
> > 
> > What about class_irqdesc_lock_constructor() ? AFAICT
> > __irq_get_desc_lock() can return NULL.
> 
> Thanks for taking a look, Peter. Yes, that is actually a very good
> catch.
> 
> For some reason I naively thought __DEFINE_UNLOCK_GUARD() wouldn't be
> used outside of include/linux/cleanup.h, but this is obviously wrong
> assumption. There are cases, where DEFINE_LOCK_GUARD_1() doesn't fit
> callers needs, so __DEFINE_UNLOCK_GUARD() is used directly.
> 
> - kernel/irq/internals.h: the case you pointed out. Can be fixed by
>   moving the NULL check into the irqdesc_lock unlock expression
>   directly.
> 
> - include/linux/tty_port.h: similar use case. NULL check can be moved
>   into tty_port_tty as well, similar to previous case.
> 
> - kernel/sched/sched.h: lock and lock2 shouldn't be NULL at destructor
>   time, since they are dereferenced unconditionally at constructor.
> 
> Below is example how this will look like. Does this look reasonable to
> you, or would you prefer a different approach?

Yeah, I don't mind changing things here, but like I said before, only
after a full audit of every single user ;-)

And yeah, fixing up those things shouldn't be hard, but it needs be
done.