linux-kernel - Re: [PATCH v6 3/3] rust: sync: Add SpinLockIrq

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: <ZwoswDGED0fWMd61@boqun-archlinux>
Date: Sat, 12 Oct 2024 01:01:04 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Lyude Paul <lyude@...hat.com>, rust-for-linux@...r.kernel.org,
	Danilo Krummrich <dakr@...hat.com>, airlied@...hat.com,
	Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
	Waiman Long <longman@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>, linux-kernel@...r.kernel.org,
	Benno Lossin <benno.lossin@...ton.me>,
	Daniel Almeida <daniel.almeida@...labora.com>,
	Gary Guo <gary@...yguo.net>, Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Wedson Almeida Filho <wedsonaf@...il.com>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Andreas Hindborg <a.hindborg@...sung.com>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Martin Rodriguez Reboredo <yakoyoku@...il.com>,
	Valentin Obst <kernel@...entinobst.de>
Subject: Re: [PATCH v6 3/3] rust: sync: Add SpinLockIrq

On Tue, Oct 08, 2024 at 05:21:19PM +0200, Thomas Gleixner wrote:
> On Mon, Oct 07 2024 at 14:30, Lyude Paul wrote:
> > On Mon, 2024-10-07 at 14:01 +0200, Thomas Gleixner wrote:
> > So actually the new solution I suggested a little after that original email
> > wouldn't need to call local_irq_save() directly - sorry, I just explained it
> > kind of poorly and it hadn't been in my head for very long. I think you'll
> > like this solution a lot more though, lemme explain:
> >
> > Basically instead of having functions like with_interrupts_disabled, we would
> > instead introduce a new trait that can be implemented by locks with context
> > tokens: BackendWithContext:
> >
> > pub trait BackendWithContext: Backend {
> > type ContextState;
> >
> > unsafe fn lock_first(ptr: *Self::State)
> > -> (Self::Context, Self::ContextState, Self::GuardState);
> >
> > unsafe fn unlock_last(
> > ptr: *Self::State,
> > context_state: Self::ContextState,
> > guard_state: &Self::GuardState
> > );
> > }
> >
> > Where the idea is that a type like SpinlockIrq would define ContextState to be
> > a u64 (holding the flags argument from spin_lock_irqsave). lock_first() would

I would suggest we use a usize for the ContextState. And the name of
these two functions can be lock_with_context_saved()
unlock_with_context_restored(), _first() and _last() may be misleading,
because technically, you can have a nested lock_first(), e.g.

	let (c1, c1state, g) = unsafe { lock_first(...); }
	let (c2, c2state, g) = unsafe { lock_first(...); }

we will just need to make sure `unlock_last()` called in the reverse
ordering as a safety requirement. Then _first() and _last() doesn't make
sense.

Regards,
Boqun

> > use spin_lock_irqsave and create the token, unlock_last() would use
> > spin_unlock_irqrestore with the saved ContextState. Then we could use those
> > unsafe primitives to implement a method on Lock like this:
> >
> > impl<T: ?Sized, B: BackendWithContext> Lock<T, B> {
> > pub fn lock_with_new<'a>(
> > &self,
> > cb: impl FnOnce(Self::Context, &mut Guard<'a, T, B>) -> R
> > ) -> R;
> > }
> >
> > What lock_with_new would do is:
> >
> >  * call B::first_lock() (which would be spin_lock_irqsave)
> >  * call cb() with a LocalInterruptsDisabled token and a &mut to the Guard (so
> >    that the caller can't drop the lock before exiting the noirq context)
> >  * Call B::last_unlock() with the ContextState that was passed to first_lock()
> >    (which would be spin_unlock_irqrestore)
> >
> > So we'd absolutely still be modeling around the locking primitives
> > spin_lock_irqsave() and spin_unlock_irqrestore(). And subsequently we could
> > still nest lock contexts like normal. with_irqs_disabled() wouldn't be needed
> > in this arrangement - but we would still need the Interrupt tokens (which
> > would be fine since they're just for enforcing correctness anyway).
> 
> Makes sense.
> 
[...]