[PATCH v3 0/2] rust: miscdevice: add additional data to MiscDeviceRegistration

[PATCH v3 0/2] rust: miscdevice: add additional data to MiscDeviceRegistration

[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 implements the changes and fixes the build of the sample
without changing any functionality (this is currently the only in tree 
user).

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

This series its based on my `UnsafePinned` series [0] and the 
pin-init-next branch.

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

Link: https://lore.kernel.org/rust-for-linux/20250430-rust_unsafe_pinned-v2-0-fc8617a74024@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 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 (2):
      rust: miscdevice: add additional data to MiscDeviceRegistration
      rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

 rust/kernel/miscdevice.rs        |  78 ++++++++++++++++++-------
 samples/rust/rust_misc_device.rs | 120 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 168 insertions(+), 30 deletions(-)
---
base-commit: 9de1a293c8ece00d226b21a35751ec178be2a9fa
change-id: 20250119-b4-rust_miscdevice_registrationdata-a11d88dcb284
prerequisite-change-id: 20250418-rust_unsafe_pinned-891dce27418d:v4
prerequisite-patch-id: 25a01c330b680122836e386fe455a203b8c1f0a4
prerequisite-patch-id: 988474c343f8f54d296c73f6719c35e6b1e671e1
prerequisite-patch-id: 1d97c71229c1cd9c177a6346a4c846006f05f918

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

[PATCH v3 1/2] rust: miscdevice: add additional data to MiscDeviceRegistration

[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>
Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
 rust/kernel/miscdevice.rs        | 78 +++++++++++++++++++++++++++++-----------
 samples/rust/rust_misc_device.rs |  4 ++-
 2 files changed, 60 insertions(+), 22 deletions(-)

diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index fa9ecc42602a477328a25b5d357db90b59dc72ae..43cb8b1ce4a2f112a7cab73a3d126845887833ad 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},
@@ -17,9 +17,10 @@
     prelude::*,
     seq_file::SeqFile,
     str::CStr,
-    types::{ForeignOwnable, Opaque},
+    types::{ForeignOwnable, Opaque, UnsafePinned},
 };
 use core::{marker::PhantomData, mem::MaybeUninit, pin::Pin};
+use pin_init::Wrapper;
 
 /// Options for creating a misc device.
 #[derive(Copy, Clone)]
@@ -45,32 +46,46 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
 /// # Invariants
 ///
 /// `inner` is a registered misc device.
-#[repr(transparent)]
+#[repr(C)]
 #[pin_data(PinnedDrop)]
