rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/3] rust: miscdevice: add additional data to MiscDeviceRegistration
@ 2025-06-10 20:27 Christian Schrefl
  2025-06-10 20:27 ` [PATCH v6 1/3] rust: implement `Wrapper<T>` for `Opaque<T>` Christian Schrefl
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Christian Schrefl @ 2025-06-10 20:27 UTC (permalink / raw)
  To: Miguel Ojeda, Danilo Krummrich, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Arnd Bergmann, Greg Kroah-Hartman, Lee Jones, Daniel Almeida,
	Benno Lossin, Benno Lossin
  Cc: Gerald Wisböck, rust-for-linux, linux-kernel,
	Christian Schrefl

Currently there is no good way to pass arbitrary data from the driver to
a `miscdevice` or to share data between individual handles to a 
`miscdevice` in rust.

This series adds additional (generic) data to the MiscDeviceRegistration
for this purpose.

The first patch originally comes from my `UnsafePinned` patch series [0].

The second patch implements the changes and fixes the build of the sample
without changing any functionality (this is currently the only in tree 
user).

The third patch changes the `rust_misc_device` sample to use this to 
share the same data between multiple handles to the `miscdevice`.
I have tested the sample with qemu and the C userspace example
from the doc comments.

Some discussion on Zulip about the motivation and approach [1].
Thanks a lot to everyone helping me out with this.

This patch series is based on the v6.16-rc1 tag.

Link: https://lore.kernel.org/rust-for-linux/20250511-rust_unsafe_pinned-v4-2-a86c32e47e3d@gmail.com/  [0]
Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Passing.20a.20DevRes.20to.20a.20miscdev/near/494553814 [1]

Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
Changes in v6:
- add `Send` bounds required to use `assert_send`
- rebase on-top of v6.16-rc1
- added missing newline before `impl Wrapper..`
- Link to v5: https://lore.kernel.org/r/20250607-b4-rust_miscdevice_registrationdata-v5-0-b77b5b7aab5b@gmail.com

Changes in v5:
- Remove repr(C) and PhantomData (Benno)
- Rename `RegistrationData` to just `Data` (Benno)
- Add bound `Data: Send` bound to `impl Send for
    MiscDeviceRegistration` (Benno)
- Add safety comment about `MiscDeviceRegistration: Send` requirement
- Add Invariants to `MiscDeviceRegistration` (Benno)
- Slightly reword safety comment in drop.
- Removed spurious newline changes in sample (Benno)
- Removed spurious typo fix (Miguel)
- Add Alice's Reviewed-by from v3.
- Link to v4: https://lore.kernel.org/r/20250530-b4-rust_miscdevice_registrationdata-v4-0-d313aafd7e59@gmail.com

Changes in v4:
- Rework to use Opaque instead of `UnsafePinned`.
- Include `impl Wrapper for Opaque` patch.
- Link to v3: https://lore.kernel.org/r/20250517-b4-rust_miscdevice_registrationdata-v3-0-cdb33e228d37@gmail.com

Changes in v3:
- Rebased on top of my `UnsafePinned` series.
- Link to v2: https://lore.kernel.org/r/20250131-b4-rust_miscdevice_registrationdata-v2-0-588f1e6cfabe@gmail.com

Changes in v2:
- Don't use associated_type_bounds since the MSRV does not support
    that on stable yet (Kernel test robot)
- Doc changes and add intra-doc links (Miguel)
- Use container_of macro instead of pointer cast in `fops_open` (Greg)
- Rename `Aliased` to `UnsafePinned` (Boqun)
- Make sure Data is initialized before `misc_register` is called
- Rework the example to use an additional shared value instead of 
    replacing the unique one
- Expanded the c code for the example to use the new ioctls
- Link to v1: https://lore.kernel.org/r/20250119-b4-rust_miscdevice_registrationdata-v1-0-edbf18dde5fc@gmail.com

---
Christian Schrefl (3):
      rust: implement `Wrapper<T>` for `Opaque<T>`
      rust: miscdevice: add additional data to `MiscDeviceRegistration`
      rust: miscdevice: adjust the `rust_misc_device` sample to use `Data`.

 rust/kernel/miscdevice.rs        | 104 ++++++++++++++++++++++++++---------
 rust/kernel/revocable.rs         |   2 +
 rust/kernel/types.rs             |  26 +++++----
 samples/rust/rust_misc_device.rs | 116 +++++++++++++++++++++++++++++++++++++--
 4 files changed, 205 insertions(+), 43 deletions(-)
---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250119-b4-rust_miscdevice_registrationdata-a11d88dcb284

Best regards,
-- 
Christian Schrefl <chrisi.schrefl@gmail.com>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v6 1/3] rust: implement `Wrapper<T>` for `Opaque<T>`
  2025-06-10 20:27 [PATCH v6 0/3] rust: miscdevice: add additional data to MiscDeviceRegistration Christian Schrefl
@ 2025-06-10 20:27 ` Christian Schrefl
  2025-06-27 14:43   ` Miguel Ojeda
  2025-06-29 15:11   ` Danilo Krummrich
  2025-06-10 20:27 ` [PATCH v6 2/3] rust: miscdevice: add additional data to `MiscDeviceRegistration` Christian Schrefl
  2025-06-10 20:27 ` [PATCH v6 3/3] rust: miscdevice: adjust the `rust_misc_device` sample to use `Data` Christian Schrefl
  2 siblings, 2 replies; 6+ messages in thread
