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

[Bart Van Assche <bvanassche@acm.org>]

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.