[PATCH v4 01/13] rust: sync: introduce `LockClassKey`

[PATCH v4 01/13] rust: sync: introduce `LockClassKey`

[Wedson Almeida Filho]

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

It is a wrapper around C's `lock_class_key`, which is used by the
synchronisation primitives that are checked with lockdep. This is in
preparation for introducing Rust abstractions for these primitives.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Will Deacon <will@kernel.org>
Cc: Waiman Long <longman@redhat.com>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
v1 -> v2: Fixed a typo in comment
v2 -> v3: Replaced `core` with `::core` in macro
v3 -> v4:
- Rebased on top of rust-next now that all dependencies are applied

 rust/kernel/sync.rs | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 33da23e3076d..541d235ffbeb 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -5,6 +5,51 @@
 //! This module contains the kernel APIs related to synchronisation that have been ported or
 //! wrapped for usage by Rust code in the kernel.
 
+use crate::types::Opaque;
+
 mod arc;
 
 pub use arc::{Arc, ArcBorrow, UniqueArc};
+
+/// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
+#[repr(transparent)]
+pub struct LockClassKey(Opaque<bindings::lock_class_key>);
+
+// SAFETY: `bindings::lock_class_key` is designed to be used concurrently from multiple threads and
+// provides its own synchronization.
+unsafe impl Sync for LockClassKey {}
+
+impl LockClassKey {
+    /// Creates a new lock class key.
+    pub const fn new() -> Self {
+        Self(Opaque::uninit())
+    }
+
+    #[allow(dead_code)]
+    pub(crate) fn as_ptr(&self) -> *mut bindings::lock_class_key {
+        self.0.get()
+    }
+}
+
+/// Defines a new static lock class and returns a pointer to it.
+#[doc(hidden)]
+#[macro_export]
+macro_rules! static_lock_class {
+    () => {{
+        static CLASS: $crate::sync::LockClassKey = $crate::sync::LockClassKey::new();
+        &CLASS
+    }};
+}
+
+/// Returns the given string, if one is provided, otherwise generates one based on the source code
+/// location.
+#[doc(hidden)]
+#[macro_export]
+macro_rules! optional_name {
+    () => {
+        $crate::c_str!(::core::concat!(::core::file!(), ":", ::core::line!()))
+    };
+    ($name:literal) => {
+        $crate::c_str!($name)
+    };
+}

base-commit: 2d0dec625d872a41632a68fce2e69453ed87df91
-- 
2.34.1

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

[Wedson Almeida Filho]

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

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

[Gary Guo]

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

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

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

[Alice Ryhl]

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.

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

[Benno Lossin]

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
>

[PATCH v4 03/13] rust: lock: introduce `Mutex`

[Wedson Almeida Filho]

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

This is the `struct mutex` lock backend and allows Rust code to use the
kernel mutex idiomatically.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Will Deacon <will@kernel.org>
Cc: Waiman Long <longman@redhat.com>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
v1 -> v2: No changes
v2 -> v3: No changes
v4 -> v4: No changes

 rust/helpers.c                 |   7 ++
 rust/kernel/sync.rs            |   1 +
 rust/kernel/sync/lock.rs       |   2 +
 rust/kernel/sync/lock/mutex.rs | 118 +++++++++++++++++++++++++++++++++
 4 files changed, 128 insertions(+)
 create mode 100644 rust/kernel/sync/lock/mutex.rs

diff --git a/rust/helpers.c b/rust/helpers.c
index 04b9be46e887..86af099d2d66 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -22,6 +22,7 @@
 #include <linux/build_bug.h>
 #include <linux/err.h>
 #include <linux/refcount.h>
+#include <linux/mutex.h>
 
 __noreturn void rust_helper_BUG(void)
 {
@@ -29,6 +30,12 @@ __noreturn void rust_helper_BUG(void)
 }
 EXPORT_SYMBOL_GPL(rust_helper_BUG);
 
+void rust_helper_mutex_lock(struct mutex *lock)
+{
+	mutex_lock(lock);
+}
+EXPORT_SYMBOL_GPL(rust_helper_mutex_lock);
+
 refcount_t rust_helper_REFCOUNT_INIT(int n)
 {
 	return (refcount_t)REFCOUNT_INIT(n);
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 81b0998eaa18..693f0b7f4e4f 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -11,6 +11,7 @@ mod arc;
 pub mod lock;
 
 pub use arc::{Arc, ArcBorrow, UniqueArc};
+pub use lock::mutex::Mutex;
 
 /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
 #[repr(transparent)]
diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index 1a8ecccf4f24..98de109d9e40 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -10,6 +10,8 @@ use crate::{bindings, init::PinInit, pin_init, str::CStr, types::Opaque};
 use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned};
 use macros::pin_data;
 
+pub mod mutex;
+
 /// The "backend" of a lock.
 ///
 /// It is the actual implementation of the lock, without the need to repeat patterns used in all
diff --git a/rust/kernel/sync/lock/mutex.rs b/rust/kernel/sync/lock/mutex.rs
new file mode 100644
index 000000000000..923472f04af4
--- /dev/null
+++ b/rust/kernel/sync/lock/mutex.rs
@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! A kernel mutex.
+//!
+//! This module allows Rust code to use the kernel's `struct mutex`.
+
+use crate::bindings;
+
+/// Creates a [`Mutex`] initialiser with the given name and a newly-created lock class.
+///
+/// It uses the name if one is given, otherwise it generates one based on the file name and line
+/// number.
+#[macro_export]
+macro_rules! new_mutex {
+    ($inner:expr $(, $name:literal)? $(,)?) => {
+        $crate::sync::Mutex::new(
+            $inner, $crate::optional_name!($($name)?), $crate::static_lock_class!())
+    };
+}
+
+/// A mutual exclusion primitive.
+///
+/// Exposes the kernel's [`struct mutex`]. When multiple threads attempt to lock the same mutex,
+/// only one at a time is allowed to progress, the others will block (sleep) until the mutex is
+/// unlocked, at which point another thread will be allowed to wake up and make progress.
+///
+/// Since it may block, [`Mutex`] needs to be used with care in atomic contexts.
+///
+/// Instances of [`Mutex`] need a lock class and to be pinned. The recommended way to create such
+/// instances is with the [`pin_init`](crate::pin_init) and [`new_mutex`] macros.
+///
+/// # Examples
+///
+/// The following example shows how to declare, allocate and initialise a struct (`Example`) that
+/// contains an inner struct (`Inner`) that is protected by a mutex.
+///
+/// ```
+/// use kernel::{init::InPlaceInit, init::PinInit, new_mutex, pin_init, sync::Mutex};
+///
+/// struct Inner {
+///     a: u32,
+///     b: u32,
+/// }
+///
+/// #[pin_data]
+/// struct Example {
+///     c: u32,
+///     #[pin]
+///     d: Mutex<Inner>,
+/// }
+///
+/// impl Example {
+///     fn new() -> impl PinInit<Self> {
+///         pin_init!(Self {
+///             c: 10,
+///             d <- new_mutex!(Inner { a: 20, b: 30 }),
+///         })
+///     }
+/// }
+///
+/// // Allocate a boxed `Example`.
+/// let e = Box::pin_init(Example::new())?;
+/// assert_eq!(e.c, 10);
+/// assert_eq!(e.d.lock().a, 20);
+/// assert_eq!(e.d.lock().b, 30);
+/// ```
+///
+/// The following example shows how to use interior mutability to modify the contents of a struct
+/// protected by a mutex despite only having a shared reference:
+///
+/// ```
+/// use kernel::sync::Mutex;
+///
+/// struct Example {
+///     a: u32,
+///     b: u32,
+/// }
+///
+/// fn example(m: &Mutex<Example>) {
+///     let mut guard = m.lock();
+///     guard.a += 10;
+///     guard.b += 20;
+/// }
+/// ```
+///
+/// [`struct mutex`]: ../../../../include/linux/mutex.h
+pub type Mutex<T> = super::Lock<T, MutexBackend>;
+
+/// A kernel `struct mutex` lock backend.
+pub struct MutexBackend;
+
+// SAFETY: The underlying kernel `struct mutex` object ensures mutual exclusion.
+unsafe impl super::Backend for MutexBackend {
+    type State = bindings::mutex;
+    type GuardState = ();
+
+    unsafe fn init(
+        ptr: *mut Self::State,
+        name: *const core::ffi::c_char,
+        key: *mut bindings::lock_class_key,
+    ) {
+        // SAFETY: The safety requirements ensure that `ptr` is valid for writes, and `name` and
+        // `key` are valid for read indefinitely.
+        unsafe { bindings::__mutex_init(ptr, name, key) }
+    }
+
+    unsafe fn lock(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.
+        unsafe { bindings::mutex_lock(ptr) };
+    }
+
+    unsafe fn unlock(ptr: *mut Self::State, _guard_state: &Self::GuardState) {
+        // SAFETY: The safety requirements of this function ensure that `ptr` is valid and that the
+        // caller is the owner of the mutex.
+        unsafe { bindings::mutex_unlock(ptr) };
+    }
+}
-- 
2.34.1

Re: [PATCH v4 03/13] rust: lock: introduce `Mutex`

[Benno Lossin]

On 11.04.23 07:45, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
>
> This is the `struct mutex` lock backend and allows Rust code to use the
> kernel mutex idiomatically.
>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Waiman Long <longman@redhat.com>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
> v1 -> v2: No changes
> v2 -> v3: No changes
> v4 -> v4: No changes
>
>   rust/helpers.c                 |   7 ++
>   rust/kernel/sync.rs            |   1 +
>   rust/kernel/sync/lock.rs       |   2 +
>   rust/kernel/sync/lock/mutex.rs | 118 +++++++++++++++++++++++++++++++++
>   4 files changed, 128 insertions(+)
>   create mode 100644 rust/kernel/sync/lock/mutex.rs
>
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 04b9be46e887..86af099d2d66 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -22,6 +22,7 @@
>   #include <linux/build_bug.h>
>   #include <linux/err.h>
>   #include <linux/refcount.h>
> +#include <linux/mutex.h>
>
>   __noreturn void rust_helper_BUG(void)
>   {
> @@ -29,6 +30,12 @@ __noreturn void rust_helper_BUG(void)
>   }
>   EXPORT_SYMBOL_GPL(rust_helper_BUG);
>
> +void rust_helper_mutex_lock(struct mutex *lock)
> +{
> +	mutex_lock(lock);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_mutex_lock);
> +
>   refcount_t rust_helper_REFCOUNT_INIT(int n)
>   {
>   	return (refcount_t)REFCOUNT_INIT(n);
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index 81b0998eaa18..693f0b7f4e4f 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -11,6 +11,7 @@ mod arc;
>   pub mod lock;
>
>   pub use arc::{Arc, ArcBorrow, UniqueArc};
> +pub use lock::mutex::Mutex;
>
>   /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
>   #[repr(transparent)]
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index 1a8ecccf4f24..98de109d9e40 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -10,6 +10,8 @@ use crate::{bindings, init::PinInit, pin_init, str::CStr, types::Opaque};
>   use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned};
>   use macros::pin_data;
>
> +pub mod mutex;
> +
>   /// The "backend" of a lock.
>   ///
>   /// It is the actual implementation of the lock, without the need to repeat patterns used in all
> diff --git a/rust/kernel/sync/lock/mutex.rs b/rust/kernel/sync/lock/mutex.rs
> new file mode 100644
> index 000000000000..923472f04af4
> --- /dev/null
> +++ b/rust/kernel/sync/lock/mutex.rs
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! A kernel mutex.
> +//!
> +//! This module allows Rust code to use the kernel's `struct mutex`.
> +
> +use crate::bindings;
> +
> +/// Creates a [`Mutex`] initialiser with the given name and a newly-created lock class.
> +///
> +/// It uses the name if one is given, otherwise it generates one based on the file name and line
> +/// number.
> +#[macro_export]
> +macro_rules! new_mutex {
> +    ($inner:expr $(, $name:literal)? $(,)?) => {
> +        $crate::sync::Mutex::new(
> +            $inner, $crate::optional_name!($($name)?), $crate::static_lock_class!())
> +    };
> +}
> +
> +/// A mutual exclusion primitive.
> +///
> +/// Exposes the kernel's [`struct mutex`]. When multiple threads attempt to lock the same mutex,
> +/// only one at a time is allowed to progress, the others will block (sleep) until the mutex is
> +/// unlocked, at which point another thread will be allowed to wake up and make progress.
> +///
> +/// Since it may block, [`Mutex`] needs to be used with care in atomic contexts.
> +///
> +/// Instances of [`Mutex`] need a lock class and to be pinned. The recommended way to create such

The first sentence reads weird, missing another 'need'?

> +/// instances is with the [`pin_init`](crate::pin_init) and [`new_mutex`] macros.
> +///
> +/// # Examples
> +///
> +/// The following example shows how to declare, allocate and initialise a struct (`Example`) that
> +/// contains an inner struct (`Inner`) that is protected by a mutex.
> +///
> +/// ```
> +/// use kernel::{init::InPlaceInit, init::PinInit, new_mutex, pin_init, sync::Mutex};
> +///
> +/// struct Inner {
> +///     a: u32,
> +///     b: u32,
> +/// }
> +///
> +/// #[pin_data]
> +/// struct Example {
> +///     c: u32,
> +///     #[pin]
> +///     d: Mutex<Inner>,
> +/// }
> +///
> +/// impl Example {
> +///     fn new() -> impl PinInit<Self> {
> +///         pin_init!(Self {
> +///             c: 10,
> +///             d <- new_mutex!(Inner { a: 20, b: 30 }),
> +///         })
> +///     }
> +/// }
> +///
> +/// // Allocate a boxed `Example`.
> +/// let e = Box::pin_init(Example::new())?;
> +/// assert_eq!(e.c, 10);
> +/// assert_eq!(e.d.lock().a, 20);
> +/// assert_eq!(e.d.lock().b, 30);
> +/// ```
> +///
> +/// The following example shows how to use interior mutability to modify the contents of a struct
> +/// protected by a mutex despite only having a shared reference:

I would not bring up interior mutability here, the `Mutex` uses it, but
not the user of the `Mutex`. I would just say:

     To access the data behind a `Mutex`, simply take a shared reference to
     the `Mutex` and call `lock()` on it. This function returns a guard object
     that dereferences to the protected data. The `Mutex` is unlocked when the
     guard object goes out of scope.

> +///
> +/// ```
> +/// use kernel::sync::Mutex;
> +///
> +/// struct Example {
> +///     a: u32,
> +///     b: u32,
> +/// }
> +///
> +/// fn example(m: &Mutex<Example>) {
> +///     let mut guard = m.lock();
> +///     guard.a += 10;
> +///     guard.b += 20;
> +/// }
> +/// ```
> +///
> +/// [`struct mutex`]: ../../../../include/linux/mutex.h
> +pub type Mutex<T> = super::Lock<T, MutexBackend>;
> +
> +/// A kernel `struct mutex` lock backend.
> +pub struct MutexBackend;
> +
> +// SAFETY: The underlying kernel `struct mutex` object ensures mutual exclusion.
> +unsafe impl super::Backend for MutexBackend {
> +    type State = bindings::mutex;
> +    type GuardState = ();
> +
> +    unsafe fn init(
> +        ptr: *mut Self::State,
> +        name: *const core::ffi::c_char,
> +        key: *mut bindings::lock_class_key,
> +    ) {
> +        // SAFETY: The safety requirements ensure that `ptr` is valid for writes, and `name` and
> +        // `key` are valid for read indefinitely.
> +        unsafe { bindings::__mutex_init(ptr, name, key) }
> +    }
> +
> +    unsafe fn lock(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.
> +        unsafe { bindings::mutex_lock(ptr) };
> +    }
> +
> +    unsafe fn unlock(ptr: *mut Self::State, _guard_state: &Self::GuardState) {
> +        // SAFETY: The safety requirements of this function ensure that `ptr` is valid and that the
> +        // caller is the owner of the mutex.
> +        unsafe { bindings::mutex_unlock(ptr) };
> +    }
> +}
> --
> 2.34.1
>

[PATCH v4 04/13] locking/spinlock: introduce spin_lock_init_with_key

[Wedson Almeida Filho]

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

Rust cannot call C macros, so it has its own macro to create a new lock
class when a spin lock is initialised. This new function allows Rust
code to pass the lock class it generates to the C implementation.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Will Deacon <will@kernel.org>
Cc: Waiman Long <longman@redhat.com>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
v1 -> v2: No changes
v2 -> v3: No changes
v3 -> v4: No changes

 include/linux/spinlock.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index be48f1cb1878..cdc92d095133 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -327,12 +327,17 @@ static __always_inline raw_spinlock_t *spinlock_check(spinlock_t *lock)
 
 #ifdef CONFIG_DEBUG_SPINLOCK
 