From: Christian Schrefl @ 2025-06-10 20:27 UTC (permalink / raw)
  To: Miguel Ojeda, Danilo Krummrich, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Arnd Bergmann, Greg Kroah-Hartman, Lee Jones, Daniel Almeida,
	Benno Lossin, Benno Lossin
  Cc: Gerald Wisböck, rust-for-linux, linux-kernel,
	Christian Schrefl

Moves the implementation for `pin-init` from an associated function
to the trait function of the `Wrapper` trait and extends the
implementation to support pin-initializers with error types.

Adds a use for the `Wrapper` trait in `revocable.rs`, to use the new
`pin-init` function. This is currently the only usage in the kernel.

Reviewed-by: Danilo Krummrich <dakr@kernel.org>
Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>

---
This patch is also required for my `UnsafePinned`series. I'm not sure
how this should be handeld (in case both series make it in this cycle).
---
 rust/kernel/revocable.rs |  2 ++
 rust/kernel/types.rs     | 26 ++++++++++++++------------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index db4aa46bb1216d18868dcbdcb0b7033a757306b1..3d41374a911482385faa93170cd31a703bb0d42d 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -5,6 +5,8 @@
 //! The [`Revocable`] type wraps other types and allows access to them to be revoked. The existence
 //! of a [`RevocableGuard`] ensures that objects remain valid.
 
+use pin_init::Wrapper;
+
 use crate::{bindings, prelude::*, sync::rcu, types::Opaque};
 use core::{
     marker::PhantomData,
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 22985b6f69820d6df8ff3aae0bf815fad36a9d92..3958a5f44d567bbf2ff4e3e891b27c065909431a 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -9,7 +9,7 @@
     ops::{Deref, DerefMut},
     ptr::NonNull,
 };
-use pin_init::{PinInit, Zeroable};
+use pin_init::{PinInit, Wrapper, Zeroable};
 
 /// Used to transfer ownership to and from foreign (non-Rust) languages.
 ///
@@ -353,17 +353,6 @@ pub const fn zeroed() -> Self {
         }
     }
 
-    /// Create an opaque pin-initializer from the given pin-initializer.
-    pub fn pin_init(slot: impl PinInit<T>) -> impl PinInit<Self> {
-        Self::ffi_init(|ptr: *mut T| {
-            // SAFETY:
-            //   - `ptr` is a valid pointer to uninitialized memory,
-            //   - `slot` is not accessed on error; the call is infallible,
-            //   - `slot` is pinned in memory.
-            let _ = unsafe { PinInit::<T>::__pinned_init(slot, ptr) };
-        })
-    }
-
     /// Creates a pin-initializer from the given initializer closure.
     ///
     /// The returned initializer calls the given closure with the pointer to the inner `T` of this
@@ -415,6 +404,19 @@ pub const fn raw_get(this: *const Self) -> *mut T {
     }
 }
 
