Re: [PATCH v2 1/3] rust: Introduce irq module

[Benno Lossin]

On 01.08.24 11:51, Benno Lossin wrote:
> On 01.08.24 00:35, Lyude Paul wrote:
>> +/// Run the closure `cb` with interrupts disabled on the local CPU.
>> +///
>> +/// This creates an [`IrqDisabled`] token, which can be passed to functions that must be run
>> +/// without interrupts.
>> +///
>> +/// # Examples
>> +///
>> +/// Using [`with_irqs_disabled`] to call a function that can only be called with interrupts
>> +/// disabled:
>> +///
>> +/// ```
>> +/// use kernel::irq::{IrqDisabled, with_irqs_disabled};
>> +///
>> +/// // Requiring interrupts be disabled to call a function
>> +/// fn dont_interrupt_me(_irq: IrqDisabled<'_>) {
>> +///     /* When this token is available, IRQs are known to be disabled. Actions that rely on this
>> +///      * can be safely performed
>> +///      */
>> +/// }
>> +///
>> +/// // Disabling interrupts. They'll be re-enabled once this closure completes.
>> +/// with_irqs_disabled(|irq| dont_interrupt_me(irq));
>> +/// ```
>> +#[inline]
>> +pub fn with_irqs_disabled<'a, T, F>(cb: F) -> T
>> +where
>> +    F: FnOnce(IrqDisabled<'a>) -> T,
> 
> You can use this as the signature:
> 
>     pub fn with_irqs_disabled<'a, T>(cb: impl FnOnce(IrqDisabled<'a>) -> T) -> T

I just noticed that this and the version above are wrong, since they
allow `T` to depend on `'a` ie you can do the following:

    pub fn cheat() -> IrqDisabled<'static> {
        with_irqs_disabled(|irq| irq)
    }

And thus obtain an `IrqDisabled` token without IRQs being disabled.

To fix this, you must use the `for<'a>` notation, so either

    pub fn with_irqs_disabled<T>(cb: impl for<'a> FnOnce(IrqDisabled<'a>) -> T) -> T

or

    pub fn with_irqs_disabled<T, F>(cb: F) -> T
    where
        F: for<'a> FnOnce(IrqDisabled<'a>) -> T,

This ensures that the callback works for any lifetime and thus `T` is
not allowed to depend on it.

---
Cheers,
Benno