Re: [PATCH] locking: Revert switching guards to _irq_{disable,enable}()

[Boqun Feng <boqun@kernel.org>]

On Tue, Aug 25, 2026 at 04:48:03PM -0700, Boqun Feng wrote:
> On Tue, Aug 25, 2026 at 04:28:59PM -0700, Boqun Feng wrote:
> > On Wed, Aug 26, 2026 at 12:59:25AM +0200, Thomas Gleixner wrote:
> > > On Mon, Aug 24 2026 at 18:33, Boqun Feng wrote:
> > > > On Mon, Aug 24, 2026 at 12:55:23PM +0200, Peter Zijlstra wrote:
> > > >> 
> > > >> While the guards are properly nested, not all wrapped code is nice, as already
> > > >> highlighted by that fair.c hunk.
> > > >> 
> > > >> Syzbot found another instance of this pattern in posix_timer_delete(), which
> > > >> does spin_unlock_irq()+spin_lock_irq() inside scoped_guard(spinlock_irq).
> > > >> Combined with this patch, that goes sideways most spectacular.
> > > >> 
> 
> I'm not saying the posix_timer_delete() implementation has any problem,
> but TBH allowing spin_unlock_irq()+spin_lock_irq() inside
> scoped_guard(spinlock_irq) is questionable design, and can result into
> foot-gun code like:
> 
> 	scoped_guard(spinlock_irq) {
> 		...
> 		spin_unlock_irq();
> 		if (cond)
> 			return; // BOOM, double unlock
> 		spin_lock_irq();
> 	}
> 
> Sure, if handling carefully, it won't cause problem, but it undermines
> the easy-to-use and less-err-prone features of scoped_guard().
> 

One idea is since each scoped_guard() creates a scope guard, the guard
should be used as token for the inner unlock (guard_drop()) and lock
(guard_retake()). So the above code become:

	scoped_guard(spinlock_irq, ...) {
		guard_drop(spinlock_irq, &scope);
		// ^ scope is modified to know that no more unlock is
		// needed.
		if (cond)
			return; // no double unlock
		guard_retake(spinlock_irq, &scope, ...);
	}

The following shows the idea (only compile test). Also applies to
lock_timer as well.

Regards,
Boqun

--------------------------------->8
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index b1b5698cbf1b..459c533e4cc8 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -249,6 +249,19 @@ const volatile void * __must_check_fn(const volatile void *val)
  */
 #define retain_and_null_ptr(p)		((void)__get_and_null(p, NULL))
 
+/*
+ * Drops a scoped guard
+ */
+#define guard_drop(name, p)	class_##name##_destructor(p)
+
+/*
+ * Re-takes a scoped guard
+ */
+#define guard_retake(name, p, ...)					\
+do {									\
+	class_##name##_retake(p, __VA_ARGS__);				\
+} while (0)
+
 /*
  * DEFINE_CLASS(name, type, exit, init, init_args...):
  *	helper to define the destructor and constructor for a type.
@@ -492,7 +505,10 @@ typedef struct {							\
 static __always_inline void class_##_name##_destructor(class_##_name##_t *_T) \
 	__no_context_analysis						\
 {									\
-	_unlock;							\
+	if ((_T)->lock) {						\
+		_unlock;						\
+		(_T)->lock = NULL;					\
+	}								\
 }									\
 									\
 __DEFINE_GUARD_LOCK_PTR(_name, &_T->lock)
@@ -505,6 +521,14 @@ class_##_name##_t class_##_name##_constructor(_type *l)			\
 	class_##_name##_t _t = { .lock = l }, *_T = &_t;		\
 	__VA_ARGS__;							\
 	return _t;							\
+}									\
+static __always_inline __nonnull_args(2)				\
+void class_##_name##_retake(class_##_name##_t *_T, _type *l)		\
+	__no_context_analysis						\
+{									\
+	BUG_ON((_T)->lock);						\
+	(_T)->lock = l;							\
+	__VA_ARGS__;							\
 }
 
 #define __DEFINE_LOCK_GUARD_0(_name, ...)				\
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 436ba794cc0b..b34e8292e989 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -1026,7 +1026,8 @@ static inline void posix_timer_cleanup_ignored(struct k_itimer *tmr)
 	}
 }
 
-static void posix_timer_delete(struct k_itimer *timer)
+static void posix_timer_delete(struct k_itimer *timer,
+			       class_spinlock_irq_t *guard)
 {
 	/*
 	 * Invalidate the timer, remove it from the linked list and remove
@@ -1057,9 +1058,10 @@ static void posix_timer_delete(struct k_itimer *timer)
 
 	while (timer->kclock->timer_del(timer) == TIMER_RETRY) {
 		guard(rcu)();
-		spin_unlock_irq(&timer->it_lock);
+
+		guard_drop(spinlock_irq, guard);
 		timer_wait_running(timer);
-		spin_lock_irq(&timer->it_lock);
+		guard_retake(spinlock_irq, guard, &timer->it_lock);
 	}
 }
 
@@ -1069,8 +1071,14 @@ SYSCALL_DEFINE1(timer_delete, timer_t, timer_id)
 	struct k_itimer *timer;
 
 	scoped_timer_get_or_fail(timer_id) {
+		// Needs a better to "cast" a guard of "lock_timer" to
+		// "spinlock_irq".
+		class_spinlock_irq_t guard = {
+			.lock = &scoped_timer->it_lock,
+		};
+
 		timer = scoped_timer;
-		posix_timer_delete(timer);
+		posix_timer_delete(timer, &guard);
 	}
 	/* Remove it from the hash, which frees up the timer ID */
 	posix_timer_unhash_and_free(timer);
@@ -1101,7 +1109,7 @@ void exit_itimers(struct task_struct *tsk)
 	/* The timers are not longer accessible via tsk::signal */
 	hlist_for_each_entry_safe(timer, next, &timers, list) {
 		scoped_guard (spinlock_irq, &timer->it_lock)
-			posix_timer_delete(timer);
+			posix_timer_delete(timer, &scope);
 		posix_timer_unhash_and_free(timer);
 		cond_resched();
 	}