-pub struct MiscDeviceRegistration<T> {
+pub struct MiscDeviceRegistration<T: MiscDevice> {
     #[pin]
     inner: Opaque<bindings::miscdevice>,
+    #[pin]
+    data: UnsafePinned<T::RegistrationData>,
     _t: PhantomData<T>,
 }
 
-// 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::RegistrationData` is also `Send`.
+unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
+
+// SAFETY:
+// - All `&self` methods on this type are written to ensure that it is safe to call them in
+//   parallel.
+// - `MiscDevice::RegistrationData` is always `Sync`.
+unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
 
 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::RegistrationData, Error>,
+    ) -> impl PinInit<Self, Error> {
         try_pin_init!(Self {
+            data <- UnsafePinned::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) })
@@ -93,10 +108,18 @@ 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::RegistrationData {
+        // 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()) };
@@ -109,6 +132,13 @@ pub trait MiscDevice: Sized {
     /// What kind of pointer should `Self` be wrapped in.
     type Ptr: ForeignOwnable + Send + Sync;
 
+    /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
+    /// If no additional data is required than the unit type `()` should be used.
+    ///
+    /// This data can be accessed in [`MiscDevice::open()`] using
+    /// [`MiscDeviceRegistration::data()`].
+    type RegistrationData: Sync;
+
     /// Called when the misc device is opened.
     ///
     /// The returned pointer will be stored as the private data for the file.
@@ -178,18 +208,24 @@ 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::<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.
+        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..67a6172fbbf72dd42a1b655f5f5a782101432707 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 RegistrationData = ();
+
     fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
         let dev = ARef::from(misc.device());
 

-- 
2.49.0

Re: [PATCH v3 1/2] rust: miscdevice: add additional data to MiscDeviceRegistration

[Danilo Krummrich]

On Sat, May 17, 2025 at 01:33:49PM +0200, Christian Schrefl wrote:
> +pub struct MiscDeviceRegistration<T: MiscDevice> {
>      #[pin]
>      inner: Opaque<bindings::miscdevice>,
> +    #[pin]
> +    data: UnsafePinned<T::RegistrationData>,
>      _t: PhantomData<T>,
>  }

I recommend not to store data within a Registration type itself.

I know that this is designed with the focus on using misc device directly from
the module scope; and in this context it works great.

However, it becomes quite suboptimal when used from a driver scope. For
instance, if the misc device is registered within a platform driver's probe()
function.

I know this probably isn't supported yet. At least, I assume it isn't supported
"officially", given that the abstraction does not provide an option to set a
parent device. Yet I think we should consider it.

The reason this is suboptimal is that, from the callbacks of a misc device we
may want to access device resources from the platform device.

Since device resources have to be protected with Devres, we'd need to access
them with Revocable::try_access_with() for instance.

However, it would be much better if we had proof that the parent device of the
misc device (i.e. the platform device) is bound (i.e. provide a &Device<Bound>)
and hence are able to access device resources directly.

The only way to prove this, is to prove that the misc device registration is
guaranteed to be removed when the parent device (i.e. the platform driver) is
unbound.

And this we can only prove if we wrap MiscDeviceRegistration itself in a
Devres; we don't want MiscDeviceRegistration to out-live the driver it was
registered by anyways, so that's a free optimization.

If the data above is stored directly in the MiscDeviceRegistration however it
means that we can only access it through a Devres<MiscDeviceRegistration>, which
would be annoying.

To be fair, storing data in MiscDeviceRegistration is not the main issue of why
this is suboptimal in driver, but it adds to the problem.

In general, the design of MiscDeviceRegistration is a bit suboptimal to be used
within drivers. For drivers it works much better when the Registration type
really *only* represents the state of a thing being registered, such that we can
guard it with Devres *without* any downsides or additional complexity. One
example for that would be the drm::driver::Registration [1].

If we want misc device to work optimally with drivers as well, we need to split
things in two types: `misc::Device`:

	struct Device<T: MiscDevice> {
	   #[pin]
	   misc: Opaque<bindings::miscdevice>,
	   #[pin]
	   data: UnsafePinned<T::RegistrationData>,
	   _t: PhantomData<T>,
	}

and `misc::Registration`:

	struct Registration(ARef<misc::Device>);


and make the `misc::Device` own the data, not the `misc::Registration`.

This way we can wrap misc::Registration into a Devres, with all guarantees it
gives us and an no downsides.

I'm not saying that I want to block this patch, especially given that using the
misc device abstraction doesn't seem to be supported to be used from drivers,
but please understand that the design of the misc device abstraction, while it
works fine for the module scope, really is sub-optimal for the use within
drivers and hence should be re-worked.

Can we please either do the re-work right away or add a proper TODO?

[1] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/drm/driver.rs?ref_type=heads#L121

Re: [PATCH v3 1/2] rust: miscdevice: add additional data to MiscDeviceRegistration

[Boqun Feng]

On Sat, May 17, 2025 at 03:42:29PM +0200, Danilo Krummrich wrote:
> On Sat, May 17, 2025 at 01:33:49PM +0200, Christian Schrefl wrote:
> > +pub struct MiscDeviceRegistration<T: MiscDevice> {
> >      #[pin]
> >      inner: Opaque<bindings::miscdevice>,
> > +    #[pin]
> > +    data: UnsafePinned<T::RegistrationData>,
> >      _t: PhantomData<T>,
> >  }
> 
> I recommend not to store data within a Registration type itself.
> 
> I know that this is designed with the focus on using misc device directly from
> the module scope; and in this context it works great.
> 
> However, it becomes quite suboptimal when used from a driver scope. For
> instance, if the misc device is registered within a platform driver's probe()
> function.
> 
> I know this probably isn't supported yet. At least, I assume it isn't supported
> "officially", given that the abstraction does not provide an option to set a
> parent device. Yet I think we should consider it.
> 
> The reason this is suboptimal is that, from the callbacks of a misc device we
> may want to access device resources from the platform device.
> 
> Since device resources have to be protected with Devres, we'd need to access
> them with Revocable::try_access_with() for instance.
> 
> However, it would be much better if we had proof that the parent device of the
> misc device (i.e. the platform device) is bound (i.e. provide a &Device<Bound>)
> and hence are able to access device resources directly.
> 
> The only way to prove this, is to prove that the misc device registration is
> guaranteed to be removed when the parent device (i.e. the platform driver) is
> unbound.
> 
> And this we can only prove if we wrap MiscDeviceRegistration itself in a
> Devres; we don't want MiscDeviceRegistration to out-live the driver it was
> registered by anyways, so that's a free optimization.
> 
> If the data above is stored directly in the MiscDeviceRegistration however it
> means that we can only access it through a Devres<MiscDeviceRegistration>, which
> would be annoying.
> 
> To be fair, storing data in MiscDeviceRegistration is not the main issue of why
> this is suboptimal in driver, but it adds to the problem.
> 
> In general, the design of MiscDeviceRegistration is a bit suboptimal to be used
> within drivers. For drivers it works much better when the Registration type
> really *only* represents the state of a thing being registered, such that we can
> guard it with Devres *without* any downsides or additional complexity. One
> example for that would be the drm::driver::Registration [1].
> 
> If we want misc device to work optimally with drivers as well, we need to split
> things in two types: `misc::Device`:
> 
> 	struct Device<T: MiscDevice> {
> 	   #[pin]
> 	   misc: Opaque<bindings::miscdevice>,
> 	   #[pin]
> 	   data: UnsafePinned<T::RegistrationData>,
> 	   _t: PhantomData<T>,
> 	}
> 
> and `misc::Registration`:
> 
> 	struct Registration(ARef<misc::Device>);
> 
> 
> and make the `misc::Device` own the data, not the `misc::Registration`.
> 
> This way we can wrap misc::Registration into a Devres, with all guarantees it
> gives us and an no downsides.
> 
> I'm not saying that I want to block this patch, especially given that using the
> misc device abstraction doesn't seem to be supported to be used from drivers,
> but please understand that the design of the misc device abstraction, while it
> works fine for the module scope, really is sub-optimal for the use within
> drivers and hence should be re-worked.
> 
> Can we please either do the re-work right away or add a proper TODO?
> 

Well, I'd say we do the re-work right away, because I don't see any
other work depends on this right now. Let's do the right thing.

Regards,
Boqun

> [1] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/drm/driver.rs?ref_type=heads#L121

Re: [PATCH v3 1/2] rust: miscdevice: add additional data to MiscDeviceRegistration

[Greg Kroah-Hartman]

On Sat, May 17, 2025 at 03:42:29PM +0200, Danilo Krummrich wrote:
> On Sat, May 17, 2025 at 01:33:49PM +0200, Christian Schrefl wrote:
> > +pub struct MiscDeviceRegistration<T: MiscDevice> {
> >      #[pin]
> >      inner: Opaque<bindings::miscdevice>,
> > +    #[pin]
> > +    data: UnsafePinned<T::RegistrationData>,
> >      _t: PhantomData<T>,
> >  }
> 
> I recommend not to store data within a Registration type itself.
> 
> I know that this is designed with the focus on using misc device directly from
> the module scope; and in this context it works great.
> 
> However, it becomes quite suboptimal when used from a driver scope. For
> instance, if the misc device is registered within a platform driver's probe()
> function.
> 
> I know this probably isn't supported yet. At least, I assume it isn't supported
> "officially", given that the abstraction does not provide an option to set a
> parent device. Yet I think we should consider it.

It's going to be a requirement to properly set the parent device, and
as you point out, this really should be in some sort of scope, not just
a module.

But, we have two types of users of a misc device, one like this is
written, for a module-scope, and one for the "normal" device scope.  The
device scope is going to be tricker as it can, and will, get
disconnected from the device separately from the misc device lifespan,
so when that logic is added, it's going to be tricky as you point out.

So I'll take this now, but in the future this is going to have to be
cleaned up and modified.

thanks,

greg k-h

Re: [PATCH v3 1/2] rust: miscdevice: add additional data to MiscDeviceRegistration

[Greg Kroah-Hartman]

On Wed, May 21, 2025 at 01:55:36PM +0200, Greg Kroah-Hartman wrote:
> On Sat, May 17, 2025 at 03:42:29PM +0200, Danilo Krummrich wrote:
> > On Sat, May 17, 2025 at 01:33:49PM +0200, Christian Schrefl wrote:
> > > +pub struct MiscDeviceRegistration<T: MiscDevice> {
> > >      #[pin]
> > >      inner: Opaque<bindings::miscdevice>,
> > > +    #[pin]
> > > +    data: UnsafePinned<T::RegistrationData>,
> > >      _t: PhantomData<T>,
> > >  }
> > 
> > I recommend not to store data within a Registration type itself.
> > 
> > I know that this is designed with the focus on using misc device directly from
> > the module scope; and in this context it works great.
> > 
> > However, it becomes quite suboptimal when used from a driver scope. For
> > instance, if the misc device is registered within a platform driver's probe()
> > function.
> > 
> > I know this probably isn't supported yet. At least, I assume it isn't supported
> > "officially", given that the abstraction does not provide an option to set a
> > parent device. Yet I think we should consider it.
> 
> It's going to be a requirement to properly set the parent device, and
> as you point out, this really should be in some sort of scope, not just
> a module.
> 
> But, we have two types of users of a misc device, one like this is
> written, for a module-scope, and one for the "normal" device scope.  The
> device scope is going to be tricker as it can, and will, get
> disconnected from the device separately from the misc device lifespan,
> so when that logic is added, it's going to be tricky as you point out.
> 
> So I'll take this now, but in the future this is going to have to be
> cleaned up and modified.

Nope, can't take it, it breaks the build from my tree:

error[E0432]: unresolved import `crate::types::UnsafePinned`
  --> rust/kernel/miscdevice.rs:20:37
   |
20 |     types::{ForeignOwnable, Opaque, UnsafePinned},
   |                                     ^^^^^^^^^^^^ no `UnsafePinned` in `types`

error[E0432]: unresolved import `pin_init::Wrapper`
  --> rust/kernel/miscdevice.rs:23:5
   |
23 | use pin_init::Wrapper;
   |     ^^^^^^^^^^^^^^^^^ no `Wrapper` in the root

error: aborting due to 2 previous errors

:(

Re: [PATCH v3 1/2] rust: miscdevice: add additional data to MiscDeviceRegistration

[Christian Schrefl]

Hi Greg,

On 21.05.25 2:04 PM, Greg Kroah-Hartman wrote:
> On Wed, May 21, 2025 at 01:55:36PM +0200, Greg Kroah-Hartman wrote:
>> On Sat, May 17, 2025 at 03:42:29PM +0200, Danilo Krummrich wrote:
>>> On Sat, May 17, 2025 at 01:33:49PM +0200, Christian Schrefl wrote:
>>>> +pub struct MiscDeviceRegistration<T: MiscDevice> {
>>>>      #[pin]
>>>>      inner: Opaque<bindings::miscdevice>,
>>>> +    #[pin]
>>>> +    data: UnsafePinned<T::RegistrationData>,
>>>>      _t: PhantomData<T>,
>>>>  }
>>>
>>> I recommend not to store data within a Registration type itself.
>>>
>>> I know that this is designed with the focus on using misc device directly from
>>> the module scope; and in this context it works great.
>>>
>>> However, it becomes quite suboptimal when used from a driver scope. For
>>> instance, if the misc device is registered within a platform driver's probe()
>>> function.
>>>
>>> I know this probably isn't supported yet. At least, I assume it isn't supported
>>> "officially", given that the abstraction does not provide an option to set a
>>> parent device. Yet I think we should consider it.
>>
>> It's going to be a requirement to properly set the parent device, and
>> as you point out, this really should be in some sort of scope, not just
>> a module.
>>
>> But, we have two types of users of a misc device, one like this is
>> written, for a module-scope, and one for the "normal" device scope.  The
>> device scope is going to be tricker as it can, and will, get
>> disconnected from the device separately from the misc device lifespan,
>> so when that logic is added, it's going to be tricky as you point out.
>>
>> So I'll take this now, but in the future this is going to have to be
>> cleaned up and modified.
> 
> Nope, can't take it, it breaks the build from my tree:
> 
> error[E0432]: unresolved import `crate::types::UnsafePinned`
>   --> rust/kernel/miscdevice.rs:20:37
>    |
> 20 |     types::{ForeignOwnable, Opaque, UnsafePinned},
>    |                                     ^^^^^^^^^^^^ no `UnsafePinned` in `types`
> 
> error[E0432]: unresolved import `pin_init::Wrapper`
>   --> rust/kernel/miscdevice.rs:23:5
>    |
> 23 | use pin_init::Wrapper;
>    |     ^^^^^^^^^^^^^^^^^ no `Wrapper` in the root
> 
> error: aborting due to 2 previous errors
> 
> :(

This requires my `UnsafePinned` [0] patches (& with that pin-init-next) like I wrote
in the cover letter.

[0]: https://lore.kernel.org/rust-for-linux/20250511-rust_unsafe_pinned-v4-0-a86c32e47e3d@gmail.com/

Cheers
Christian

Re: [PATCH v3 1/2] rust: miscdevice: add additional data to MiscDeviceRegistration

[Greg Kroah-Hartman]

On Wed, May 21, 2025 at 02:16:20PM +0200, Christian Schrefl wrote:
> Hi Greg,
> 
> On 21.05.25 2:04 PM, Greg Kroah-Hartman wrote:
> > On Wed, May 21, 2025 at 01:55:36PM +0200, Greg Kroah-Hartman wrote:
> >> On Sat, May 17, 2025 at 03:42:29PM +0200, Danilo Krummrich wrote:
> >>> On Sat, May 17, 2025 at 01:33:49PM +0200, Christian Schrefl wrote:
> >>>> +pub struct MiscDeviceRegistration<T: MiscDevice> {
> >>>>      #[pin]
> >>>>      inner: Opaque<bindings::miscdevice>,
> >>>> +    #[pin]
> >>>> +    data: UnsafePinned<T::RegistrationData>,
> >>>>      _t: PhantomData<T>,
> >>>>  }
> >>>
> >>> I recommend not to store data within a Registration type itself.
> >>>
> >>> I know that this is designed with the focus on using misc device directly from
> >>> the module scope; and in this context it works great.
> >>>
> >>> However, it becomes quite suboptimal when used from a driver scope. For
> >>> instance, if the misc device is registered within a platform driver's probe()
> >>> function.
> >>>
> >>> I know this probably isn't supported yet. At least, I assume it isn't supported
> >>> "officially", given that the abstraction does not provide an option to set a
> >>> parent device. Yet I think we should consider it.
> >>
> >> It's going to be a requirement to properly set the parent device, and
> >> as you point out, this really should be in some sort of scope, not just
> >> a module.
> >>
> >> But, we have two types of users of a misc device, one like this is
> >> written, for a module-scope, and one for the "normal" device scope.  The
> >> device scope is going to be tricker as it can, and will, get
> >> disconnected from the device separately from the misc device lifespan,
> >> so when that logic is added, it's going to be tricky as you point out.
> >>
> >> So I'll take this now, but in the future this is going to have to be
> >> cleaned up and modified.
> > 
> > Nope, can't take it, it breaks the build from my tree:
> > 
> > error[E0432]: unresolved import `crate::types::UnsafePinned`
> >   --> rust/kernel/miscdevice.rs:20:37
> >    |
> > 20 |     types::{ForeignOwnable, Opaque, UnsafePinned},
> >    |                                     ^^^^^^^^^^^^ no `UnsafePinned` in `types`
> > 
> > error[E0432]: unresolved import `pin_init::Wrapper`
> >   --> rust/kernel/miscdevice.rs:23:5
> >    |
> > 23 | use pin_init::Wrapper;
> >    |     ^^^^^^^^^^^^^^^^^ no `Wrapper` in the root
> > 
> > error: aborting due to 2 previous errors
> > 
> > :(
> 
> This requires my `UnsafePinned` [0] patches (& with that pin-init-next) like I wrote
> in the cover letter.
> 
> [0]: https://lore.kernel.org/rust-for-linux/20250511-rust_unsafe_pinned-v4-0-a86c32e47e3d@gmail.com/

I never read cover letters, all of the needed information should be in
the individual commits as that's what ends up in the changelog :)

thanks,

greg k-h

Re: [PATCH v3 1/2] rust: miscdevice: add additional data to MiscDeviceRegistration

[Alice Ryhl]

On Wed, May 21, 2025 at 02:16:20PM +0200, Christian Schrefl wrote:
> Hi Greg,
> 
> On 21.05.25 2:04 PM, Greg Kroah-Hartman wrote:
> > On Wed, May 21, 2025 at 01:55:36PM +0200, Greg Kroah-Hartman wrote:
> >> On Sat, May 17, 2025 at 03:42:29PM +0200, Danilo Krummrich wrote:
> >>> On Sat, May 17, 2025 at 01:33:49PM +0200, Christian Schrefl wrote:
> >>>> +pub struct MiscDeviceRegistration<T: MiscDevice> {
> >>>>      #[pin]
> >>>>      inner: Opaque<bindings::miscdevice>,
> >>>> +    #[pin]
> >>>> +    data: UnsafePinned<T::RegistrationData>,
> >>>>      _t: PhantomData<T>,
> >>>>  }
> >>>
> >>> I recommend not to store data within a Registration type itself.
> >>>
> >>> I know that this is designed with the focus on using misc device directly from
> >>> the module scope; and in this context it works great.
> >>>
> >>> However, it becomes quite suboptimal when used from a driver scope. For
> >>> instance, if the misc device is registered within a platform driver's probe()
> >>> function.
> >>>
> >>> I know this probably isn't supported yet. At least, I assume it isn't supported
> >>> "officially", given that the abstraction does not provide an option to set a
> >>> parent device. Yet I think we should consider it.
> >>
> >> It's going to be a requirement to properly set the parent device, and
> >> as you point out, this really should be in some sort of scope, not just
> >> a module.
> >>
> >> But, we have two types of users of a misc device, one like this is
> >> written, for a module-scope, and one for the "normal" device scope.  The
> >> device scope is going to be tricker as it can, and will, get
> >> disconnected from the device separately from the misc device lifespan,
> >> so when that logic is added, it's going to be tricky as you point out.
> >>
> >> So I'll take this now, but in the future this is going to have to be
> >> cleaned up and modified.
> > 
> > Nope, can't take it, it breaks the build from my tree:
> > 
> > error[E0432]: unresolved import `crate::types::UnsafePinned`
> >   --> rust/kernel/miscdevice.rs:20:37
> >    |
> > 20 |     types::{ForeignOwnable, Opaque, UnsafePinned},
> >    |                                     ^^^^^^^^^^^^ no `UnsafePinned` in `types`
> > 
> > error[E0432]: unresolved import `pin_init::Wrapper`
> >   --> rust/kernel/miscdevice.rs:23:5
> >    |
> > 23 | use pin_init::Wrapper;
> >    |     ^^^^^^^^^^^^^^^^^ no `Wrapper` in the root
> > 
> > error: aborting due to 2 previous errors
> > 
> > :(
> 
> This requires my `UnsafePinned` [0] patches (& with that pin-init-next) like I wrote
> in the cover letter.
> 
> [0]: https://lore.kernel.org/rust-for-linux/20250511-rust_unsafe_pinned-v4-0-a86c32e47e3d@gmail.com/

It looks like there's still a fair amount of discussion going on there.
Do you want to just use Opaque for now and then I'll fix it later once
the other series lands?

Alice

Re: [PATCH v3 1/2] rust: miscdevice: add additional data to MiscDeviceRegistration

[Danilo Krummrich]

On Wed, May 21, 2025 at 01:55:36PM +0200, Greg Kroah-Hartman wrote:
> On Sat, May 17, 2025 at 03:42:29PM +0200, Danilo Krummrich wrote:
> > On Sat, May 17, 2025 at 01:33:49PM +0200, Christian Schrefl wrote:
> > > +pub struct MiscDeviceRegistration<T: MiscDevice> {
> > >      #[pin]
> > >      inner: Opaque<bindings::miscdevice>,
> > > +    #[pin]
> > > +    data: UnsafePinned<T::RegistrationData>,
> > >      _t: PhantomData<T>,
> > >  }
> > 
> > I recommend not to store data within a Registration type itself.
> > 
> > I know that this is designed with the focus on using misc device directly from
> > the module scope; and in this context it works great.
> > 
> > However, it becomes quite suboptimal when used from a driver scope. For
> > instance, if the misc device is registered within a platform driver's probe()
> > function.
> > 
> > I know this probably isn't supported yet. At least, I assume it isn't supported
> > "officially", given that the abstraction does not provide an option to set a
> > parent device. Yet I think we should consider it.
> 
> It's going to be a requirement to properly set the parent device, and
> as you point out, this really should be in some sort of scope, not just
> a module.
> 
> But, we have two types of users of a misc device, one like this is
> written, for a module-scope, and one for the "normal" device scope.  The
> device scope is going to be tricker as it can, and will, get
> disconnected from the device separately from the misc device lifespan,
> so when that logic is added, it's going to be tricky as you point out.
> 
> So I'll take this now, but in the future this is going to have to be
> cleaned up and modified.

I'm about to sketch up something based on this patch that works properly for
both cases, i.e. module-scope and driver-scope.

I think it would also be good for the misc device abstraction to demonstrate
how to properly make class device abstractions (such as misc device, DRM device,
input device etc.) go along with bus devices in the context of a driver.

misc device isn't *the* perfect example, given that it doesn't have the typical
create and register split and another complication is that we also have to deal
with the module-scope case, but it's still a very good candidate given that it
is very simple compared to other class devices.

Re: [PATCH v3 1/2] rust: miscdevice: add additional data to MiscDeviceRegistration

[Christian Schrefl]

Hi Danilo

On 21.05.25 4:12 PM, Danilo Krummrich wrote:
> On Wed, May 21, 2025 at 01:55:36PM +0200, Greg Kroah-Hartman wrote:
>> On Sat, May 17, 2025 at 03:42:29PM +0200, Danilo Krummrich wrote:
>>> On Sat, May 17, 2025 at 01:33:49PM +0200, Christian Schrefl wrote:
>>>> +pub struct MiscDeviceRegistration<T: MiscDevice> {
>>>>      #[pin]
>>>>      inner: Opaque<bindings::miscdevice>,
>>>> +    #[pin]
>>>> +    data: UnsafePinned<T::RegistrationData>,
>>>>      _t: PhantomData<T>,
>>>>  }
>>>
>>> I recommend not to store data within a Registration type itself.
>>>
>>> I know that this is designed with the focus on using misc device directly from
>>> the module scope; and in this context it works great.
>>>
>>> However, it becomes quite suboptimal when used from a driver scope. For
>>> instance, if the misc device is registered within a platform driver's probe()
>>> function.
>>>
>>> I know this probably isn't supported yet. At least, I assume it isn't supported
>>> "officially", given that the abstraction does not provide an option to set a
>>> parent device. Yet I think we should consider it.
>>
>> It's going to be a requirement to properly set the parent device, and
>> as you point out, this really should be in some sort of scope, not just
>> a module.
>>
>> But, we have two types of users of a misc device, one like this is
>> written, for a module-scope, and one for the "normal" device scope.  The
>> device scope is going to be tricker as it can, and will, get
>> disconnected from the device separately from the misc device lifespan,
>> so when that logic is added, it's going to be tricky as you point out.
>>
>> So I'll take this now, but in the future this is going to have to be
>> cleaned up and modified.
> 
> I'm about to sketch up something based on this patch that works properly for
> both cases, i.e. module-scope and driver-scope.

Let me know if you have any questions for me (if you want also privately 
or on Zulip).

I currently don't have the time to work on this myself. I'm not that
familiar with the C side and the Devres abstraction and would need to look
into these before getting much work done here.

> I think it would also be good for the misc device abstraction to demonstrate
> how to properly make class device abstractions (such as misc device, DRM device,
> input device etc.) go along with bus devices in the context of a driver.
> 
> misc device isn't *the* perfect example, given that it doesn't have the typical
> create and register split and another complication is that we also have to deal
> with the module-scope case, but it's still a very good candidate given that it
> is very simple compared to other class devices.

Cheers
Christian

[PATCH v3 2/2] rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

[Christian Schrefl]

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

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

Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
 samples/rust/rust_misc_device.rs | 120 +++++++++++++++++++++++++++++++++++----
 1 file changed, 110 insertions(+), 10 deletions(-)

diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index 67a6172fbbf72dd42a1b655f5f5a782101432707..3c96cf8fe747427106f2e436c3dba33008c7fd53 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);
@@ -94,7 +152,6 @@
 //!   return 0;
 //! }
 //! ```
-
 use core::pin::Pin;
 
 use kernel::{
@@ -105,7 +162,7 @@
     miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
     new_mutex,
     prelude::*,
-    sync::Mutex,
+    sync::{Arc, Mutex},
     types::ARef,
     uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
 };
@@ -113,6 +170,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,
@@ -130,14 +189,17 @@ struct RustMiscDeviceModule {
 
 impl kernel::InPlaceModule for RustMiscDeviceModule {
     fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
-        pr_info!("Initialising Rust Misc Device Sample\n");
+        pr_info!("Initializing Rust Misc Device Sample\n");
 
         let options = MiscDeviceOptions {
             name: c_str!("rust-misc-device"),
         };
 
         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 +210,9 @@ struct Inner {
 
 #[pin_data(PinnedDrop)]
 struct RustMiscDevice {
+    shared: Arc<Mutex<Inner>>,
     #[pin]
-    inner: Mutex<Inner>,
+    unique: Mutex<Inner>,
     dev: ARef<Device>,
 }
 
@@ -157,7 +220,7 @@ struct RustMiscDevice {
 impl MiscDevice for RustMiscDevice {
     type Ptr = Pin<KBox<Self>>;
 
-    type RegistrationData = ();
+    type RegistrationData = Arc<Mutex<Inner>>;
 
     fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
         let dev = ARef::from(misc.device());
@@ -167,7 +230,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 +247,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);
@@ -193,7 +263,6 @@ fn ioctl(me: Pin<&RustMiscDevice>, _file: &File, cmd: u32, arg: usize) -> Result
         Ok(0)
     }
 }
-
 #[pinned_drop]
 impl PinnedDrop for RustMiscDevice {
     fn drop(self: Pin<&mut Self>) {
@@ -204,7 +273,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 +286,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

Re: [PATCH v3 0/2] rust: miscdevice: add additional data to MiscDeviceRegistration

[Alice Ryhl]

On Sat, May 17, 2025 at 01:33:48PM +0200, Christian Schrefl wrote:
> 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 implements the changes and fixes the build of the sample
> without changing any functionality (this is currently the only in tree 
> user).
> 
> The second 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.
> 
> This series its based on my `UnsafePinned` series [0] and the 
> pin-init-next branch.
> 
> Some discussion on Zulip about the motivation and approach [1].
> Thanks a lot to everyone helping me out with this.
> 
> Link: https://lore.kernel.org/rust-for-linux/20250430-rust_unsafe_pinned-v2-0-fc8617a74024@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>

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

Danilo's comment on the design of miscdevice is fair - it does not match
other types of drivers. But this series is an improvement over what is
here today.

Alice