Date:   Wed, 12 Apr 2023 08:16:34 -0300
From:   Wedson Almeida Filho <wedsonaf@...il.com>
To:     Gary Guo <gary@...yguo.net>
Cc:     rust-for-linux@...r.kernel.org, Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        linux-kernel@...r.kernel.org,
        Wedson Almeida Filho <walmeida@...rosoft.com>,
        Martin Rodriguez Reboredo <yakoyoku@...il.com>
Subject: Re: [PATCH v4 11/13] rust: lock: add `Guard::do_unlocked`

On Tue, 11 Apr 2023 at 17:54, Gary Guo <gary@...yguo.net> wrote:
>
> On Tue, 11 Apr 2023 02:45:41 -0300
> Wedson Almeida Filho <wedsonaf@...il.com> wrote:
>
> > From: Wedson Almeida Filho <walmeida@...rosoft.com>
> >
> > It releases the lock, executes some function provided by the caller,
> > then reacquires the lock. This is preparation for the implementation of
> > condvars, which will sleep after between unlocking and relocking.
> >
> > We need an explicit `relock` method for primitives like `SpinLock` that
> > have an irqsave variant: we use the guard state to determine if the lock
> > was originally acquired with the regular `lock` function or
> > `lock_irqsave`.
> >
> > Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@...il.com>
> > Signed-off-by: Wedson Almeida Filho <walmeida@...rosoft.com>
> > ---
> > v1 -> v2: No changes
> > v2 -> v3: No changes
> > v3 -> v4: No changes
> >
> >  rust/kernel/sync/lock.rs          | 25 +++++++++++++++++++++++++
> >  rust/kernel/sync/lock/spinlock.rs | 17 +++++++++++++++--
> >  2 files changed, 40 insertions(+), 2 deletions(-)
> >
> > diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> > index 819b8ea5ba2b..cde57756795f 100644
> > --- a/rust/kernel/sync/lock.rs
> > +++ b/rust/kernel/sync/lock.rs
> > @@ -22,6 +22,9 @@ pub mod spinlock;
> >  ///
> >  /// - Implementers must ensure that only one thread/CPU may access the protected data once the lock
> >  /// is owned, that is, between calls to `lock` and `unlock`.
> > +/// - Implementers must also ensure that `relock` uses the same locking method as the original
> > +/// lock operation. For example, it should disable interrupts if [`IrqSaveBackend::lock_irqsave`]
> > +/// is used.
> >  pub unsafe trait Backend {
> >      /// The state required by the lock.
> >      type State;
> > @@ -55,6 +58,17 @@ pub unsafe trait Backend {
> >      ///
> >      /// It must only be called by the current owner of the lock.
> >      unsafe fn unlock(ptr: *mut Self::State, guard_state: &Self::GuardState);
> > +
> > +    /// Reacquires the lock, making the caller its owner.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that `state` comes from a previous call to [`Backend::lock`] (or
> > +    /// variant) that has been unlocked with [`Backend::unlock`] and will be relocked now.
> > +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> > +        // SAFETY: The safety requirements ensure that the lock is initialised.
> > +        *guard_state = unsafe { Self::lock(ptr) };
> > +    }
> >  }
> >
> >  /// The "backend" of a lock that supports the irq-save variant.
> > @@ -164,6 +178,17 @@ pub struct Guard<'a, T: ?Sized, B: Backend> {
> >  // SAFETY: `Guard` is sync when the data protected by the lock is also sync.
> >  unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
> >
> > +impl<T: ?Sized, B: Backend> Guard<'_, T, B> {
> > +    #[allow(dead_code)]
> > +    pub(crate) fn do_unlocked(&mut self, cb: impl FnOnce()) {
> > +        // SAFETY: The caller owns the lock, so it is safe to unlock it.
> > +        unsafe { B::unlock(self.lock.state.get(), &self.state) };
> > +        cb();
> > +        // SAFETY: The lock was just unlocked above and is being relocked now.
> > +        unsafe { B::relock(self.lock.state.get(), &mut self.state) };
>
> This should be
>
>         let _guard = ScopeGuard::new(|| unsafe {
>             B::relock(self.lock.state.get(), &mut self.state) }
>         });
>         cb();
>
> Although we currently use `-Cpanic=abort`, I think as a general rule we
> should still try to make code unwind-safe, so it can remain sound if
> someone takes the code and use it for userspace (e.g. for testing
> purpose, or maybe sharing codebase with tools).

Good point. Although this has not been something we cared about in the
last couple of years because we abort, I think we should carefully
review code for this as we upstream.

It is also important for async scenarios: we need to go back to a
consistent state when we tear down `Future` instances.

> > +    }
> > +}
> > +
> >  impl<T: ?Sized, B: Backend> core::ops::Deref for Guard<'_, T, B> {
> >      type Target = T;
> >
> > diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs
> > index 34dec09a97c0..e2a2f68e6d93 100644
> > --- a/rust/kernel/sync/lock/spinlock.rs
> > +++ b/rust/kernel/sync/lock/spinlock.rs
> > @@ -4,6 +4,7 @@
> >  //!
> >  //! This module allows Rust code to use the kernel's `spinlock_t`.
> >
> > +use super::IrqSaveBackend;
> >  use crate::bindings;
> >
> >  /// Creates a [`SpinLock`] initialiser with the given name and a newly-created lock class.
> > @@ -95,7 +96,8 @@ pub type SpinLock<T> = super::Lock<T, SpinLockBackend>;
> >  /// A kernel `spinlock_t` lock backend.
> >  pub struct SpinLockBackend;
> >
> > -// SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion.
> > +// SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. `relock` uses the
> > +// same scheme as `unlock` to figure out which locking method was used originally.
> >  unsafe impl super::Backend for SpinLockBackend {
> >      type State = bindings::spinlock_t;
> >      type GuardState = Option<core::ffi::c_ulong>;
> > @@ -127,13 +129,24 @@ unsafe impl super::Backend for SpinLockBackend {
> >              None => unsafe { bindings::spin_unlock(ptr) },
> >          }
> >      }
> > +
> > +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> > +        let _ = match guard_state {
> > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > +            // initialised.
> > +            None => unsafe { Self::lock(ptr) },
> > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > +            // initialised.
> > +            Some(_) => unsafe { Self::lock_irqsave(ptr) },
> > +        };
> > +    }
> >  }
> >
> >  // SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. We use the `irqsave`
> >  // variant of the C lock acquisition functions to disable interrupts and retrieve the original
> >  // interrupt state, and the `irqrestore` variant of the lock release functions to restore the state
> >  // in `unlock` -- we use the guard context to determine which method was used to acquire the lock.
> > -unsafe impl super::IrqSaveBackend for SpinLockBackend {
> > +unsafe impl IrqSaveBackend for SpinLockBackend {
> >      unsafe fn lock_irqsave(ptr: *mut Self::State) -> Self::GuardState {
> >          // SAFETY: The safety requirements of this function ensure that `ptr` points to valid
> >          // memory, and that it has been initialised before.
>

Powered by blists - more mailing lists