Re: [PATCH v4 02/13] rust: sync: introduce `Lock` and `Guard`

[Wedson Almeida Filho]

On Tue, 11 Apr 2023 at 17:42, Gary Guo <gary@xxxxxxxxxxx> wrote:
>
> On Tue, 11 Apr 2023 02:45:32 -0300
> Wedson Almeida Filho <wedsonaf@xxxxxxxxx> wrote:
>
> > From: Wedson Almeida Filho <walmeida@xxxxxxxxxxxxx>
> >
> > They are generic Rust implementations of a lock and a lock guard that
> > contain code that is common to all locks. Different backends will be
> > introduced in subsequent commits.
> >
> > Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@xxxxxxxxx>
> > Suggested-by: Gary Guo <gary@xxxxxxxxxxx>
> > Signed-off-by: Wedson Almeida Filho <walmeida@xxxxxxxxxxxxx>
> > ---
> > v1 -> v2: No changes
> > v2 -> v3: Use new Opaque::ffi_init from Benno's series
> > v3 -> v4: Fixed name of parameter in Lock comment
> >
> >  rust/kernel/sync.rs      |   2 +-
> >  rust/kernel/sync/lock.rs | 162 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 163 insertions(+), 1 deletion(-)
> >  create mode 100644 rust/kernel/sync/lock.rs
> >
> > diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> > index 541d235ffbeb..81b0998eaa18 100644
> > --- a/rust/kernel/sync.rs
> > +++ b/rust/kernel/sync.rs
> > @@ -8,6 +8,7 @@
> >  use crate::types::Opaque;
> >
> >  mod arc;
> > +pub mod lock;
> >
> >  pub use arc::{Arc, ArcBorrow, UniqueArc};
> >
> > @@ -25,7 +26,6 @@ impl LockClassKey {
> >          Self(Opaque::uninit())
> >      }
> >
> > -    #[allow(dead_code)]
> >      pub(crate) fn as_ptr(&self) -> *mut bindings::lock_class_key {
> >          self.0.get()
> >      }
> > diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> > new file mode 100644
> > index 000000000000..1a8ecccf4f24
> > --- /dev/null
> > +++ b/rust/kernel/sync/lock.rs
> > @@ -0,0 +1,162 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Generic kernel lock and guard.
> > +//!
> > +//! It contains a generic Rust lock and guard that allow for different backends (e.g., mutexes,
> > +//! spinlocks, raw spinlocks) to be provided with minimal effort.
> > +
> > +use super::LockClassKey;
> > +use crate::{bindings, init::PinInit, pin_init, str::CStr, types::Opaque};
> > +use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned};
> > +use macros::pin_data;
> > +
> > +/// The "backend" of a lock.
> > +///
> > +/// It is the actual implementation of the lock, without the need to repeat patterns used in all
> > +/// locks.
> > +///
> > +/// # Safety
> > +///
> > +/// - 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`.
> > +pub unsafe trait Backend {
> > +    /// The state required by the lock.
> > +    type State;
> > +
> > +    /// The state required to be kept between lock and unlock.
> > +    type GuardState;
> > +
> > +    /// Initialises the lock.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// `ptr` must be valid for write for the duration of the call, while `name` and `key` must
> > +    /// remain valid for read indefinitely.
> > +    unsafe fn init(
> > +        ptr: *mut Self::State,
> > +        name: *const core::ffi::c_char,
> > +        key: *mut bindings::lock_class_key,
> > +    );
>
> Any reason that this takes FFI types rather than just `&'static CStr` and `&'static LockClassKey`?

Yes, because we want to move work that is done by all backend
implementations into `Lock`. This includes calls to convert these to
ffi types.

> > +
> > +    /// Acquires the lock, making the caller its owner.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that [`Backend::init`] has been previously called.
> > +    #[must_use]
> > +    unsafe fn lock(ptr: *mut Self::State) -> Self::GuardState;
> > +
> > +    /// Releases the lock, giving up its ownership.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// It must only be called by the current owner of the lock.
> > +    unsafe fn unlock(ptr: *mut Self::State, guard_state: &Self::GuardState);
> > +}
> > +
> > +/// A mutual exclusion primitive.
> > +///
> > +/// Exposes one of the kernel locking primitives. Which one is exposed depends on the lock banckend
> > +/// specified as the generic parameter `B`.
> > +#[pin_data]
> > +pub struct Lock<T: ?Sized, B: Backend> {
> > +    /// The kernel lock object.
> > +    #[pin]
> > +    state: Opaque<B::State>,
> > +
> > +    /// Some locks are known to be self-referential (e.g., mutexes), while others are architecture
> > +    /// or config defined (e.g., spinlocks). So we conservatively require them to be pinned in case
> > +    /// some architecture uses self-references now or in the future.
> > +    #[pin]
> > +    _pin: PhantomPinned,
> > +
> > +    /// The data protected by the lock.
> > +    data: UnsafeCell<T>,
> > +}
> > +
> > +// SAFETY: `Lock` can be transferred across thread boundaries iff the data it protects can.
> > +unsafe impl<T: ?Sized + Send, B: Backend> Send for Lock<T, B> {}
> > +
> > +// SAFETY: `Lock` serialises the interior mutability it provides, so it is `Sync` as long as the
> > +// data it protects is `Send`.
> > +unsafe impl<T: ?Sized + Send, B: Backend> Sync for Lock<T, B> {}
> > +
> > +impl<T, B: Backend> Lock<T, B> {
> > +    /// Constructs a new lock initialiser.
> > +    #[allow(clippy::new_ret_no_self)]
> > +    pub fn new(t: T, name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> {
> > +        pin_init!(Self {
> > +            data: UnsafeCell::new(t),
> > +            _pin: PhantomPinned,
> > +            // SAFETY: `slot` is valid while the closure is called and both `name` and `key` have
> > +            // static lifetimes so they live indefinitely.
> > +            state <- Opaque::ffi_init(|slot| unsafe {
> > +                B::init(slot, name.as_char_ptr(), key.as_ptr())
> > +            }),
> > +        })
> > +    }
> > +}
>
> There is not drop implementation on `Lock`, which implies all locks can
> be just forgotten?

Yes, all locks can be forgotten.

> I believe we discussed a case where this is can lead to UAF when a lock
> is dropped while it is locked (e.g. because the guard is forgotten).

Yes, this is the issue brought up by Boqun:
https://github.com/Rust-for-Linux/linux/issues/862

The issue arises when a mutex guard is forgotten and the task that
owns it exits. Then another task trying to acquire the mutex will lead
to a UAF. A drop implementation on the lock doesn't solve this.

One solution is to increment the refcount on the current task when we
acquire the mutex and decrement it when we release, but if we do that,
the cost of acquiring/releasing a mutex gets much worse in Rust than
it is in C.

Another solution might be to force disable CONFIG_MUTEX_SPIN_ON_OWNER
when Rust is enabled, which is undesirable because it affects the
performance of C code as well.

Even a closure-based lock (which I believe you suggested at the time)
doesn't solve this completely because the thread may exit during the
closure execution and leave a dangling pointer in the mutex.

So we don't have a good solution for this yet.

> > +
> > +impl<T: ?Sized, B: Backend> Lock<T, B> {
> > +    /// Acquires the lock and gives the caller access to the data protected by it.
> > +    pub fn lock(&self) -> Guard<'_, T, B> {
> > +        // SAFETY: The constructor of the type calls `init`, so the existence of the object proves
> > +        // that `init` was called.
> > +        let state = unsafe { B::lock(self.state.get()) };
> > +        // SAFETY: The lock was just acquired.
> > +        unsafe { Guard::new(self, state) }
> > +    }
> > +}
> > +
> > +/// A lock guard.
> > +///
> > +/// Allows mutual exclusion primitives that implement the `Backend` trait to automatically unlock
> > +/// when a guard goes out of scope. It also provides a safe and convenient way to access the data
> > +/// protected by the lock.
> > +#[must_use = "the lock unlocks immediately when the guard is unused"]
> > +pub struct Guard<'a, T: ?Sized, B: Backend> {
> > +    pub(crate) lock: &'a Lock<T, B>,
> > +    pub(crate) state: B::GuardState,
> > +    _not_send: PhantomData<*mut ()>,
> > +}
> > +
> > +// 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> core::ops::Deref for Guard<'_, T, B> {
> > +    type Target = T;
> > +
> > +    fn deref(&self) -> &Self::Target {
> > +        // SAFETY: The caller owns the lock, so it is safe to deref the protected data.
> > +        unsafe { &*self.lock.data.get() }
> > +    }
> > +}
> > +
> > +impl<T: ?Sized, B: Backend> core::ops::DerefMut for Guard<'_, T, B> {
> > +    fn deref_mut(&mut self) -> &mut Self::Target {
> > +        // SAFETY: The caller owns the lock, so it is safe to deref the protected data.
> > +        unsafe { &mut *self.lock.data.get() }
> > +    }
> > +}
> > +
> > +impl<T: ?Sized, B: Backend> Drop for Guard<'_, T, B> {
> > +    fn drop(&mut self) {
> > +        // SAFETY: The caller owns the lock, so it is safe to unlock it.
> > +        unsafe { B::unlock(self.lock.state.get(), &self.state) };
> > +    }
> > +}
> > +
> > +impl<'a, T: ?Sized, B: Backend> Guard<'a, T, B> {
> > +    /// Constructs a new immutable lock guard.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The caller must ensure that it owns the lock.
> > +    pub(crate) unsafe fn new(lock: &'a Lock<T, B>, state: B::GuardState) -> Self {
> > +        Self {
> > +            lock,
> > +            state,
> > +            _not_send: PhantomData,
> > +        }
> > +    }
> > +}
>