linux-kernel - Re: [PATCH v13 05/17] irq & spin_lock: Add counted interrupt disabling/enabling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7a98eb42-bc54-4f22-bc85-0f6d41f39fc7@acm.org>
Date: Wed, 15 Oct 2025 13:54:05 -0700
From: Bart Van Assche <bvanassche@....org>
To: Lyude Paul <lyude@...hat.com>, rust-for-linux@...r.kernel.org,
 Thomas Gleixner <tglx@...utronix.de>, Boqun Feng <boqun.feng@...il.com>,
 linux-kernel@...r.kernel.org, Daniel Almeida <daniel.almeida@...labora.com>
Cc: Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
 Juri Lelli <juri.lelli@...hat.com>,
 Vincent Guittot <vincent.guittot@...aro.org>,
 Dietmar Eggemann <dietmar.eggemann@....com>,
 Steven Rostedt <rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>,
 Mel Gorman <mgorman@...e.de>, Valentin Schneider <vschneid@...hat.com>,
 Will Deacon <will@...nel.org>, Waiman Long <longman@...hat.com>,
 Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
 Gary Guo <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, Benno Lossin <lossin@...nel.org>,
 Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>,
 David Woodhouse <dwmw@...zon.co.uk>,
 Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
 Joel Fernandes <joelagnelf@...dia.com>, Ryo Takakura <ryotkkr98@...il.com>,
 K Prateek Nayak <kprateek.nayak@....com>
Subject: Re: [PATCH v13 05/17] irq & spin_lock: Add counted interrupt
 disabling/enabling

On 10/13/25 8:48 AM, Lyude Paul wrote:
> Currently the nested interrupt disabling and enabling is present by
> _irqsave() and _irqrestore() APIs, which are relatively unsafe, for
> example:
> 
> 	<interrupts are enabled as beginning>
> 	spin_lock_irqsave(l1, flag1);
> 	spin_lock_irqsave(l2, flag2);
> 	spin_unlock_irqrestore(l1, flags1);
> 	<l2 is still held but interrupts are enabled>
> 	// accesses to interrupt-disable protect data will cause races.
> 
> This is even easier to triggered with guard facilities:
> 
> 	unsigned long flag2;
> 
> 	scoped_guard(spin_lock_irqsave, l1) {
> 		spin_lock_irqsave(l2, flag2);
> 	}
> 	// l2 locked but interrupts are enabled.
> 	spin_unlock_irqrestore(l2, flag2);
> 
> (Hand-to-hand locking critical sections are not uncommon for a
> fine-grained lock design)
> 
> And because this unsafety, Rust cannot easily wrap the
> interrupt-disabling locks in a safe API, which complicates the design.
> 
> To resolve this, introduce a new set of interrupt disabling APIs:
> 
> *	local_interrupt_disable();
> *	local_interrupt_enable();
> 
> They work like local_irq_save() and local_irq_restore() except that 1)
> the outermost local_interrupt_disable() call save the interrupt state
> into a percpu variable, so that the outermost local_interrupt_enable()
> can restore the state, and 2) a percpu counter is added to record the
> nest level of these calls, so that interrupts are not accidentally
> enabled inside the outermost critical section.
> 
> Also add the corresponding spin_lock primitives: spin_lock_irq_disable()
> and spin_unlock_irq_enable(), as a result, code as follow:
> 
> 	spin_lock_irq_disable(l1);
> 	spin_lock_irq_disable(l2);
> 	spin_unlock_irq_enable(l1);
> 	// Interrupts are still disabled.
> 	spin_unlock_irq_enable(l2);
> 
> doesn't have the issue that interrupts are accidentally enabled.
> 
> This also makes the wrapper of interrupt-disabling locks on Rust easier
> to design.

Is a new counter really required to fix the issues that exist in the
above examples? Has it been considered to remove the spin_lock_irqsave()
and spin_lock_irqrestore() definitions, to introduce a macro that saves
the interrupt state in a local variable and restores the interrupt state
from the same local variable? With this new macro, the first two examples
above would be changed into the following (this code has not been tested
in any way):

scoped_irq_disable {
   spin_lock(l1);
   spin_lock(l2);
   ...
   spin_unlock(l1);
   spin_unlock(l2);
}

scoped_irq_disable {
   scoped_irq_disable {
     scoped_guard(spin_lock, l1) {
       spin_lock(l2);
     }
   }
   spin_unlock(l2);
}

scoped_irq_disable could be defined as follows:

static inline void __local_irq_restore(void *flags)
{
	local_irq_restore(*(unsigned long *)flags);
}

#define scoped_irq_disable \
	__scoped_irq_disable(__UNIQUE_ID(flags), __UNIQUE_ID(label))

#define __scoped_irq_disable(_flags, _label)                          \
	for (unsigned long _flags __cleanup(__local_irq_restore);     \
	     ({ local_irq_save(_flags); }), true; ({ goto _label; })) \
		if (0) {                                              \
_label:                                                               \
			break;                                        \
		} else


Thanks,

Bart.