+static inline void spin_lock_init_with_key(spinlock_t *lock, const char *name,
+					   struct lock_class_key *key)
+{
+	__raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
+}
+
 # define spin_lock_init(lock)					\
 do {								\
 	static struct lock_class_key __key;			\
 								\
-	__raw_spin_lock_init(spinlock_check(lock),		\
-			     #lock, &__key, LD_WAIT_CONFIG);	\
+	spin_lock_init_with_key(lock, #lock, &__key);		\
 } while (0)
 
 #else
-- 
2.34.1

Re: [PATCH v4 04/13] locking/spinlock: introduce spin_lock_init_with_key

[Wedson Almeida Filho]

On Tue, 11 Apr 2023 at 02:46, Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>
> From: Wedson Almeida Filho <walmeida@microsoft.com>
>
> Rust cannot call C macros, so it has its own macro to create a new lock
> class when a spin lock is initialised. This new function allows Rust
> code to pass the lock class it generates to the C implementation.
>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Waiman Long <longman@redhat.com>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
> v1 -> v2: No changes
> v2 -> v3: No changes
> v3 -> v4: No changes
>
>  include/linux/spinlock.h | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> index be48f1cb1878..cdc92d095133 100644
> --- a/include/linux/spinlock.h
> +++ b/include/linux/spinlock.h
> @@ -327,12 +327,17 @@ static __always_inline raw_spinlock_t *spinlock_check(spinlock_t *lock)
>
>  #ifdef CONFIG_DEBUG_SPINLOCK
>
> +static inline void spin_lock_init_with_key(spinlock_t *lock, const char *name,
> +                                          struct lock_class_key *key)
> +{
> +       __raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
> +}
> +
>  # define spin_lock_init(lock)                                  \
>  do {                                                           \
>         static struct lock_class_key __key;                     \
>                                                                 \
> -       __raw_spin_lock_init(spinlock_check(lock),              \
> -                            #lock, &__key, LD_WAIT_CONFIG);    \
> +       spin_lock_init_with_key(lock, #lock, &__key);           \
>  } while (0)

Peter, the code above is just factoring out spin lock init when
lockdep is enabled to take a lock class key.

Would you be able to review it?

If it's ok with you, we'd like to carry it through the rust tree
because we have code that depends on it.

Thanks,
-Wedson

>
>  #else
> --
> 2.34.1
>

Re: [PATCH v4 04/13] locking/spinlock: introduce spin_lock_init_with_key

[Boqun Feng]

On Tue, Apr 11, 2023 at 03:05:07PM -0300, Wedson Almeida Filho wrote:
> On Tue, 11 Apr 2023 at 02:46, Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
> >
> > From: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> > Rust cannot call C macros, so it has its own macro to create a new lock
> > class when a spin lock is initialised. This new function allows Rust
> > code to pass the lock class it generates to the C implementation.
> >
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Waiman Long <longman@redhat.com>
> > Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

> > ---
> > v1 -> v2: No changes
> > v2 -> v3: No changes
> > v3 -> v4: No changes
> >
> >  include/linux/spinlock.h | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> > index be48f1cb1878..cdc92d095133 100644
> > --- a/include/linux/spinlock.h
> > +++ b/include/linux/spinlock.h
> > @@ -327,12 +327,17 @@ static __always_inline raw_spinlock_t *spinlock_check(spinlock_t *lock)
> >
> >  #ifdef CONFIG_DEBUG_SPINLOCK
> >
> > +static inline void spin_lock_init_with_key(spinlock_t *lock, const char *name,
> > +                                          struct lock_class_key *key)
> > +{
> > +       __raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
> > +}
> > +
> >  # define spin_lock_init(lock)                                  \
> >  do {                                                           \
> >         static struct lock_class_key __key;                     \
> >                                                                 \
> > -       __raw_spin_lock_init(spinlock_check(lock),              \
> > -                            #lock, &__key, LD_WAIT_CONFIG);    \
> > +       spin_lock_init_with_key(lock, #lock, &__key);           \
> >  } while (0)
> 
> Peter, the code above is just factoring out spin lock init when
> lockdep is enabled to take a lock class key.
> 
> Would you be able to review it?
> 
> If it's ok with you, we'd like to carry it through the rust tree
> because we have code that depends on it.

Same ask here ;-) Peter, do you think it's Ok to take it via rust tree?
Thanks!

Regards,
Boqun

> 
> Thanks,
> -Wedson
> 
> >
> >  #else
> > --
> > 2.34.1
> >

[PATCH v4 05/13] rust: lock: introduce `SpinLock`

[Wedson Almeida Filho]

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

This is the `spinlock_t` lock backend and allows Rust code to use the
kernel spinlock idiomatically.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Will Deacon <will@kernel.org>
Cc: Waiman Long <longman@redhat.com>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
v1 -> v2: No changes
v2 -> v3: No changes
v3 -> v4: No changes

 rust/helpers.c                    |  24 +++++++
 rust/kernel/sync.rs               |   2 +-
 rust/kernel/sync/lock.rs          |   1 +
 rust/kernel/sync/lock/spinlock.rs | 116 ++++++++++++++++++++++++++++++
 4 files changed, 142 insertions(+), 1 deletion(-)
 create mode 100644 rust/kernel/sync/lock/spinlock.rs

diff --git a/rust/helpers.c b/rust/helpers.c
index 86af099d2d66..141308a78775 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -23,6 +23,7 @@
 #include <linux/err.h>
 #include <linux/refcount.h>
 #include <linux/mutex.h>
+#include <linux/spinlock.h>
 
 __noreturn void rust_helper_BUG(void)
 {
@@ -36,6 +37,29 @@ void rust_helper_mutex_lock(struct mutex *lock)
 }
 EXPORT_SYMBOL_GPL(rust_helper_mutex_lock);
 
+void rust_helper___spin_lock_init(spinlock_t *lock, const char *name,
+				  struct lock_class_key *key)
+{
+#ifdef CONFIG_DEBUG_SPINLOCK
+	spin_lock_init_with_key(lock, name, key);
+#else
+	spin_lock_init(lock);
+#endif
+}
+EXPORT_SYMBOL_GPL(rust_helper___spin_lock_init);
+
+void rust_helper_spin_lock(spinlock_t *lock)
+{
+	spin_lock(lock);
+}
+EXPORT_SYMBOL_GPL(rust_helper_spin_lock);
+
+void rust_helper_spin_unlock(spinlock_t *lock)
+{
+	spin_unlock(lock);
+}
+EXPORT_SYMBOL_GPL(rust_helper_spin_unlock);
+
 refcount_t rust_helper_REFCOUNT_INIT(int n)
 {
 	return (refcount_t)REFCOUNT_INIT(n);
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 693f0b7f4e4f..c997ff7e951e 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -11,7 +11,7 @@ mod arc;
 pub mod lock;
 
 pub use arc::{Arc, ArcBorrow, UniqueArc};
-pub use lock::mutex::Mutex;
+pub use lock::{mutex::Mutex, spinlock::SpinLock};
 
 /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
 #[repr(transparent)]
diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index 98de109d9e40..08adc3747033 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -11,6 +11,7 @@ use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned};
 use macros::pin_data;
 
 pub mod mutex;
+pub mod spinlock;
 
 /// The "backend" of a lock.
 ///
diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs
new file mode 100644
index 000000000000..a52d20fc9755
--- /dev/null
+++ b/rust/kernel/sync/lock/spinlock.rs
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! A kernel spinlock.
+//!
+//! This module allows Rust code to use the kernel's `spinlock_t`.
+
+use crate::bindings;
+
+/// Creates a [`SpinLock`] initialiser with the given name and a newly-created lock class.
+///
+/// It uses the name if one is given, otherwise it generates one based on the file name and line
+/// number.
+#[macro_export]
+macro_rules! new_spinlock {
+    ($inner:expr $(, $name:literal)? $(,)?) => {
+        $crate::sync::SpinLock::new(
+            $inner, $crate::optional_name!($($name)?), $crate::static_lock_class!())
+    };
+}
+
+/// A spinlock.
+///
+/// Exposes the kernel's [`spinlock_t`]. When multiple CPUs attempt to lock the same spinlock, only
+/// one at a time is allowed to progress, the others will block (spinning) until the spinlock is
+/// unlocked, at which point another CPU will be allowed to make progress.
+///
+/// Instances of [`SpinLock`] need a lock class and to be pinned. The recommended way to create such
+/// instances is with the [`pin_init`](crate::pin_init) and [`new_spinlock`] macros.
+///
+/// # Examples
+///
+/// The following example shows how to declare, allocate and initialise a struct (`Example`) that
+/// contains an inner struct (`Inner`) that is protected by a spinlock.
+///
+/// ```
+/// use kernel::{init::InPlaceInit, init::PinInit, new_spinlock, pin_init, sync::SpinLock};
+///
+/// struct Inner {
+///     a: u32,
+///     b: u32,
+/// }
+///
+/// #[pin_data]
+/// struct Example {
+///     c: u32,
+///     #[pin]
+///     d: SpinLock<Inner>,
+/// }
+///
+/// impl Example {
+///     fn new() -> impl PinInit<Self> {
+///         pin_init!(Self {
+///             c: 10,
+///             d <- new_spinlock!(Inner { a: 20, b: 30 }),
+///         })
+///     }
+/// }
+///
+/// // Allocate a boxed `Example`.
+/// let e = Box::pin_init(Example::new())?;
+/// assert_eq!(e.c, 10);
+/// assert_eq!(e.d.lock().a, 20);
+/// assert_eq!(e.d.lock().b, 30);
+/// ```
+///
+/// The following example shows how to use interior mutability to modify the contents of a struct
+/// protected by a spinlock despite only having a shared reference:
+///
+/// ```
+/// use kernel::sync::SpinLock;
+///
+/// struct Example {
+///     a: u32,
+///     b: u32,
+/// }
+///
+/// fn example(m: &SpinLock<Example>) {
+///     let mut guard = m.lock();
+///     guard.a += 10;
+///     guard.b += 20;
+/// }
+/// ```
+///
+/// [`spinlock_t`]: ../../../../include/linux/spinlock.h
+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.
+unsafe impl super::Backend for SpinLockBackend {
+    type State = bindings::spinlock_t;
+    type GuardState = ();
+
+    unsafe fn init(
+        ptr: *mut Self::State,
+        name: *const core::ffi::c_char,
+        key: *mut bindings::lock_class_key,
+    ) {
+        // SAFETY: The safety requirements ensure that `ptr` is valid for writes, and `name` and
+        // `key` are valid for read indefinitely.
+        unsafe { bindings::__spin_lock_init(ptr, name, key) }
+    }
+
+    unsafe fn lock(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.
+        unsafe { bindings::spin_lock(ptr) }
+    }
+
+    unsafe fn unlock(ptr: *mut Self::State, _guard_state: &Self::GuardState) {
+        // SAFETY: The safety requirements of this function ensure that `ptr` is valid and that the
+        // caller is the owner of the mutex.
+        unsafe { bindings::spin_unlock(ptr) }
+    }
+}
-- 
2.34.1

[PATCH v4 06/13] rust: lock: add support for `Lock::lock_irqsave`

[Wedson Almeida Filho]

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

This allows locks like spinlocks and raw spinlocks to expose a
`lock_irqsave` variant in Rust that corresponds to the C version.

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
v1 -> v2: No changes
v2 -> v3: No changes
v3 -> v4: No changes

 rust/kernel/sync/lock.rs | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index 08adc3747033..819b8ea5ba2b 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -57,6 +57,29 @@ pub unsafe trait Backend {
     unsafe fn unlock(ptr: *mut Self::State, guard_state: &Self::GuardState);
 }
 
+/// The "backend" of a lock that supports the irq-save variant.
+///
+/// # Safety
+///
+/// The same requirements wrt mutual exclusion in [`Backend`] apply for acquiring the lock via
+/// [`IrqSaveBackend::lock_irqsave`].
+///
+/// Additionally, when [`IrqSaveBackend::lock_irqsave`] is used to acquire the lock, implementers
+/// must disable interrupts on lock, and restore interrupt state on unlock. Implementers may use
+/// [`Backend::GuardState`] to store state needed to keep track of the interrupt state.
+pub unsafe trait IrqSaveBackend: Backend {
+    /// Acquires the lock, making the caller its owner.
+    ///
+    /// Before acquiring the lock, it disables interrupts, and returns the previous interrupt state
+    /// as its guard state so that the guard can restore it when it is dropped.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that [`Backend::init`] has been previously called.
+    #[must_use]
+    unsafe fn lock_irqsave(ptr: *mut Self::State) -> Self::GuardState;
+}
+
 /// A mutual exclusion primitive.
 ///
 /// Exposes one of the kernel locking primitives. Which one is exposed depends on the lock banckend
@@ -111,6 +134,21 @@ impl<T: ?Sized, B: Backend> Lock<T, B> {
     }
 }
 
+impl<T: ?Sized, B: IrqSaveBackend> Lock<T, B> {
+    /// Acquires the lock and gives the caller access to the data protected by it.
+    ///
+    /// Before acquiring the lock, it disables interrupts. When the guard is dropped, the interrupt
+    /// state (either enabled or disabled) is restored to its state before
+    /// [`lock_irqsave`](Self::lock_irqsave) was called.
+    pub fn lock_irqsave(&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_irqsave(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
-- 
2.34.1

[PATCH v4 07/13] rust: lock: implement `IrqSaveBackend` for `SpinLock`

[Wedson Almeida Filho]

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

This allows Rust code to use the `lock_irqsave` variant of spinlocks.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Will Deacon <will@kernel.org>
Cc: Waiman Long <longman@redhat.com>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
v1 -> v2: No changes
v2 -> v3: No changes
v3 -> v4: No changes

 rust/helpers.c                    | 16 +++++++++++++
 rust/kernel/sync/lock/spinlock.rs | 38 ++++++++++++++++++++++++++-----
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/rust/helpers.c b/rust/helpers.c
index 141308a78775..efd5b1e86f6e 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -60,6 +60,22 @@ void rust_helper_spin_unlock(spinlock_t *lock)
 }
 EXPORT_SYMBOL_GPL(rust_helper_spin_unlock);
 
+unsigned long rust_helper_spin_lock_irqsave(spinlock_t *lock)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(lock, flags);
+
+	return flags;
+}
+EXPORT_SYMBOL_GPL(rust_helper_spin_lock_irqsave);
+
+void rust_helper_spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
+{
+	spin_unlock_irqrestore(lock, flags);
+}
+EXPORT_SYMBOL_GPL(rust_helper_spin_unlock_irqrestore);
+
 refcount_t rust_helper_REFCOUNT_INIT(int n)
 {
 	return (refcount_t)REFCOUNT_INIT(n);
diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs
index a52d20fc9755..34dec09a97c0 100644
--- a/rust/kernel/sync/lock/spinlock.rs
+++ b/rust/kernel/sync/lock/spinlock.rs
@@ -61,6 +61,8 @@ macro_rules! new_spinlock {
 /// assert_eq!(e.c, 10);
 /// assert_eq!(e.d.lock().a, 20);
 /// assert_eq!(e.d.lock().b, 30);
+/// assert_eq!(e.d.lock_irqsave().a, 20);
+/// assert_eq!(e.d.lock_irqsave().b, 30);
 /// ```
 ///
 /// The following example shows how to use interior mutability to modify the contents of a struct
@@ -79,6 +81,12 @@ macro_rules! new_spinlock {
 ///     guard.a += 10;
 ///     guard.b += 20;
 /// }
+///
+/// fn example2(m: &SpinLock<Example>) {
+///     let mut guard = m.lock_irqsave();
+///     guard.a += 10;
+///     guard.b += 20;
+/// }
 /// ```
 ///
 /// [`spinlock_t`]: ../../../../include/linux/spinlock.h
@@ -90,7 +98,7 @@ pub struct SpinLockBackend;
 // SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion.
 unsafe impl super::Backend for SpinLockBackend {
     type State = bindings::spinlock_t;
-    type GuardState = ();
+    type GuardState = Option<core::ffi::c_ulong>;
 
     unsafe fn init(
         ptr: *mut Self::State,
@@ -105,12 +113,30 @@ unsafe impl super::Backend for SpinLockBackend {
     unsafe fn lock(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.
-        unsafe { bindings::spin_lock(ptr) }
+        unsafe { bindings::spin_lock(ptr) };
+        None
     }
 
-    unsafe fn unlock(ptr: *mut Self::State, _guard_state: &Self::GuardState) {
-        // SAFETY: The safety requirements of this function ensure that `ptr` is valid and that the
-        // caller is the owner of the mutex.
-        unsafe { bindings::spin_unlock(ptr) }
+    unsafe fn unlock(ptr: *mut Self::State, guard_state: &Self::GuardState) {
+        match guard_state {
+            // SAFETY: The safety requirements of this function ensure that `ptr` is valid and that
+            // the caller is the owner of the mutex.
+            Some(flags) => unsafe { bindings::spin_unlock_irqrestore(ptr, *flags) },
+            // SAFETY: The safety requirements of this function ensure that `ptr` is valid and that
+            // the caller is the owner of the mutex.
+            None => unsafe { bindings::spin_unlock(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 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.
+        Some(unsafe { bindings::spin_lock_irqsave(ptr) })
     }
 }
-- 
2.34.1

[PATCH v4 08/13] rust: introduce `ARef`

[Wedson Almeida Filho]

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

This is an owned reference to an object that is always ref-counted. This
is meant to be used in wrappers for C types that have their own ref
counting functions, for example, tasks, files, inodes, dentries, etc.

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
v1 -> v2: No changes
v2 -> v3: No changes
v3 -> v4: No changes

 rust/kernel/types.rs | 107 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index a4b1e3778da7..29db59d6119a 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -6,8 +6,10 @@ use crate::init::{self, PinInit};
 use alloc::boxed::Box;
 use core::{
     cell::UnsafeCell,
+    marker::PhantomData,
     mem::MaybeUninit,
     ops::{Deref, DerefMut},
+    ptr::NonNull,
 };
 
 /// Used to transfer ownership to and from foreign (non-Rust) languages.
@@ -268,6 +270,111 @@ impl<T> Opaque<T> {
     }
 }
 
+/// Types that are _always_ reference counted.
+///
+/// It allows such types to define their own custom ref increment and decrement functions.
+/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
+/// [`ARef<T>`].
+///
+/// This is usually implemented by wrappers to existing structures on the C side of the code. For
+/// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
+/// instances of a type.
+///
+/// # Safety
+///
+/// Implementers must ensure that increments to the reference count keep the object alive in memory
+/// at least until matching decrements are performed.
+///
+/// Implementers must also ensure that all instances are reference-counted. (Otherwise they
+/// won't be able to honour the requirement that [`AlwaysRefCounted::inc_ref`] keep the object
+/// alive.)
+pub unsafe trait AlwaysRefCounted {
+    /// Increments the reference count on the object.
+    fn inc_ref(&self);
+
+    /// Decrements the reference count on the object.
+    ///
+    /// Frees the object when the count reaches zero.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that there was a previous matching increment to the reference count,
+    /// and that the object is no longer used after its reference count is decremented (as it may
+    /// result in the object being freed), unless the caller owns another increment on the refcount
+    /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
+    /// [`AlwaysRefCounted::dec_ref`] once).
+    unsafe fn dec_ref(obj: NonNull<Self>);
+}
+
+/// An owned reference to an always-reference-counted object.
+///
+/// The object's reference count is automatically decremented when an instance of [`ARef`] is
+/// dropped. It is also automatically incremented when a new instance is created via
+/// [`ARef::clone`].
+///
+/// # Invariants
+///
+/// The pointer stored in `ptr` is non-null and valid for the lifetime of the [`ARef`] instance. In
+/// particular, the [`ARef`] instance owns an increment on the underlying object's reference count.
+pub struct ARef<T: AlwaysRefCounted> {
+    ptr: NonNull<T>,
+    _p: PhantomData<T>,
+}
+
+impl<T: AlwaysRefCounted> ARef<T> {
+    /// Creates a new instance of [`ARef`].
+    ///
+    /// It takes over an increment of the reference count on the underlying object.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that the reference count was incremented at least once, and that they
+    /// are properly relinquishing one increment. That is, if there is only one increment, callers
+    /// must not use the underlying object anymore -- it is only safe to do so via the newly
+    /// created [`ARef`].
+    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
+        // INVARIANT: The safety requirements guarantee that the new instance now owns the
+        // increment on the refcount.
+        Self {
+            ptr,
+            _p: PhantomData,
+        }
+    }
+}
+
+impl<T: AlwaysRefCounted> Clone for ARef<T> {
+    fn clone(&self) -> Self {
+        self.inc_ref();
+        // SAFETY: We just incremented the refcount above.
+        unsafe { Self::from_raw(self.ptr) }
+    }
+}
+
+impl<T: AlwaysRefCounted> Deref for ARef<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: The type invariants guarantee that the object is valid.
+        unsafe { self.ptr.as_ref() }
+    }
+}
+
+impl<T: AlwaysRefCounted> From<&T> for ARef<T> {
+    fn from(b: &T) -> Self {
+        b.inc_ref();
+        // SAFETY: We just incremented the refcount above.
+        unsafe { Self::from_raw(NonNull::from(b)) }
+    }
+}
+
+impl<T: AlwaysRefCounted> Drop for ARef<T> {
+    fn drop(&mut self) {
+        // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
+        // decrement.
+        unsafe { T::dec_ref(self.ptr) };
+    }
+}
+
 /// A sum type that always holds either a value of type `L` or `R`.
 pub enum Either<L, R> {
     /// Constructs an instance of [`Either`] containing a value of type `L`.
-- 
2.34.1

Re: [PATCH v4 08/13] rust: introduce `ARef`

[Gary Guo]

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

> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> This is an owned reference to an object that is always ref-counted. This
> is meant to be used in wrappers for C types that have their own ref
> counting functions, for example, tasks, files, inodes, dentries, etc.
> 
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
> v1 -> v2: No changes
> v2 -> v3: No changes
> v3 -> v4: No changes
> 
>  rust/kernel/types.rs | 107 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 107 insertions(+)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index a4b1e3778da7..29db59d6119a 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -6,8 +6,10 @@ use crate::init::{self, PinInit};
>  use alloc::boxed::Box;
>  use core::{
>      cell::UnsafeCell,
> +    marker::PhantomData,
>      mem::MaybeUninit,
>      ops::{Deref, DerefMut},
> +    ptr::NonNull,
>  };
>  
>  /// Used to transfer ownership to and from foreign (non-Rust) languages.
> @@ -268,6 +270,111 @@ impl<T> Opaque<T> {
>      }
>  }
>  
> +/// Types that are _always_ reference counted.
> +///
> +/// It allows such types to define their own custom ref increment and decrement functions.
> +/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
> +/// [`ARef<T>`].
> +///
> +/// This is usually implemented by wrappers to existing structures on the C side of the code. For
> +/// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
> +/// instances of a type.
> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that increments to the reference count keep the object alive in memory
> +/// at least until matching decrements are performed.
> +///
> +/// Implementers must also ensure that all instances are reference-counted. (Otherwise they
> +/// won't be able to honour the requirement that [`AlwaysRefCounted::inc_ref`] keep the object
> +/// alive.)
> +pub unsafe trait AlwaysRefCounted {
> +    /// Increments the reference count on the object.
> +    fn inc_ref(&self);
> +
> +    /// Decrements the reference count on the object.
> +    ///
> +    /// Frees the object when the count reaches zero.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that there was a previous matching increment to the reference count,
> +    /// and that the object is no longer used after its reference count is decremented (as it may
> +    /// result in the object being freed), unless the caller owns another increment on the refcount
> +    /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
> +    /// [`AlwaysRefCounted::dec_ref`] once).
> +    unsafe fn dec_ref(obj: NonNull<Self>);
> +}
> +
> +/// An owned reference to an always-reference-counted object.
> +///
> +/// The object's reference count is automatically decremented when an instance of [`ARef`] is
> +/// dropped. It is also automatically incremented when a new instance is created via
> +/// [`ARef::clone`].
> +///
> +/// # Invariants
> +///
> +/// The pointer stored in `ptr` is non-null and valid for the lifetime of the [`ARef`] instance. In
> +/// particular, the [`ARef`] instance owns an increment on the underlying object's reference count.
> +pub struct ARef<T: AlwaysRefCounted> {
> +    ptr: NonNull<T>,
> +    _p: PhantomData<T>,
> +}
> +
> +impl<T: AlwaysRefCounted> ARef<T> {
> +    /// Creates a new instance of [`ARef`].
> +    ///
> +    /// It takes over an increment of the reference count on the underlying object.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the reference count was incremented at least once, and that they
> +    /// are properly relinquishing one increment. That is, if there is only one increment, callers
> +    /// must not use the underlying object anymore -- it is only safe to do so via the newly
> +    /// created [`ARef`].
> +    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> +        // INVARIANT: The safety requirements guarantee that the new instance now owns the
> +        // increment on the refcount.
> +        Self {
> +            ptr,
> +            _p: PhantomData,
> +        }
> +    }
> +}
> +
> +impl<T: AlwaysRefCounted> Clone for ARef<T> {
> +    fn clone(&self) -> Self {
> +        self.inc_ref();
> +        // SAFETY: We just incremented the refcount above.
> +        unsafe { Self::from_raw(self.ptr) }
> +    }
> +}
> +
> +impl<T: AlwaysRefCounted> Deref for ARef<T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: The type invariants guarantee that the object is valid.
> +        unsafe { self.ptr.as_ref() }
> +    }
> +}
> +
> +impl<T: AlwaysRefCounted> From<&T> for ARef<T> {
> +    fn from(b: &T) -> Self {
> +        b.inc_ref();
> +        // SAFETY: We just incremented the refcount above.
> +        unsafe { Self::from_raw(NonNull::from(b)) }
> +    }
> +}
> +
> +impl<T: AlwaysRefCounted> Drop for ARef<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
> +        // decrement.
> +        unsafe { T::dec_ref(self.ptr) };
> +    }
> +}
> +
>  /// A sum type that always holds either a value of type `L` or `R`.
>  pub enum Either<L, R> {
>      /// Constructs an instance of [`Either`] containing a value of type `L`.

Re: [PATCH v4 08/13] rust: introduce `ARef`

[Benno Lossin]

On 11.04.23 07:45, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
>
> This is an owned reference to an object that is always ref-counted. This
> is meant to be used in wrappers for C types that have their own ref
> counting functions, for example, tasks, files, inodes, dentries, etc.
>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
> v1 -> v2: No changes
> v2 -> v3: No changes
> v3 -> v4: No changes
>
>   rust/kernel/types.rs | 107 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 107 insertions(+)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index a4b1e3778da7..29db59d6119a 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -6,8 +6,10 @@ use crate::init::{self, PinInit};
>   use alloc::boxed::Box;
>   use core::{
>       cell::UnsafeCell,
> +    marker::PhantomData,
>       mem::MaybeUninit,
>       ops::{Deref, DerefMut},
> +    ptr::NonNull,
>   };
>
>   /// Used to transfer ownership to and from foreign (non-Rust) languages.
> @@ -268,6 +270,111 @@ impl<T> Opaque<T> {
>       }
>   }
>
> +/// Types that are _always_ reference counted.
> +///
> +/// It allows such types to define their own custom ref increment and decrement functions.
> +/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
> +/// [`ARef<T>`].
> +///
> +/// This is usually implemented by wrappers to existing structures on the C side of the code. For
> +/// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
> +/// instances of a type.
> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that increments to the reference count keep the object alive in memory
> +/// at least until matching decrements are performed.
> +///
> +/// Implementers must also ensure that all instances are reference-counted. (Otherwise they
> +/// won't be able to honour the requirement that [`AlwaysRefCounted::inc_ref`] keep the object
> +/// alive.)

`dec_ref` states below that it 'Frees the object when the count reaches
zero.', this should also be stated here, since implementers should adhere
to that when implementing `dec_ref`.

> +pub unsafe trait AlwaysRefCounted {
> +    /// Increments the reference count on the object.
> +    fn inc_ref(&self);



> +
> +    /// Decrements the reference count on the object.
> +    ///
> +    /// Frees the object when the count reaches zero.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that there was a previous matching increment to the reference count,
> +    /// and that the object is no longer used after its reference count is decremented (as it may
> +    /// result in the object being freed), unless the caller owns another increment on the refcount
> +    /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
> +    /// [`AlwaysRefCounted::dec_ref`] once).
> +    unsafe fn dec_ref(obj: NonNull<Self>);
> +}
> +
> +/// An owned reference to an always-reference-counted object.
> +///
> +/// The object's reference count is automatically decremented when an instance of [`ARef`] is
> +/// dropped. It is also automatically incremented when a new instance is created via
> +/// [`ARef::clone`].
> +///
> +/// # Invariants
> +///
> +/// The pointer stored in `ptr` is non-null and valid for the lifetime of the [`ARef`] instance. In
> +/// particular, the [`ARef`] instance owns an increment on the underlying object's reference count.
> +pub struct ARef<T: AlwaysRefCounted> {
> +    ptr: NonNull<T>,
> +    _p: PhantomData<T>,
> +}
> +
> +impl<T: AlwaysRefCounted> ARef<T> {
> +    /// Creates a new instance of [`ARef`].
> +    ///
> +    /// It takes over an increment of the reference count on the underlying object.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the reference count was incremented at least once, and that they
> +    /// are properly relinquishing one increment. That is, if there is only one increment, callers
> +    /// must not use the underlying object anymore -- it is only safe to do so via the newly
> +    /// created [`ARef`].
> +    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> +        // INVARIANT: The safety requirements guarantee that the new instance now owns the
> +        // increment on the refcount.
> +        Self {
> +            ptr,
> +            _p: PhantomData,
> +        }
> +    }
> +}
> +
> +impl<T: AlwaysRefCounted> Clone for ARef<T> {
> +    fn clone(&self) -> Self {
> +        self.inc_ref();
> +        // SAFETY: We just incremented the refcount above.
> +        unsafe { Self::from_raw(self.ptr) }
> +    }
> +}
> +
> +impl<T: AlwaysRefCounted> Deref for ARef<T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: The type invariants guarantee that the object is valid.
> +        unsafe { self.ptr.as_ref() }
> +    }
> +}
> +
> +impl<T: AlwaysRefCounted> From<&T> for ARef<T> {
> +    fn from(b: &T) -> Self {
> +        b.inc_ref();
> +        // SAFETY: We just incremented the refcount above.
> +        unsafe { Self::from_raw(NonNull::from(b)) }
> +    }
> +}

This impl seems unsound to me, as we can do this:

     struct MyStruct {
         raw: Opaque<bindings::my_struct>, // This has a `refcount_t` inside.
     }

     impl MyStruct {
         fn new() -> Self { ... }
     }

     unsafe impl AlwaysRefCounted for MyStruct { ... } // Implemented correctly.

     fn evil() -> ARef<MyStruct> {
         let my_struct = MyStruct::new();
         ARef::from(&my_struct) // We return a pointer to the stack!
     }

similarly, this can also be done with a `Box`:

     fn evil2() -> ARef<MyStruct> {
         let my_struct = Box::new(MyStruct::new());
         ARef::from(&*my_struct)
         // Box is freed here, even just dropping the `ARef` will result in
         // a UAF.
     }

Additionally, I think that `AlwaysRefCounted::inc_ref` should not be safe,
as the caller must not deallocate the memory until the refcount is zero.

Another pitfall of `ARef`: it does not deallocate the memory when the
refcount reaches zero. People might expect that this code would not leak
memory:

     let foo = Box::try_new(Foo::new())?;
     let foo = Box::leak(foo); // Leak the box, such that we do not
                               // deallocate the memory too early.
     let foo = ARef::from(foo);
     drop(foo); // refcount is now zero, but the memory is never deallocated.


> +
> +impl<T: AlwaysRefCounted> Drop for ARef<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
> +        // decrement.
> +        unsafe { T::dec_ref(self.ptr) };
> +    }
> +}
> +
>   /// A sum type that always holds either a value of type `L` or `R`.
>   pub enum Either<L, R> {
>       /// Constructs an instance of [`Either`] containing a value of type `L`.
> --
> 2.34.1
>

Re: [PATCH v4 08/13] rust: introduce `ARef`

[Wedson Almeida Filho]

On Thu, 13 Apr 2023 at 06:19, Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 11.04.23 07:45, Wedson Almeida Filho wrote:
> > From: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> > This is an owned reference to an object that is always ref-counted. This
> > is meant to be used in wrappers for C types that have their own ref
> > counting functions, for example, tasks, files, inodes, dentries, etc.
> >
> > Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> > ---
> > v1 -> v2: No changes
> > v2 -> v3: No changes
> > v3 -> v4: No changes
> >
> >   rust/kernel/types.rs | 107 +++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 107 insertions(+)
> >
> > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> > index a4b1e3778da7..29db59d6119a 100644
> > --- a/rust/kernel/types.rs
> > +++ b/rust/kernel/types.rs
> > @@ -6,8 +6,10 @@ use crate::init::{self, PinInit};
> >   use alloc::boxed::Box;
> >   use core::{
> >       cell::UnsafeCell,
> > +    marker::PhantomData,
> >       mem::MaybeUninit,
> >       ops::{Deref, DerefMut},
> > +    ptr::NonNull,
> >   };
> >
> >   /// Used to transfer ownership to and from foreign (non-Rust) languages.
> > @@ -268,6 +270,111 @@ impl<T> Opaque<T> {
> >       }
> >   }
> >
> > +/// Types that are _always_ reference counted.
> > +///
> > +/// It allows such types to define their own custom ref increment and decrement functions.
> > +/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
> > +/// [`ARef<T>`].
> > +///
> > +/// This is usually implemented by wrappers to existing structures on the C side of the code. For
> > +/// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
> > +/// instances of a type.
> > +///
> > +/// # Safety
> > +///
> > +/// Implementers must ensure that increments to the reference count keep the object alive in memory
> > +/// at least until matching decrements are performed.
> > +///
> > +/// Implementers must also ensure that all instances are reference-counted. (Otherwise they
> > +/// won't be able to honour the requirement that [`AlwaysRefCounted::inc_ref`] keep the object
> > +/// alive.)
>
> `dec_ref` states below that it 'Frees the object when the count reaches
> zero.', this should also be stated here, since implementers should adhere
> to that when implementing `dec_ref`.

