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

  From: Wedson Almeida Filho <walmeida@microsoft.com>

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@gmail.com>
Suggested-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
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
  

On Tue, 11 Apr 2023 02:45:32 -0300
Wedson Almeida Filho <wedsonaf@gmail.com> wrote:

> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> 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@gmail.com>
> Suggested-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
> 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`?

> +
> +    /// 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?

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).

> +
> +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,
> +        }
> +    }
> +}
  

On Tue, 11 Apr 2023 at 17:42, Gary Guo <gary@garyguo.net> wrote:
>
> On Tue, 11 Apr 2023 02:45:32 -0300
> Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>
> > From: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> > 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@gmail.com>
> > Suggested-by: Gary Guo <gary@garyguo.net>
> > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> > ---
> > 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,
> > +        }
> > +    }
> > +}
>
  

On 11.04.23 07:45, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
>
> 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@gmail.com>
> Suggested-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
> 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`.

I am a bit confused which data is protected. The `Backend` should not be
aware of any data it protects. To me the safety requirement is:
- Up to one thread may be in the critical section at any one time.
- When `lock` returns, the current thread has entered the critical
   section.
- A thread leaves the critical section by calling `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.

- Only call this function once on uninitialized memory.

Do we require some sort of drop guarantee like pin? This function does not
state anything about `State` having to be pinned, but `Lock` is `!Unpin`
(and also pins the state). If other people decide to use `Backend`
somewhere, these things should be clear.

> +    unsafe fn init(
> +        ptr: *mut Self::State,
> +        name: *const core::ffi::c_char,
> +        key: *mut bindings::lock_class_key,
> +    );
> +
> +    /// Acquires the lock, making the caller its owner.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that [`Backend::init`] has been previously called.

Is it safe to call this function twice? Does `unlock` have to be
called, or is one allowed to drop the value before?

> +    #[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

Typo: 'banckend' -> '[`Backend`]' (I would prefer a link here)

> +/// 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.

Are there backends where `Backend::State: !Send`? If so we should add
this as a bound, if not, there should be a comment here, since it is
unconditionally implemented. Same for `Sync`

> +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())
> +            }),
> +        })
> +    }
> +}
> +
> +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

I would make 'the type' concrete here: 'The constructor of `Self` ...'. And
then '... so the existence of `self` ...'.

> +        // 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

Make `Backend` a link.

> +/// 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,
> +        }
> +    }
> +}
> --
> 2.34.1
>
  

On 4/12/23 13:38, Wedson Almeida Filho wrote:
> On Tue, 11 Apr 2023 at 17:42, Gary Guo <gary@garyguo.net> wrote:
>>
>> On Tue, 11 Apr 2023 02:45:32 -0300
>> Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>>
>>> From: Wedson Almeida Filho <walmeida@microsoft.com>
>>>
>>> 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@gmail.com>
>>> Suggested-by: Gary Guo <gary@garyguo.net>
>>> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
>>> --- >>
>> There is not drop implementation on `Lock`, which implies all locks can
>> be just forgotten?
> 
> Yes, all locks can be forgotten.

Are the locks not pinned? Pinning comes with the guarantee that the 
value remains valid until the destructor runs, so the only kind of 
forgetting you can do to a lock is the one where the memory containing 
it is leaked, meaning that its not a UAF to use it after forgetting it.

My understanding is that the issue had to do with forgetting the guard 
types, since those were not pinned.

>> 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.