+impl<T> Wrapper<T> for Opaque<T> {
+    /// Create an opaque pin-initializer from the given pin-initializer.
+    fn pin_init<E>(slot: impl PinInit<T, E>) -> impl PinInit<Self, E> {
+        Self::try_ffi_init(|ptr: *mut T| {
+            // SAFETY:
+            //   - `ptr` is a valid pointer to uninitialized memory,
+            //   - `slot` is not accessed on error,
+            //   - `slot` is pinned in memory.
+            unsafe { PinInit::<T, E>::__pinned_init(slot, ptr) }
+        })
+    }
+}
+
 /// Types that are _always_ reference counted.
 ///
 /// It allows such types to define their own custom ref increment and decrement functions.

-- 
2.49.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v6 2/3] rust: miscdevice: add additional data to `MiscDeviceRegistration`
  2025-06-10 20:27 [PATCH v6 0/3] rust: miscdevice: add additional data to MiscDeviceRegistration Christian Schrefl
  2025-06-10 20:27 ` [PATCH v6 1/3] rust: implement `Wrapper<T>` for `Opaque<T>` Christian Schrefl
@ 2025-06-10 20:27 ` Christian Schrefl
  2025-06-10 20:27 ` [PATCH v6 3/3] rust: miscdevice: adjust the `rust_misc_device` sample to use `Data` Christian Schrefl
  2 siblings, 0 replies; 6+ messages in thread
From: Christian Schrefl @ 2025-06-10 20:27 UTC (permalink / raw)
  To: Miguel Ojeda, Danilo Krummrich, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Arnd Bergmann, Greg Kroah-Hartman, Lee Jones, Daniel Almeida,
	Benno Lossin, Benno Lossin
  Cc: Gerald Wisböck, rust-for-linux, linux-kernel,
	Christian Schrefl

When using the Rust miscdevice bindings, you generally embed the
`MiscDeviceRegistration` within another struct:

struct MyDriverData {
    data: SomeOtherData,
    misc: MiscDeviceRegistration<MyMiscFile>
}

In the `fops->open` callback of the miscdevice, you are given a
reference to the registration, which allows you to access its fields.
For example, as of commit 284ae0be4dca ("rust: miscdevice: Provide
accessor to pull out miscdevice::this_device") you can access the
internal `struct device`. However, there is still no way to access the
`data` field in the above example, because you only have a reference to
the registration.

Using `container_of` is also not possible to do safely. For example, if
the destructor of `MyDriverData` runs, then the destructor of `data`
would run before the miscdevice is deregistered, so using `container_of`
to access `data` from `fops->open` could result in a UAF. A similar
problem can happen on initialization if `misc` is not the last field to
be initialized.

To provide a safe way to access user-defined data stored next to the
`struct miscdevice`, make `MiscDeviceRegistration` into a container that
can store a user-provided piece of data. This way, `fops->open` can
access that data via the registration, since the data is stored inside
the registration.

The container enforces that the additional user data is initialized
before the miscdevice is registered, and that the miscdevice is
deregistered before the user data is destroyed. This ensures that access
to the userdata is safe.

For the same reasons as in commit 88441d5c6d17 ("rust: miscdevice:
access the `struct miscdevice` from fops->open()"), you cannot access
the user data in any other fops callback than open. This is because a
miscdevice can be deregistered while there are still open files.

A situation where this user data might be required is when a platform
driver acquires a resource in `probe` and wants to use this resource in
the `fops` implementation of a `MiscDevice`.

This solution is similar to the approach used by the initial downstream
Rust-for-Linux/Rust branch [0].

Link: https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/miscdev.rs#L108 [0]
Suggested-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
 rust/kernel/miscdevice.rs        | 104 +++++++++++++++++++++++++++++----------
 samples/rust/rust_misc_device.rs |   4 +-
 2 files changed, 82 insertions(+), 26 deletions(-)

diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index 939278bc7b03489a647b697012e09223871c90cd..8daf3724b75068c998cb361eac38f43a39884b0c 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -9,7 +9,7 @@
 //! Reference: <https://www.kernel.org/doc/html/latest/driver-api/misc_devices.html>
 
 use crate::{
-    bindings,
+    bindings, container_of,
     device::Device,
     error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
     ffi::{c_int, c_long, c_uint, c_ulong},
@@ -21,6 +21,7 @@
     types::{ForeignOwnable, Opaque},
 };
 use core::{marker::PhantomData, mem::MaybeUninit, pin::Pin};
+use pin_init::Wrapper;
 
 /// Options for creating a misc device.
 #[derive(Copy, Clone)]
@@ -31,7 +32,10 @@ pub struct MiscDeviceOptions {
 
 impl MiscDeviceOptions {
     /// Create a raw `struct miscdev` ready for registration.
-    pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
+    pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice
+    where
+        T::Data: Sync,
+    {
         // SAFETY: All zeros is valid for this C type.
         let mut result: bindings::miscdevice = unsafe { MaybeUninit::zeroed().assume_init() };
         result.minor = bindings::MISC_DYNAMIC_MINOR as _;
@@ -45,38 +49,55 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
 ///
 /// # Invariants
 ///
-/// `inner` is a registered misc device.
-#[repr(transparent)]
+/// - `inner` is a registered misc device.
+/// - `data` contains a valid `T::Data` for the whole lifetime of [`MiscDeviceRegistration`]
+/// - `data` must be valid until `misc_deregister` (called when dropped) has returned.
+/// - no mutable references to `data` may be created.
 #[pin_data(PinnedDrop)]
-pub struct MiscDeviceRegistration<T> {
+pub struct MiscDeviceRegistration<T: MiscDevice> {
     #[pin]
     inner: Opaque<bindings::miscdevice>,
-    _t: PhantomData<T>,
+    #[pin]
+    data: Opaque<T::Data>,
 }
 
-// SAFETY: It is allowed to call `misc_deregister` on a different thread from where you called
-// `misc_register`.
-unsafe impl<T> Send for MiscDeviceRegistration<T> {}
-// SAFETY: All `&self` methods on this type are written to ensure that it is safe to call them in
-// parallel.
-unsafe impl<T> Sync for MiscDeviceRegistration<T> {}
+// SAFETY:
+// - It is allowed to call `misc_deregister` on a different thread from where you called
+//   `misc_register`.
+// - Only implements `Send` if `MiscDevice::Data` is also `Send`.
+unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::Data: Send {}
+
+// SAFETY:
+// - All `&self` methods on this type are written to ensure that it is safe to call them in
+//   parallel.
+// - Only implements `Sync` if `MiscDevice::Data` is also `Sync`.
+unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> where T::Data: Sync {}
 
 impl<T: MiscDevice> MiscDeviceRegistration<T> {
     /// Register a misc device.
-    pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
+    pub fn register(
+        opts: MiscDeviceOptions,
+        data: impl PinInit<T::Data, Error>,
+    ) -> impl PinInit<Self, Error>
+    where
+        T::Data: Sync,
+    {
         try_pin_init!(Self {
+            data <- Opaque::pin_init(data),
             inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
                 // SAFETY: The initializer can write to the provided `slot`.
                 unsafe { slot.write(opts.into_raw::<T>()) };
 
-                // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
-                // get unregistered before `slot` is deallocated because the memory is pinned and
-                // the destructor of this type deallocates the memory.
+                // SAFETY:
+                // * We just wrote the misc device options to the slot. The miscdevice will
+                //   get unregistered before `slot` is deallocated because the memory is pinned and
+                //   the destructor of this type deallocates the memory.
+                // * `data` is Initialized before `misc_register` so no race with `fops->open()`
+                //   is possible.
                 // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
                 // misc device.
                 to_result(unsafe { bindings::misc_register(slot) })
             }),
-            _t: PhantomData,
         })
     }
 
@@ -94,13 +115,24 @@ pub fn device(&self) -> &Device {
         // before the underlying `struct miscdevice` is destroyed.
         unsafe { Device::as_ref((*self.as_raw()).this_device) }
     }
+
+    /// Access the additional data stored in this registration.
+    pub fn data(&self) -> &T::Data {
+        // SAFETY:
+        // * No mutable reference to the value contained by `self.data` can ever be created.
+        // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
+        unsafe { &*self.data.get() }
+    }
 }
 
 #[pinned_drop]
-impl<T> PinnedDrop for MiscDeviceRegistration<T> {
+impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
     fn drop(self: Pin<&mut Self>) {
         // SAFETY: We know that the device is registered by the type invariants.
         unsafe { bindings::misc_deregister(self.inner.get()) };
+
+        // SAFETY: `self.data` contains a valid `Data` and does not need to be valid anymore.
+        unsafe { core::ptr::drop_in_place(self.data.get()) };
     }
 }
 
@@ -110,6 +142,13 @@ pub trait MiscDevice: Sized {
     /// What kind of pointer should `Self` be wrapped in.
     type Ptr: ForeignOwnable + Send + Sync;
 
+    /// Additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
+    /// If no additional data is required than the unit type `()` should be used.
+    ///
+    /// This can be accessed in [`MiscDevice::open()`] using
+    /// [`MiscDeviceRegistration::data()`].
+    type Data;
+
     /// Called when the misc device is opened.
     ///
     /// The returned pointer will be stored as the private data for the file.
@@ -180,7 +219,10 @@ fn show_fdinfo(
 /// A vtable for the file operations of a Rust miscdevice.
 struct MiscdeviceVTable<T: MiscDevice>(PhantomData<T>);
 
-impl<T: MiscDevice> MiscdeviceVTable<T> {
+impl<T: MiscDevice> MiscdeviceVTable<T>
+where
+    T::Data: Sync,
+{
     /// # Safety
     ///
     /// `file` and `inode` must be the file and inode for a file that is undergoing initialization.
@@ -195,18 +237,30 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
         // SAFETY: The open call of a file can access the private data.
         let misc_ptr = unsafe { (*raw_file).private_data };
 
-        // SAFETY: This is a miscdevice, so `misc_open()` set the private data to a pointer to the
-        // associated `struct miscdevice` before calling into this method. Furthermore,
-        // `misc_open()` ensures that the miscdevice can't be unregistered and freed during this
-        // call to `fops_open`.
-        let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() };
+        // This is a miscdevice, so `misc_open()` sets the private data to a pointer to the
+        // associated `struct miscdevice` before calling into this method.
+        let misc_ptr = misc_ptr.cast::<Opaque<bindings::miscdevice>>();
+
+        // SAFETY:
+        // * `misc_open()` ensures that the `struct miscdevice` can't be unregistered and freed
+        //   during this call to `fops_open`.
+        // * The `misc_ptr` always points to the `inner` field of a `MiscDeviceRegistration<T>`.
+        // * The `MiscDeviceRegistration<T>` is valid until the `struct miscdevice` was
+        //   unregistered.
+        // * `MiscDeviceRegistration<T>` is `Send` since `MiscDeviceRegistration::register` has a
+        //   `T::Data: Sync` bound,  `MiscDeviceRegistration<T>` is Send if `T::Data: Sync` and is
+        //   the only way to create a `MiscDeviceRegistration`. This means that a reference to it
+        //   can be shared between contexts.
+        // TODO: add `assert_sync` for `MiscDeviceRegistration<T>` and
+        // `MiscDeviceRegistration<T>::Data`.
+        let registration = unsafe { &*container_of!(misc_ptr, MiscDeviceRegistration<T>, inner) };
 
         // SAFETY:
         // * This underlying file is valid for (much longer than) the duration of `T::open`.
         // * There is no active fdget_pos region on the file on this thread.
         let file = unsafe { File::from_raw_file(raw_file) };
 
-        let ptr = match T::open(file, misc) {
+        let ptr = match T::open(file, registration) {
             Ok(ptr) => ptr,
             Err(err) => return err.to_errno(),
         };
diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index c881fd6dbd08cf4308fe1bd37d11d28374c1f034..c0b912920d6c4b60e747d9d298900ad64df67339 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -137,7 +137,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
         };
 
         try_pin_init!(Self {
-            _miscdev <- MiscDeviceRegistration::register(options),
+            _miscdev <- MiscDeviceRegistration::register(options, ()),
         })
     }
 }
@@ -157,6 +157,8 @@ struct RustMiscDevice {
 impl MiscDevice for RustMiscDevice {
     type Ptr = Pin<KBox<Self>>;
 
+    type Data = ();
+
     fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
         let dev = ARef::from(misc.device());
 

-- 
2.49.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v6 3/3] rust: miscdevice: adjust the `rust_misc_device` sample to use `Data`.
  2025-06-10 20:27 [PATCH v6 0/3] rust: miscdevice: add additional data to MiscDeviceRegistration Christian Schrefl
  2025-06-10 20:27 ` [PATCH v6 1/3] rust: implement `Wrapper<T>` for `Opaque<T>` Christian Schrefl
  2025-06-10 20:27 ` [PATCH v6 2/3] rust: miscdevice: add additional data to `MiscDeviceRegistration` Christian Schrefl
@ 2025-06-10 20:27 ` Christian Schrefl
  2 siblings, 0 replies; 6+ messages in thread