This section is for safety requirements. Freeing the object doesn't
fall into this category.

> > +pub unsafe trait AlwaysRefCounted {
> > +    /// Increments the reference count on the object.
> > +    fn inc_ref(&self);
>
>
>
> > +
> > +    /// Decrements the reference count on the object.
> > +    ///
> > +    /// Frees the object when the count reaches zero.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that there was a previous matching increment to the reference count,
> > +    /// and that the object is no longer used after its reference count is decremented (as it may
> > +    /// result in the object being freed), unless the caller owns another increment on the refcount
> > +    /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
> > +    /// [`AlwaysRefCounted::dec_ref`] once).
> > +    unsafe fn dec_ref(obj: NonNull<Self>);
> > +}
> > +
> > +/// An owned reference to an always-reference-counted object.
> > +///
> > +/// The object's reference count is automatically decremented when an instance of [`ARef`] is
> > +/// dropped. It is also automatically incremented when a new instance is created via
> > +/// [`ARef::clone`].
> > +///
> > +/// # Invariants
> > +///
> > +/// The pointer stored in `ptr` is non-null and valid for the lifetime of the [`ARef`] instance. In
> > +/// particular, the [`ARef`] instance owns an increment on the underlying object's reference count.
> > +pub struct ARef<T: AlwaysRefCounted> {
> > +    ptr: NonNull<T>,
> > +    _p: PhantomData<T>,
> > +}
> > +
> > +impl<T: AlwaysRefCounted> ARef<T> {
> > +    /// Creates a new instance of [`ARef`].
> > +    ///
> > +    /// It takes over an increment of the reference count on the underlying object.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that the reference count was incremented at least once, and that they
> > +    /// are properly relinquishing one increment. That is, if there is only one increment, callers
> > +    /// must not use the underlying object anymore -- it is only safe to do so via the newly
> > +    /// created [`ARef`].
> > +    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> > +        // INVARIANT: The safety requirements guarantee that the new instance now owns the
> > +        // increment on the refcount.
> > +        Self {
> > +            ptr,
> > +            _p: PhantomData,
> > +        }
> > +    }
> > +}
> > +
> > +impl<T: AlwaysRefCounted> Clone for ARef<T> {
> > +    fn clone(&self) -> Self {
> > +        self.inc_ref();
> > +        // SAFETY: We just incremented the refcount above.
> > +        unsafe { Self::from_raw(self.ptr) }
> > +    }
> > +}
> > +
> > +impl<T: AlwaysRefCounted> Deref for ARef<T> {
> > +    type Target = T;
> > +
> > +    fn deref(&self) -> &Self::Target {
> > +        // SAFETY: The type invariants guarantee that the object is valid.
> > +        unsafe { self.ptr.as_ref() }
> > +    }
> > +}
> > +
> > +impl<T: AlwaysRefCounted> From<&T> for ARef<T> {
> > +    fn from(b: &T) -> Self {
> > +        b.inc_ref();
> > +        // SAFETY: We just incremented the refcount above.
> > +        unsafe { Self::from_raw(NonNull::from(b)) }
> > +    }
> > +}
>
> This impl seems unsound to me, as we can do this:
>
>      struct MyStruct {
>          raw: Opaque<bindings::my_struct>, // This has a `refcount_t` inside.
>      }
>
>      impl MyStruct {
>          fn new() -> Self { ... }
>      }
>
>      unsafe impl AlwaysRefCounted for MyStruct { ... } // Implemented correctly.
>
>      fn evil() -> ARef<MyStruct> {
>          let my_struct = MyStruct::new();
>          ARef::from(&my_struct) // We return a pointer to the stack!
>      }
>
> similarly, this can also be done with a `Box`:
>
>      fn evil2() -> ARef<MyStruct> {
>          let my_struct = Box::new(MyStruct::new());
>          ARef::from(&*my_struct)
>          // Box is freed here, even just dropping the `ARef` will result in
>          // a UAF.
>      }

This implementation of `AlwaysRefCounted` is in violation of the
safety requirements of the trait, namely:

/// Implementers must ensure that increments to the reference count
keep the object alive in memory
/// at least until matching decrements are performed.
///
/// Implementers must also ensure that all instances are
reference-counted. (Otherwise they
/// won't be able to honour the requirement that
[`AlwaysRefCounted::inc_ref`] keep the object
/// alive.)

It boils down `MyStruct::new` in your example. It's not refcounted.

> Additionally, I think that `AlwaysRefCounted::inc_ref` should not be safe,
> as the caller must not deallocate the memory until the refcount is zero.

The existence of an `&T` is evidence that the refcount is non-zero, so
it is safe to increment it. The caller cannot free the object without
violating the safety requirements.

> Another pitfall of `ARef`: it does not deallocate the memory when the
> refcount reaches zero. People might expect that this code would not leak
> memory:
>
>      let foo = Box::try_new(Foo::new())?;
>      let foo = Box::leak(foo); // Leak the box, such that we do not
>                                // deallocate the memory too early.
>      let foo = ARef::from(foo);
>      drop(foo); // refcount is now zero, but the memory is never deallocated.

This is also in violation of the safety requirements of `AlwaysRefCounted`.

> > +
> > +impl<T: AlwaysRefCounted> Drop for ARef<T> {
> > +    fn drop(&mut self) {
> > +        // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
> > +        // decrement.
> > +        unsafe { T::dec_ref(self.ptr) };
> > +    }
> > +}
> > +
> >   /// A sum type that always holds either a value of type `L` or `R`.
> >   pub enum Either<L, R> {
> >       /// Constructs an instance of [`Either`] containing a value of type `L`.
> > --
> > 2.34.1
> >
>

Re: [PATCH v4 08/13] rust: introduce `ARef`

[Benno Lossin]

On 13.04.23 19:06, Wedson Almeida Filho wrote:
> On Thu, 13 Apr 2023 at 06:19, Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> On 11.04.23 07:45, Wedson Almeida Filho wrote:
>>> From: Wedson Almeida Filho <walmeida@microsoft.com>
>>>
>>> This is an owned reference to an object that is always ref-counted. This
>>> is meant to be used in wrappers for C types that have their own ref
>>> counting functions, for example, tasks, files, inodes, dentries, etc.
>>>
>>> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
>>> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
>>> ---
>>> v1 -> v2: No changes
>>> v2 -> v3: No changes
>>> v3 -> v4: No changes
>>>
>>>    rust/kernel/types.rs | 107 +++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 107 insertions(+)
>>>
>>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>>> index a4b1e3778da7..29db59d6119a 100644
>>> --- a/rust/kernel/types.rs
>>> +++ b/rust/kernel/types.rs
>>> @@ -6,8 +6,10 @@ use crate::init::{self, PinInit};
>>>    use alloc::boxed::Box;
>>>    use core::{
>>>        cell::UnsafeCell,
>>> +    marker::PhantomData,
>>>        mem::MaybeUninit,
>>>        ops::{Deref, DerefMut},
>>> +    ptr::NonNull,
>>>    };
>>>
>>>    /// Used to transfer ownership to and from foreign (non-Rust) languages.
>>> @@ -268,6 +270,111 @@ impl<T> Opaque<T> {
>>>        }
>>>    }
>>>
>>> +/// Types that are _always_ reference counted.
>>> +///
>>> +/// It allows such types to define their own custom ref increment and decrement functions.
>>> +/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
>>> +/// [`ARef<T>`].
>>> +///
>>> +/// This is usually implemented by wrappers to existing structures on the C side of the code. For
>>> +/// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
>>> +/// instances of a type.
>>> +///
>>> +/// # Safety
>>> +///
>>> +/// Implementers must ensure that increments to the reference count keep the object alive in memory
>>> +/// at least until matching decrements are performed.
>>> +///
>>> +/// Implementers must also ensure that all instances are reference-counted. (Otherwise they
>>> +/// won't be able to honour the requirement that [`AlwaysRefCounted::inc_ref`] keep the object
>>> +/// alive.)
>>
>> `dec_ref` states below that it 'Frees the object when the count reaches
>> zero.', this should also be stated here, since implementers should adhere
>> to that when implementing `dec_ref`.
>
> This section is for safety requirements. Freeing the object doesn't
> fall into this category.

It still needs to be upheld by the implementer, since it is guaranteed by
the documentation on the `dec_ref` function. Even non-safety requirements
are listed on the `unsafe` traits, if users should be able to rely on them.
If users should not rely on this, then maybe change the docs of `dec_ref`
to "when the refcount reaches zero, the object might be freed.".

>
>>> +pub unsafe trait AlwaysRefCounted {
>>> +    /// Increments the reference count on the object.
>>> +    fn inc_ref(&self);
>>
>>
>>
>>> +
>>> +    /// Decrements the reference count on the object.
>>> +    ///
>>> +    /// Frees the object when the count reaches zero.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// Callers must ensure that there was a previous matching increment to the reference count,
>>> +    /// and that the object is no longer used after its reference count is decremented (as it may
>>> +    /// result in the object being freed), unless the caller owns another increment on the refcount
>>> +    /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
>>> +    /// [`AlwaysRefCounted::dec_ref`] once).
>>> +    unsafe fn dec_ref(obj: NonNull<Self>);
>>> +}
>>> +
>>> +/// An owned reference to an always-reference-counted object.
>>> +///
>>> +/// The object's reference count is automatically decremented when an instance of [`ARef`] is
>>> +/// dropped. It is also automatically incremented when a new instance is created via
>>> +/// [`ARef::clone`].
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// The pointer stored in `ptr` is non-null and valid for the lifetime of the [`ARef`] instance. In
>>> +/// particular, the [`ARef`] instance owns an increment on the underlying object's reference count.
>>> +pub struct ARef<T: AlwaysRefCounted> {
>>> +    ptr: NonNull<T>,
>>> +    _p: PhantomData<T>,
>>> +}
>>> +
>>> +impl<T: AlwaysRefCounted> ARef<T> {
>>> +    /// Creates a new instance of [`ARef`].
>>> +    ///
>>> +    /// It takes over an increment of the reference count on the underlying object.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// Callers must ensure that the reference count was incremented at least once, and that they
>>> +    /// are properly relinquishing one increment. That is, if there is only one increment, callers
>>> +    /// must not use the underlying object anymore -- it is only safe to do so via the newly
>>> +    /// created [`ARef`].
>>> +    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
>>> +        // INVARIANT: The safety requirements guarantee that the new instance now owns the
>>> +        // increment on the refcount.
>>> +        Self {
>>> +            ptr,
>>> +            _p: PhantomData,
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +impl<T: AlwaysRefCounted> Clone for ARef<T> {
>>> +    fn clone(&self) -> Self {
>>> +        self.inc_ref();
>>> +        // SAFETY: We just incremented the refcount above.
>>> +        unsafe { Self::from_raw(self.ptr) }
>>> +    }
>>> +}
>>> +
>>> +impl<T: AlwaysRefCounted> Deref for ARef<T> {
>>> +    type Target = T;
>>> +
>>> +    fn deref(&self) -> &Self::Target {
>>> +        // SAFETY: The type invariants guarantee that the object is valid.
>>> +        unsafe { self.ptr.as_ref() }
>>> +    }
>>> +}
>>> +
>>> +impl<T: AlwaysRefCounted> From<&T> for ARef<T> {
>>> +    fn from(b: &T) -> Self {
>>> +        b.inc_ref();
>>> +        // SAFETY: We just incremented the refcount above.
>>> +        unsafe { Self::from_raw(NonNull::from(b)) }
>>> +    }
>>> +}
>>
>> This impl seems unsound to me, as we can do this:
>>
>>       struct MyStruct {
>>           raw: Opaque<bindings::my_struct>, // This has a `refcount_t` inside.
>>       }
>>
>>       impl MyStruct {
>>           fn new() -> Self { ... }
>>       }
>>
>>       unsafe impl AlwaysRefCounted for MyStruct { ... } // Implemented correctly.
>>
>>       fn evil() -> ARef<MyStruct> {
>>           let my_struct = MyStruct::new();
>>           ARef::from(&my_struct) // We return a pointer to the stack!
>>       }
>>
>> similarly, this can also be done with a `Box`:
>>
>>       fn evil2() -> ARef<MyStruct> {
>>           let my_struct = Box::new(MyStruct::new());
>>           ARef::from(&*my_struct)
>>           // Box is freed here, even just dropping the `ARef` will result in
>>           // a UAF.
>>       }
>
> This implementation of `AlwaysRefCounted` is in violation of the
> safety requirements of the trait, namely:
>
> /// Implementers must ensure that increments to the reference count
> keep the object alive in memory
> /// at least until matching decrements are performed.
> ///
> /// Implementers must also ensure that all instances are
> reference-counted. (Otherwise they
> /// won't be able to honour the requirement that
> [`AlwaysRefCounted::inc_ref`] keep the object
> /// alive.)
>
> It boils down `MyStruct::new` in your example. It's not refcounted.
>
>> Additionally, I think that `AlwaysRefCounted::inc_ref` should not be safe,
>> as the caller must not deallocate the memory until the refcount is zero.
>
> The existence of an `&T` is evidence that the refcount is non-zero, so
> it is safe to increment it. The caller cannot free the object without
> violating the safety requirements.
>
>> Another pitfall of `ARef`: it does not deallocate the memory when the
>> refcount reaches zero. People might expect that this code would not leak
>> memory:
>>
>>       let foo = Box::try_new(Foo::new())?;
>>       let foo = Box::leak(foo); // Leak the box, such that we do not
>>                                 // deallocate the memory too early.
>>       let foo = ARef::from(foo);
>>       drop(foo); // refcount is now zero, but the memory is never deallocated.
>
> This is also in violation of the safety requirements of `AlwaysRefCounted`.

It seems I have misunderstood the term "always reference counted".
We should document this in more detail, since this places a lot of
constraints on the implementers:

     Implementing `AlwaysRefCounted` for `T` places the following constraint on shared references `&T`:
     - Every `&T` points to memory that is not deallocated until the reference count reaches zero.
     - The existence of `&T` proves that the reference count is at least 1.

     This has some important consequences:
     - Exposing safe a way to get `T` is not allowed, since stack allocations are freed when the scope
       ends even though the reference count is non-zero.
     - Similarly giving safe access to `Box<T>` or other smart pointers is not allowed, since a `Box` can
       be freed independent from the reference count.

     This type is intended to be implemented for C types that embedd a `refcount_t` and that are both
     created and destroyed by C. Static references also work with this type, since they stay live
     indefinitely.

     Implementers must also ensure that they never give out `&mut T`, since
     - it can be reborrowed as `&T`,
     - converted to `ARef<T>`,
     - which can yield a `&T` that is alive at the same time as the `&mut T`.

>>> +
>>> +impl<T: AlwaysRefCounted> Drop for ARef<T> {
>>> +    fn drop(&mut self) {
>>> +        // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
>>> +        // decrement.
>>> +        unsafe { T::dec_ref(self.ptr) };
>>> +    }
>>> +}
>>> +
>>>    /// A sum type that always holds either a value of type `L` or `R`.
>>>    pub enum Either<L, R> {
>>>        /// Constructs an instance of [`Either`] containing a value of type `L`.
>>> --
>>> 2.34.1
>>>
>>

Re: [PATCH v4 08/13] rust: introduce `ARef`

[Wedson Almeida Filho]

On Thu, 13 Apr 2023 at 19:30, Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 13.04.23 19:06, Wedson Almeida Filho wrote:
> > On Thu, 13 Apr 2023 at 06:19, Benno Lossin <benno.lossin@proton.me> wrote:
> >>
> >> On 11.04.23 07:45, Wedson Almeida Filho wrote:
> >>> From: Wedson Almeida Filho <walmeida@microsoft.com>
> >>>
> >>> This is an owned reference to an object that is always ref-counted. This
> >>> is meant to be used in wrappers for C types that have their own ref
> >>> counting functions, for example, tasks, files, inodes, dentries, etc.
> >>>
> >>> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> >>> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> >>> ---
> >>> v1 -> v2: No changes
> >>> v2 -> v3: No changes
> >>> v3 -> v4: No changes
> >>>
> >>>    rust/kernel/types.rs | 107 +++++++++++++++++++++++++++++++++++++++++++
> >>>    1 file changed, 107 insertions(+)
> >>>
> >>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> >>> index a4b1e3778da7..29db59d6119a 100644
> >>> --- a/rust/kernel/types.rs
> >>> +++ b/rust/kernel/types.rs
> >>> @@ -6,8 +6,10 @@ use crate::init::{self, PinInit};
> >>>    use alloc::boxed::Box;
> >>>    use core::{
> >>>        cell::UnsafeCell,
> >>> +    marker::PhantomData,
> >>>        mem::MaybeUninit,
> >>>        ops::{Deref, DerefMut},
> >>> +    ptr::NonNull,
> >>>    };
> >>>
> >>>    /// Used to transfer ownership to and from foreign (non-Rust) languages.
> >>> @@ -268,6 +270,111 @@ impl<T> Opaque<T> {
> >>>        }
> >>>    }
> >>>
> >>> +/// Types that are _always_ reference counted.
> >>> +///
> >>> +/// It allows such types to define their own custom ref increment and decrement functions.
> >>> +/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
> >>> +/// [`ARef<T>`].
> >>> +///
> >>> +/// This is usually implemented by wrappers to existing structures on the C side of the code. For
> >>> +/// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
> >>> +/// instances of a type.
> >>> +///
> >>> +/// # Safety
> >>> +///
> >>> +/// Implementers must ensure that increments to the reference count keep the object alive in memory
> >>> +/// at least until matching decrements are performed.
> >>> +///
> >>> +/// Implementers must also ensure that all instances are reference-counted. (Otherwise they
> >>> +/// won't be able to honour the requirement that [`AlwaysRefCounted::inc_ref`] keep the object
> >>> +/// alive.)
> >>
> >> `dec_ref` states below that it 'Frees the object when the count reaches
> >> zero.', this should also be stated here, since implementers should adhere
> >> to that when implementing `dec_ref`.
> >
> > This section is for safety requirements. Freeing the object doesn't
> > fall into this category.
>
> It still needs to be upheld by the implementer, since it is guaranteed by
> the documentation on the `dec_ref` function. Even non-safety requirements
> are listed on the `unsafe` traits, if users should be able to rely on them.
> If users should not rely on this, then maybe change the docs of `dec_ref`
> to "when the refcount reaches zero, the object might be freed.".

I disagree that non-safety requirements should be listed under the
Safety section.

This section is meant for rules that implementers must adhere to to
ensure their implementations are safe. So it's usually read before
writing a "SAFETY:" comment for their "unsafe impl" blocks -- adding
extraneous information is counterproductive.

> >>> +pub unsafe trait AlwaysRefCounted {
> >>> +    /// Increments the reference count on the object.
> >>> +    fn inc_ref(&self);
> >>
> >>
> >>
> >>> +
> >>> +    /// Decrements the reference count on the object.
> >>> +    ///
> >>> +    /// Frees the object when the count reaches zero.
> >>> +    ///
> >>> +    /// # Safety
> >>> +    ///
> >>> +    /// Callers must ensure that there was a previous matching increment to the reference count,
> >>> +    /// and that the object is no longer used after its reference count is decremented (as it may
> >>> +    /// result in the object being freed), unless the caller owns another increment on the refcount
> >>> +    /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
> >>> +    /// [`AlwaysRefCounted::dec_ref`] once).
> >>> +    unsafe fn dec_ref(obj: NonNull<Self>);
> >>> +}
> >>> +
> >>> +/// An owned reference to an always-reference-counted object.
> >>> +///
> >>> +/// The object's reference count is automatically decremented when an instance of [`ARef`] is
> >>> +/// dropped. It is also automatically incremented when a new instance is created via
> >>> +/// [`ARef::clone`].
> >>> +///
> >>> +/// # Invariants
> >>> +///
> >>> +/// The pointer stored in `ptr` is non-null and valid for the lifetime of the [`ARef`] instance. In
> >>> +/// particular, the [`ARef`] instance owns an increment on the underlying object's reference count.
> >>> +pub struct ARef<T: AlwaysRefCounted> {
> >>> +    ptr: NonNull<T>,
> >>> +    _p: PhantomData<T>,
> >>> +}
> >>> +
> >>> +impl<T: AlwaysRefCounted> ARef<T> {
> >>> +    /// Creates a new instance of [`ARef`].
> >>> +    ///
> >>> +    /// It takes over an increment of the reference count on the underlying object.
> >>> +    ///
> >>> +    /// # Safety
> >>> +    ///
> >>> +    /// Callers must ensure that the reference count was incremented at least once, and that they
> >>> +    /// are properly relinquishing one increment. That is, if there is only one increment, callers
> >>> +    /// must not use the underlying object anymore -- it is only safe to do so via the newly
> >>> +    /// created [`ARef`].
> >>> +    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> >>> +        // INVARIANT: The safety requirements guarantee that the new instance now owns the
> >>> +        // increment on the refcount.
> >>> +        Self {
> >>> +            ptr,
> >>> +            _p: PhantomData,
> >>> +        }
> >>> +    }
> >>> +}
> >>> +
> >>> +impl<T: AlwaysRefCounted> Clone for ARef<T> {
> >>> +    fn clone(&self) -> Self {
> >>> +        self.inc_ref();
> >>> +        // SAFETY: We just incremented the refcount above.
> >>> +        unsafe { Self::from_raw(self.ptr) }
> >>> +    }
> >>> +}
> >>> +
> >>> +impl<T: AlwaysRefCounted> Deref for ARef<T> {
> >>> +    type Target = T;
> >>> +
> >>> +    fn deref(&self) -> &Self::Target {
> >>> +        // SAFETY: The type invariants guarantee that the object is valid.
> >>> +        unsafe { self.ptr.as_ref() }
> >>> +    }
> >>> +}
> >>> +
> >>> +impl<T: AlwaysRefCounted> From<&T> for ARef<T> {
> >>> +    fn from(b: &T) -> Self {
> >>> +        b.inc_ref();
> >>> +        // SAFETY: We just incremented the refcount above.
> >>> +        unsafe { Self::from_raw(NonNull::from(b)) }
> >>> +    }
> >>> +}
> >>
> >> This impl seems unsound to me, as we can do this:
> >>
> >>       struct MyStruct {
> >>           raw: Opaque<bindings::my_struct>, // This has a `refcount_t` inside.
> >>       }
> >>
> >>       impl MyStruct {
> >>           fn new() -> Self { ... }
> >>       }
> >>
> >>       unsafe impl AlwaysRefCounted for MyStruct { ... } // Implemented correctly.
> >>
> >>       fn evil() -> ARef<MyStruct> {
> >>           let my_struct = MyStruct::new();
> >>           ARef::from(&my_struct) // We return a pointer to the stack!
> >>       }
> >>
> >> similarly, this can also be done with a `Box`:
> >>
> >>       fn evil2() -> ARef<MyStruct> {
> >>           let my_struct = Box::new(MyStruct::new());
> >>           ARef::from(&*my_struct)
> >>           // Box is freed here, even just dropping the `ARef` will result in
> >>           // a UAF.
> >>       }
> >
> > This implementation of `AlwaysRefCounted` is in violation of the
> > safety requirements of the trait, namely:
> >
> > /// Implementers must ensure that increments to the reference count
> > keep the object alive in memory
> > /// at least until matching decrements are performed.
> > ///
> > /// Implementers must also ensure that all instances are
> > reference-counted. (Otherwise they
> > /// won't be able to honour the requirement that
> > [`AlwaysRefCounted::inc_ref`] keep the object
> > /// alive.)
> >
> > It boils down `MyStruct::new` in your example. It's not refcounted.
> >
> >> Additionally, I think that `AlwaysRefCounted::inc_ref` should not be safe,
> >> as the caller must not deallocate the memory until the refcount is zero.
> >
> > The existence of an `&T` is evidence that the refcount is non-zero, so
> > it is safe to increment it. The caller cannot free the object without
> > violating the safety requirements.
> >
> >> Another pitfall of `ARef`: it does not deallocate the memory when the
> >> refcount reaches zero. People might expect that this code would not leak
> >> memory:
> >>
> >>       let foo = Box::try_new(Foo::new())?;
> >>       let foo = Box::leak(foo); // Leak the box, such that we do not
> >>                                 // deallocate the memory too early.
> >>       let foo = ARef::from(foo);
> >>       drop(foo); // refcount is now zero, but the memory is never deallocated.
> >
> > This is also in violation of the safety requirements of `AlwaysRefCounted`.
>
> It seems I have misunderstood the term "always reference counted".
> We should document this in more detail, since this places a lot of
> constraints on the implementers:
>
>      Implementing `AlwaysRefCounted` for `T` places the following constraint on shared references `&T`:
>      - Every `&T` points to memory that is not deallocated until the reference count reaches zero.
>      - The existence of `&T` proves that the reference count is at least 1.

This is implied by the existing safety rules.

>      This has some important consequences:
>      - Exposing safe a way to get `T` is not allowed, since stack allocations are freed when the scope
>        ends even though the reference count is non-zero.

Stack allocations are ok, as long as they wait for the refcount to
drop to zero before the variable goes out of scope.

>      - Similarly giving safe access to `Box<T>` or other smart pointers is not allowed, since a `Box` can
>        be freed independent from the reference count.

`ARef<T>` is a smart pointer and it is definitely allowed.

Similarly to stack allocations I mention above, a `Box<T>`
implementation is conceivable as long as it ensures that the
allocation is freed only once the refcount reaches zero, for example,
by having a drop implementation that performs such a wait. (IOW, when
`Box<T>` goes out of scope, it always calls `drop` on `T` before
actually freeing the memory, so this implementation could block until
it is safe to do so, i.e., until the refcount reaches zero.)

>      This type is intended to be implemented for C types that embedd a `refcount_t` and that are both
>      created and destroyed by C. Static references also work with this type, since they stay live
>      indefinitely.

Embedding a `refcount_t` is not a requirement. I already mention in
the documentation that this is usually used for C structs and that
Rust code should use `Arc`.

>      Implementers must also ensure that they never give out `&mut T`, since
>      - it can be reborrowed as `&T`,
>      - converted to `ARef<T>`,
>      - which can yield a `&T` that is alive at the same time as the `&mut T`.

I agree with this. And I don't think this is a direct consequence of
the safety requirements, so I think it makes sense to add something
that covers this.

> >>> +
> >>> +impl<T: AlwaysRefCounted> Drop for ARef<T> {
> >>> +    fn drop(&mut self) {
> >>> +        // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
> >>> +        // decrement.
> >>> +        unsafe { T::dec_ref(self.ptr) };
> >>> +    }
> >>> +}
> >>> +
> >>>    /// A sum type that always holds either a value of type `L` or `R`.
> >>>    pub enum Either<L, R> {
> >>>        /// Constructs an instance of [`Either`] containing a value of type `L`.
> >>> --
> >>> 2.34.1
> >>>
> >>
>

Re: [PATCH v4 08/13] rust: introduce `ARef`

[Benno Lossin]

On 14.04.23 11:00, Wedson Almeida Filho wrote:
> On Thu, 13 Apr 2023 at 19:30, Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> On 13.04.23 19:06, Wedson Almeida Filho wrote:
>>> On Thu, 13 Apr 2023 at 06:19, Benno Lossin <benno.lossin@proton.me> wrote:
>>>>
>>>> On 11.04.23 07:45, Wedson Almeida Filho wrote:
>>>>> From: Wedson Almeida Filho <walmeida@microsoft.com>
>>>>>
>>>>> This is an owned reference to an object that is always ref-counted. This
>>>>> is meant to be used in wrappers for C types that have their own ref
>>>>> counting functions, for example, tasks, files, inodes, dentries, etc.
>>>>>
>>>>> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
>>>>> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
>>>>> ---
>>>>> v1 -> v2: No changes
>>>>> v2 -> v3: No changes
>>>>> v3 -> v4: No changes
>>>>>
>>>>>     rust/kernel/types.rs | 107 +++++++++++++++++++++++++++++++++++++++++++
>>>>>     1 file changed, 107 insertions(+)
>>>>>
>>>>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>>>>> index a4b1e3778da7..29db59d6119a 100644
>>>>> --- a/rust/kernel/types.rs
>>>>> +++ b/rust/kernel/types.rs
>>>>> @@ -6,8 +6,10 @@ use crate::init::{self, PinInit};
>>>>>     use alloc::boxed::Box;
>>>>>     use core::{
>>>>>         cell::UnsafeCell,
>>>>> +    marker::PhantomData,
>>>>>         mem::MaybeUninit,
>>>>>         ops::{Deref, DerefMut},
>>>>> +    ptr::NonNull,
>>>>>     };
>>>>>
>>>>>     /// Used to transfer ownership to and from foreign (non-Rust) languages.
>>>>> @@ -268,6 +270,111 @@ impl<T> Opaque<T> {
>>>>>         }
>>>>>     }
>>>>>
>>>>> +/// Types that are _always_ reference counted.
>>>>> +///
>>>>> +/// It allows such types to define their own custom ref increment and decrement functions.
>>>>> +/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
>>>>> +/// [`ARef<T>`].
>>>>> +///
>>>>> +/// This is usually implemented by wrappers to existing structures on the C side of the code. For
>>>>> +/// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
>>>>> +/// instances of a type.
>>>>> +///
>>>>> +/// # Safety
>>>>> +///
>>>>> +/// Implementers must ensure that increments to the reference count keep the object alive in memory
>>>>> +/// at least until matching decrements are performed.
>>>>> +///
>>>>> +/// Implementers must also ensure that all instances are reference-counted. (Otherwise they
>>>>> +/// won't be able to honour the requirement that [`AlwaysRefCounted::inc_ref`] keep the object
>>>>> +/// alive.)
>>>>
>>>> `dec_ref` states below that it 'Frees the object when the count reaches
>>>> zero.', this should also be stated here, since implementers should adhere
>>>> to that when implementing `dec_ref`.
>>>
>>> This section is for safety requirements. Freeing the object doesn't
>>> fall into this category.
>>
>> It still needs to be upheld by the implementer, since it is guaranteed by
>> the documentation on the `dec_ref` function. Even non-safety requirements
>> are listed on the `unsafe` traits, if users should be able to rely on them.
>> If users should not rely on this, then maybe change the docs of `dec_ref`
>> to "when the refcount reaches zero, the object might be freed.".
>
> I disagree that non-safety requirements should be listed under the
> Safety section.
>
> This section is meant for rules that implementers must adhere to to
> ensure their implementations are safe. So it's usually read before
> writing a "SAFETY:" comment for their "unsafe impl" blocks -- adding
> extraneous information is counterproductive.

Sure, I think it you could still mention it outside of the safety section.

>>>>> +pub unsafe trait AlwaysRefCounted {
>>>>> +    /// Increments the reference count on the object.
>>>>> +    fn inc_ref(&self);
>>>>
>>>>
>>>>
>>>>> +
>>>>> +    /// Decrements the reference count on the object.
>>>>> +    ///
>>>>> +    /// Frees the object when the count reaches zero.
>>>>> +    ///
>>>>> +    /// # Safety
>>>>> +    ///
>>>>> +    /// Callers must ensure that there was a previous matching increment to the reference count,
>>>>> +    /// and that the object is no longer used after its reference count is decremented (as it may
>>>>> +    /// result in the object being freed), unless the caller owns another increment on the refcount
>>>>> +    /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
>>>>> +    /// [`AlwaysRefCounted::dec_ref`] once).
>>>>> +    unsafe fn dec_ref(obj: NonNull<Self>);
>>>>> +}
>>>>> +
>>>>> +/// An owned reference to an always-reference-counted object.
>>>>> +///
>>>>> +/// The object's reference count is automatically decremented when an instance of [`ARef`] is
>>>>> +/// dropped. It is also automatically incremented when a new instance is created via
>>>>> +/// [`ARef::clone`].
>>>>> +///
>>>>> +/// # Invariants
>>>>> +///
>>>>> +/// The pointer stored in `ptr` is non-null and valid for the lifetime of the [`ARef`] instance. In
>>>>> +/// particular, the [`ARef`] instance owns an increment on the underlying object's reference count.
>>>>> +pub struct ARef<T: AlwaysRefCounted> {
>>>>> +    ptr: NonNull<T>,
>>>>> +    _p: PhantomData<T>,
>>>>> +}
>>>>> +
>>>>> +impl<T: AlwaysRefCounted> ARef<T> {
>>>>> +    /// Creates a new instance of [`ARef`].
>>>>> +    ///
>>>>> +    /// It takes over an increment of the reference count on the underlying object.
>>>>> +    ///
>>>>> +    /// # Safety
>>>>> +    ///
>>>>> +    /// Callers must ensure that the reference count was incremented at least once, and that they
>>>>> +    /// are properly relinquishing one increment. That is, if there is only one increment, callers
>>>>> +    /// must not use the underlying object anymore -- it is only safe to do so via the newly
>>>>> +    /// created [`ARef`].
>>>>> +    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
>>>>> +        // INVARIANT: The safety requirements guarantee that the new instance now owns the
>>>>> +        // increment on the refcount.
>>>>> +        Self {
>>>>> +            ptr,
>>>>> +            _p: PhantomData,
>>>>> +        }
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +impl<T: AlwaysRefCounted> Clone for ARef<T> {
>>>>> +    fn clone(&self) -> Self {
>>>>> +        self.inc_ref();
>>>>> +        // SAFETY: We just incremented the refcount above.
>>>>> +        unsafe { Self::from_raw(self.ptr) }
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +impl<T: AlwaysRefCounted> Deref for ARef<T> {
>>>>> +    type Target = T;
>>>>> +
>>>>> +    fn deref(&self) -> &Self::Target {
>>>>> +        // SAFETY: The type invariants guarantee that the object is valid.
>>>>> +        unsafe { self.ptr.as_ref() }
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +impl<T: AlwaysRefCounted> From<&T> for ARef<T> {
>>>>> +    fn from(b: &T) -> Self {
>>>>> +        b.inc_ref();
>>>>> +        // SAFETY: We just incremented the refcount above.
>>>>> +        unsafe { Self::from_raw(NonNull::from(b)) }
>>>>> +    }
>>>>> +}
>>>>
>>>> This impl seems unsound to me, as we can do this:
>>>>
>>>>        struct MyStruct {
>>>>            raw: Opaque<bindings::my_struct>, // This has a `refcount_t` inside.
>>>>        }
>>>>
>>>>        impl MyStruct {
>>>>            fn new() -> Self { ... }
>>>>        }
>>>>
>>>>        unsafe impl AlwaysRefCounted for MyStruct { ... } // Implemented correctly.
>>>>
>>>>        fn evil() -> ARef<MyStruct> {
>>>>            let my_struct = MyStruct::new();
>>>>            ARef::from(&my_struct) // We return a pointer to the stack!
>>>>        }
>>>>
>>>> similarly, this can also be done with a `Box`:
>>>>
>>>>        fn evil2() -> ARef<MyStruct> {
>>>>            let my_struct = Box::new(MyStruct::new());
>>>>            ARef::from(&*my_struct)
>>>>            // Box is freed here, even just dropping the `ARef` will result in
>>>>            // a UAF.
>>>>        }
>>>
>>> This implementation of `AlwaysRefCounted` is in violation of the
>>> safety requirements of the trait, namely:
>>>
>>> /// Implementers must ensure that increments to the reference count
>>> keep the object alive in memory
>>> /// at least until matching decrements are performed.
>>> ///
>>> /// Implementers must also ensure that all instances are
>>> reference-counted. (Otherwise they
>>> /// won't be able to honour the requirement that
>>> [`AlwaysRefCounted::inc_ref`] keep the object
>>> /// alive.)
>>>
>>> It boils down `MyStruct::new` in your example. It's not refcounted.
>>>
>>>> Additionally, I think that `AlwaysRefCounted::inc_ref` should not be safe,
>>>> as the caller must not deallocate the memory until the refcount is zero.
>>>
>>> The existence of an `&T` is evidence that the refcount is non-zero, so
>>> it is safe to increment it. The caller cannot free the object without
>>> violating the safety requirements.
>>>
>>>> Another pitfall of `ARef`: it does not deallocate the memory when the
>>>> refcount reaches zero. People might expect that this code would not leak
>>>> memory:
>>>>
>>>>        let foo = Box::try_new(Foo::new())?;
>>>>        let foo = Box::leak(foo); // Leak the box, such that we do not
>>>>                                  // deallocate the memory too early.
>>>>        let foo = ARef::from(foo);
>>>>        drop(foo); // refcount is now zero, but the memory is never deallocated.
>>>
>>> This is also in violation of the safety requirements of `AlwaysRefCounted`.
>>
>> It seems I have misunderstood the term "always reference counted".
>> We should document this in more detail, since this places a lot of
>> constraints on the implementers:
>>
>>       Implementing `AlwaysRefCounted` for `T` places the following constraint on shared references `&T`:
>>       - Every `&T` points to memory that is not deallocated until the reference count reaches zero.
>>       - The existence of `&T` proves that the reference count is at least 1.
>
> This is implied by the existing safety rules.

This was not obvious to me at all, I think we should extend the docs and
make this very clear.

>>       This has some important consequences:
>>       - Exposing safe a way to get `T` is not allowed, since stack allocations are freed when the scope
>>         ends even though the reference count is non-zero.
>
> Stack allocations are ok, as long as they wait for the refcount to
> drop to zero before the variable goes out of scope.

"Exposing a **safe** way", you cannot under any circumstances allow safe
code access to by value `T` when it implements `AlwaysRefCounted`, since
safe code can create a `&T` without upholding the invariants.

In your controlled function, you can create `T`s on the stack if you want,
but giving them out to safe code is the problem.

>>       - Similarly giving safe access to `Box<T>` or other smart pointers is not allowed, since a `Box` can
>>         be freed independent from the reference count.
>
> `ARef<T>` is a smart pointer and it is definitely allowed.

Yes, I meant smart pointers except `ARef`. E.g. putting `T` inside of an
`Arc` has the same problem as `Box<T>`.

>
> Similarly to stack allocations I mention above, a `Box<T>`
> implementation is conceivable as long as it ensures that the
> allocation is freed only once the refcount reaches zero, for example,
> by having a drop implementation that performs such a wait. (IOW, when
> `Box<T>` goes out of scope, it always calls `drop` on `T` before
> actually freeing the memory, so this implementation could block until
> it is safe to do so, i.e., until the refcount reaches zero.)

Is this something you want to do? Because to me this sounds like something
that could end up deadlocking very easily.

AIUI `AlwaysRefCounted` is intended for wrapper structs that are never
created on the Rust side. They are created and destroyed by C. The scenario
of putting them into a `Box` or `Arc` should never happen, since Rust does
not have a way to create them.

As soon as this is not the only use case for this trait, I feel like this
trait becomes very dangerous, since there are many different ways for you
to mess up via normal coding patterns.

Hence I think it is better to spell out the dangerous patterns here and
forbid them, since the only use case should never use them anyway.

Also in the `Box` case, the same problem as with `&mut T` exists, since
you can derive a `&mut T` from it.

>
>>       This type is intended to be implemented for C types that embedd a `refcount_t` and that are both
>>       created and destroyed by C. Static references also work with this type, since they stay live
>>       indefinitely.
>
> Embedding a `refcount_t` is not a requirement. I already mention in
> the documentation that this is usually used for C structs and that
> Rust code should use `Arc`.

I would prefer if we change the wording in the docs from `usually` to
`only`. If you agree with my interpretation above, then Rust types should
not implement this trait.

>
>>       Implementers must also ensure that they never give out `&mut T`, since
>>       - it can be reborrowed as `&T`,
>>       - converted to `ARef<T>`,
>>       - which can yield a `&T` that is alive at the same time as the `&mut T`.
>
> I agree with this. And I don't think this is a direct consequence of
> the safety requirements, so I think it makes sense to add something
> that covers this.
>
>>>>> +
>>>>> +impl<T: AlwaysRefCounted> Drop for ARef<T> {
>>>>> +    fn drop(&mut self) {
>>>>> +        // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
>>>>> +        // decrement.
>>>>> +        unsafe { T::dec_ref(self.ptr) };
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>     /// A sum type that always holds either a value of type `L` or `R`.
>>>>>     pub enum Either<L, R> {
>>>>>         /// Constructs an instance of [`Either`] containing a value of type `L`.
>>>>> --
>>>>> 2.34.1
>>>>>
>>>>
>>

Re: [PATCH v4 08/13] rust: introduce `ARef`

[Gary Guo]

On Fri, 14 Apr 2023 09:46:53 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 14.04.23 11:00, Wedson Almeida Filho wrote:
> > On Thu, 13 Apr 2023 at 19:30, Benno Lossin <benno.lossin@proton.me> wrote:  
> >>
> >> On 13.04.23 19:06, Wedson Almeida Filho wrote:  
> >>> On Thu, 13 Apr 2023 at 06:19, Benno Lossin <benno.lossin@proton.me> wrote:  
> >>>>
> >>>> On 11.04.23 07:45, Wedson Almeida Filho wrote:  
> >>>>> From: Wedson Almeida Filho <walmeida@microsoft.com>
> >>>>>
> >>>>> This is an owned reference to an object that is always ref-counted. This
> >>>>> is meant to be used in wrappers for C types that have their own ref
> >>>>> counting functions, for example, tasks, files, inodes, dentries, etc.
> >>>>>
> >>>>> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> >>>>> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> >>>>> ---
> >>>>> v1 -> v2: No changes
> >>>>> v2 -> v3: No changes
> >>>>> v3 -> v4: No changes
> >>>>>
> >>>>>     rust/kernel/types.rs | 107 +++++++++++++++++++++++++++++++++++++++++++
> >>>>>     1 file changed, 107 insertions(+)
> >>>>>
> >>>>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> >>>>> index a4b1e3778da7..29db59d6119a 100644
> >>>>> --- a/rust/kernel/types.rs
> >>>>> +++ b/rust/kernel/types.rs
> >>>>> @@ -6,8 +6,10 @@ use crate::init::{self, PinInit};
> >>>>>     use alloc::boxed::Box;
> >>>>>     use core::{
> >>>>>         cell::UnsafeCell,
> >>>>> +    marker::PhantomData,
> >>>>>         mem::MaybeUninit,
> >>>>>         ops::{Deref, DerefMut},
> >>>>> +    ptr::NonNull,
> >>>>>     };
> >>>>>
> >>>>>     /// Used to transfer ownership to and from foreign (non-Rust) languages.
> >>>>> @@ -268,6 +270,111 @@ impl<T> Opaque<T> {
> >>>>>         }
> >>>>>     }
> >>>>>
> >>>>> +/// Types that are _always_ reference counted.
> >>>>> +///
> >>>>> +/// It allows such types to define their own custom ref increment and decrement functions.
> >>>>> +/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
> >>>>> +/// [`ARef<T>`].
> >>>>> +///
> >>>>> +/// This is usually implemented by wrappers to existing structures on the C side of the code. For
> >>>>> +/// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
> >>>>> +/// instances of a type.
> >>>>> +///
> >>>>> +/// # Safety
> >>>>> +///
> >>>>> +/// Implementers must ensure that increments to the reference count keep the object alive in memory
> >>>>> +/// at least until matching decrements are performed.
> >>>>> +///
> >>>>> +/// Implementers must also ensure that all instances are reference-counted. (Otherwise they
> >>>>> +/// won't be able to honour the requirement that [`AlwaysRefCounted::inc_ref`] keep the object
> >>>>> +/// alive.)  
> >>>>
> >>>> `dec_ref` states below that it 'Frees the object when the count reaches
> >>>> zero.', this should also be stated here, since implementers should adhere
> >>>> to that when implementing `dec_ref`.  
> >>>
> >>> This section is for safety requirements. Freeing the object doesn't
> >>> fall into this category.  
> >>
> >> It still needs to be upheld by the implementer, since it is guaranteed by
> >> the documentation on the `dec_ref` function. Even non-safety requirements
> >> are listed on the `unsafe` traits, if users should be able to rely on them.
> >> If users should not rely on this, then maybe change the docs of `dec_ref`
> >> to "when the refcount reaches zero, the object might be freed.".  
> >
> > I disagree that non-safety requirements should be listed under the
> > Safety section.
> >
> > This section is meant for rules that implementers must adhere to to
> > ensure their implementations are safe. So it's usually read before
> > writing a "SAFETY:" comment for their "unsafe impl" blocks -- adding
> > extraneous information is counterproductive.  
> 
> Sure, I think it you could still mention it outside of the safety section.
> 
> >>>>> +pub unsafe trait AlwaysRefCounted {
> >>>>> +    /// Increments the reference count on the object.
> >>>>> +    fn inc_ref(&self);  
> >>>>
> >>>>
> >>>>  
> >>>>> +
> >>>>> +    /// Decrements the reference count on the object.
> >>>>> +    ///
> >>>>> +    /// Frees the object when the count reaches zero.
> >>>>> +    ///
> >>>>> +    /// # Safety
> >>>>> +    ///
> >>>>> +    /// Callers must ensure that there was a previous matching increment to the reference count,
> >>>>> +    /// and that the object is no longer used after its reference count is decremented (as it may
> >>>>> +    /// result in the object being freed), unless the caller owns another increment on the refcount
> >>>>> +    /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
> >>>>> +    /// [`AlwaysRefCounted::dec_ref`] once).
> >>>>> +    unsafe fn dec_ref(obj: NonNull<Self>);
> >>>>> +}
> >>>>> +
> >>>>> +/// An owned reference to an always-reference-counted object.
> >>>>> +///
> >>>>> +/// The object's reference count is automatically decremented when an instance of [`ARef`] is
> >>>>> +/// dropped. It is also automatically incremented when a new instance is created via
> >>>>> +/// [`ARef::clone`].
> >>>>> +///
> >>>>> +/// # Invariants
> >>>>> +///
> >>>>> +/// The pointer stored in `ptr` is non-null and valid for the lifetime of the [`ARef`] instance. In
> >>>>> +/// particular, the [`ARef`] instance owns an increment on the underlying object's reference count.
> >>>>> +pub struct ARef<T: AlwaysRefCounted> {
> >>>>> +    ptr: NonNull<T>,
> >>>>> +    _p: PhantomData<T>,
> >>>>> +}
> >>>>> +
> >>>>> +impl<T: AlwaysRefCounted> ARef<T> {
> >>>>> +    /// Creates a new instance of [`ARef`].
> >>>>> +    ///
> >>>>> +    /// It takes over an increment of the reference count on the underlying object.
> >>>>> +    ///
> >>>>> +    /// # Safety
> >>>>> +    ///
> >>>>> +    /// Callers must ensure that the reference count was incremented at least once, and that they
> >>>>> +    /// are properly relinquishing one increment. That is, if there is only one increment, callers
> >>>>> +    /// must not use the underlying object anymore -- it is only safe to do so via the newly
> >>>>> +    /// created [`ARef`].
> >>>>> +    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> >>>>> +        // INVARIANT: The safety requirements guarantee that the new instance now owns the
> >>>>> +        // increment on the refcount.
> >>>>> +        Self {
> >>>>> +            ptr,
> >>>>> +            _p: PhantomData,
> >>>>> +        }
> >>>>> +    }
> >>>>> +}
> >>>>> +
> >>>>> +impl<T: AlwaysRefCounted> Clone for ARef<T> {
> >>>>> +    fn clone(&self) -> Self {
> >>>>> +        self.inc_ref();
> >>>>> +        // SAFETY: We just incremented the refcount above.
> >>>>> +        unsafe { Self::from_raw(self.ptr) }
> >>>>> +    }
> >>>>> +}
> >>>>> +
> >>>>> +impl<T: AlwaysRefCounted> Deref for ARef<T> {
> >>>>> +    type Target = T;
> >>>>> +
> >>>>> +    fn deref(&self) -> &Self::Target {
> >>>>> +        // SAFETY: The type invariants guarantee that the object is valid.
> >>>>> +        unsafe { self.ptr.as_ref() }
> >>>>> +    }
> >>>>> +}
> >>>>> +
> >>>>> +impl<T: AlwaysRefCounted> From<&T> for ARef<T> {
> >>>>> +    fn from(b: &T) -> Self {
> >>>>> +        b.inc_ref();
> >>>>> +        // SAFETY: We just incremented the refcount above.
> >>>>> +        unsafe { Self::from_raw(NonNull::from(b)) }
> >>>>> +    }
> >>>>> +}  
> >>>>
> >>>> This impl seems unsound to me, as we can do this:
> >>>>
> >>>>        struct MyStruct {
> >>>>            raw: Opaque<bindings::my_struct>, // This has a `refcount_t` inside.
> >>>>        }
> >>>>
> >>>>        impl MyStruct {
> >>>>            fn new() -> Self { ... }
> >>>>        }
> >>>>
> >>>>        unsafe impl AlwaysRefCounted for MyStruct { ... } // Implemented correctly.
> >>>>
> >>>>        fn evil() -> ARef<MyStruct> {
> >>>>            let my_struct = MyStruct::new();
> >>>>            ARef::from(&my_struct) // We return a pointer to the stack!
> >>>>        }
> >>>>
> >>>> similarly, this can also be done with a `Box`:
> >>>>
> >>>>        fn evil2() -> ARef<MyStruct> {
> >>>>            let my_struct = Box::new(MyStruct::new());
> >>>>            ARef::from(&*my_struct)
> >>>>            // Box is freed here, even just dropping the `ARef` will result in
> >>>>            // a UAF.
> >>>>        }  
> >>>
> >>> This implementation of `AlwaysRefCounted` is in violation of the
> >>> safety requirements of the trait, namely:
> >>>
> >>> /// Implementers must ensure that increments to the reference count
> >>> keep the object alive in memory
> >>> /// at least until matching decrements are performed.
> >>> ///
> >>> /// Implementers must also ensure that all instances are
> >>> reference-counted. (Otherwise they
> >>> /// won't be able to honour the requirement that
> >>> [`AlwaysRefCounted::inc_ref`] keep the object
> >>> /// alive.)
> >>>
> >>> It boils down `MyStruct::new` in your example. It's not refcounted.
> >>>  
> >>>> Additionally, I think that `AlwaysRefCounted::inc_ref` should not be safe,
> >>>> as the caller must not deallocate the memory until the refcount is zero.  
> >>>
> >>> The existence of an `&T` is evidence that the refcount is non-zero, so
> >>> it is safe to increment it. The caller cannot free the object without
> >>> violating the safety requirements.
> >>>  
> >>>> Another pitfall of `ARef`: it does not deallocate the memory when the
> >>>> refcount reaches zero. People might expect that this code would not leak
> >>>> memory:
> >>>>
> >>>>        let foo = Box::try_new(Foo::new())?;
> >>>>        let foo = Box::leak(foo); // Leak the box, such that we do not
> >>>>                                  // deallocate the memory too early.
> >>>>        let foo = ARef::from(foo);
> >>>>        drop(foo); // refcount is now zero, but the memory is never deallocated.  
> >>>
> >>> This is also in violation of the safety requirements of `AlwaysRefCounted`.  
> >>
> >> It seems I have misunderstood the term "always reference counted".
> >> We should document this in more detail, since this places a lot of
> >> constraints on the implementers:
> >>
> >>       Implementing `AlwaysRefCounted` for `T` places the following constraint on shared references `&T`:
> >>       - Every `&T` points to memory that is not deallocated until the reference count reaches zero.
> >>       - The existence of `&T` proves that the reference count is at least 1.  
> >
> > This is implied by the existing safety rules.  
> 
> This was not obvious to me at all, I think we should extend the docs and
> make this very clear.
> 
> >>       This has some important consequences:
> >>       - Exposing safe a way to get `T` is not allowed, since stack allocations are freed when the scope
> >>         ends even though the reference count is non-zero.  
> >
> > Stack allocations are ok, as long as they wait for the refcount to
> > drop to zero before the variable goes out of scope.  
> 
> "Exposing a **safe** way", you cannot under any circumstances allow safe
> code access to by value `T` when it implements `AlwaysRefCounted`, since
> safe code can create a `&T` without upholding the invariants.
> 
> In your controlled function, you can create `T`s on the stack if you want,
> but giving them out to safe code is the problem.
> 
> >>       - Similarly giving safe access to `Box<T>` or other smart pointers is not allowed, since a `Box` can
> >>         be freed independent from the reference count.  
> >
> > `ARef<T>` is a smart pointer and it is definitely allowed.  
> 
> Yes, I meant smart pointers except `ARef`. E.g. putting `T` inside of an
> `Arc` has the same problem as `Box<T>`.
> 
> >
> > Similarly to stack allocations I mention above, a `Box<T>`
> > implementation is conceivable as long as it ensures that the
> > allocation is freed only once the refcount reaches zero, for example,
> > by having a drop implementation that performs such a wait. (IOW, when
> > `Box<T>` goes out of scope, it always calls `drop` on `T` before
> > actually freeing the memory, so this implementation could block until
> > it is safe to do so, i.e., until the refcount reaches zero.)  
> 
> Is this something you want to do? Because to me this sounds like something
> that could end up deadlocking very easily.
> 
> AIUI `AlwaysRefCounted` is intended for wrapper structs that are never
> created on the Rust side. They are created and destroyed by C.

No.

It's perfectly legal for Rust code to implement this trait, and it
might even be desirable in some cases, because it gives more control
than relying on `Arc` and `Drop`.

For example, if a type is usable in RCU, then you might want to have
some code like this:

impl AlwaysRefCounted for MyType {
    fn inc_ref(&self) {
        self.refcnt.fetch_add(1, Ordering::Relaxed);
    }

    fn dec_ref(this: NonNull<Self>) {
       let refcnt = this.as_ref().refcnt.fetch_sub(1, Ordering::Relaxed) - 1;
       if refcnt == 0 {
           // Unpublish the pointer from some RCU data structure
           synchronize_rcu();
           // proceed to drop the type and box
       }
    }
}

The example given above can't rely on dtor because `drop` takes a
mutable reference.

> The scenario
> of putting them into a `Box` or `Arc` should never happen, since Rust does
> not have a way to create them.
> 
> As soon as this is not the only use case for this trait, I feel like this
> trait becomes very dangerous, since there are many different ways for you
> to mess up via normal coding patterns.
> 
> Hence I think it is better to spell out the dangerous patterns here and
> forbid them, since the only use case should never use them anyway.
> 
> Also in the `Box` case, the same problem as with `&mut T` exists, since
> you can derive a `&mut T` from it.
> 
> >  
> >>       This type is intended to be implemented for C types that embedd a `refcount_t` and that are both
> >>       created and destroyed by C. Static references also work with this type, since they stay live
> >>       indefinitely.  
> >
> > Embedding a `refcount_t` is not a requirement. I already mention in
> > the documentation that this is usually used for C structs and that
> > Rust code should use `Arc`.  
> 
> I would prefer if we change the wording in the docs from `usually` to
> `only`. If you agree with my interpretation above, then Rust types should
> not implement this trait.
> 
> >  
> >>       Implementers must also ensure that they never give out `&mut T`, since
> >>       - it can be reborrowed as `&T`,
> >>       - converted to `ARef<T>`,
> >>       - which can yield a `&T` that is alive at the same time as the `&mut T`.  
> >
> > I agree with this. And I don't think this is a direct consequence of
> > the safety requirements, so I think it makes sense to add something
> > that covers this.
> >  
> >>>>> +
> >>>>> +impl<T: AlwaysRefCounted> Drop for ARef<T> {
> >>>>> +    fn drop(&mut self) {
> >>>>> +        // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
> >>>>> +        // decrement.
> >>>>> +        unsafe { T::dec_ref(self.ptr) };
> >>>>> +    }
> >>>>> +}
> >>>>> +
> >>>>>     /// A sum type that always holds either a value of type `L` or `R`.
> >>>>>     pub enum Either<L, R> {
> >>>>>         /// Constructs an instance of [`Either`] containing a value of type `L`.
> >>>>> --
> >>>>> 2.34.1
> >>>>>  
> >>>>  
> >>  
> 

Re: [PATCH v4 08/13] rust: introduce `ARef`

[Boqun Feng]

On Fri, Apr 14, 2023 at 03:38:06PM +0100, Gary Guo wrote:
> On Fri, 14 Apr 2023 09:46:53 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
> > On 14.04.23 11:00, Wedson Almeida Filho wrote:
> > > On Thu, 13 Apr 2023 at 19:30, Benno Lossin <benno.lossin@proton.me> wrote:  
> > >>
> > >> On 13.04.23 19:06, Wedson Almeida Filho wrote:  
> > >>> On Thu, 13 Apr 2023 at 06:19, Benno Lossin <benno.lossin@proton.me> wrote:  
> > >>>>
> > >>>> On 11.04.23 07:45, Wedson Almeida Filho wrote:  
> > >>>>> From: Wedson Almeida Filho <walmeida@microsoft.com>
> > >>>>>
> > >>>>> This is an owned reference to an object that is always ref-counted. This
> > >>>>> is meant to be used in wrappers for C types that have their own ref
> > >>>>> counting functions, for example, tasks, files, inodes, dentries, etc.
> > >>>>>
> > >>>>> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> > >>>>> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> > >>>>> ---
> > >>>>> v1 -> v2: No changes
> > >>>>> v2 -> v3: No changes
> > >>>>> v3 -> v4: No changes
> > >>>>>
> > >>>>>     rust/kernel/types.rs | 107 +++++++++++++++++++++++++++++++++++++++++++
> > >>>>>     1 file changed, 107 insertions(+)
> > >>>>>
> > >>>>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> > >>>>> index a4b1e3778da7..29db59d6119a 100644
> > >>>>> --- a/rust/kernel/types.rs
> > >>>>> +++ b/rust/kernel/types.rs
> > >>>>> @@ -6,8 +6,10 @@ use crate::init::{self, PinInit};
> > >>>>>     use alloc::boxed::Box;
> > >>>>>     use core::{
> > >>>>>         cell::UnsafeCell,
> > >>>>> +    marker::PhantomData,
> > >>>>>         mem::MaybeUninit,
> > >>>>>         ops::{Deref, DerefMut},
> > >>>>> +    ptr::NonNull,
> > >>>>>     };
> > >>>>>
> > >>>>>     /// Used to transfer ownership to and from foreign (non-Rust) languages.
> > >>>>> @@ -268,6 +270,111 @@ impl<T> Opaque<T> {
> > >>>>>         }
> > >>>>>     }
> > >>>>>
> > >>>>> +/// Types that are _always_ reference counted.
> > >>>>> +///
> > >>>>> +/// It allows such types to define their own custom ref increment and decrement functions.
> > >>>>> +/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
> > >>>>> +/// [`ARef<T>`].
> > >>>>> +///
> > >>>>> +/// This is usually implemented by wrappers to existing structures on the C side of the code. For
> > >>>>> +/// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
> > >>>>> +/// instances of a type.
> > >>>>> +///
> > >>>>> +/// # Safety
> > >>>>> +///
> > >>>>> +/// Implementers must ensure that increments to the reference count keep the object alive in memory
> > >>>>> +/// at least until matching decrements are performed.
> > >>>>> +///
> > >>>>> +/// Implementers must also ensure that all instances are reference-counted. (Otherwise they
> > >>>>> +/// won't be able to honour the requirement that [`AlwaysRefCounted::inc_ref`] keep the object
> > >>>>> +/// alive.)  
> > >>>>
> > >>>> `dec_ref` states below that it 'Frees the object when the count reaches
> > >>>> zero.', this should also be stated here, since implementers should adhere
> > >>>> to that when implementing `dec_ref`.  
> > >>>
> > >>> This section is for safety requirements. Freeing the object doesn't
> > >>> fall into this category.  
> > >>
> > >> It still needs to be upheld by the implementer, since it is guaranteed by
> > >> the documentation on the `dec_ref` function. Even non-safety requirements
> > >> are listed on the `unsafe` traits, if users should be able to rely on them.
> > >> If users should not rely on this, then maybe change the docs of `dec_ref`
> > >> to "when the refcount reaches zero, the object might be freed.".  
> > >
> > > I disagree that non-safety requirements should be listed under the
> > > Safety section.
> > >
> > > This section is meant for rules that implementers must adhere to to
> > > ensure their implementations are safe. So it's usually read before
> > > writing a "SAFETY:" comment for their "unsafe impl" blocks -- adding
> > > extraneous information is counterproductive.  
> > 
> > Sure, I think it you could still mention it outside of the safety section.
> > 
> > >>>>> +pub unsafe trait AlwaysRefCounted {
> > >>>>> +    /// Increments the reference count on the object.
> > >>>>> +    fn inc_ref(&self);  
> > >>>>
> > >>>>
> > >>>>  
> > >>>>> +
> > >>>>> +    /// Decrements the reference count on the object.
> > >>>>> +    ///
> > >>>>> +    /// Frees the object when the count reaches zero.
> > >>>>> +    ///
> > >>>>> +    /// # Safety
> > >>>>> +    ///
> > >>>>> +    /// Callers must ensure that there was a previous matching increment to the reference count,
> > >>>>> +    /// and that the object is no longer used after its reference count is decremented (as it may
> > >>>>> +    /// result in the object being freed), unless the caller owns another increment on the refcount
> > >>>>> +    /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
> > >>>>> +    /// [`AlwaysRefCounted::dec_ref`] once).
> > >>>>> +    unsafe fn dec_ref(obj: NonNull<Self>);
> > >>>>> +}
> > >>>>> +
> > >>>>> +/// An owned reference to an always-reference-counted object.
> > >>>>> +///
> > >>>>> +/// The object's reference count is automatically decremented when an instance of [`ARef`] is
> > >>>>> +/// dropped. It is also automatically incremented when a new instance is created via
> > >>>>> +/// [`ARef::clone`].
> > >>>>> +///
> > >>>>> +/// # Invariants
> > >>>>> +///
> > >>>>> +/// The pointer stored in `ptr` is non-null and valid for the lifetime of the [`ARef`] instance. In
> > >>>>> +/// particular, the [`ARef`] instance owns an increment on the underlying object's reference count.
> > >>>>> +pub struct ARef<T: AlwaysRefCounted> {
> > >>>>> +    ptr: NonNull<T>,
> > >>>>> +    _p: PhantomData<T>,
> > >>>>> +}
> > >>>>> +
> > >>>>> +impl<T: AlwaysRefCounted> ARef<T> {
> > >>>>> +    /// Creates a new instance of [`ARef`].
> > >>>>> +    ///
> > >>>>> +    /// It takes over an increment of the reference count on the underlying object.
> > >>>>> +    ///
> > >>>>> +    /// # Safety
> > >>>>> +    ///
> > >>>>> +    /// Callers must ensure that the reference count was incremented at least once, and that they
> > >>>>> +    /// are properly relinquishing one increment. That is, if there is only one increment, callers
> > >>>>> +    /// must not use the underlying object anymore -- it is only safe to do so via the newly
> > >>>>> +    /// created [`ARef`].
> > >>>>> +    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> > >>>>> +        // INVARIANT: The safety requirements guarantee that the new instance now owns the
> > >>>>> +        // increment on the refcount.
> > >>>>> +        Self {
> > >>>>> +            ptr,
> > >>>>> +            _p: PhantomData,
> > >>>>> +        }
> > >>>>> +    }
> > >>>>> +}
> > >>>>> +
> > >>>>> +impl<T: AlwaysRefCounted> Clone for ARef<T> {
> > >>>>> +    fn clone(&self) -> Self {
> > >>>>> +        self.inc_ref();
> > >>>>> +        // SAFETY: We just incremented the refcount above.
> > >>>>> +        unsafe { Self::from_raw(self.ptr) }
> > >>>>> +    }
> > >>>>> +}
> > >>>>> +
> > >>>>> +impl<T: AlwaysRefCounted> Deref for ARef<T> {
> > >>>>> +    type Target = T;
> > >>>>> +
> > >>>>> +    fn deref(&self) -> &Self::Target {
> > >>>>> +        // SAFETY: The type invariants guarantee that the object is valid.
> > >>>>> +        unsafe { self.ptr.as_ref() }
> > >>>>> +    }
> > >>>>> +}
> > >>>>> +
> > >>>>> +impl<T: AlwaysRefCounted> From<&T> for ARef<T> {
> > >>>>> +    fn from(b: &T) -> Self {
> > >>>>> +        b.inc_ref();
> > >>>>> +        // SAFETY: We just incremented the refcount above.
> > >>>>> +        unsafe { Self::from_raw(NonNull::from(b)) }
> > >>>>> +    }
> > >>>>> +}  
> > >>>>
> > >>>> This impl seems unsound to me, as we can do this:
> > >>>>
> > >>>>        struct MyStruct {
> > >>>>            raw: Opaque<bindings::my_struct>, // This has a `refcount_t` inside.
> > >>>>        }
> > >>>>
> > >>>>        impl MyStruct {
> > >>>>            fn new() -> Self { ... }
> > >>>>        }
> > >>>>
> > >>>>        unsafe impl AlwaysRefCounted for MyStruct { ... } // Implemented correctly.
> > >>>>
> > >>>>        fn evil() -> ARef<MyStruct> {
> > >>>>            let my_struct = MyStruct::new();
> > >>>>            ARef::from(&my_struct) // We return a pointer to the stack!
> > >>>>        }
> > >>>>
> > >>>> similarly, this can also be done with a `Box`:
> > >>>>
> > >>>>        fn evil2() -> ARef<MyStruct> {
> > >>>>            let my_struct = Box::new(MyStruct::new());
> > >>>>            ARef::from(&*my_struct)
> > >>>>            // Box is freed here, even just dropping the `ARef` will result in
> > >>>>            // a UAF.
> > >>>>        }  
> > >>>
> > >>> This implementation of `AlwaysRefCounted` is in violation of the
> > >>> safety requirements of the trait, namely:
> > >>>
> > >>> /// Implementers must ensure that increments to the reference count
> > >>> keep the object alive in memory
> > >>> /// at least until matching decrements are performed.
> > >>> ///
> > >>> /// Implementers must also ensure that all instances are
> > >>> reference-counted. (Otherwise they
> > >>> /// won't be able to honour the requirement that
> > >>> [`AlwaysRefCounted::inc_ref`] keep the object
> > >>> /// alive.)
> > >>>
> > >>> It boils down `MyStruct::new` in your example. It's not refcounted.
> > >>>  
> > >>>> Additionally, I think that `AlwaysRefCounted::inc_ref` should not be safe,
> > >>>> as the caller must not deallocate the memory until the refcount is zero.  
> > >>>
> > >>> The existence of an `&T` is evidence that the refcount is non-zero, so
> > >>> it is safe to increment it. The caller cannot free the object without
> > >>> violating the safety requirements.
> > >>>  
> > >>>> Another pitfall of `ARef`: it does not deallocate the memory when the
> > >>>> refcount reaches zero. People might expect that this code would not leak
> > >>>> memory:
> > >>>>
> > >>>>        let foo = Box::try_new(Foo::new())?;
> > >>>>        let foo = Box::leak(foo); // Leak the box, such that we do not
> > >>>>                                  // deallocate the memory too early.
> > >>>>        let foo = ARef::from(foo);
> > >>>>        drop(foo); // refcount is now zero, but the memory is never deallocated.  
> > >>>
> > >>> This is also in violation of the safety requirements of `AlwaysRefCounted`.  
> > >>
> > >> It seems I have misunderstood the term "always reference counted".
> > >> We should document this in more detail, since this places a lot of
> > >> constraints on the implementers:
> > >>
> > >>       Implementing `AlwaysRefCounted` for `T` places the following constraint on shared references `&T`:
> > >>       - Every `&T` points to memory that is not deallocated until the reference count reaches zero.
> > >>       - The existence of `&T` proves that the reference count is at least 1.  
> > >
> > > This is implied by the existing safety rules.  
> > 
> > This was not obvious to me at all, I think we should extend the docs and
> > make this very clear.
> > 
> > >>       This has some important consequences:
> > >>       - Exposing safe a way to get `T` is not allowed, since stack allocations are freed when the scope
> > >>         ends even though the reference count is non-zero.  
> > >
> > > Stack allocations are ok, as long as they wait for the refcount to
> > > drop to zero before the variable goes out of scope.  
> > 
> > "Exposing a **safe** way", you cannot under any circumstances allow safe
> > code access to by value `T` when it implements `AlwaysRefCounted`, since
> > safe code can create a `&T` without upholding the invariants.
> > 
> > In your controlled function, you can create `T`s on the stack if you want,
> > but giving them out to safe code is the problem.
> > 
> > >>       - Similarly giving safe access to `Box<T>` or other smart pointers is not allowed, since a `Box` can
> > >>         be freed independent from the reference count.  
> > >
> > > `ARef<T>` is a smart pointer and it is definitely allowed.  
> > 
> > Yes, I meant smart pointers except `ARef`. E.g. putting `T` inside of an
> > `Arc` has the same problem as `Box<T>`.
> > 
> > >
> > > Similarly to stack allocations I mention above, a `Box<T>`
> > > implementation is conceivable as long as it ensures that the
> > > allocation is freed only once the refcount reaches zero, for example,
> > > by having a drop implementation that performs such a wait. (IOW, when
> > > `Box<T>` goes out of scope, it always calls `drop` on `T` before
> > > actually freeing the memory, so this implementation could block until
> > > it is safe to do so, i.e., until the refcount reaches zero.)  
> > 
> > Is this something you want to do? Because to me this sounds like something
> > that could end up deadlocking very easily.
> > 
> > AIUI `AlwaysRefCounted` is intended for wrapper structs that are never
> > created on the Rust side. They are created and destroyed by C.
> 
> No.
> 
> It's perfectly legal for Rust code to implement this trait, and it
> might even be desirable in some cases, because it gives more control
> than relying on `Arc` and `Drop`.
> 
> For example, if a type is usable in RCU, then you might want to have
> some code like this:
> 
> impl AlwaysRefCounted for MyType {
>     fn inc_ref(&self) {
>         self.refcnt.fetch_add(1, Ordering::Relaxed);
>     }
> 
>     fn dec_ref(this: NonNull<Self>) {
>        let refcnt = this.as_ref().refcnt.fetch_sub(1, Ordering::Relaxed) - 1;
>        if refcnt == 0 {
>            // Unpublish the pointer from some RCU data structure
>            synchronize_rcu();
>            // proceed to drop the type and box
>        }
>     }
> }
> 
> The example given above can't rely on dtor because `drop` takes a
> mutable reference.
> 

How would you implement the `drop` of `MyType`? Will there be a
synchronize_rcu() there?

My understanding of Benno's point is that you won't never implement a
safe function that directly return `MyType` (maybe `ARef<MyType>`,
RCU<MyType>`, but not the type directly).

Regards,
Boqun

> > The scenario
> > of putting them into a `Box` or `Arc` should never happen, since Rust does
> > not have a way to create them.
> > 
> > As soon as this is not the only use case for this trait, I feel like this
> > trait becomes very dangerous, since there are many different ways for you
> > to mess up via normal coding patterns.
> > 
> > Hence I think it is better to spell out the dangerous patterns here and
> > forbid them, since the only use case should never use them anyway.
> > 
> > Also in the `Box` case, the same problem as with `&mut T` exists, since
> > you can derive a `&mut T` from it.
> > 
> > >  
> > >>       This type is intended to be implemented for C types that embedd a `refcount_t` and that are both
> > >>       created and destroyed by C. Static references also work with this type, since they stay live
> > >>       indefinitely.  
> > >
> > > Embedding a `refcount_t` is not a requirement. I already mention in
> > > the documentation that this is usually used for C structs and that
> > > Rust code should use `Arc`.  
> > 
> > I would prefer if we change the wording in the docs from `usually` to
> > `only`. If you agree with my interpretation above, then Rust types should
> > not implement this trait.
> > 
> > >  
> > >>       Implementers must also ensure that they never give out `&mut T`, since
> > >>       - it can be reborrowed as `&T`,
> > >>       - converted to `ARef<T>`,
> > >>       - which can yield a `&T` that is alive at the same time as the `&mut T`.  
> > >
> > > I agree with this. And I don't think this is a direct consequence of
> > > the safety requirements, so I think it makes sense to add something
> > > that covers this.
> > >  
> > >>>>> +
> > >>>>> +impl<T: AlwaysRefCounted> Drop for ARef<T> {
> > >>>>> +    fn drop(&mut self) {
> > >>>>> +        // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
> > >>>>> +        // decrement.
> > >>>>> +        unsafe { T::dec_ref(self.ptr) };
> > >>>>> +    }
> > >>>>> +}
> > >>>>> +
> > >>>>>     /// A sum type that always holds either a value of type `L` or `R`.
> > >>>>>     pub enum Either<L, R> {
> > >>>>>         /// Constructs an instance of [`Either`] containing a value of type `L`.
> > >>>>> --
> > >>>>> 2.34.1
> > >>>>>  
> > >>>>  
> > >>  
> > 
> 

[PATCH v4 09/13] rust: add basic `Task`

[Wedson Almeida Filho]

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

It is an abstraction for C's `struct task_struct`. It implements
`AlwaysRefCounted`, so the refcount of the wrapped object is managed
safely on the Rust side.

Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
v1 -> v2: No changes
v2 -> v3: Wrap task_struct with `Opaque` instead of `UnsafeCell`
v3 -> v4: No changes

 rust/bindings/bindings_helper.h |  1 +
 rust/helpers.c                  | 19 +++++++++
 rust/kernel/lib.rs              |  1 +
 rust/kernel/task.rs             | 75 +++++++++++++++++++++++++++++++++
 4 files changed, 96 insertions(+)
 create mode 100644 rust/kernel/task.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 75d85bd6c592..03656a44a83f 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -8,6 +8,7 @@
 
 #include <linux/slab.h>
 #include <linux/refcount.h>
+#include <linux/sched.h>
 
 /* `bindgen` gets confused at certain things. */
 const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
diff --git a/rust/helpers.c b/rust/helpers.c
index efd5b1e86f6e..f545923aedd8 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -24,6 +24,7 @@
 #include <linux/refcount.h>
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
+#include <linux/sched/signal.h>
 
 __noreturn void rust_helper_BUG(void)
 {
@@ -76,6 +77,12 @@ void rust_helper_spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
 }
 EXPORT_SYMBOL_GPL(rust_helper_spin_unlock_irqrestore);
 
+int rust_helper_signal_pending(struct task_struct *t)
+{
+	return signal_pending(t);
+}
+EXPORT_SYMBOL_GPL(rust_helper_signal_pending);
+
 refcount_t rust_helper_REFCOUNT_INIT(int n)
 {
 	return (refcount_t)REFCOUNT_INIT(n);
@@ -112,6 +119,18 @@ long rust_helper_PTR_ERR(__force const void *ptr)
 }
 EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR);
 
+void rust_helper_get_task_struct(struct task_struct *t)
+{
+	get_task_struct(t);
+}
+EXPORT_SYMBOL_GPL(rust_helper_get_task_struct);
+
+void rust_helper_put_task_struct(struct task_struct *t)
+{
+	put_task_struct(t);
+}
+EXPORT_SYMBOL_GPL(rust_helper_put_task_struct);
+
 /*
  * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
  * as the Rust `usize` type, so we can use it in contexts where Rust
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 2d7606135ef6..ee27e10da479 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -44,6 +44,7 @@ mod static_assert;
 pub mod std_vendor;
 pub mod str;
 pub mod sync;
+pub mod task;
 pub mod types;
 
 #[doc(hidden)]
diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
new file mode 100644
index 000000000000..d70cad131956
--- /dev/null
+++ b/rust/kernel/task.rs
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Tasks (threads and processes).
+//!
+//! C header: [`include/linux/sched.h`](../../../../include/linux/sched.h).
+
+use crate::{bindings, types::Opaque};
+use core::ptr;
+
+/// Wraps the kernel's `struct task_struct`.
+///
+/// # Invariants
+///
+/// All instances are valid tasks created by the C portion of the kernel.
+///
+/// Instances of this type are always ref-counted, that is, a call to `get_task_struct` ensures
+/// that the allocation remains valid at least until the matching call to `put_task_struct`.
+#[repr(transparent)]
+pub struct Task(pub(crate) Opaque<bindings::task_struct>);
+
+// SAFETY: It's OK to access `Task` through references from other threads because we're either
+// accessing properties that don't change (e.g., `pid`, `group_leader`) or that are properly
+// synchronised by C code (e.g., `signal_pending`).
+unsafe impl Sync for Task {}
+
+/// The type of process identifiers (PIDs).
+type Pid = bindings::pid_t;
+
+impl Task {
+    /// Returns the group leader of the given task.
+    pub fn group_leader(&self) -> &Task {
+        // SAFETY: By the type invariant, we know that `self.0` is a valid task. Valid tasks always
+        // have a valid group_leader.
+        let ptr = unsafe { *ptr::addr_of!((*self.0.get()).group_leader) };
+
+        // SAFETY: The lifetime of the returned task reference is tied to the lifetime of `self`,
+        // and given that a task has a reference to its group leader, we know it must be valid for
+        // the lifetime of the returned task reference.
+        unsafe { &*ptr.cast() }
+    }
+
+    /// Returns the PID of the given task.
+    pub fn pid(&self) -> Pid {
+        // SAFETY: By the type invariant, we know that `self.0` is a valid task. Valid tasks always
+        // have a valid pid.
+        unsafe { *ptr::addr_of!((*self.0.get()).pid) }
+    }
+
+    /// Determines whether the given task has pending signals.
+    pub fn signal_pending(&self) -> bool {
+        // SAFETY: By the type invariant, we know that `self.0` is valid.
+        unsafe { bindings::signal_pending(self.0.get()) != 0 }
+    }
+
+    /// Wakes up the task.
+    pub fn wake_up(&self) {
+        // SAFETY: By the type invariant, we know that `self.0.get()` is non-null and valid.
+        // And `wake_up_process` is safe to be called for any valid task, even if the task is
+        // running.
+        unsafe { bindings::wake_up_process(self.0.get()) };
+    }
+}
+
+// SAFETY: The type invariants guarantee that `Task` is always ref-counted.
+unsafe impl crate::types::AlwaysRefCounted for Task {
+    fn inc_ref(&self) {
+        // SAFETY: The existence of a shared reference means that the refcount is nonzero.
+        unsafe { bindings::get_task_struct(self.0.get()) };
+    }
+
+    unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
+        // SAFETY: The safety requirements guarantee that the refcount is nonzero.
+        unsafe { bindings::put_task_struct(obj.cast().as_ptr()) }
+    }
+}
-- 
2.34.1

Re: [PATCH v4 09/13] rust: add basic `Task`

[Gary Guo]

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

> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> It is an abstraction for C's `struct task_struct`. It implements
> `AlwaysRefCounted`, so the refcount of the wrapped object is managed
> safely on the Rust side.
> 
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
> v1 -> v2: No changes
> v2 -> v3: Wrap task_struct with `Opaque` instead of `UnsafeCell`
> v3 -> v4: No changes
> 
>  rust/bindings/bindings_helper.h |  1 +
>  rust/helpers.c                  | 19 +++++++++
>  rust/kernel/lib.rs              |  1 +
>  rust/kernel/task.rs             | 75 +++++++++++++++++++++++++++++++++
>  4 files changed, 96 insertions(+)
>  create mode 100644 rust/kernel/task.rs
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 75d85bd6c592..03656a44a83f 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -8,6 +8,7 @@
>  
>  #include <linux/slab.h>
>  #include <linux/refcount.h>
> +#include <linux/sched.h>
>  
>  /* `bindgen` gets confused at certain things. */
>  const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
> diff --git a/rust/helpers.c b/rust/helpers.c
> index efd5b1e86f6e..f545923aedd8 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -24,6 +24,7 @@
>  #include <linux/refcount.h>
>  #include <linux/mutex.h>
>  #include <linux/spinlock.h>
> +#include <linux/sched/signal.h>
>  
>  __noreturn void rust_helper_BUG(void)
>  {
> @@ -76,6 +77,12 @@ void rust_helper_spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_spin_unlock_irqrestore);
>  
> +int rust_helper_signal_pending(struct task_struct *t)
> +{
> +	return signal_pending(t);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_signal_pending);
> +
>  refcount_t rust_helper_REFCOUNT_INIT(int n)
>  {
>  	return (refcount_t)REFCOUNT_INIT(n);
> @@ -112,6 +119,18 @@ long rust_helper_PTR_ERR(__force const void *ptr)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR);
>  
> +void rust_helper_get_task_struct(struct task_struct *t)
> +{
> +	get_task_struct(t);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_get_task_struct);
> +
> +void rust_helper_put_task_struct(struct task_struct *t)
> +{
> +	put_task_struct(t);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_put_task_struct);
> +
>  /*
>   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
>   * as the Rust `usize` type, so we can use it in contexts where Rust
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 2d7606135ef6..ee27e10da479 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -44,6 +44,7 @@ mod static_assert;
>  pub mod std_vendor;
>  pub mod str;
>  pub mod sync;
> +pub mod task;
>  pub mod types;
>  
>  #[doc(hidden)]
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> new file mode 100644
> index 000000000000..d70cad131956
> --- /dev/null
> +++ b/rust/kernel/task.rs
> @@ -0,0 +1,75 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Tasks (threads and processes).
> +//!
> +//! C header: [`include/linux/sched.h`](../../../../include/linux/sched.h).
> +
> +use crate::{bindings, types::Opaque};
> +use core::ptr;
> +
> +/// Wraps the kernel's `struct task_struct`.
> +///
> +/// # Invariants
> +///
> +/// All instances are valid tasks created by the C portion of the kernel.
> +///
> +/// Instances of this type are always ref-counted, that is, a call to `get_task_struct` ensures
> +/// that the allocation remains valid at least until the matching call to `put_task_struct`.
> +#[repr(transparent)]
> +pub struct Task(pub(crate) Opaque<bindings::task_struct>);
> +
> +// SAFETY: It's OK to access `Task` through references from other threads because we're either
> +// accessing properties that don't change (e.g., `pid`, `group_leader`) or that are properly
> +// synchronised by C code (e.g., `signal_pending`).
> +unsafe impl Sync for Task {}
> +
> +/// The type of process identifiers (PIDs).
> +type Pid = bindings::pid_t;

This'll just end up being `i32`. I think it'll be more desirable that
this is defined as a new type.

> +
> +impl Task {
> +    /// Returns the group leader of the given task.
> +    pub fn group_leader(&self) -> &Task {
> +        // SAFETY: By the type invariant, we know that `self.0` is a valid task. Valid tasks always
> +        // have a valid group_leader.
> +        let ptr = unsafe { *ptr::addr_of!((*self.0.get()).group_leader) };
> +
> +        // SAFETY: The lifetime of the returned task reference is tied to the lifetime of `self`,
> +        // and given that a task has a reference to its group leader, we know it must be valid for
> +        // the lifetime of the returned task reference.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    /// Returns the PID of the given task.
> +    pub fn pid(&self) -> Pid {
> +        // SAFETY: By the type invariant, we know that `self.0` is a valid task. Valid tasks always
> +        // have a valid pid.
> +        unsafe { *ptr::addr_of!((*self.0.get()).pid) }
> +    }
> +
> +    /// Determines whether the given task has pending signals.
> +    pub fn signal_pending(&self) -> bool {
> +        // SAFETY: By the type invariant, we know that `self.0` is valid.
> +        unsafe { bindings::signal_pending(self.0.get()) != 0 }
> +    }
> +
> +    /// Wakes up the task.
> +    pub fn wake_up(&self) {
> +        // SAFETY: By the type invariant, we know that `self.0.get()` is non-null and valid.
> +        // And `wake_up_process` is safe to be called for any valid task, even if the task is
> +        // running.
> +        unsafe { bindings::wake_up_process(self.0.get()) };
> +    }
> +}
> +
> +// SAFETY: The type invariants guarantee that `Task` is always ref-counted.
> +unsafe impl crate::types::AlwaysRefCounted for Task {
> +    fn inc_ref(&self) {
> +        // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> +        unsafe { bindings::get_task_struct(self.0.get()) };
> +    }
> +
> +    unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> +        // SAFETY: The safety requirements guarantee that the refcount is nonzero.
> +        unsafe { bindings::put_task_struct(obj.cast().as_ptr()) }
> +    }
> +}

Re: [PATCH v4 09/13] rust: add basic `Task`

[Wedson Almeida Filho]

On Tue, 11 Apr 2023 at 17:47, Gary Guo <gary@garyguo.net> wrote:
>
> On Tue, 11 Apr 2023 02:45:39 -0300
> Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>
> > From: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> > It is an abstraction for C's `struct task_struct`. It implements
> > `AlwaysRefCounted`, so the refcount of the wrapped object is managed
> > safely on the Rust side.
> >
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> > ---
> > v1 -> v2: No changes
> > v2 -> v3: Wrap task_struct with `Opaque` instead of `UnsafeCell`
> > v3 -> v4: No changes
> >
> >  rust/bindings/bindings_helper.h |  1 +
> >  rust/helpers.c                  | 19 +++++++++
> >  rust/kernel/lib.rs              |  1 +
> >  rust/kernel/task.rs             | 75 +++++++++++++++++++++++++++++++++
> >  4 files changed, 96 insertions(+)
> >  create mode 100644 rust/kernel/task.rs
> >
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index 75d85bd6c592..03656a44a83f 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -8,6 +8,7 @@
> >
> >  #include <linux/slab.h>
> >  #include <linux/refcount.h>
> > +#include <linux/sched.h>
> >
> >  /* `bindgen` gets confused at certain things. */
> >  const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
> > diff --git a/rust/helpers.c b/rust/helpers.c
> > index efd5b1e86f6e..f545923aedd8 100644
> > --- a/rust/helpers.c
> > +++ b/rust/helpers.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/refcount.h>
> >  #include <linux/mutex.h>
> >  #include <linux/spinlock.h>
> > +#include <linux/sched/signal.h>
> >
> >  __noreturn void rust_helper_BUG(void)
> >  {
> > @@ -76,6 +77,12 @@ void rust_helper_spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
> >  }
> >  EXPORT_SYMBOL_GPL(rust_helper_spin_unlock_irqrestore);
> >
> > +int rust_helper_signal_pending(struct task_struct *t)
> > +{
> > +     return signal_pending(t);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_signal_pending);
> > +
> >  refcount_t rust_helper_REFCOUNT_INIT(int n)
> >  {
> >       return (refcount_t)REFCOUNT_INIT(n);
> > @@ -112,6 +119,18 @@ long rust_helper_PTR_ERR(__force const void *ptr)
> >  }
> >  EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR);
> >
> > +void rust_helper_get_task_struct(struct task_struct *t)
> > +{
> > +     get_task_struct(t);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_get_task_struct);
> > +
> > +void rust_helper_put_task_struct(struct task_struct *t)
> > +{
> > +     put_task_struct(t);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_put_task_struct);
> > +
> >  /*
> >   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
> >   * as the Rust `usize` type, so we can use it in contexts where Rust
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 2d7606135ef6..ee27e10da479 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -44,6 +44,7 @@ mod static_assert;
> >  pub mod std_vendor;
> >  pub mod str;
> >  pub mod sync;
> > +pub mod task;
> >  pub mod types;
> >
> >  #[doc(hidden)]
> > diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> > new file mode 100644
> > index 000000000000..d70cad131956
> > --- /dev/null
> > +++ b/rust/kernel/task.rs
> > @@ -0,0 +1,75 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Tasks (threads and processes).
> > +//!
> > +//! C header: [`include/linux/sched.h`](../../../../include/linux/sched.h).
> > +
> > +use crate::{bindings, types::Opaque};
> > +use core::ptr;
> > +
> > +/// Wraps the kernel's `struct task_struct`.
> > +///
> > +/// # Invariants
> > +///
> > +/// All instances are valid tasks created by the C portion of the kernel.
> > +///
> > +/// Instances of this type are always ref-counted, that is, a call to `get_task_struct` ensures
> > +/// that the allocation remains valid at least until the matching call to `put_task_struct`.
> > +#[repr(transparent)]
> > +pub struct Task(pub(crate) Opaque<bindings::task_struct>);
> > +
> > +// SAFETY: It's OK to access `Task` through references from other threads because we're either
> > +// accessing properties that don't change (e.g., `pid`, `group_leader`) or that are properly
> > +// synchronised by C code (e.g., `signal_pending`).
> > +unsafe impl Sync for Task {}
> > +
> > +/// The type of process identifiers (PIDs).
> > +type Pid = bindings::pid_t;
>
> This'll just end up being `i32`. I think it'll be more desirable that
> this is defined as a new type.

We may very well decide that we want to make this a new type.

But that should be done in a different patch.

> > +
> > +impl Task {
> > +    /// Returns the group leader of the given task.
> > +    pub fn group_leader(&self) -> &Task {
> > +        // SAFETY: By the type invariant, we know that `self.0` is a valid task. Valid tasks always
> > +        // have a valid group_leader.
> > +        let ptr = unsafe { *ptr::addr_of!((*self.0.get()).group_leader) };
> > +
> > +        // SAFETY: The lifetime of the returned task reference is tied to the lifetime of `self`,
> > +        // and given that a task has a reference to its group leader, we know it must be valid for
> > +        // the lifetime of the returned task reference.
> > +        unsafe { &*ptr.cast() }
> > +    }
> > +
> > +    /// Returns the PID of the given task.
> > +    pub fn pid(&self) -> Pid {
> > +        // SAFETY: By the type invariant, we know that `self.0` is a valid task. Valid tasks always
> > +        // have a valid pid.
> > +        unsafe { *ptr::addr_of!((*self.0.get()).pid) }
> > +    }
> > +
> > +    /// Determines whether the given task has pending signals.
> > +    pub fn signal_pending(&self) -> bool {
> > +        // SAFETY: By the type invariant, we know that `self.0` is valid.
> > +        unsafe { bindings::signal_pending(self.0.get()) != 0 }
> > +    }
> > +
> > +    /// Wakes up the task.
> > +    pub fn wake_up(&self) {
> > +        // SAFETY: By the type invariant, we know that `self.0.get()` is non-null and valid.
> > +        // And `wake_up_process` is safe to be called for any valid task, even if the task is
> > +        // running.
> > +        unsafe { bindings::wake_up_process(self.0.get()) };
> > +    }
> > +}
> > +
> > +// SAFETY: The type invariants guarantee that `Task` is always ref-counted.
> > +unsafe impl crate::types::AlwaysRefCounted for Task {
> > +    fn inc_ref(&self) {
> > +        // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> > +        unsafe { bindings::get_task_struct(self.0.get()) };
> > +    }
> > +
> > +    unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> > +        // SAFETY: The safety requirements guarantee that the refcount is nonzero.
> > +        unsafe { bindings::put_task_struct(obj.cast().as_ptr()) }
> > +    }
> > +}
>

[PATCH v4 10/13] rust: introduce `current`

[Wedson Almeida Filho]

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

This allows Rust code to get a reference to the current task without
having to increment the refcount, but still guaranteeing memory safety.

Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
v1 -> v2: Make `current` a macro to prevent it from escaping the caller
v2 -> v3:
- Mention `current` macro in `Task::current`
- Hide implementation of `TaskRef` inside `Task::current`
v3 -> v4: Remove impl of `From<TaskRef<'_>>` for `ARef<Task>`

 rust/helpers.c         |  6 ++++
 rust/kernel/prelude.rs |  2 ++
 rust/kernel/task.rs    | 82 +++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/rust/helpers.c b/rust/helpers.c
index f545923aedd8..fba3c62a77f1 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -119,6 +119,12 @@ long rust_helper_PTR_ERR(__force const void *ptr)
 }
 EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR);
 
+struct task_struct *rust_helper_get_current(void)
+{
+	return current;
+}
+EXPORT_SYMBOL_GPL(rust_helper_get_current);
+
 void rust_helper_get_task_struct(struct task_struct *t)
 {
 	get_task_struct(t);
diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
index fcdc511d2ce8..c28587d68ebc 100644
--- a/rust/kernel/prelude.rs
+++ b/rust/kernel/prelude.rs
@@ -36,3 +36,5 @@ pub use super::error::{code::*, Error, Result};
 pub use super::{str::CStr, ThisModule};
 
 pub use super::init::{InPlaceInit, Init, PinInit};
+
+pub use super::current;
diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index d70cad131956..526d29a0ae27 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -5,7 +5,17 @@
 //! C header: [`include/linux/sched.h`](../../../../include/linux/sched.h).
 
 use crate::{bindings, types::Opaque};
-use core::ptr;
+use core::{marker::PhantomData, ops::Deref, ptr};
+
+/// Returns the currently running task.
+#[macro_export]
+macro_rules! current {
+    () => {
+        // SAFETY: Deref + addr-of below create a temporary `TaskRef` that cannot outlive the
+        // caller.
+        unsafe { &*$crate::task::Task::current() }
+    };
+}
 
 /// Wraps the kernel's `struct task_struct`.
 ///
@@ -15,6 +25,42 @@ use core::ptr;
 ///
 /// Instances of this type are always ref-counted, that is, a call to `get_task_struct` ensures
 /// that the allocation remains valid at least until the matching call to `put_task_struct`.
+///
+/// # Examples
+///
+/// The following is an example of getting the PID of the current thread with zero additional cost
+/// when compared to the C version:
+///
+/// ```
+/// let pid = current!().pid();
+/// ```
+///
+/// Getting the PID of the current process, also zero additional cost:
+///
+/// ```
+/// let pid = current!().group_leader().pid();
+/// ```
+///
+/// Getting the current task and storing it in some struct. The reference count is automatically
+/// incremented when creating `State` and decremented when it is dropped:
+///
+/// ```
+/// use kernel::{task::Task, types::ARef};
+///
+/// struct State {
+///     creator: ARef<Task>,
+///     index: u32,
+/// }
+///
+/// impl State {
+///     fn new() -> Self {
+///         Self {
+///             creator: current!().into(),
+///             index: 0,
+///         }
+///     }
+/// }
+/// ```
 #[repr(transparent)]
 pub struct Task(pub(crate) Opaque<bindings::task_struct>);
 
@@ -27,6 +73,40 @@ unsafe impl Sync for Task {}
 type Pid = bindings::pid_t;
 
 impl Task {
+    /// Returns a task reference for the currently executing task/thread.
+    ///
+    /// The recommended way to get the current task/thread is to use the
+    /// [`current`](crate::current) macro because it is safe.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that the returned object doesn't outlive the current task/thread.
+    pub unsafe fn current() -> impl Deref<Target = Task> {
+        struct TaskRef<'a> {
+            task: &'a Task,
+            _not_send: PhantomData<*mut ()>,
+        }
+
+        impl Deref for TaskRef<'_> {
+            type Target = Task;
+
+            fn deref(&self) -> &Self::Target {
+                self.task
+            }
+        }
+
+        // SAFETY: Just an FFI call with no additional safety requirements.
+        let ptr = unsafe { bindings::get_current() };
+
+        TaskRef {
+            // SAFETY: If the current thread is still running, the current task is valid. Given
+            // that `TaskRef` is not `Send`, we know it cannot be transferred to another thread
+            // (where it could potentially outlive the caller).
+            task: unsafe { &*ptr.cast() },
+            _not_send: PhantomData,
+        }
+    }
+
     /// Returns the group leader of the given task.
     pub fn group_leader(&self) -> &Task {
         // SAFETY: By the type invariant, we know that `self.0` is a valid task. Valid tasks always
-- 
2.34.1

[PATCH v4 11/13] rust: lock: add `Guard::do_unlocked`

[Wedson Almeida Filho]

From: Wedson Almeida Filho <walmeida@microsoft.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@gmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.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) };
+    }
+}
+
 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.
-- 
2.34.1

Re: [PATCH v4 11/13] rust: lock: add `Guard::do_unlocked`

[Gary Guo]

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

> From: Wedson Almeida Filho <walmeida@microsoft.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@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.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).

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

Re: [PATCH v4 11/13] rust: lock: add `Guard::do_unlocked`

[Wedson Almeida Filho]

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

Re: [PATCH v4 11/13] rust: lock: add `Guard::do_unlocked`

[Boqun Feng]

On Tue, Apr 11, 2023 at 02:45:41AM -0300, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.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@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.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

I think you mean

"Callers must ensure that `guard_state` comes ..."

, right?

Regards,
Boqun

> +    /// 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) };
> +    }
> +}
> +
>  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.
> -- 
> 2.34.1
> 

Re: [PATCH v4 11/13] rust: lock: add `Guard::do_unlocked`

[Wedson Almeida Filho]

On Tue, 11 Apr 2023 at 18:18, Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Tue, Apr 11, 2023 at 02:45:41AM -0300, Wedson Almeida Filho wrote:
> > From: Wedson Almeida Filho <walmeida@microsoft.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@gmail.com>
> > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.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
>
> I think you mean
>
> "Callers must ensure that `guard_state` comes ..."
>
> , right?

That's right, thanks for spotting this!

I renamed the parameter names (based on your feedback :)) but didn't
update the comment accordingly. Will do.

> Regards,
> Boqun
>
> > +    /// 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) };
> > +    }
> > +}
> > +
> >  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.
> > --
> > 2.34.1
> >

Re: [PATCH v4 11/13] rust: lock: add `Guard::do_unlocked`

[Boqun Feng]

On Tue, Apr 11, 2023 at 02:45:41AM -0300, Wedson Almeida Filho wrote:
[...]
> +
> +    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) },
> +        };
> +    }
>  }
>  

