From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9A15F209F43; Wed, 26 Aug 2026 01:33:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787708018; cv=none; b=axHe+wpPHzGFXtR5hwhceh4alDKpFnhXUPw8d1H1pUvaTKYWNUlKRKOFDjxyU0KhPhx/RRqIOm3mGf7e90yAMztURSxRNzNPZy3/OzkdqvZoMjCtuTiiS0wRMVoKGjvBAWqoumIBZAJZCDQajYH6CXaczFRC15V7QGR1CAb5XEY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787708018; c=relaxed/simple; bh=1xHs+DBqZLyW+q3iNKh54Iy8lVMqjmW6z2EPHBYaMgM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=HCZMGfY9OyXYEjVi43dIumR9QAoRXMAywMJ7gQEjMUjp37H21jiDn9+DRyT/DlBrXBNvV5/+ozPmMF2LKBuZlQ2I60gMXCQF3kI9GFO0sC/B7kWups1114h/wRl5NAUKfp+zaC7krRF876TCJWOEVdiyrmSKPmH584tCZ14nBnA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=WAwC8VoC; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="WAwC8VoC" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 06D721F000E9; Wed, 26 Aug 2026 01:33:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1787708017; bh=AETJxl6uYUK6KKw6tSCQPpxO6R/EX5YgQwGoXOJm0rY=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=WAwC8VoCyI15B9ix3oxJGPwndFBTke/Ecx1CkB0lGVajx7MAamJDZKe/53QDA/bKq 18FHcSvx9EeE4jF2dFZIhIxLqJicbLmHWZh0ECO7deoBIoY0VqHTR2bolwpktteKBx 0BZt+3kcs3mUJ8X+A0Py+sIxABqD+oTEIpVQPpwYf5/zXPZjMnBPxLlGFZboLGcy5u 1E5oA5nOhl/UqWfgDrFvHhOnI5+XZLIUpFah8o0zNcub7rHeJ1XyCmxqy8KL0n6Z9I x1DJPRbpL9GKAuyjg2YecQMRzXjXcqvyNRofN89Dr7IXoTlbtMfsOhvGoM/Nqi2VyJ l1lCrYspfZ8FA== Received: from phl-compute-11.internal (phl-compute-11.internal [10.202.2.51]) by mailfauth.phl.internal (Postfix) with ESMTP id 188CBF40066; Tue, 25 Aug 2026 21:33:36 -0400 (EDT) Received: from phl-frontend-04 ([10.202.2.163]) by phl-compute-11.internal (MEProxy); Tue, 25 Aug 2026 21:33:36 -0400 X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: dmFkZTEy0AWBO0GYTrp5Bsu6xTNSYrB1iJUx4KL6ozhWhS5dewweeaAGYSmq9U98YbRxEJ GMunhY8mMndJKnr8CVov6sAdmZAIKg7ces8UP73tHTRF3Zy5j5/6/8BYAMc9OLJkCiSB1j EZUdqtKnV9YqmiS+USMNFDmWmLEUBfQWGZOTYP4rs8MotOCR1US12tnHJvkVXqjrkcPYkp 0V4nM048XAUHkEDGachv3YugVqOsOzVbr8JSyBEgLRFhZCeuI5+w009AKnPyoiylEA+DQq ygujckcubTTTxIJtqxmwq3c+Jvw1B5Un0dkmPO6T8Grt9QdCZPeToie378hIsvRDJ/sdqj h+Qk/3YrXO3kOmlZTKhh1hNraFwe4OWdvcpPM+/ZOBIRoPphJa9Yratc7CjOJnEoJWpY0a c3yQ70qis+X+D3BtRheRpe7JY2eqIU4q1PpoPi97WHtQuHZQ9NralzQ3UmHXMo4vfGWO1L pGDBjQCSWZMQBYwVQWPxdpySZF4+SfzZKOilH71xSEgJQ+0OdOAp6BelVMK3lrI3QJmQgM wS+GPfaXvX8kjLfql0TgxwkTp2WNCCl3XXIPMe7t019ycAmJMKsLYtHST7OJy5Hei830xr j0GQn3lP+rm55UO6feh7whHBn2JKyyguBcYMsIGmBLV0rajpLFLdkTQl1J3A X-ME-Proxy: Feedback-ID: i8dbe485b:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 25 Aug 2026 21:33:35 -0400 (EDT) Date: Tue, 25 Aug 2026 18:33:34 -0700 From: Boqun Feng To: Thomas Gleixner Cc: Peter Zijlstra , linux-kernel@vger.kernel.org, linux-tip-commits@vger.kernel.org, x86@kernel.org Subject: Re: [PATCH] locking: Revert switching guards to _irq_{disable,enable}() Message-ID: References: <20260804161447.84806-8-boqun@kernel.org> <178635226387.442315.3868294476114711805.tip-bot2@tip-bot2> <20260824104704.GA4121339@noisy.programming.kicks-ass.net> <20260824105523.GA4121620@noisy.programming.kicks-ass.net> <877bldhkmq.ffs@fw13> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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(); }