From: Christian Schrefl @ 2025-06-10 20:27 UTC (permalink / raw)
  To: Miguel Ojeda, Danilo Krummrich, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Arnd Bergmann, Greg Kroah-Hartman, Lee Jones, Daniel Almeida,
	Benno Lossin, Benno Lossin
  Cc: Gerald Wisböck, rust-for-linux, linux-kernel,
	Christian Schrefl

Add a second mutex to the `RustMiscDevice``, which is shared between all
instances of the device using an `Arc` and the `Data` of
`MiscDeviceRegistration`.

This is mostly to demonstrate the capability to share data in this way.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
 samples/rust/rust_misc_device.rs | 116 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 109 insertions(+), 7 deletions(-)

diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index c0b912920d6c4b60e747d9d298900ad64df67339..7519ff6a79985e8bdbb7f4c79d8a6ebf160ef8cc 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -18,6 +18,8 @@
 //! #define RUST_MISC_DEV_HELLO _IO('|', 0x80)
 //! #define RUST_MISC_DEV_GET_VALUE _IOR('|', 0x81, int)
 //! #define RUST_MISC_DEV_SET_VALUE _IOW('|', 0x82, int)
+//! #define RUST_MISC_DEV_GET_SHARED_VALUE _IOR('|', 0x83, int)
+//! #define RUST_MISC_DEV_SET_SHARED_VALUE _IOW('|', 0x84, int)
 //!
 //! int main() {
 //!   int value, new_value;
@@ -86,6 +88,62 @@
 //!     return -1;
 //!   }
 //!
+//!   value++;
+//!
+//!   // Set shared value to something different
+//!   printf("Submitting new shared value (%d)\n", value);
+//!   ret = ioctl(fd, RUST_MISC_DEV_SET_SHARED_VALUE, &value);
+//!   if (ret < 0) {
+//!     perror("ioctl: Failed to submit new value");
+//!     close(fd);
+//!     return errno;
+//!   }
+//!
+//!   // Close the device file
+//!   printf("Closing /dev/rust-misc-device\n");
+//!   close(fd);
+//!
+//!   // Open the device file again
+//!   printf("Opening /dev/rust-misc-device again for reading\n");
+//!   fd = open("/dev/rust-misc-device", O_RDWR);
+//!   if (fd < 0) {
+//!     perror("open");
+//!     return errno;
+//!   }
+//!
+//!   // Ensure new value was applied
+//!   printf("Fetching new value\n");
+//!   ret = ioctl(fd, RUST_MISC_DEV_GET_SHARED_VALUE, &new_value);
+//!   if (ret < 0) {
+//!     perror("ioctl: Failed to fetch the new value");
+//!     close(fd);
+//!     return errno;
+//!   }
+//!
+//!   if (value != new_value) {
+//!     printf("Failed: Committed and retrieved values are different (%d - %d)\n",
+//!            value, new_value);
+//!     close(fd);
+//!     return -1;
+//!   }
+//!
+//!   value = 0;
+//!   // Ensure non-shared value is still 0
+//!   printf("Fetching new value\n");
+//!   ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &new_value);
+//!   if (ret < 0) {
+//!     perror("ioctl: Failed to fetch the new value");
+//!     close(fd);
+//!     return errno;
+//!   }
+//!
+//!   if (value != new_value) {
+//!     printf("Failed: Committed and retrieved values are different (%d - %d)\n",
+//!            value, new_value);
+//!     close(fd);
+//!     return -1;
+//!   }
+//!
 //!   // Close the device file
 //!   printf("Closing /dev/rust-misc-device\n");
 //!   close(fd);
@@ -105,7 +163,7 @@
     miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
     new_mutex,
     prelude::*,
-    sync::Mutex,
+    sync::{Arc, Mutex},
     types::ARef,
     uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
 };
@@ -113,6 +171,8 @@
 const RUST_MISC_DEV_HELLO: u32 = _IO('|' as u32, 0x80);
 const RUST_MISC_DEV_GET_VALUE: u32 = _IOR::<i32>('|' as u32, 0x81);
 const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('|' as u32, 0x82);
+const RUST_MISC_DEV_GET_SHARED_VALUE: u32 = _IOR::<i32>('|' as u32, 0x83);
+const RUST_MISC_DEV_SET_SHARED_VALUE: u32 = _IOW::<i32>('|' as u32, 0x84);
 
 module! {
     type: RustMiscDeviceModule,
@@ -137,7 +197,10 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
         };
 
         try_pin_init!(Self {
-            _miscdev <- MiscDeviceRegistration::register(options, ()),
+            _miscdev <- MiscDeviceRegistration::register(
+                options,
+                Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL)?
+            ),
         })
     }
 }
@@ -148,8 +211,9 @@ struct Inner {
 
 #[pin_data(PinnedDrop)]
 struct RustMiscDevice {
+    shared: Arc<Mutex<Inner>>,
     #[pin]
-    inner: Mutex<Inner>,
+    unique: Mutex<Inner>,
     dev: ARef<Device>,
 }
 
@@ -157,7 +221,7 @@ struct RustMiscDevice {
 impl MiscDevice for RustMiscDevice {
     type Ptr = Pin<KBox<Self>>;
 
-    type Data = ();
+    type Data = Arc<Mutex<Inner>>;
 
     fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
         let dev = ARef::from(misc.device());
@@ -167,7 +231,8 @@ fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Se
         KBox::try_pin_init(
             try_pin_init! {
                 RustMiscDevice {
-                    inner <- new_mutex!( Inner{ value: 0_i32 } ),
+                    shared: misc.data().clone(),
+                    unique <- new_mutex!(Inner { value: 0_i32 }),
                     dev: dev,
                 }
             },
@@ -183,6 +248,12 @@ fn ioctl(me: Pin<&RustMiscDevice>, _file: &File, cmd: u32, arg: usize) -> Result
         match cmd {
             RUST_MISC_DEV_GET_VALUE => me.get_value(UserSlice::new(arg, size).writer())?,
             RUST_MISC_DEV_SET_VALUE => me.set_value(UserSlice::new(arg, size).reader())?,
+            RUST_MISC_DEV_GET_SHARED_VALUE => {
+                me.get_shared_value(UserSlice::new(arg, size).writer())?
+            }
+            RUST_MISC_DEV_SET_SHARED_VALUE => {
+                me.set_shared_value(UserSlice::new(arg, size).reader())?
+            }
             RUST_MISC_DEV_HELLO => me.hello()?,
             _ => {
                 dev_err!(me.dev, "-> IOCTL not recognised: {}\n", cmd);
@@ -204,7 +275,7 @@ fn drop(self: Pin<&mut Self>) {
 impl RustMiscDevice {
     fn set_value(&self, mut reader: UserSliceReader) -> Result<isize> {
         let new_value = reader.read::<i32>()?;
-        let mut guard = self.inner.lock();
+        let mut guard = self.unique.lock();
 
         dev_info!(
             self.dev,
@@ -217,7 +288,38 @@ fn set_value(&self, mut reader: UserSliceReader) -> Result<isize> {
     }
 
     fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
-        let guard = self.inner.lock();
+        let guard = self.unique.lock();
+        let value = guard.value;
+
+        // Free-up the lock and use our locally cached instance from here
+        drop(guard);
+
+        dev_info!(
+            self.dev,
+            "-> Copying data to userspace (value: {})\n",
+            &value
+        );
+
+        writer.write::<i32>(&value)?;
+        Ok(0)
+    }
+
+    fn set_shared_value(&self, mut reader: UserSliceReader) -> Result<isize> {
+        let new_value = reader.read::<i32>()?;
+        let mut guard = self.shared.lock();
+
+        dev_info!(
+            self.dev,
+            "-> Copying data from userspace (value: {})\n",
+            new_value
+        );
+
+        guard.value = new_value;
+        Ok(0)
+    }
+
+    fn get_shared_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
+        let guard = self.shared.lock();
         let value = guard.value;
 
         // Free-up the lock and use our locally cached instance from here

-- 
2.49.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 1/3] rust: implement `Wrapper<T>` for `Opaque<T>`
  2025-06-10 20:27 ` [PATCH v6 1/3] rust: implement `Wrapper<T>` for `Opaque<T>` Christian Schrefl