One thing I'm little worried about the above is that we don't store back
the new GuardState into `guard_state`, the particular case I'm worried
about is as follow:

	// IRQ is enabled.
	// Disabling IRQ
	unsafe { bindings::local_irq_disable(); }

	let mut g = unsafe { SpinLockBackend::lock(&mut lock as *mut _) };
	// `g` records irq state is "irq disabled"

	unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
	// restore into "irq disabled" mode.
	// IRQ is disabled.

	// Enabling IRQ
	unsafe { bindings::local_irq_enable(); }
	// IRQ is enabled.

	unsafe { SpinLockBackend::relock(&mut lock as *mut _, &mut g) }
	// `g` still records irq state is "irq disabled"

	unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
	// restore into "irq disabled" mode.
	// IRQ is disabled.


This looks pretty scary to me, I would expect `relock()` updates the
latest GuardState to the guard. Any reason it's implemented this way?

Regards,
Boqun

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

Re: [PATCH v4 11/13] rust: lock: add `Guard::do_unlocked`

[Wedson Almeida Filho]

On Wed, 12 Apr 2023 at 03:25, Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Tue, Apr 11, 2023 at 02:45:41AM -0300, Wedson Almeida Filho wrote:
> [...]
> > +
> > +    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) },
> > +        };
> > +    }
> >  }
> >
>
> One thing I'm little worried about the above is that we don't store back
> the new GuardState into `guard_state`, the particular case I'm worried
> about is as follow:
>
>         // IRQ is enabled.
>         // Disabling IRQ
>         unsafe { bindings::local_irq_disable(); }
>
>         let mut g = unsafe { SpinLockBackend::lock(&mut lock as *mut _) };
>         // `g` records irq state is "irq disabled"
>
>         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
>         // restore into "irq disabled" mode.
>         // IRQ is disabled.
>
>         // Enabling IRQ
>         unsafe { bindings::local_irq_enable(); }
>         // IRQ is enabled.
>
>         unsafe { SpinLockBackend::relock(&mut lock as *mut _, &mut g) }
>         // `g` still records irq state is "irq disabled"

