Rust-for-Linux / linux

Adding support for the Rust language to the Linux kernel.
https://rust-for-linux.com
Other
3.95k stars 420 forks source link

Spinlock irq disable state handling #998

Open fbq opened 1 year ago

fbq commented 1 year ago

@nbdd0121 has an example to demonstrate the current problem:

// Assume irq is enabled here.
let mut g1 = lock1.lock_irqsave();
let mut g2 = lock2.lock_irqsave();
drop(g1);
// IRQ is enabled here!!! Deadlock on lock2 if lock2 is also used in irq handler, or race condtion.
drop(g2);
// IRQ is disabled!!! Which is unexpected to someone.

The above is a valid pattern we'd like to support, the corresponding correct C code is:

unsigned long flags1, flags;
spin_lock_irqsave(lock1, flags1);
spin_lock_irqsave(lock2, flags2);

spin_unlock(lock1); // or spin_unlock_irqrestore(lock1, flags2);

// has to do something with lock1 dropped. For example, acquire a new lock which may deadlock with lock1

spin_unlock_irqrestore(lock2, flags1);

, which looks weird, but correct.

fbq commented 1 year ago

Maybe we introduce this:

impl<T: ?Sized, B: Backend> Guard<'_, T, B> {
    pub fn swap_state(x: &mut Self, y: &mut Self) {
        core::mem::swap(&mut x.state, &mut y.state);
    } 
}

and

// Assume irq is enabled here.
let mut g1 = lock1.lock_irqsave();
let mut g2 = lock2.lock_irqsave();

// swap the state, so we can drop g1 earlier.
Guard::swap_state(&mut g1, &mut g2);

drop(g1);

drop(g2);
wedsonaf commented 1 year ago

How about the case below? It's a simpler incarnation of the issue at hand:

// Assume irq is enabled here.
let g = spinlock.lock();
// IRQ is enabled here!!! Deadlock if spinlock is also used in irq handler, or race condtion.

If we establish that there's a soundness issue (which I don't think we have yet, race conditions are not safety issues), then we should address the case above as well.

Maybe klint can help? Perhaps different types for spinlocks that are used in irq context vs those that aren't?