@ 2025-06-27 14:43   ` Miguel Ojeda
  2025-06-29 15:11   ` Danilo Krummrich
  1 sibling, 0 replies; 6+ messages in thread
From: Miguel Ojeda @ 2025-06-27 14:43 UTC (permalink / raw)
  To: Christian Schrefl
  Cc: Miguel Ojeda, Danilo Krummrich, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Arnd Bergmann, Greg Kroah-Hartman, Lee Jones, Daniel Almeida,
	Benno Lossin, Gerald Wisböck, rust-for-linux, linux-kernel

On Tue, Jun 10, 2025 at 10:28 PM Christian Schrefl
<chrisi.schrefl@gmail.com> wrote:
>
> This patch is also required for my `UnsafePinned`series. I'm not sure
> how this should be handeld (in case both series make it in this cycle).

Danilo may need it this cycle for
https://lore.kernel.org/lkml/20250626200054.243480-1-dakr@kernel.org/,
so if it goes through another tree, please feel free to take:

Acked-by: Miguel Ojeda <ojeda@kernel.org>

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 1/3] rust: implement `Wrapper<T>` for `Opaque<T>`
  2025-06-10 20:27 ` [PATCH v6 1/3] rust: implement `Wrapper<T>` for `Opaque<T>` Christian Schrefl
  2025-06-27 14:43   ` Miguel Ojeda