Yes, that's by design. If you want it to record the new "irq enabled"
state, then you should call `lock()`, not `relock()`.

>         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
>         // restore into "irq disabled" mode.
>         // IRQ is disabled.
>
>
> This looks pretty scary to me, I would expect `relock()` updates the
> latest GuardState to the guard. Any reason it's implemented this way?

A `relock()` followed by an `unlock()` takes the state back to how it
was when `lock()` was originally called: this is precisely why
`relock()` exists.

Consider the following case:

```
local_disable_irq();
let mut guard = spinlock.lock();

guard.do_unlocked(|| {
    local_irq_enable();
    schedule();
});

drop(guard);
```

What would you expect the state to be? It's meant to be the state
right before `spinlock.lock()` was called, that's what the guard
represents.

If you want to preserve a new state, then you don't want `relock()`,
you just want a new `lock()` call.

> Regards,
> Boqun
>
> >  // 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.
> > --
> > 2.34.1
> >

Re: [PATCH v4 11/13] rust: lock: add `Guard::do_unlocked`

[Boqun Feng]

On Wed, Apr 12, 2023 at 08:07:40AM -0300, Wedson Almeida Filho wrote:
> On Wed, 12 Apr 2023 at 03:25, Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Tue, Apr 11, 2023 at 02:45:41AM -0300, Wedson Almeida Filho wrote:
> > [...]
> > > +
> > > +    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) },
> > > +        };
> > > +    }
> > >  }
> > >
> >
> > One thing I'm little worried about the above is that we don't store back
> > the new GuardState into `guard_state`, the particular case I'm worried
> > about is as follow:
> >
> >         // IRQ is enabled.
> >         // Disabling IRQ
> >         unsafe { bindings::local_irq_disable(); }
> >
> >         let mut g = unsafe { SpinLockBackend::lock(&mut lock as *mut _) };
> >         // `g` records irq state is "irq disabled"
> >
> >         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
> >         // restore into "irq disabled" mode.
> >         // IRQ is disabled.
> >
> >         // Enabling IRQ
> >         unsafe { bindings::local_irq_enable(); }
> >         // IRQ is enabled.
> >
> >         unsafe { SpinLockBackend::relock(&mut lock as *mut _, &mut g) }
> >         // `g` still records irq state is "irq disabled"
> 
> Yes, that's by design. If you want it to record the new "irq enabled"
> state, then you should call `lock()`, not `relock()`.
> 
> >         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
> >         // restore into "irq disabled" mode.
> >         // IRQ is disabled.
> >
> >
> > This looks pretty scary to me, I would expect `relock()` updates the
> > latest GuardState to the guard. Any reason it's implemented this way?
> 
> A `relock()` followed by an `unlock()` takes the state back to how it
> was when `lock()` was originally called: this is precisely why
> `relock()` exists.
> 
> Consider the following case:
> 
> ```
> local_disable_irq();
> let mut guard = spinlock.lock();

I think you meant `spinlock.lock_irqsave()` here, right?

> 
> guard.do_unlocked(|| {
>     local_irq_enable();
>     schedule();
> });
> 
> drop(guard);
> ```
> 
> What would you expect the state to be? It's meant to be the state

I understand your point but I would expect people to code like:

```
local_disable_irq();
let mut guard = spinlock.lock(); // or lock_irqsave(), doesn't matter

guard.do_unlocked(|| {
    local_irq_enable();
    schedule();
    local_irq_disable();
});

drop(guard);
```

And the closure in do_unlocked() can also be something like:
```
	guard.do_unlocked(|| {
	    local_irq_enabled();
	    let _g = ScopeGuard::new(|| {
	        local_irq_disabled();
	    });

	    schedule();

	    if (some_cond) {
	    	return; // return early
	    }

	    if (other_cond) {
	    	return;
	    }
	})

```

One benefit (other that code looks symmetric) is we can use the same
closure in other place. Also it helps klint since we keep the irq
enablement state change as local as possible: we can go ahead and
require irq enabled state should not be changed between the closure in
do_unlock().

Maybe I'm missing something, but the current `relock` semantics is
really tricky to get ;-)

Regards,
Boqun

> right before `spinlock.lock()` was called, that's what the guard
> represents.
> 
> If you want to preserve a new state, then you don't want `relock()`,
> you just want a new `lock()` call.
> 
> > Regards,
> > Boqun
> >
> > >  // 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.
> > > --
> > > 2.34.1
> > >

Re: [PATCH v4 11/13] rust: lock: add `Guard::do_unlocked`

[Wedson Almeida Filho]

On Wed, 12 Apr 2023 at 11:35, Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Wed, Apr 12, 2023 at 08:07:40AM -0300, Wedson Almeida Filho wrote:
> > On Wed, 12 Apr 2023 at 03:25, Boqun Feng <boqun.feng@gmail.com> wrote:
> > >
> > > On Tue, Apr 11, 2023 at 02:45:41AM -0300, Wedson Almeida Filho wrote:
> > > [...]
> > > > +
> > > > +    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) },
> > > > +        };
> > > > +    }
> > > >  }
> > > >
> > >
> > > One thing I'm little worried about the above is that we don't store back
> > > the new GuardState into `guard_state`, the particular case I'm worried
> > > about is as follow:
> > >
> > >         // IRQ is enabled.
> > >         // Disabling IRQ
> > >         unsafe { bindings::local_irq_disable(); }
> > >
> > >         let mut g = unsafe { SpinLockBackend::lock(&mut lock as *mut _) };
> > >         // `g` records irq state is "irq disabled"
> > >
> > >         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
> > >         // restore into "irq disabled" mode.
> > >         // IRQ is disabled.
> > >
> > >         // Enabling IRQ
> > >         unsafe { bindings::local_irq_enable(); }
> > >         // IRQ is enabled.
> > >
> > >         unsafe { SpinLockBackend::relock(&mut lock as *mut _, &mut g) }
> > >         // `g` still records irq state is "irq disabled"
> >
> > Yes, that's by design. If you want it to record the new "irq enabled"
> > state, then you should call `lock()`, not `relock()`.
> >
> > >         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
> > >         // restore into "irq disabled" mode.
> > >         // IRQ is disabled.
> > >
> > >
> > > This looks pretty scary to me, I would expect `relock()` updates the
> > > latest GuardState to the guard. Any reason it's implemented this way?
> >
> > A `relock()` followed by an `unlock()` takes the state back to how it
> > was when `lock()` was originally called: this is precisely why
> > `relock()` exists.
> >
> > Consider the following case:
> >
> > ```
> > local_disable_irq();
> > let mut guard = spinlock.lock();
>
> I think you meant `spinlock.lock_irqsave()` here, right?

Yes, sorry, I meant `lock_irqsave()`.

> >
> > guard.do_unlocked(|| {
> >     local_irq_enable();
> >     schedule();
> > });
> >
> > drop(guard);
> > ```
> >
> > What would you expect the state to be? It's meant to be the state
>
> I understand your point but I would expect people to code like:
>
> ```
> local_disable_irq();
> let mut guard = spinlock.lock(); // or lock_irqsave(), doesn't matter
>
> guard.do_unlocked(|| {
>     local_irq_enable();
>     schedule();
>     local_irq_disable();
> });
>
> drop(guard);
> ```

Well, `relock` works with the code above as well.

> And the closure in do_unlocked() can also be something like:
> ```
>         guard.do_unlocked(|| {
>             local_irq_enabled();
>             let _g = ScopeGuard::new(|| {
>                 local_irq_disabled();
>             });
>
>             schedule();
>
>             if (some_cond) {
>                 return; // return early
>             }
>
>             if (other_cond) {
>                 return;
>             }
>         })
>
> ```
>
> One benefit (other that code looks symmetric) is we can use the same
> closure in other place. Also it helps klint since we keep the irq
> enablement state change as local as possible: we can go ahead and
> require irq enabled state should not be changed between the closure in
> do_unlock().

Note that the only user of `do_unlocked` at the moment works for any
type of lock, including mutexes, so we can't really have this kind of
code there. All irq handling needs to happen on the backends.

> Maybe I'm missing something, but the current `relock` semantics is
> really tricky to get ;-)

It seems straightforward to me: reacquire the lock in the same mode as
the original lock() call (e.g., lock() vs lock_irqsave()) such that
the `unlock()` will restore any state it manages to what it was right
before the original locking call.

But callers are not going to deal with these unsafe backend functions
directly, they'll deal with guards, so the high-level requirement that
`relock()` enforces is the following, given something like:

```
// 1
let guard = spinlock.lock_irqsave();
// Some code, which may include calls to a condvar, and may change the
irq state.
drop(guard);
// 2
```

At 2, the irq state must be the same as in 1, that's why one would use
the `lock_irqsave` variant.

This is a common pattern (seen in a bunch of unrelated places): save
the current state, make changes to the state, and eventually restore
the saved state (independently of what changes were made between
saving and restoring).

> Regards,
> Boqun
>
> > right before `spinlock.lock()` was called, that's what the guard
> > represents.
> >
> > If you want to preserve a new state, then you don't want `relock()`,
> > you just want a new `lock()` call.
> >
> > > Regards,
> > > Boqun
> > >
> > > >  // 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.
> > > > --
> > > > 2.34.1
> > > >

[PATCH v4 12/13] rust: sync: introduce `CondVar`

[Wedson Almeida Filho]

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

This is the traditional condition variable or monitor synchronisation
primitive. It is implemented with C's `wait_queue_head_t`.

It allows users to release a lock and go to sleep while guaranteeing
that notifications won't be missed. This is achieved by enqueuing a wait
entry before releasing the lock.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Will Deacon <will@kernel.org>
Cc: Waiman Long <longman@redhat.com>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
v1 -> v2: Use the new `current` macro
v2 -> v3: Use new Opaque::ffi_init from Benno's series
v3 -> v4: Fixed typo in example in documentation

 rust/bindings/bindings_helper.h |   1 +
 rust/helpers.c                  |   7 ++
 rust/kernel/sync.rs             |   2 +
 rust/kernel/sync/condvar.rs     | 174 ++++++++++++++++++++++++++++++++
 rust/kernel/sync/lock.rs        |   1 -
 5 files changed, 184 insertions(+), 1 deletion(-)
 create mode 100644 rust/kernel/sync/condvar.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 03656a44a83f..50e7a76d5455 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -8,6 +8,7 @@
 
 #include <linux/slab.h>
 #include <linux/refcount.h>
+#include <linux/wait.h>
 #include <linux/sched.h>
 
 /* `bindgen` gets confused at certain things. */
diff --git a/rust/helpers.c b/rust/helpers.c
index fba3c62a77f1..2512252f621e 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -25,6 +25,7 @@
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
 #include <linux/sched/signal.h>
+#include <linux/wait.h>
 
 __noreturn void rust_helper_BUG(void)
 {
@@ -77,6 +78,12 @@ void rust_helper_spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
 }
 EXPORT_SYMBOL_GPL(rust_helper_spin_unlock_irqrestore);
 
+void rust_helper_init_wait(struct wait_queue_entry *wq_entry)
+{
+	init_wait(wq_entry);
+}
+EXPORT_SYMBOL_GPL(rust_helper_init_wait);
+
 int rust_helper_signal_pending(struct task_struct *t)
 {
 	return signal_pending(t);
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index c997ff7e951e..431402180aa8 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -8,9 +8,11 @@
 use crate::types::Opaque;
 
 mod arc;
+mod condvar;
 pub mod lock;
 
 pub use arc::{Arc, ArcBorrow, UniqueArc};
+pub use condvar::CondVar;
 pub use lock::{mutex::Mutex, spinlock::SpinLock};
 
 /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
new file mode 100644
index 000000000000..ed353399c4e5
--- /dev/null
+++ b/rust/kernel/sync/condvar.rs
@@ -0,0 +1,174 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! A condition variable.
+//!
+//! This module allows Rust code to use the kernel's [`struct wait_queue_head`] as a condition
+//! variable.
+
+use super::{lock::Backend, lock::Guard, LockClassKey};
+use crate::{bindings, init::PinInit, pin_init, str::CStr, types::Opaque};
+use core::marker::PhantomPinned;
+use macros::pin_data;
+
+/// Creates a [`CondVar`] initialiser with the given name and a newly-created lock class.
+#[macro_export]
+macro_rules! new_condvar {
+    ($($name:literal)?) => {
+        $crate::sync::CondVar::new($crate::optional_name!($($name)?), $crate::static_lock_class!())
+    };
+}
+
+/// A conditional variable.
+///
+/// Exposes the kernel's [`struct wait_queue_head`] as a condition variable. It allows the caller to
+/// atomically release the given lock and go to sleep. It reacquires the lock when it wakes up. And
+/// it wakes up when notified by another thread (via [`CondVar::notify_one`] or
+/// [`CondVar::notify_all`]) or because the thread received a signal. It may also wake up
+/// spuriously.
+///
+/// Instances of [`CondVar`] need a lock class and to be pinned. The recommended way to create such
+/// instances is with the [`pin_init`](crate::pin_init) and [`new_condvar`] macros.
+///
+/// # Examples
+///
+/// The following is an example of using a condvar with a mutex:
+///
+/// ```
+/// use kernel::sync::{CondVar, Mutex};
+/// use kernel::{new_condvar, new_mutex};
+///
+/// #[pin_data]
+/// pub struct Example {
+///     #[pin]
+///     value: Mutex<u32>,
+///
+///     #[pin]
+///     value_changed: CondVar,
+/// }
+///
+/// /// Waits for `e.value` to become `v`.
+/// fn wait_for_value(e: &Example, v: u32) {
+///     let mut guard = e.value.lock();
+///     while *guard != v {
+///         e.value_changed.wait_uninterruptible(&mut guard);
+///     }
+/// }
+///
+/// /// Increments `e.value` and notifies all potential waiters.
+/// fn increment(e: &Example) {
+///     *e.value.lock() += 1;
+///     e.value_changed.notify_all();
+/// }
+///
+/// /// Allocates a new boxed `Example`.
+/// fn new_example() -> Result<Pin<Box<Example>>> {
+///     Box::pin_init(pin_init!(Example {
+///         value <- new_mutex!(0),
+///         value_changed <- new_condvar!(),
+///     }))
+/// }
+/// ```
+///
+/// [`struct wait_queue_head`]: ../../../include/linux/wait.h
+#[pin_data]
+pub struct CondVar {
+    #[pin]
+    pub(crate) wait_list: Opaque<bindings::wait_queue_head>,
+
+    /// A condvar needs to be pinned because it contains a [`struct list_head`] that is
+    /// self-referential, so it cannot be safely moved once it is initialised.
+    #[pin]
+    _pin: PhantomPinned,
+}
+
+// SAFETY: `CondVar` only uses a `struct wait_queue_head`, which is safe to use on any thread.
+#[allow(clippy::non_send_fields_in_send_ty)]
+unsafe impl Send for CondVar {}
+
+// SAFETY: `CondVar` only uses a `struct wait_queue_head`, which is safe to use on multiple threads
+// concurrently.
+unsafe impl Sync for CondVar {}
+
+impl CondVar {
+    /// Constructs a new condvar initialiser.
+    #[allow(clippy::new_ret_no_self)]
+    pub fn new(name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> {
+        pin_init!(Self {
+            _pin: PhantomPinned,
+            // SAFETY: `slot` is valid while the closure is called and both `name` and `key` have
+            // static lifetimes so they live indefinitely.
+            wait_list <- Opaque::ffi_init(|slot| unsafe {
+                bindings::__init_waitqueue_head(slot, name.as_char_ptr(), key.as_ptr())
+            }),
+        })
+    }
+
+    fn wait_internal<T: ?Sized, B: Backend>(&self, wait_state: u32, guard: &mut Guard<'_, T, B>) {
+        let wait = Opaque::<bindings::wait_queue_entry>::uninit();
+
+        // SAFETY: `wait` points to valid memory.
+        unsafe { bindings::init_wait(wait.get()) };
+
+        // SAFETY: Both `wait` and `wait_list` point to valid memory.
+        unsafe {
+            bindings::prepare_to_wait_exclusive(self.wait_list.get(), wait.get(), wait_state as _)
+        };
+
+        // SAFETY: No arguments, switches to another thread.
+        guard.do_unlocked(|| unsafe { bindings::schedule() });
+
+        // SAFETY: Both `wait` and `wait_list` point to valid memory.
+        unsafe { bindings::finish_wait(self.wait_list.get(), wait.get()) };
+    }
+
+    /// Releases the lock and waits for a notification in interruptible mode.
+    ///
+    /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the
+    /// thread to sleep, reacquiring the lock on wake up. It wakes up when notified by
+    /// [`CondVar::notify_one`] or [`CondVar::notify_all`], or when the thread receives a signal.
+    /// It may also wake up spuriously.
+    ///
+    /// Returns whether there is a signal pending.
+    #[must_use = "wait returns if a signal is pending, so the caller must check the return value"]
+    pub fn wait<T: ?Sized, B: Backend>(&self, guard: &mut Guard<'_, T, B>) -> bool {
+        self.wait_internal(bindings::TASK_INTERRUPTIBLE, guard);
+        crate::current!().signal_pending()
+    }
+
+    /// Releases the lock and waits for a notification in uninterruptible mode.
+    ///
+    /// Similar to [`CondVar::wait`], except that the wait is not interruptible. That is, the
+    /// thread won't wake up due to signals. It may, however, wake up supirously.
+    pub fn wait_uninterruptible<T: ?Sized, B: Backend>(&self, guard: &mut Guard<'_, T, B>) {
+        self.wait_internal(bindings::TASK_UNINTERRUPTIBLE, guard)
+    }
+
+    /// Calls the kernel function to notify the appropriate number of threads with the given flags.
+    fn notify(&self, count: i32, flags: u32) {
+        // SAFETY: `wait_list` points to valid memory.
+        unsafe {
+            bindings::__wake_up(
+                self.wait_list.get(),
+                bindings::TASK_NORMAL,
+                count,
+                flags as _,
+            )
+        };
+    }
+
+    /// Wakes a single waiter up, if any.
+    ///
+    /// This is not 'sticky' in the sense that if no thread is waiting, the notification is lost
+    /// completely (as opposed to automatically waking up the next waiter).
+    pub fn notify_one(&self) {
+        self.notify(1, 0);
+    }
+
+    /// Wakes all waiters up, if any.
+    ///
+    /// This is not 'sticky' in the sense that if no thread is waiting, the notification is lost
+    /// completely (as opposed to automatically waking up the next waiter).
+    pub fn notify_all(&self) {
+        self.notify(0, 0);
+    }
+}
diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index cde57756795f..816c5d3bba12 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -179,7 +179,6 @@ pub struct Guard<'a, T: ?Sized, B: Backend> {
 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) };
-- 
2.34.1

Re: [PATCH v4 12/13] rust: sync: introduce `CondVar`

[Alice Ryhl]

On 4/11/23 07:45, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> This is the traditional condition variable or monitor synchronisation
> primitive. It is implemented with C's `wait_queue_head_t`.
> 
> It allows users to release a lock and go to sleep while guaranteeing
> that notifications won't be missed. This is achieved by enqueuing a wait
> entry before releasing the lock.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Waiman Long <longman@redhat.com>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

I have a few methods below that the binder driver will need on the 
condvar. I'll let it be up to you whether you wish to include them in 
this patch. Otherwise, we will add them when we start upstreaming binder.