@ 2025-06-29 15:11   ` Danilo Krummrich
  1 sibling, 0 replies; 6+ messages in thread
From: Danilo Krummrich @ 2025-06-29 15:11 UTC (permalink / raw)
  To: Christian Schrefl
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Arnd Bergmann, Greg Kroah-Hartman, Lee Jones, Daniel Almeida,
	Benno Lossin, Gerald Wisböck, rust-for-linux, linux-kernel

On Tue, Jun 10, 2025 at 10:27:55PM +0200, Christian Schrefl wrote:
> Moves the implementation for `pin-init` from an associated function
> to the trait function of the `Wrapper` trait and extends the
> implementation to support pin-initializers with error types.
> 
> Adds a use for the `Wrapper` trait in `revocable.rs`, to use the new
> `pin-init` function. This is currently the only usage in the kernel.
> 
> Reviewed-by: Danilo Krummrich <dakr@kernel.org>
> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Benno Lossin <lossin@kernel.org>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>

Applied to driver-core-testing, thanks!

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-06-29 15:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-10 20:27 [PATCH v6 0/3] rust: miscdevice: add additional data to MiscDeviceRegistration Christian Schrefl
2025-06-10 20:27 ` [PATCH v6 1/3] rust: implement `Wrapper<T>` for `Opaque<T>` Christian Schrefl
2025-06-27 14:43   ` Miguel Ojeda
2025-06-29 15:11   ` Danilo Krummrich
2025-06-10 20:27 ` [PATCH v6 2/3] rust: miscdevice: add additional data to `MiscDeviceRegistration` Christian Schrefl
2025-06-10 20:27 ` [PATCH v6 3/3] rust: miscdevice: adjust the `rust_misc_device` sample to use `Data` Christian Schrefl

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).