> +    /// Releases the lock and waits for a notification in interruptible mode.
> +    ///
> +    /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the
> +    /// thread to sleep, reacquiring the lock on wake up. It wakes up when notified by
> +    /// [`CondVar::notify_one`] or [`CondVar::notify_all`], or when the thread receives a signal.
> +    /// It may also wake up spuriously.
> +    ///
> +    /// Returns whether there is a signal pending.
> +    #[must_use = "wait returns if a signal is pending, so the caller must check the return value"]
> +    pub fn wait<T: ?Sized, B: Backend>(&self, guard: &mut Guard<'_, T, B>) -> bool {
> +        self.wait_internal(bindings::TASK_INTERRUPTIBLE, guard);
> +        crate::current!().signal_pending()
> +    }

The binder driver will need a `wait_timeout` method.

> +    /// Calls the kernel function to notify the appropriate number of threads with the given flags.
> +    fn notify(&self, count: i32, flags: u32) {
> +        // SAFETY: `wait_list` points to valid memory.
> +        unsafe {
> +            bindings::__wake_up(
> +                self.wait_list.get(),
> +                bindings::TASK_NORMAL,
> +                count,
> +                flags as _,
> +            )
> +        };
> +    }
> +
> +    /// Wakes a single waiter up, if any.
> +    ///
> +    /// This is not 'sticky' in the sense that if no thread is waiting, the notification is lost
> +    /// completely (as opposed to automatically waking up the next waiter).
> +    pub fn notify_one(&self) {
> +        self.notify(1, 0);
> +    }
> +
> +    /// Wakes all waiters up, if any.
> +    ///
> +    /// This is not 'sticky' in the sense that if no thread is waiting, the notification is lost
> +    /// completely (as opposed to automatically waking up the next waiter).
> +    pub fn notify_all(&self) {
> +        self.notify(0, 0);
> +    }

Android binder will also need a `notify_sync` method. It could be 
implemented like this:

/// Calls the kernel function to notify one thread synchronously.
pub fn notify_sync(&self) {
     // SAFETY: `wait_list` points to valid memory.
     unsafe { bindings::__wake_up_sync(self.wait_list.get(), 
bindings::TASK_NORMAL) };
}

[PATCH v4 13/13] rust: sync: introduce `LockedBy`

[Wedson Almeida Filho]

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

This allows us to have data protected by a lock despite not being
wrapped by it. Access is granted by providing evidence that the lock is
held by the caller.

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
v1 -> v2: Added build_assert to rule out zero-sized types
v2 -> v3: Improve comments around panics and support for ZSTs
v3 -> v4:
- Added build_assert to LockBy::new
- Updated build_assert calls with messages
- Included crate::build_assert and mem::size_of since they're used
  repeatedly.

 rust/kernel/sync.rs           |   2 +
 rust/kernel/sync/lock.rs      |   2 +-
 rust/kernel/sync/locked_by.rs | 156 ++++++++++++++++++++++++++++++++++
 3 files changed, 159 insertions(+), 1 deletion(-)
 create mode 100644 rust/kernel/sync/locked_by.rs

diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 431402180aa8..d219ee518eff 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -10,10 +10,12 @@ use crate::types::Opaque;
 mod arc;
 mod condvar;
 pub mod lock;
+mod locked_by;
 
 pub use arc::{Arc, ArcBorrow, UniqueArc};
 pub use condvar::CondVar;
 pub use lock::{mutex::Mutex, spinlock::SpinLock};
+pub use locked_by::LockedBy;
 
 /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
 #[repr(transparent)]
diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index 816c5d3bba12..14940312141d 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -111,7 +111,7 @@ pub struct Lock<T: ?Sized, B: Backend> {
     _pin: PhantomPinned,
 
     /// The data protected by the lock.
-    data: UnsafeCell<T>,
+    pub(crate) data: UnsafeCell<T>,
 }
 
 // SAFETY: `Lock` can be transferred across thread boundaries iff the data it protects can.
diff --git a/rust/kernel/sync/locked_by.rs b/rust/kernel/sync/locked_by.rs
new file mode 100644
index 000000000000..b17ee5cd98f3
--- /dev/null
+++ b/rust/kernel/sync/locked_by.rs
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! A wrapper for data protected by a lock that does not wrap it.
+
+use super::{lock::Backend, lock::Lock};
+use crate::build_assert;
+use core::{cell::UnsafeCell, mem::size_of, ptr};
+
+/// Allows access to some data to be serialised by a lock that does not wrap it.
+///
+/// In most cases, data protected by a lock is wrapped by the appropriate lock type, e.g.,
+/// [`super::Mutex`] or [`super::SpinLock`]. [`LockedBy`] is meant for cases when this is not
+/// possible. For example, if a container has a lock and some data in the contained elements needs
+/// to be protected by the same lock.
+///
+/// [`LockedBy`] wraps the data in lieu of another locking primitive, and only allows access to it
+/// when the caller shows evidence that the 'external' lock is locked. It panics if the evidence
+/// refers to the wrong instance of the lock.
+///
+/// # Examples
+///
+/// The following is an example for illustrative purposes: `InnerDirectory::bytes_used` is an
+/// aggregate of all `InnerFile::bytes_used` and must be kept consistent; so we wrap `InnerFile` in
+/// a `LockedBy` so that it shares a lock with `InnerDirectory`. This allows us to enforce at
+/// compile-time that access to `InnerFile` is only granted when an `InnerDirectory` is also
+/// locked; we enforce at run time that the right `InnerDirectory` is locked.
+///
+/// ```
+/// use kernel::sync::{LockedBy, Mutex};
+///
+/// struct InnerFile {
+///     bytes_used: u64,
+/// }
+///
+/// struct File {
+///     _ino: u32,
+///     inner: LockedBy<InnerFile, InnerDirectory>,
+/// }
+///
+/// struct InnerDirectory {
+///     /// The sum of the bytes used by all files.
+///     bytes_used: u64,
+///     _files: Vec<File>,
+/// }
+///
+/// struct Directory {
+///     _ino: u32,
+///     inner: Mutex<InnerDirectory>,
+/// }
+///
+/// /// Prints `bytes_used` from both the directory and file.
+/// fn print_bytes_used(dir: &Directory, file: &File) {
+///     let guard = dir.inner.lock();
+///     let inner_file = file.inner.access(&guard);
+///     pr_info!("{} {}", guard.bytes_used, inner_file.bytes_used);
+/// }
+///
+/// /// Increments `bytes_used` for both the directory and file.
+/// fn inc_bytes_used(dir: &Directory, file: &File) {
+///     let mut guard = dir.inner.lock();
+///     guard.bytes_used += 10;
+///
+///     let file_inner = file.inner.access_mut(&mut guard);
+///     file_inner.bytes_used += 10;
+/// }
+///
+/// /// Creates a new file.
+/// fn new_file(ino: u32, dir: &Directory) -> File {
+///     File {
+///         _ino: ino,
+///         inner: LockedBy::new(&dir.inner, InnerFile { bytes_used: 0 }),
+///     }
+/// }
+/// ```
+pub struct LockedBy<T: ?Sized, U: ?Sized> {
+    owner: *const U,
+    data: UnsafeCell<T>,
+}
+
+// SAFETY: `LockedBy` can be transferred across thread boundaries iff the data it protects can.
+unsafe impl<T: ?Sized + Send, U: ?Sized> Send for LockedBy<T, U> {}
+
+// SAFETY: `LockedBy` serialises the interior mutability it provides, so it is `Sync` as long as the
+// data it protects is `Send`.
+unsafe impl<T: ?Sized + Send, U: ?Sized> Sync for LockedBy<T, U> {}
+
+impl<T, U> LockedBy<T, U> {
+    /// Constructs a new instance of [`LockedBy`].
+    ///
+    /// It stores a raw pointer to the owner that is never dereferenced. It is only used to ensure
+    /// that the right owner is being used to access the protected data. If the owner is freed, the
+    /// data becomes inaccessible; if another instance of the owner is allocated *on the same
+    /// memory location*, the data becomes accessible again: none of this affects memory safety
+    /// because in any case at most one thread (or CPU) can access the protected data at a time.
+    pub fn new<B: Backend>(owner: &Lock<U, B>, data: T) -> Self {
+        build_assert!(
+            size_of::<Lock<U, B>>() > 0,
+            "The lock type cannot be a ZST because it may be impossible to distinguish instances"
+        );
+        Self {
+            owner: owner.data.get(),
+            data: UnsafeCell::new(data),
+        }
+    }
+}
+
+impl<T: ?Sized, U> LockedBy<T, U> {
+    /// Returns a reference to the protected data when the caller provides evidence (via a
+    /// reference) that the owner is locked.
+    ///
+    /// `U` cannot be a zero-sized type (ZST) because there are ways to get an `&U` that matches
+    /// the data protected by the lock without actually holding it.
+    ///
+    /// # Panics
+    ///
+    /// Panics if `owner` is different from the data protected by the lock used in
+    /// [`new`](LockedBy::new).
+    pub fn access<'a>(&'a self, owner: &'a U) -> &'a T {
+        build_assert!(
+            size_of::<U>() > 0,
+            "`U` cannot be a ZST because `owner` wouldn't be unique"
+        );
+        if !ptr::eq(owner, self.owner) {
+            panic!("mismatched owners");
+        }
+
+        // SAFETY: `owner` is evidence that the owner is locked.
+        unsafe { &*self.data.get() }
+    }
+
+    /// Returns a mutable reference to the protected data when the caller provides evidence (via a
+    /// mutable owner) that the owner is locked mutably.
+    ///
+    /// `U` cannot be a zero-sized type (ZST) because there are ways to get an `&mut U` that
+    /// matches the data protected by the lock without actually holding it.
+    ///
+    /// Showing a mutable reference to the owner is sufficient because we know no other references
+    /// can exist to it.
+    ///
+    /// # Panics
+    ///
+    /// Panics if `owner` is different from the data protected by the lock used in
+    /// [`new`](LockedBy::new).
+    pub fn access_mut<'a>(&'a self, owner: &'a mut U) -> &'a mut T {
+        build_assert!(
+            size_of::<U>() > 0,
+            "`U` cannot be a ZST because `owner` wouldn't be unique"
+        );
+        if !ptr::eq(owner, self.owner) {
+            panic!("mismatched owners");
+        }
+
+        // SAFETY: `owner` is evidence that there is only one reference to the owner.
+        unsafe { &mut *self.data.get() }
+    }
+}
-- 
2.34.1

Re: [PATCH v4 13/13] rust: sync: introduce `LockedBy`

[Benno Lossin]

On 11.04.23 07:45, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
>
> This allows us to have data protected by a lock despite not being
> wrapped by it. Access is granted by providing evidence that the lock is
> held by the caller.
>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

> ---
> v1 -> v2: Added build_assert to rule out zero-sized types
> v2 -> v3: Improve comments around panics and support for ZSTs
> v3 -> v4:
> - Added build_assert to LockBy::new
> - Updated build_assert calls with messages
> - Included crate::build_assert and mem::size_of since they're used
>    repeatedly.
>
>   rust/kernel/sync.rs           |   2 +
>   rust/kernel/sync/lock.rs      |   2 +-
>   rust/kernel/sync/locked_by.rs | 156 ++++++++++++++++++++++++++++++++++
>   3 files changed, 159 insertions(+), 1 deletion(-)
>   create mode 100644 rust/kernel/sync/locked_by.rs
>
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index 431402180aa8..d219ee518eff 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -10,10 +10,12 @@ use crate::types::Opaque;
>   mod arc;
>   mod condvar;
>   pub mod lock;
> +mod locked_by;
>
>   pub use arc::{Arc, ArcBorrow, UniqueArc};
>   pub use condvar::CondVar;
>   pub use lock::{mutex::Mutex, spinlock::SpinLock};
> +pub use locked_by::LockedBy;
>
>   /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
>   #[repr(transparent)]
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index 816c5d3bba12..14940312141d 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -111,7 +111,7 @@ pub struct Lock<T: ?Sized, B: Backend> {
>       _pin: PhantomPinned,
>
>       /// The data protected by the lock.
> -    data: UnsafeCell<T>,
> +    pub(crate) data: UnsafeCell<T>,
>   }
>
>   // SAFETY: `Lock` can be transferred across thread boundaries iff the data it protects can.
> diff --git a/rust/kernel/sync/locked_by.rs b/rust/kernel/sync/locked_by.rs
> new file mode 100644
> index 000000000000..b17ee5cd98f3
> --- /dev/null
> +++ b/rust/kernel/sync/locked_by.rs
> @@ -0,0 +1,156 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! A wrapper for data protected by a lock that does not wrap it.
> +
> +use super::{lock::Backend, lock::Lock};
> +use crate::build_assert;
> +use core::{cell::UnsafeCell, mem::size_of, ptr};
> +
> +/// Allows access to some data to be serialised by a lock that does not wrap it.
> +///
> +/// In most cases, data protected by a lock is wrapped by the appropriate lock type, e.g.,
> +/// [`super::Mutex`] or [`super::SpinLock`]. [`LockedBy`] is meant for cases when this is not
> +/// possible. For example, if a container has a lock and some data in the contained elements needs
> +/// to be protected by the same lock.
> +///
> +/// [`LockedBy`] wraps the data in lieu of another locking primitive, and only allows access to it
> +/// when the caller shows evidence that the 'external' lock is locked. It panics if the evidence
> +/// refers to the wrong instance of the lock.
> +///
> +/// # Examples
> +///
> +/// The following is an example for illustrative purposes: `InnerDirectory::bytes_used` is an
> +/// aggregate of all `InnerFile::bytes_used` and must be kept consistent; so we wrap `InnerFile` in
> +/// a `LockedBy` so that it shares a lock with `InnerDirectory`. This allows us to enforce at
> +/// compile-time that access to `InnerFile` is only granted when an `InnerDirectory` is also
> +/// locked; we enforce at run time that the right `InnerDirectory` is locked.
> +///
> +/// ```
> +/// use kernel::sync::{LockedBy, Mutex};
> +///
> +/// struct InnerFile {
> +///     bytes_used: u64,
> +/// }
> +///
> +/// struct File {
> +///     _ino: u32,
> +///     inner: LockedBy<InnerFile, InnerDirectory>,
> +/// }
> +///
> +/// struct InnerDirectory {
> +///     /// The sum of the bytes used by all files.
> +///     bytes_used: u64,
> +///     _files: Vec<File>,
> +/// }
> +///
> +/// struct Directory {
> +///     _ino: u32,
> +///     inner: Mutex<InnerDirectory>,
> +/// }
> +///
> +/// /// Prints `bytes_used` from both the directory and file.
> +/// fn print_bytes_used(dir: &Directory, file: &File) {
> +///     let guard = dir.inner.lock();
> +///     let inner_file = file.inner.access(&guard);
> +///     pr_info!("{} {}", guard.bytes_used, inner_file.bytes_used);
> +/// }
> +///
> +/// /// Increments `bytes_used` for both the directory and file.
> +/// fn inc_bytes_used(dir: &Directory, file: &File) {
> +///     let mut guard = dir.inner.lock();
> +///     guard.bytes_used += 10;
> +///
> +///     let file_inner = file.inner.access_mut(&mut guard);
> +///     file_inner.bytes_used += 10;
> +/// }
> +///
> +/// /// Creates a new file.
> +/// fn new_file(ino: u32, dir: &Directory) -> File {
> +///     File {
> +///         _ino: ino,
> +///         inner: LockedBy::new(&dir.inner, InnerFile { bytes_used: 0 }),
> +///     }
> +/// }
> +/// ```
> +pub struct LockedBy<T: ?Sized, U: ?Sized> {
> +    owner: *const U,
> +    data: UnsafeCell<T>,
> +}
> +
> +// SAFETY: `LockedBy` can be transferred across thread boundaries iff the data it protects can.
> +unsafe impl<T: ?Sized + Send, U: ?Sized> Send for LockedBy<T, U> {}
> +
> +// SAFETY: `LockedBy` serialises the interior mutability it provides, so it is `Sync` as long as the
> +// data it protects is `Send`.
> +unsafe impl<T: ?Sized + Send, U: ?Sized> Sync for LockedBy<T, U> {}
> +
> +impl<T, U> LockedBy<T, U> {
> +    /// Constructs a new instance of [`LockedBy`].
> +    ///
> +    /// It stores a raw pointer to the owner that is never dereferenced. It is only used to ensure
> +    /// that the right owner is being used to access the protected data. If the owner is freed, the
> +    /// data becomes inaccessible; if another instance of the owner is allocated *on the same
> +    /// memory location*, the data becomes accessible again: none of this affects memory safety
> +    /// because in any case at most one thread (or CPU) can access the protected data at a time.
> +    pub fn new<B: Backend>(owner: &Lock<U, B>, data: T) -> Self {
> +        build_assert!(
> +            size_of::<Lock<U, B>>() > 0,
> +            "The lock type cannot be a ZST because it may be impossible to distinguish instances"
> +        );
> +        Self {
> +            owner: owner.data.get(),
> +            data: UnsafeCell::new(data),
> +        }
> +    }
> +}
> +
> +impl<T: ?Sized, U> LockedBy<T, U> {
> +    /// Returns a reference to the protected data when the caller provides evidence (via a
> +    /// reference) that the owner is locked.
> +    ///
> +    /// `U` cannot be a zero-sized type (ZST) because there are ways to get an `&U` that matches
> +    /// the data protected by the lock without actually holding it.
> +    ///
> +    /// # Panics
> +    ///
> +    /// Panics if `owner` is different from the data protected by the lock used in
> +    /// [`new`](LockedBy::new).
> +    pub fn access<'a>(&'a self, owner: &'a U) -> &'a T {
> +        build_assert!(
> +            size_of::<U>() > 0,
> +            "`U` cannot be a ZST because `owner` wouldn't be unique"
> +        );
> +        if !ptr::eq(owner, self.owner) {
> +            panic!("mismatched owners");
> +        }
> +
> +        // SAFETY: `owner` is evidence that the owner is locked.
> +        unsafe { &*self.data.get() }
> +    }
> +
> +    /// Returns a mutable reference to the protected data when the caller provides evidence (via a
> +    /// mutable owner) that the owner is locked mutably.
> +    ///
> +    /// `U` cannot be a zero-sized type (ZST) because there are ways to get an `&mut U` that
> +    /// matches the data protected by the lock without actually holding it.
> +    ///
> +    /// Showing a mutable reference to the owner is sufficient because we know no other references
> +    /// can exist to it.
> +    ///
> +    /// # Panics
> +    ///
> +    /// Panics if `owner` is different from the data protected by the lock used in
> +    /// [`new`](LockedBy::new).
> +    pub fn access_mut<'a>(&'a self, owner: &'a mut U) -> &'a mut T {
> +        build_assert!(
> +            size_of::<U>() > 0,
> +            "`U` cannot be a ZST because `owner` wouldn't be unique"
> +        );
> +        if !ptr::eq(owner, self.owner) {
> +            panic!("mismatched owners");
> +        }
> +
> +        // SAFETY: `owner` is evidence that there is only one reference to the owner.
> +        unsafe { &mut *self.data.get() }
> +    }
> +}
> --
> 2.34.1
>

Re: [PATCH v4 01/13] rust: sync: introduce `LockClassKey`

[Gary Guo]

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

> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> It is a wrapper around C's `lock_class_key`, which is used by the
> synchronisation primitives that are checked with lockdep. This is in
> preparation for introducing Rust abstractions for these primitives.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Waiman Long <longman@redhat.com>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
> v1 -> v2: Fixed a typo in comment
> v2 -> v3: Replaced `core` with `::core` in macro
> v3 -> v4:
> - Rebased on top of rust-next now that all dependencies are applied
> 
>  rust/kernel/sync.rs | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index 33da23e3076d..541d235ffbeb 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -5,6 +5,51 @@
>  //! This module contains the kernel APIs related to synchronisation that have been ported or
>  //! wrapped for usage by Rust code in the kernel.
>  
> +use crate::types::Opaque;
> +
>  mod arc;
>  
>  pub use arc::{Arc, ArcBorrow, UniqueArc};
> +
> +/// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
> +#[repr(transparent)]
> +pub struct LockClassKey(Opaque<bindings::lock_class_key>);
> +
> +// SAFETY: `bindings::lock_class_key` is designed to be used concurrently from multiple threads and
> +// provides its own synchronization.
> +unsafe impl Sync for LockClassKey {}
> +
> +impl LockClassKey {
> +    /// Creates a new lock class key.
> +    pub const fn new() -> Self {
> +        Self(Opaque::uninit())
> +    }
> +
> +    #[allow(dead_code)]
> +    pub(crate) fn as_ptr(&self) -> *mut bindings::lock_class_key {
> +        self.0.get()
> +    }
> +}
> +
> +/// Defines a new static lock class and returns a pointer to it.
> +#[doc(hidden)]
> +#[macro_export]
> +macro_rules! static_lock_class {
> +    () => {{
> +        static CLASS: $crate::sync::LockClassKey = $crate::sync::LockClassKey::new();
> +        &CLASS
> +    }};
> +}
> +
> +/// Returns the given string, if one is provided, otherwise generates one based on the source code
> +/// location.
> +#[doc(hidden)]
> +#[macro_export]
> +macro_rules! optional_name {
> +    () => {
> +        $crate::c_str!(::core::concat!(::core::file!(), ":", ::core::line!()))
> +    };
> +    ($name:literal) => {
> +        $crate::c_str!($name)
> +    };
> +}
> 
> base-commit: 2d0dec625d872a41632a68fce2e69453ed87df91

Re: [PATCH v4 01/13] rust: sync: introduce `LockClassKey`

[Benno Lossin]

On 11.04.23 07:45, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
>
> It is a wrapper around C's `lock_class_key`, which is used by the
> synchronisation primitives that are checked with lockdep. This is in
> preparation for introducing Rust abstractions for these primitives.
>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Waiman Long <longman@redhat.com>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

> ---
> v1 -> v2: Fixed a typo in comment
> v2 -> v3: Replaced `core` with `::core` in macro
> v3 -> v4:
> - Rebased on top of rust-next now that all dependencies are applied
>
>   rust/kernel/sync.rs | 45 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 45 insertions(+)
>
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index 33da23e3076d..541d235ffbeb 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -5,6 +5,51 @@
>   //! This module contains the kernel APIs related to synchronisation that have been ported or
>   //! wrapped for usage by Rust code in the kernel.
>
> +use crate::types::Opaque;
> +
>   mod arc;
>
>   pub use arc::{Arc, ArcBorrow, UniqueArc};
> +
> +/// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
> +#[repr(transparent)]
> +pub struct LockClassKey(Opaque<bindings::lock_class_key>);
> +
> +// SAFETY: `bindings::lock_class_key` is designed to be used concurrently from multiple threads and
> +// provides its own synchronization.
> +unsafe impl Sync for LockClassKey {}
> +
> +impl LockClassKey {
> +    /// Creates a new lock class key.
> +    pub const fn new() -> Self {
> +        Self(Opaque::uninit())
> +    }
> +
> +    #[allow(dead_code)]
> +    pub(crate) fn as_ptr(&self) -> *mut bindings::lock_class_key {
> +        self.0.get()
> +    }
> +}
> +
> +/// Defines a new static lock class and returns a pointer to it.
> +#[doc(hidden)]
> +#[macro_export]
> +macro_rules! static_lock_class {
> +    () => {{
> +        static CLASS: $crate::sync::LockClassKey = $crate::sync::LockClassKey::new();
> +        &CLASS
> +    }};
> +}
> +
> +/// Returns the given string, if one is provided, otherwise generates one based on the source code
> +/// location.
> +#[doc(hidden)]
> +#[macro_export]
> +macro_rules! optional_name {
> +    () => {
> +        $crate::c_str!(::core::concat!(::core::file!(), ":", ::core::line!()))
> +    };
> +    ($name:literal) => {
> +        $crate::c_str!($name)
> +    };
> +}
>
> base-commit: 2d0dec625d872a41632a68fce2e69453ed87df91
> --
> 2.34.1
>

Re: [PATCH v4 01/13] rust: sync: introduce `LockClassKey`

[Miguel Ojeda]

On Tue, Apr 11, 2023 at 7:46 AM Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>
> It is a wrapper around C's `lock_class_key`, which is used by the
> synchronisation primitives that are checked with lockdep. This is in
> preparation for introducing Rust abstractions for these primitives.

Series applied to `rust-next` with the v4.1 patches applied plus the
irqsave bits removed as well as p4 since it was not Acked yet, as
discussed. Thanks!

Cheers,
Miguel