[PATCH 0/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[PATCH 0/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Christian Schrefl]

This set is based on Greg's driver-core-next tree.

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 add the Aliased type.

The second patch implements the changes and fixes the build of the sample.

The third patch changes the rust_misc_device sample to use this to 
share the same data between multiple handles to the miscdevice.

Some discussion on Zulip about the motivation and approach
(Thanks a lot to everyone helping me out with this):
Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Passing.20a.20DevRes.20to.20a.20miscdev/near/494553814

Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
Christian Schrefl (3):
      rust: add Aliased type
      rust: miscdevice: Add additional data to MiscDeviceRegistration
      rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

 rust/kernel/miscdevice.rs        | 37 ++++++++++++++++++++++++++++++-------
 rust/kernel/types.rs             | 40 ++++++++++++++++++++++++++++++++++++++++
 samples/rust/rust_misc_device.rs | 22 ++++++++++++----------
 3 files changed, 82 insertions(+), 17 deletions(-)
---
base-commit: 01b3cb620815fc3feb90ee117d9445a5b608a9f7
change-id: 20250119-b4-rust_miscdevice_registrationdata-a11d88dcb284

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

[PATCH 1/3] rust: add Aliased type

[Christian Schrefl]

This type is useful for cases where a value might be shared with C code
but not interpreted by it.
In partiquarly this is added to for data that is shared between a Driver
and a MiscDevice implementation.

Similar to Opaque but guarantees that the value is initialized and the
inner value is dropped when Aliased is dropped.

This was origianally proposed for the IRQ abstractions [0], but also
useful for other cases where Data may be aliased, but is always valid
and automatic drop is desired.

Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiOASgjoYKFz6kWwzLaH07DqP2ph+3YyCDh2+gYqGpABA@mail.gmail.com [0]
Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
 rust/kernel/types.rs | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 3aea6af9a0bca70ee42b4bad2fe31a99750cbf11..5640128c9a9055476a0040033946ba6caa6e7076 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -528,3 +528,43 @@ pub enum Either<L, R> {
 /// [`NotThreadSafe`]: type@NotThreadSafe
 #[allow(non_upper_case_globals)]
 pub const NotThreadSafe: NotThreadSafe = PhantomData;
+
+/// Stores a value that may be aliased.
+///
+/// This is similar to `Opaque<T>` but is guaranteed to contain valid data and will
+/// Call the Drop implementation of T when dropped.
+#[repr(transparent)]
+pub struct Aliased<T> {
+    value: UnsafeCell<T>,
+    _pin: PhantomPinned,
+}
+
+impl<T> Aliased<T> {
+    /// Creates a new `Aliased` value.
+    pub const fn new(value: T) -> Self {
+        Self {
+            value: UnsafeCell::new(value),
+            _pin: PhantomPinned,
+        }
+    }
+    /// Create an `Aliased` pin-initializer from the given pin-initializer.
+    pub fn try_pin_init<E>(value: impl PinInit<T, E>) -> impl PinInit<Self, E> {
+        // SAFETY:
+        // In case of an error in value the error is returned, otherwise the slot is fully initialized,
+        // since value is initialized and _pin is a Zero sized type.
+        // The pin invariants of value are upheld, since no moving occurs.
+        unsafe { init::pin_init_from_closure(move |slot| value.__pinned_init(Self::raw_get(slot))) }
+    }
+    /// Returns a raw pointer to the opaque data.
+    pub const fn get(&self) -> *mut T {
+        UnsafeCell::get(&self.value).cast::<T>()
+    }
+
+    /// Gets the value behind `this`.
+    ///
+    /// This function is useful to get access to the value without creating intermediate
+    /// references.
+    pub const fn raw_get(this: *const Self) -> *mut T {
+        UnsafeCell::raw_get(this.cast::<UnsafeCell<MaybeUninit<T>>>()).cast::<T>()
+    }
+}

-- 
2.48.1

[PATCH 2/3] 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`.

Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
 rust/kernel/miscdevice.rs        | 37 ++++++++++++++++++++++++++++++-------
 samples/rust/rust_misc_device.rs |  4 +++-
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index dfb363630c70b7187cae91f692d38bcf42d56a0a..3a756de27644e8a14e5bbd6b8abd9604e05faed4 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -16,7 +16,7 @@
     prelude::*,
     seq_file::SeqFile,
     str::CStr,
-    types::{ForeignOwnable, Opaque},
+    types::{Aliased, ForeignOwnable, Opaque},
 };
 use core::{
     ffi::{c_int, c_long, c_uint, c_ulong},
@@ -49,24 +49,30 @@ 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: Aliased<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> {}
+unsafe impl<T: MiscDevice<RegistrationData: Send>> 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> {}
+// 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 {
             inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
                 // SAFETY: The initializer can write to the provided `slot`.
@@ -79,6 +85,7 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
                 // misc device.
                 to_result(unsafe { bindings::misc_register(slot) })
             }),
+            data <- Aliased::try_pin_init(data),
             _t: PhantomData,
         })
     }
@@ -97,10 +104,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()) };
@@ -113,6 +128,11 @@ 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 () should be used.
+    /// This data can be accessed in `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.
@@ -218,6 +238,9 @@ impl<T: MiscDevice> VtableHelper<T> {
     // 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`.
+    // Since this the `MiscDeviceRegistration` struct uses `#[repr(C)]` and the miscdevice is the
+    // first entry it is guaranteed that the address of the miscdevice is the same as the address
+    // of the entire `MiscDeviceRegistration` struct.
     let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() };
 
     // SAFETY:
diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index 40ad7266c2252e5c0b4e91e501ef9ada2eda3b16..779fcfd64119bdd5b4f8be740f7e8336c652b4d3 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -136,7 +136,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
         };
 
         try_pin_init!(Self {
-            _miscdev <- MiscDeviceRegistration::register(options),
+            _miscdev <- MiscDeviceRegistration::register(options, ()),
         })
     }
 }
@@ -156,6 +156,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.48.1

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

[Christian Schrefl]

Share the mutex stored in RustMiscDevice between all instances 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 | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index 779fcfd64119bdd5b4f8be740f7e8336c652b4d3..956b3b2bd8ef7455fcfe283dd140690ac325c0fb 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -104,7 +104,7 @@
     miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
     new_mutex,
     prelude::*,
-    sync::Mutex,
+    sync::{Arc, Mutex},
     types::ARef,
     uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
 };
@@ -136,7 +136,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)?
+            ),
         })
     }
 }
@@ -145,10 +148,8 @@ struct Inner {
     value: i32,
 }
 
-#[pin_data(PinnedDrop)]
 struct RustMiscDevice {
-    #[pin]
-    inner: Mutex<Inner>,
+    inner: Arc<Mutex<Inner>>,
     dev: ARef<Device>,
 }
 
@@ -156,7 +157,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());
@@ -164,9 +165,9 @@ fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Se
         dev_info!(dev, "Opening Rust Misc Device Sample\n");
 
         KBox::try_pin_init(
-            try_pin_init! {
+            try_init! {
                 RustMiscDevice {
-                    inner <- new_mutex!( Inner{ value: 0_i32 } ),
+                    inner: misc.data().clone(),
                     dev: dev,
                 }
             },
@@ -193,9 +194,8 @@ fn ioctl(me: Pin<&RustMiscDevice>, _file: &File, cmd: u32, arg: usize) -> Result
     }
 }
 
-#[pinned_drop]
-impl PinnedDrop for RustMiscDevice {
-    fn drop(self: Pin<&mut Self>) {
+impl Drop for RustMiscDevice {
+    fn drop(&mut self) {
         dev_info!(self.dev, "Exiting the Rust Misc Device Sample\n");
     }
 }

-- 
2.48.1

Re: [PATCH 1/3] rust: add Aliased type

[Miguel Ojeda]

Hi Christian,

Thanks for the series! A few quick comments on the documentation...

On Sun, Jan 19, 2025 at 11:11 PM Christian Schrefl
<chrisi.schrefl@gmail.com> wrote:
>
> This type is useful for cases where a value might be shared with C code
> but not interpreted by it.
> In partiquarly this is added to for data that is shared between a Driver
> and a MiscDevice implementation.

Typo: partiquarly

Also, normally you would leave an empty line between paragraphs, if
that is what you intended.

> Similar to Opaque but guarantees that the value is initialized and the
> inner value is dropped when Aliased is dropped.
>
> This was origianally proposed for the IRQ abstractions [0], but also

Typo: origianally

I usually recommend using `checkpatch.pl` with `--codespell` (it may
have caught these).

> +/// Stores a value that may be aliased.
> +///
> +/// This is similar to `Opaque<T>` but is guaranteed to contain valid data and will

Would an intra-doc link be possible? Please use then wherever reasonable.

> +/// Call the Drop implementation of T when dropped.

"Call" -> "call"?

Also, please format `Drop` and `T` and add the intra-doc link for the former.

Could we have an example or two for the type? More generally, you have
some nice comments in the commit message, so it is best to put some of
that in the actual documentation, which is what most people will read,
so that they don't need to search for the commit that introduced it.

> +    }
> +    /// Create an `Aliased` pin-initializer from the given pin-initializer.

Newline between methods/functions/etc.

> +    pub fn try_pin_init<E>(value: impl PinInit<T, E>) -> impl PinInit<Self, E> {
> +        // SAFETY:
> +        // In case of an error in value the error is returned, otherwise the slot is fully initialized,
> +        // since value is initialized and _pin is a Zero sized type.
> +        // The pin invariants of value are upheld, since no moving occurs.

For lists of requirements, we typically use bullets, e.g. `  - `. I
think this applies to another patch too.

Also, please format comments with Markdown as well (no need for
intra-doc links, of course).

> +    }
> +    /// Returns a raw pointer to the opaque data.

Missing newline?

Cheers,
Miguel

Re: [PATCH 1/3] rust: add Aliased type

[Christian Schrefl]

On 20.01.25 12:04 AM, Miguel Ojeda wrote:
> Hi Christian,
> 
> Thanks for the series! A few quick comments on the documentation...
> 
> On Sun, Jan 19, 2025 at 11:11 PM Christian Schrefl
> <chrisi.schrefl@gmail.com> wrote:
>>
>> This type is useful for cases where a value might be shared with C code
>> but not interpreted by it.
>> In partiquarly this is added to for data that is shared between a Driver
>> and a MiscDevice implementation.
> 
> Typo: partiquarly

Fixed the typos and missing newlines `--codespell` did not catch them.

> Also, normally you would leave an empty line between paragraphs, if
> that is what you intended.
> 
>> Similar to Opaque but guarantees that the value is initialized and the
>> inner value is dropped when Aliased is dropped.
>>
>> This was origianally proposed for the IRQ abstractions [0], but also
> 
> Typo: origianally
> 
> I usually recommend using `checkpatch.pl` with `--codespell` (it may
> have caught these).
>> +/// Stores a value that may be aliased.
>> +///
>> +/// This is similar to `Opaque<T>` but is guaranteed to contain valid data and will
> 
> Would an intra-doc link be possible? Please use then wherever reasonable.

Fixed

> 
>> +/// Call the Drop implementation of T when dropped.
> 
> "Call" -> "call"?
> 
> Also, please format `Drop` and `T` and add the intra-doc link for the former.
> 
> Could we have an example or two for the type? More generally, you have
> some nice comments in the commit message, so it is best to put some of
> that in the actual documentation, which is what most people will read,
> so that they don't need to search for the commit that introduced it.

I added some more to the documentation and also mentioned it in the 
`Opaque` docs.

> 
>> +    }
>> +    /// Create an `Aliased` pin-initializer from the given pin-initializer.
> 
> Newline between methods/functions/etc.
> 
>> +    pub fn try_pin_init<E>(value: impl PinInit<T, E>) -> impl PinInit<Self, E> {
>> +        // SAFETY:
>> +        // In case of an error in value the error is returned, otherwise the slot is fully initialized,
>> +        // since value is initialized and _pin is a Zero sized type.
>> +        // The pin invariants of value are upheld, since no moving occurs.
> 
> For lists of requirements, we typically use bullets, e.g. `  - `. I
> think this applies to another patch too.

Changed it, also fixed the wrapping.

> 
> Also, please format comments with Markdown as well (no need for
> intra-doc links, of course).
> 
>> +    }
>> +    /// Returns a raw pointer to the opaque data.
Changed this to contained data.

> 
> Missing newline?
> 
> Cheers,
> Miguel

In case you want to take a look the current version of the patch is available at:
https://github.com/onestacked/linux/commits/b4/rust_miscdevice_registrationdata/

Cheers
Christian

Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Christian Schrefl]

On 19.01.25 11:11 PM, Christian Schrefl wrote:
> 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`.
> 
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> ---
>  rust/kernel/miscdevice.rs        | 37 ++++++++++++++++++++++++++++++-------
>  samples/rust/rust_misc_device.rs |  4 +++-
>  2 files changed, 33 insertions(+), 8 deletions(-)
> 
> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index dfb363630c70b7187cae91f692d38bcf42d56a0a..3a756de27644e8a14e5bbd6b8abd9604e05faed4 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -16,7 +16,7 @@
>      prelude::*,
>      seq_file::SeqFile,
>      str::CStr,
> -    types::{ForeignOwnable, Opaque},
> +    types::{Aliased, ForeignOwnable, Opaque},
>  };
>  use core::{
>      ffi::{c_int, c_long, c_uint, c_ulong},
> @@ -49,24 +49,30 @@ 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: Aliased<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> {}
> +unsafe impl<T: MiscDevice<RegistrationData: Send>> 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> {}
> +// 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 {
>              inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
>                  // SAFETY: The initializer can write to the provided `slot`.
> @@ -79,6 +85,7 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>                  // misc device.
>                  to_result(unsafe { bindings::misc_register(slot) })
>              }),
> +            data <- Aliased::try_pin_init(data),
>              _t: PhantomData,
>          })
>      }
> @@ -97,10 +104,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()) };
> @@ -113,6 +128,11 @@ 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 () should be used.
> +    /// This data can be accessed in `open()` using `MiscDeviceRegistration::data()`.

Changed to intra-doc links.

> +    type RegistrationData: Sync;
> +
>      /// Called when the misc device is opened.
>      ///
>      /// The returned pointer will be stored as the private data for the file.
> @@ -218,6 +238,9 @@ impl<T: MiscDevice> VtableHelper<T> {
>      // 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`.
> +    // Since this the `MiscDeviceRegistration` struct uses `#[repr(C)]` and the miscdevice is the
> +    // first entry it is guaranteed that the address of the miscdevice is the same as the address
> +    // of the entire `MiscDeviceRegistration` struct.
>      let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() };
>  
>      // SAFETY:
> diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> index 40ad7266c2252e5c0b4e91e501ef9ada2eda3b16..779fcfd64119bdd5b4f8be740f7e8336c652b4d3 100644
> --- a/samples/rust/rust_misc_device.rs
> +++ b/samples/rust/rust_misc_device.rs
> @@ -136,7 +136,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>          };
>  
>          try_pin_init!(Self {
> -            _miscdev <- MiscDeviceRegistration::register(options),
> +            _miscdev <- MiscDeviceRegistration::register(options, ()),
>          })
>      }
>  }
> @@ -156,6 +156,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());
>  
> 

Re: [PATCH 0/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Greg Kroah-Hartman]

On Sun, Jan 19, 2025 at 11:11:12PM +0100, Christian Schrefl wrote:
> This set is based on Greg's driver-core-next tree.
> 
> 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.

Why would you want to do this?  A misc device instance should be "self
contained" with the needed stuff to make it work.

Do you have an example of where this is needed as I can't figure out the
goal here, sorry.

thanks,

greg k-h

Re: [PATCH 1/3] rust: add Aliased type

[Boqun Feng]

Hi Christian,

[Cc Daniel and Danilo]

Thanks for the patch!

On Sun, Jan 19, 2025 at 11:11:13PM +0100, Christian Schrefl wrote:
> This type is useful for cases where a value might be shared with C code
> but not interpreted by it.
> In partiquarly this is added to for data that is shared between a Driver
> and a MiscDevice implementation.
> 
> Similar to Opaque but guarantees that the value is initialized and the
> inner value is dropped when Aliased is dropped.
> 
> This was origianally proposed for the IRQ abstractions [0], but also
> useful for other cases where Data may be aliased, but is always valid
> and automatic drop is desired.
> 
> Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiOASgjoYKFz6kWwzLaH07DqP2ph+3YyCDh2+gYqGpABA@mail.gmail.com [0]
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> ---
>  rust/kernel/types.rs | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 3aea6af9a0bca70ee42b4bad2fe31a99750cbf11..5640128c9a9055476a0040033946ba6caa6e7076 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -528,3 +528,43 @@ pub enum Either<L, R> {
>  /// [`NotThreadSafe`]: type@NotThreadSafe
>  #[allow(non_upper_case_globals)]
>  pub const NotThreadSafe: NotThreadSafe = PhantomData;
> +
> +/// Stores a value that may be aliased.
> +///
> +/// This is similar to `Opaque<T>` but is guaranteed to contain valid data and will
> +/// Call the Drop implementation of T when dropped.
> +#[repr(transparent)]
> +pub struct Aliased<T> {

As I already mentioned [1], the name `Aliased` is more reflecting the
fact that this wrapper will avoid generating the "noalias" attribute(?)
on the reference/pointer to the type rather than an intuitive idea about
"why or when do I need this". Moreover, I think the argument about the
naming of the counterpart in unstable Rust (UnsafePinned) makes sense to
me [2]: this type alone won't prevent `&mut Aliased` getting `swap`, and
it has to be used with `Pin`.

Therefore, I think we should use a different name, perhaps
`(Always)Shared`, or just use `UnsafePinned`, or, as always, looking
fowards to a better name from anybody ;-)

[1]: https://lore.kernel.org/rust-for-linux/Z407egxOy7oNLpq8@boqun-archlinux/
[2]: https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html#naming

Regards,
Boqun

> +    value: UnsafeCell<T>,
> +    _pin: PhantomPinned,
> +}
> +
> +impl<T> Aliased<T> {
> +    /// Creates a new `Aliased` value.
> +    pub const fn new(value: T) -> Self {
> +        Self {
> +            value: UnsafeCell::new(value),
> +            _pin: PhantomPinned,
> +        }
> +    }
> +    /// Create an `Aliased` pin-initializer from the given pin-initializer.
> +    pub fn try_pin_init<E>(value: impl PinInit<T, E>) -> impl PinInit<Self, E> {
> +        // SAFETY:
> +        // In case of an error in value the error is returned, otherwise the slot is fully initialized,
> +        // since value is initialized and _pin is a Zero sized type.
> +        // The pin invariants of value are upheld, since no moving occurs.
> +        unsafe { init::pin_init_from_closure(move |slot| value.__pinned_init(Self::raw_get(slot))) }
> +    }
> +    /// Returns a raw pointer to the opaque data.
> +    pub const fn get(&self) -> *mut T {
> +        UnsafeCell::get(&self.value).cast::<T>()
> +    }
> +
> +    /// Gets the value behind `this`.
> +    ///
> +    /// This function is useful to get access to the value without creating intermediate
> +    /// references.
> +    pub const fn raw_get(this: *const Self) -> *mut T {
> +        UnsafeCell::raw_get(this.cast::<UnsafeCell<MaybeUninit<T>>>()).cast::<T>()
> +    }
> +}
> 
> -- 
> 2.48.1
> 

Re: [PATCH 0/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Christian Schrefl]

On 20.01.25 6:46 AM, Greg Kroah-Hartman wrote:
> On Sun, Jan 19, 2025 at 11:11:12PM +0100, Christian Schrefl wrote:
>> This set is based on Greg's driver-core-next tree.
>>
>> 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.
> 
> Why would you want to do this?  A misc device instance should be "self
> contained" with the needed stuff to make it work.
> 
> Do you have an example of where this is needed as I can't figure out the
> goal here, sorry.

The main reason for needing this was that I could not figure out how to otherwise
use a Devres from the platfrom driver probe in the implementation of the miscdevice.
The reason to add this to MiscDeviceRegistration is to avoid race-conditions between
the fops and driver registration/deregistration.
(See the commit description of patch 2/3)

If there is a better or more intended way to achieve that please let me know.

> 
> thanks,
> 
> greg k-h

Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[kernel test robot]

Hi Christian,

kernel test robot noticed the following build errors:

[auto build test ERROR on 01b3cb620815fc3feb90ee117d9445a5b608a9f7]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Schrefl/rust-add-Aliased-type/20250120-061340
base:   01b3cb620815fc3feb90ee117d9445a5b608a9f7
patch link:    https://lore.kernel.org/r/20250119-b4-rust_miscdevice_registrationdata-v1-2-edbf18dde5fc%40gmail.com
patch subject: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250121/202501211825.Vefi6H2q-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250121/202501211825.Vefi6H2q-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501211825.Vefi6H2q-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from arch/x86/kernel/asm-offsets.c:14:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:21:
   In file included from include/linux/mm.h:2224:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~ ^
   505 |                            item];
   |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~ ^
   512 |                            NR_VM_NUMA_EVENT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~ ^
   525 |                            NR_VM_NUMA_EVENT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~~
   3 warnings generated.
   ***
   *** Rust bindings generator 'bindgen' < 0.69.5 together with libclang >= 19.1
   *** may not work due to a bug (https://github.com/rust-lang/rust-bindgen/pull/2824),
   *** unless patched (like Debian's).
   ***   Your bindgen version:  0.65.1
   ***   Your libclang version: 19.1.3
   ***
   ***
   *** Please see Documentation/rust/quick-start.rst for details
   *** on how to set up the Rust support.
   ***
   In file included from rust/helpers/helpers.c:10:
   In file included from rust/helpers/blk.c:3:
   In file included from include/linux/blk-mq.h:5:
   In file included from include/linux/blkdev.h:9:
   In file included from include/linux/blk_types.h:10:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:8:
   In file included from include/linux/cacheflush.h:5:
   In file included from arch/x86/include/asm/cacheflush.h:5:
   In file included from include/linux/mm.h:2224:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~ ^
   505 |                            item];
   |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~ ^
   512 |                            NR_VM_NUMA_EVENT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~ ^
   525 |                            NR_VM_NUMA_EVENT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~~
   3 warnings generated.
   clang diag: include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
>> error[E0658]: associated type bounds are unstable
   --> rust/kernel/miscdevice.rs:64:27
   |
   64 | unsafe impl<T: MiscDevice<RegistrationData: Send>> Send for MiscDeviceRegistration<T> {}
   |                           ^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #52662 <https://github.com/rust-lang/rust/issues/52662> for more information
   = help: add `#![feature(associated_type_bounds)]` to the crate attributes to enable
   = note: this compiler was built on 2024-04-29; consider upgrading it if it is out of date

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/3] rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

[Alice Ryhl]

On Sun, Jan 19, 2025 at 11:11 PM Christian Schrefl
<chrisi.schrefl@gmail.com> wrote:
>
> Share the mutex stored in RustMiscDevice between all instances 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>

This change causes all open files to share the same value, instead of
it being per-fd.

Alice

Re: [PATCH 0/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Greg Kroah-Hartman]

On Tue, Jan 21, 2025 at 11:29:40AM +0100, Christian Schrefl wrote:
> 
> On 20.01.25 6:46 AM, Greg Kroah-Hartman wrote:
> > On Sun, Jan 19, 2025 at 11:11:12PM +0100, Christian Schrefl wrote:
> >> This set is based on Greg's driver-core-next tree.
> >>
> >> 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.
> > 
> > Why would you want to do this?  A misc device instance should be "self
> > contained" with the needed stuff to make it work.
> > 
> > Do you have an example of where this is needed as I can't figure out the
> > goal here, sorry.
> 
> The main reason for needing this was that I could not figure out how to otherwise
> use a Devres from the platfrom driver probe in the implementation of the miscdevice.

A platform device has nothing to do with a misc device.  They should be
separate as they have totally different lifecycles.

> The reason to add this to MiscDeviceRegistration is to avoid race-conditions between
> the fops and driver registration/deregistration.
> (See the commit description of patch 2/3)
> 
> If there is a better or more intended way to achieve that please let me know.

I think you need an object that contains both of these, the driver_data
pointer field for the misc device should be able to be used for this,
right?

thanks,

greg k-h

Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Greg Kroah-Hartman]

On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
> 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.

What's wrong with the driver_data pointer in the misc device structure?
Shouldn't you be in control of that as you are a misc driver owner?  Or
does the misc core handle this I can't recall at the moment, sorry.

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

"next to" feels odd, that's what a container_of is for, but be careful
as to who owns the lifecycle of the object you are trying to get to.
You can't have multiple objects with different lifecycles in the same
structure (i.e. don't mix a misc device and a platform device together).

So a real example here would be good to see, can you post your driver at
the same time so that we can see what you are doing and perhaps provide
a better way to do it?

thanks,

greg k-h

Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Alice Ryhl]

On Wed, Jan 22, 2025 at 10:28 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
> > 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.
>
> What's wrong with the driver_data pointer in the misc device structure?
> Shouldn't you be in control of that as you are a misc driver owner?  Or
> does the misc core handle this I can't recall at the moment, sorry.

There's no such pointer in struct miscdevice?

> > 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.
>
> "next to" feels odd, that's what a container_of is for, but be careful
> as to who owns the lifecycle of the object you are trying to get to.
> You can't have multiple objects with different lifecycles in the same
> structure (i.e. don't mix a misc device and a platform device together).

Ultimately, this is intended to solve the problem that container_of
solves in C. Actually using container_of runs into the challenge that
you have no way of knowing if those other fields are still valid. If
the destructor of the struct starts running, then it might have
destroyed some of the fields, but not yet have destroyed the
miscdevice field. Since you can't know if the rest of the struct is
still valid, it's not safe to use container_of.

Storing those other fields within the registration solves this issue.
The registration ensures that the miscdevice is deregistered before
the `data` field is destroyed. This means that if it's safe to access
the miscdevice field, then it's also safe to access the `data` field,
and that's the guarantee we need.

Alice

> So a real example here would be good to see, can you post your driver at
> the same time so that we can see what you are doing and perhaps provide
> a better way to do it?

Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Greg Kroah-Hartman]

On Wed, Jan 22, 2025 at 11:11:07AM +0100, Alice Ryhl wrote:
> On Wed, Jan 22, 2025 at 10:28 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
> > > 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.
> >
> > What's wrong with the driver_data pointer in the misc device structure?
> > Shouldn't you be in control of that as you are a misc driver owner?  Or
> > does the misc core handle this I can't recall at the moment, sorry.
> 
> There's no such pointer in struct miscdevice?

struct miscdevice->struct device->driver_data;

But in digging, this might not be "allowed" for a misc driver to do, I
can't remember anymore, sorry.

> > > 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.
> >
> > "next to" feels odd, that's what a container_of is for, but be careful
> > as to who owns the lifecycle of the object you are trying to get to.
> > You can't have multiple objects with different lifecycles in the same
> > structure (i.e. don't mix a misc device and a platform device together).
> 
> Ultimately, this is intended to solve the problem that container_of
> solves in C. Actually using container_of runs into the challenge that
> you have no way of knowing if those other fields are still valid.

You "know" they are valid if you have a pointer to your structure,
right?  Someone sent that to you, so by virtue of that it has to be
valid.

> If
> the destructor of the struct starts running, then it might have
> destroyed some of the fields, but not yet have destroyed the
> miscdevice field. Since you can't know if the rest of the struct is
> still valid, it's not safe to use container_of.

That's a different issue, that's a lifetime issue of "can I look at
these other fields at this point in time and trust them", it's not a
"these are not valid thing to look at".

> Storing those other fields within the registration solves this issue.
> The registration ensures that the miscdevice is deregistered before
> the `data` field is destroyed. This means that if it's safe to access
> the miscdevice field, then it's also safe to access the `data` field,
> and that's the guarantee we need.

I'm confused.  A misc device should be embedded inside of something
else.  And the misc device controls the lifespan of that "something
else" structure, right?  So by virtue of the misc device being alive,
everything else in there should be ok.

Now individual misc devices might want to do different things but if
they are being torn down, that means all references are dropped so any
"container_of()" like cast-back should not even be possible as there
should not be a pointer that you can cast-back from here.

Or am I confused?  I think a real example would be good to see here.

thanks,

greg k-h

Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Alice Ryhl]

On Wed, Jan 22, 2025 at 1:40 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jan 22, 2025 at 11:11:07AM +0100, Alice Ryhl wrote:
> > On Wed, Jan 22, 2025 at 10:28 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
> > > > 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.
> > >
> > > What's wrong with the driver_data pointer in the misc device structure?
> > > Shouldn't you be in control of that as you are a misc driver owner?  Or
> > > does the misc core handle this I can't recall at the moment, sorry.
> >
> > There's no such pointer in struct miscdevice?
>
> struct miscdevice->struct device->driver_data;
>
> But in digging, this might not be "allowed" for a misc driver to do, I
> can't remember anymore, sorry.
>
> > > > 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.
> > >
> > > "next to" feels odd, that's what a container_of is for, but be careful
> > > as to who owns the lifecycle of the object you are trying to get to.
> > > You can't have multiple objects with different lifecycles in the same
> > > structure (i.e. don't mix a misc device and a platform device together).
> >
> > Ultimately, this is intended to solve the problem that container_of
> > solves in C. Actually using container_of runs into the challenge that
> > you have no way of knowing if those other fields are still valid.
>
> You "know" they are valid if you have a pointer to your structure,
> right?  Someone sent that to you, so by virtue of that it has to be
> valid.
>
> > If
> > the destructor of the struct starts running, then it might have
> > destroyed some of the fields, but not yet have destroyed the
> > miscdevice field. Since you can't know if the rest of the struct is
> > still valid, it's not safe to use container_of.
>
> That's a different issue, that's a lifetime issue of "can I look at
> these other fields at this point in time and trust them", it's not a
> "these are not valid thing to look at".

The memory containing the field can't have been deallocated yet, but
if we provide a safe way to get access to those fields after their
destructor runs, then that's still a problem. If there's a field of
type `KBox<MyType>`, i.e. a pointer to a kmalloc allocation, then the
destructor will have freed that allocation. We cannot let you access
the field that holds the KBox after that happens because the pointer
will be dangling.

> > Storing those other fields within the registration solves this issue.
> > The registration ensures that the miscdevice is deregistered before
> > the `data` field is destroyed. This means that if it's safe to access
> > the miscdevice field, then it's also safe to access the `data` field,
> > and that's the guarantee we need.
>
> I'm confused.  A misc device should be embedded inside of something
> else.  And the misc device controls the lifespan of that "something
> else" structure, right?  So by virtue of the misc device being alive,
> everything else in there should be ok.

This patch proposes that the way you write

struct MyDriverData {
    // lifetime of these fields is controlled by misc
    field1: Foo,
    field2: Bar,
    misc: MiscDeviceRegistration<MyMiscFile>
}

is

struct MyDriverData {
    misc: MiscDeviceRegistration<MyMiscFile, Inner>
}
struct Inner {
    // lifetime of these fields is controlled by misc
    field1: Foo,
    field2: Bar,
}

in memory all three fields gets laid out next to each other.

> Now individual misc devices might want to do different things but if
> they are being torn down, that means all references are dropped so any
> "container_of()" like cast-back should not even be possible as there
> should not be a pointer that you can cast-back from here.
>
> Or am I confused?  I think a real example would be good to see here.

Christian, do you mind sharing the example you showed to me?

Alice

Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Christian Schrefl]

On 22.01.25 2:06 PM, Alice Ryhl wrote:
> On Wed, Jan 22, 2025 at 1:40 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>>
>> On Wed, Jan 22, 2025 at 11:11:07AM +0100, Alice Ryhl wrote:
>>> On Wed, Jan 22, 2025 at 10:28 AM Greg Kroah-Hartman
>>> <gregkh@linuxfoundation.org> wrote:
>>>>
>>>> On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
>>>>> 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.
>>>>
>>>> What's wrong with the driver_data pointer in the misc device structure?
>>>> Shouldn't you be in control of that as you are a misc driver owner?  Or
>>>> does the misc core handle this I can't recall at the moment, sorry.
>>>
>>> There's no such pointer in struct miscdevice?
>>
>> struct miscdevice->struct device->driver_data;
>>
>> But in digging, this might not be "allowed" for a misc driver to do, I
>> can't remember anymore, sorry.
>>
>>>>> 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.
>>>>
>>>> "next to" feels odd, that's what a container_of is for, but be careful
>>>> as to who owns the lifecycle of the object you are trying to get to.
>>>> You can't have multiple objects with different lifecycles in the same
>>>> structure (i.e. don't mix a misc device and a platform device together).
>>>
>>> Ultimately, this is intended to solve the problem that container_of
>>> solves in C. Actually using container_of runs into the challenge that
>>> you have no way of knowing if those other fields are still valid.
>>
>> You "know" they are valid if you have a pointer to your structure,
>> right?  Someone sent that to you, so by virtue of that it has to be
>> valid.
>>
>>> If
>>> the destructor of the struct starts running, then it might have
>>> destroyed some of the fields, but not yet have destroyed the
>>> miscdevice field. Since you can't know if the rest of the struct is
>>> still valid, it's not safe to use container_of.
>>
>> That's a different issue, that's a lifetime issue of "can I look at
>> these other fields at this point in time and trust them", it's not a
>> "these are not valid thing to look at".
> 
> The memory containing the field can't have been deallocated yet, but
> if we provide a safe way to get access to those fields after their
> destructor runs, then that's still a problem. If there's a field of
> type `KBox<MyType>`, i.e. a pointer to a kmalloc allocation, then the
> destructor will have freed that allocation. We cannot let you access
> the field that holds the KBox after that happens because the pointer
> will be dangling.
> 
>>> Storing those other fields within the registration solves this issue.
>>> The registration ensures that the miscdevice is deregistered before
>>> the `data` field is destroyed. This means that if it's safe to access
>>> the miscdevice field, then it's also safe to access the `data` field,
>>> and that's the guarantee we need.
>>
>> I'm confused.  A misc device should be embedded inside of something
>> else.  And the misc device controls the lifespan of that "something
>> else" structure, right?  So by virtue of the misc device being alive,
>> everything else in there should be ok.
> 
> This patch proposes that the way you write
> 
> struct MyDriverData {
>     // lifetime of these fields is controlled by misc
>     field1: Foo,
>     field2: Bar,
>     misc: MiscDeviceRegistration<MyMiscFile>
> }
> 
> is
> 
> struct MyDriverData {
>     misc: MiscDeviceRegistration<MyMiscFile, Inner>
> }
> struct Inner {
>     // lifetime of these fields is controlled by misc
>     field1: Foo,
>     field2: Bar,
> }
> 
> in memory all three fields gets laid out next to each other.
> 
>> Now individual misc devices might want to do different things but if
>> they are being torn down, that means all references are dropped so any
>> "container_of()" like cast-back should not even be possible as there
>> should not be a pointer that you can cast-back from here.
>>
>> Or am I confused?  I think a real example would be good to see here.
> 
> Christian, do you mind sharing the example you showed to me?

Sure

#[pin_data(PinnedDrop)]
struct LedPwmDriver {
    pdev: platform::Device,
    mapping: Devres<IoMem<MAPPING_SIZE, true>>,
    #[pin]
    miscdev: MiscDeviceRegistration<RustMiscDevice>,
}

impl platform::Driver for LedPwmDriver {
    type IdInfo = ();
    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
    fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>> {
        dev_dbg!(pdev.as_ref(), "Probe Rust Platform driver sample.\n");
        if info.is_some() {
            dev_info!(pdev.as_ref(), "Probed with info\n");
        }
        let mapping = pdev
            .ioremap_resource_sized::<MAPPING_SIZE, true>(pdev.resource(0).ok_or(ENXIO)?)
            .map_err(|_| ENXIO)?;

        // Initialize the HW
        mapping.try_access().ok_or(ENXIO)?.writel(0xFF, 0x0);

        let options = MiscDeviceOptions { name: c_str!("led0") };
        let drvdata = KBox::try_pin_init(
            try_pin_init!(Self {
                pdev: pdev.clone(),
                mapping,
                miscdev <- MiscDeviceRegistration::register(options),
            }),
            GFP_KERNEL,
        )?;
        Ok(drvdata)
    }
}

#[pinned_drop]
impl PinnedDrop for LedPwmDriver {
    fn drop(self: Pin<&mut Self>) {
        dev_info!(self.pdev.as_ref(), "Remove Rust Platform driver sample.\n");

        if let Some(res) = self.mapping.try_access() {
            res.writel(0x00, 0x0);
        }
    }
}

#[pin_data(PinnedDrop)]
struct RustMiscDevice {
    dev: ARef<Device>,
    mapping: &'static Devres<IoMem<MAPPING_SIZE, true>>,
}
#[vtable]
impl MiscDevice for RustMiscDevice {
    type Ptr = Pin<KBox<Self>>;
    fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
        let dev = ARef::from(misc.device());
        // SAFETY:
        // Stuff?
        let ledpwm = unsafe { &*container_of!(misc, LedPwmDriver, miscdev) };
        dev_info!(dev, "Opening Rust Misc Device Sample\n");
        let res = ledpwm.mapping.try_access().ok_or(ENXIO)?;
        res.writel(0x80, 0x0);
        KBox::try_pin_init(
            try_pin_init! {
                RustMiscDevice {
                    dev: dev,
                    mapping: &ledpwm.mapping
                }
            },
            GFP_KERNEL,
        )
    }
}

#[pinned_drop]
impl PinnedDrop for RustMiscDevice {
    fn drop(self: Pin<&mut Self>) {
        dev_info!(self.dev, "Exiting the Rust Misc Device Sample\n");

        if let Some(res) = self.mapping.try_access() {
            res.writel(0x10, 0x0);
        }
    }
}


I removed or simplified many irrelevant parts, the full driver can be found here:

https://github.com/onestacked/linux/blob/c9e2ef858e4537d33625cdf2b5d20ef4acfa9424/drivers/misc/ledpwm.rs

This driver was only an attempt to the assignment of a University course in Rust mostly to see if was 
possible or which abstractions are missing.

The driver using the patch can be found here:
https://github.com/onestacked/linux/blob/rust_ledpwm_driver/drivers/misc/ledpwm.rs


Cheers
Christian

Re: [PATCH 1/3] rust: add Aliased type

[Christian Schrefl]

On 20.01.25 6:24 PM, Boqun Feng wrote:
> Hi Christian,
> 
> [Cc Daniel and Danilo]
> 
> Thanks for the patch!
> 
> On Sun, Jan 19, 2025 at 11:11:13PM +0100, Christian Schrefl wrote:
>> This type is useful for cases where a value might be shared with C code
>> but not interpreted by it.
>> In partiquarly this is added to for data that is shared between a Driver
>> and a MiscDevice implementation.
>>
>> Similar to Opaque but guarantees that the value is initialized and the
>> inner value is dropped when Aliased is dropped.
>>
>> This was origianally proposed for the IRQ abstractions [0], but also
>> useful for other cases where Data may be aliased, but is always valid
>> and automatic drop is desired.
>>
>> Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiOASgjoYKFz6kWwzLaH07DqP2ph+3YyCDh2+gYqGpABA@mail.gmail.com [0]
>> Suggested-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
>> ---
>>  rust/kernel/types.rs | 40 ++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>>
>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> index 3aea6af9a0bca70ee42b4bad2fe31a99750cbf11..5640128c9a9055476a0040033946ba6caa6e7076 100644
>> --- a/rust/kernel/types.rs
>> +++ b/rust/kernel/types.rs
>> @@ -528,3 +528,43 @@ pub enum Either<L, R> {
>>  /// [`NotThreadSafe`]: type@NotThreadSafe
>>  #[allow(non_upper_case_globals)]
>>  pub const NotThreadSafe: NotThreadSafe = PhantomData;
>> +
>> +/// Stores a value that may be aliased.
>> +///
>> +/// This is similar to `Opaque<T>` but is guaranteed to contain valid data and will
>> +/// Call the Drop implementation of T when dropped.
>> +#[repr(transparent)]
>> +pub struct Aliased<T> {
> 
> As I already mentioned [1], the name `Aliased` is more reflecting the
> fact that this wrapper will avoid generating the "noalias" attribute(?)
> on the reference/pointer to the type rather than an intuitive idea about
> "why or when do I need this". Moreover, I think the argument about the
> naming of the counterpart in unstable Rust (UnsafePinned) makes sense to
> me [2]: this type alone won't prevent `&mut Aliased` getting `swap`, and
> it has to be used with `Pin`.
> 
> Therefore, I think we should use a different name, perhaps
> `(Always)Shared`, or just use `UnsafePinned`, or, as always, looking
> fowards to a better name from anybody ;-)
> 
> [1]: https://lore.kernel.org/rust-for-linux/Z407egxOy7oNLpq8@boqun-archlinux/
> [2]: https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html#naming

I don't particularly care about the name, I mostly used aliased, because that's
the name that Alice originally used.

`(Always)Shared` seems confusing to me.

I guess we can use `UnsafePinned`, but most people won't know what that means,
also I'm not sure if it's a good Idea to use the same name as a (future) 
language type.

If there is a consensus on a good name I'll use that for the next version.

> 
> Regards,
> Boqun
> 
>> +    value: UnsafeCell<T>,
>> +    _pin: PhantomPinned,
>> +}
>> +
>> +impl<T> Aliased<T> {
>> +    /// Creates a new `Aliased` value.
>> +    pub const fn new(value: T) -> Self {
>> +        Self {
>> +            value: UnsafeCell::new(value),
>> +            _pin: PhantomPinned,
>> +        }
>> +    }
>> +    /// Create an `Aliased` pin-initializer from the given pin-initializer.
>> +    pub fn try_pin_init<E>(value: impl PinInit<T, E>) -> impl PinInit<Self, E> {
>> +        // SAFETY:
>> +        // In case of an error in value the error is returned, otherwise the slot is fully initialized,
>> +        // since value is initialized and _pin is a Zero sized type.
>> +        // The pin invariants of value are upheld, since no moving occurs.
>> +        unsafe { init::pin_init_from_closure(move |slot| value.__pinned_init(Self::raw_get(slot))) }
>> +    }
>> +    /// Returns a raw pointer to the opaque data.
>> +    pub const fn get(&self) -> *mut T {
>> +        UnsafeCell::get(&self.value).cast::<T>()
>> +    }
>> +
>> +    /// Gets the value behind `this`.
>> +    ///
>> +    /// This function is useful to get access to the value without creating intermediate
>> +    /// references.
>> +    pub const fn raw_get(this: *const Self) -> *mut T {
>> +        UnsafeCell::raw_get(this.cast::<UnsafeCell<MaybeUninit<T>>>()).cast::<T>()
>> +    }
>> +}
>>
>> -- 
>> 2.48.1
>>

Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Christian Schrefl]

On 22.01.25 10:28 AM, Greg Kroah-Hartman wrote:
> On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
>> 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.
> 
> What's wrong with the driver_data pointer in the misc device structure?
> Shouldn't you be in control of that as you are a misc driver owner?  Or
> does the misc core handle this I can't recall at the moment, sorry.


I don't know the internals of (C) miscdevice good enough to know where I'm
allowed to store something, since there is no private_data field.

Not sure how the lifetimes of the whole device and device->driver_data are.
But even that instead we use that we will need a rust abstraction for that to
allow safe drivers.

> 
>> 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.
> 
> "next to" feels odd, that's what a container_of is for, but be careful
> as to who owns the lifecycle of the object you are trying to get to.
> You can't have multiple objects with different lifecycles in the same
> structure (i.e. don't mix a misc device and a platform device together).
> 
> So a real example here would be good to see, can you post your driver at
> the same time so that we can see what you are doing and perhaps provide
> a better way to do it?


The `struct miscdevice` is currently the first item in the 
`MiscDeviceRegistration` so the  `struct miscdevice` and the 
`MiscDeviceRegistration` have the same address.
I can use container_of! if people think that more understandable.

> 
> thanks,
> 
> greg k-h

Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Greg Kroah-Hartman]

On Thu, Jan 23, 2025 at 04:52:26PM +0100, Christian Schrefl wrote:
> 
> 
> On 22.01.25 10:28 AM, Greg Kroah-Hartman wrote:
> > On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
> >> 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.
> > 
> > What's wrong with the driver_data pointer in the misc device structure?
> > Shouldn't you be in control of that as you are a misc driver owner?  Or
> > does the misc core handle this I can't recall at the moment, sorry.
> 
> 
> I don't know the internals of (C) miscdevice good enough to know where I'm
> allowed to store something, since there is no private_data field.

You are right, I was wrong here, sorry.  A misc device either needs to
be "stand alone" or embedded into something else.

> Not sure how the lifetimes of the whole device and device->driver_data are.
> But even that instead we use that we will need a rust abstraction for that to
> allow safe drivers.

Agreed, so let's make it work properly :)

> > 
> >> 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.
> > 
> > "next to" feels odd, that's what a container_of is for, but be careful
> > as to who owns the lifecycle of the object you are trying to get to.
> > You can't have multiple objects with different lifecycles in the same
> > structure (i.e. don't mix a misc device and a platform device together).
> > 
> > So a real example here would be good to see, can you post your driver at
> > the same time so that we can see what you are doing and perhaps provide
> > a better way to do it?
> 
> 
> The `struct miscdevice` is currently the first item in the 
> `MiscDeviceRegistration` so the  `struct miscdevice` and the 
> `MiscDeviceRegistration` have the same address.
> I can use container_of! if people think that more understandable.

You always have to use container_of! in case things move around.  If the
location is the same place, then the compiler just optimizes it all away
and doesn't do any pointer math so it's fine.

thanks,

greg k-h

Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Christian Schrefl]

On 23.01.25 5:00 PM, Greg Kroah-Hartman wrote:
> On Thu, Jan 23, 2025 at 04:52:26PM +0100, Christian Schrefl wrote:
>>
>>
>> On 22.01.25 10:28 AM, Greg Kroah-Hartman wrote:
>>> On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
>>>> 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.
>>>
>>> What's wrong with the driver_data pointer in the misc device structure?
>>> Shouldn't you be in control of that as you are a misc driver owner?  Or
>>> does the misc core handle this I can't recall at the moment, sorry.
>>
>>
>> I don't know the internals of (C) miscdevice good enough to know where I'm
>> allowed to store something, since there is no private_data field.
> 
> You are right, I was wrong here, sorry.  A misc device either needs to
> be "stand alone" or embedded into something else.

The struct miscdevice is "embedded" in the Rust MiscDeviceRegistration, 
so it should be fine if I understand you correctly.

> 
>> Not sure how the lifetimes of the whole device and device->driver_data are.
>> But even that instead we use that we will need a rust abstraction for that to
>> allow safe drivers.
> 
> Agreed, so let's make it work properly :)

So keep the current approach?

> 
>>>
>>>> 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.
>>>
>>> "next to" feels odd, that's what a container_of is for, but be careful
>>> as to who owns the lifecycle of the object you are trying to get to.
>>> You can't have multiple objects with different lifecycles in the same
>>> structure (i.e. don't mix a misc device and a platform device together).
>>>
>>> So a real example here would be good to see, can you post your driver at
>>> the same time so that we can see what you are doing and perhaps provide
>>> a better way to do it?
>>
>>
>> The `struct miscdevice` is currently the first item in the 
>> `MiscDeviceRegistration` so the  `struct miscdevice` and the 
>> `MiscDeviceRegistration` have the same address.
>> I can use container_of! if people think that more understandable.
> 
> You always have to use container_of! in case things move around.  If the
> location is the same place, then the compiler just optimizes it all away
> and doesn't do any pointer math so it's fine.

Sure I'll change it to use container_of.

> 
> thanks,
> 
> greg k-h

Re: [PATCH 1/3] rust: add Aliased type

[Boqun Feng]

On Thu, Jan 23, 2025 at 11:21:23AM +0100, Christian Schrefl wrote:
[...]
> >> +
> >> +/// Stores a value that may be aliased.
> >> +///
> >> +/// This is similar to `Opaque<T>` but is guaranteed to contain valid data and will
> >> +/// Call the Drop implementation of T when dropped.
> >> +#[repr(transparent)]
> >> +pub struct Aliased<T> {
> > 
> > As I already mentioned [1], the name `Aliased` is more reflecting the
> > fact that this wrapper will avoid generating the "noalias" attribute(?)
> > on the reference/pointer to the type rather than an intuitive idea about
> > "why or when do I need this". Moreover, I think the argument about the
> > naming of the counterpart in unstable Rust (UnsafePinned) makes sense to
> > me [2]: this type alone won't prevent `&mut Aliased` getting `swap`, and
> > it has to be used with `Pin`.
> > 
> > Therefore, I think we should use a different name, perhaps
> > `(Always)Shared`, or just use `UnsafePinned`, or, as always, looking
> > fowards to a better name from anybody ;-)
> > 
> > [1]: https://lore.kernel.org/rust-for-linux/Z407egxOy7oNLpq8@boqun-archlinux/
> > [2]: https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html#naming
> 
> I don't particularly care about the name, I mostly used aliased, because that's
> the name that Alice originally used.
> 
> `(Always)Shared` seems confusing to me.
> 
> I guess we can use `UnsafePinned`, but most people won't know what that means,

Hmm.. but doesn't `Aliased` have the same effect, i.e. most people won't
know what that means? Moreover, people who already knows `UnsafePinned`
will still take some time to realize "`Aliased` is actually
`UnsafePinned`".

> also I'm not sure if it's a good Idea to use the same name as a (future) 
> language type.

The benefit is that we won't re-invent the wheel since `UnsafePinned`
already does what `Aliased` does here. If we don't have a good name, we
should use the one that most people are already using. Honestly, at this
point, I think we should just use the unstable feature unsafe_pinned.

Regards,
Boqun

> 
> If there is a consensus on a good name I'll use that for the next version.
> 
> > 
> > Regards,
> > Boqun
> > 
[...]

Re: [PATCH 3/3] rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

[Christian Schrefl]

Hi Alice

On 21.01.25 4:40 PM, Alice Ryhl wrote:
> On Sun, Jan 19, 2025 at 11:11 PM Christian Schrefl
> <chrisi.schrefl@gmail.com> wrote:
>>
>> Share the mutex stored in RustMiscDevice between all instances 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>
> 
> This change causes all open files to share the same value, instead of
> it being per-fd.

I know, if that is unwanted I'm fine with dropping this patch,
it is mostly here to show how patch 2 can be used.

> 
> Alice

Cheers
Christian

Re: [PATCH 1/3] rust: add Aliased type

[Christian Schrefl]

Hi Boqun

On 23.01.25 6:56 PM, Boqun Feng wrote:
> On Thu, Jan 23, 2025 at 11:21:23AM +0100, Christian Schrefl wrote:
> [...]
>>>> +
>>>> +/// Stores a value that may be aliased.
>>>> +///
>>>> +/// This is similar to `Opaque<T>` but is guaranteed to contain valid data and will
>>>> +/// Call the Drop implementation of T when dropped.
>>>> +#[repr(transparent)]
>>>> +pub struct Aliased<T> {
>>>
>>> As I already mentioned [1], the name `Aliased` is more reflecting the
>>> fact that this wrapper will avoid generating the "noalias" attribute(?)
>>> on the reference/pointer to the type rather than an intuitive idea about
>>> "why or when do I need this". Moreover, I think the argument about the
>>> naming of the counterpart in unstable Rust (UnsafePinned) makes sense to
>>> me [2]: this type alone won't prevent `&mut Aliased` getting `swap`, and
>>> it has to be used with `Pin`.
>>>
>>> Therefore, I think we should use a different name, perhaps
>>> `(Always)Shared`, or just use `UnsafePinned`, or, as always, looking
>>> fowards to a better name from anybody ;-)
>>>
>>> [1]: https://lore.kernel.org/rust-for-linux/Z407egxOy7oNLpq8@boqun-archlinux/
>>> [2]: https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html#naming
>>
>> I don't particularly care about the name, I mostly used aliased, because that's
>> the name that Alice originally used.
>>
>> `(Always)Shared` seems confusing to me.
>>
>> I guess we can use `UnsafePinned`, but most people won't know what that means,
> 
> Hmm.. but doesn't `Aliased` have the same effect, i.e. most people won't
> know what that means? Moreover, people who already knows `UnsafePinned`
> will still take some time to realize "`Aliased` is actually
> `UnsafePinned`

I guess I'll name it `UnsafePinned` then.

> 
>> also I'm not sure if it's a good Idea to use the same name as a (future) 
>> language type.
> 
> The benefit is that we won't re-invent the wheel since `UnsafePinned`
> already does what `Aliased` does here. If we don't have a good name, we
> should use the one that most people are already using. Honestly, at this
> point, I think we should just use the unstable feature unsafe_pinned.

I think that's not implemented in rustc yet.
At least no implementation is linked in the tracking issue:
https://github.com/rust-lang/rust/issues/125735

Once that's implemented we can use a `cfg` to disable this 
implementation on new versions that provide it.
(Assuming we use the same API)

> [...]

Cheers
Christian

Re: [PATCH 1/3] rust: add Aliased type

[Boqun Feng]

On Thu, Jan 23, 2025 at 07:04:40PM +0100, Christian Schrefl wrote:
> Hi Boqun
> 
> On 23.01.25 6:56 PM, Boqun Feng wrote:
> > On Thu, Jan 23, 2025 at 11:21:23AM +0100, Christian Schrefl wrote:
> > [...]
> >>>> +
> >>>> +/// Stores a value that may be aliased.
> >>>> +///
> >>>> +/// This is similar to `Opaque<T>` but is guaranteed to contain valid data and will
> >>>> +/// Call the Drop implementation of T when dropped.
> >>>> +#[repr(transparent)]
> >>>> +pub struct Aliased<T> {
> >>>
> >>> As I already mentioned [1], the name `Aliased` is more reflecting the
> >>> fact that this wrapper will avoid generating the "noalias" attribute(?)
> >>> on the reference/pointer to the type rather than an intuitive idea about
> >>> "why or when do I need this". Moreover, I think the argument about the
> >>> naming of the counterpart in unstable Rust (UnsafePinned) makes sense to
> >>> me [2]: this type alone won't prevent `&mut Aliased` getting `swap`, and
> >>> it has to be used with `Pin`.
> >>>
> >>> Therefore, I think we should use a different name, perhaps
> >>> `(Always)Shared`, or just use `UnsafePinned`, or, as always, looking
> >>> fowards to a better name from anybody ;-)
> >>>
> >>> [1]: https://lore.kernel.org/rust-for-linux/Z407egxOy7oNLpq8@boqun-archlinux/
> >>> [2]: https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html#naming
> >>
> >> I don't particularly care about the name, I mostly used aliased, because that's
> >> the name that Alice originally used.
> >>
> >> `(Always)Shared` seems confusing to me.
> >>
> >> I guess we can use `UnsafePinned`, but most people won't know what that means,
> > 
> > Hmm.. but doesn't `Aliased` have the same effect, i.e. most people won't
> > know what that means? Moreover, people who already knows `UnsafePinned`
> > will still take some time to realize "`Aliased` is actually
> > `UnsafePinned`
> 
> I guess I'll name it `UnsafePinned` then.
> 
> > 
> >> also I'm not sure if it's a good Idea to use the same name as a (future) 
> >> language type.
> > 
> > The benefit is that we won't re-invent the wheel since `UnsafePinned`
> > already does what `Aliased` does here. If we don't have a good name, we
> > should use the one that most people are already using. Honestly, at this
> > point, I think we should just use the unstable feature unsafe_pinned.
> 
> I think that's not implemented in rustc yet.
> At least no implementation is linked in the tracking issue:
> https://github.com/rust-lang/rust/issues/125735
> 
> Once that's implemented we can use a `cfg` to disable this 
> implementation on new versions that provide it.
> (Assuming we use the same API)
> 

Sounds good to me, thanks!

Regards,
Boqun

> > [...]
> 
> Cheers
> Christian

Re: [PATCH 1/3] rust: add Aliased type

[Christian Schrefl]

On 23.01.25 7:25 PM, Boqun Feng wrote:
> On Thu, Jan 23, 2025 at 07:04:40PM +0100, Christian Schrefl wrote:
>> Hi Boqun
>>
>> On 23.01.25 6:56 PM, Boqun Feng wrote:
>>> On Thu, Jan 23, 2025 at 11:21:23AM +0100, Christian Schrefl wrote:
>>> [...]
>>>>> [1]: https://lore.kernel.org/rust-for-linux/Z407egxOy7oNLpq8@boqun-archlinux/
>>>>> [2]: https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html#naming
>>>>
>>>> I don't particularly care about the name, I mostly used aliased, because that's
>>>> the name that Alice originally used.
>>>>
>>>> `(Always)Shared` seems confusing to me.
>>>>
>>>> I guess we can use `UnsafePinned`, but most people won't know what that means,
>>>
>>> Hmm.. but doesn't `Aliased` have the same effect, i.e. most people won't
>>> know what that means? Moreover, people who already knows `UnsafePinned`
>>> will still take some time to realize "`Aliased` is actually
>>> `UnsafePinned`
>>
>> I guess I'll name it `UnsafePinned` then.
>>
>>>
>>>> also I'm not sure if it's a good Idea to use the same name as a (future) 
>>>> language type.
>>>
>>> The benefit is that we won't re-invent the wheel since `UnsafePinned`
>>> already does what `Aliased` does here. If we don't have a good name, we
>>> should use the one that most people are already using. Honestly, at this
>>> point, I think we should just use the unstable feature unsafe_pinned.
>>
>> I think that's not implemented in rustc yet.
>> At least no implementation is linked in the tracking issue:
>> https://github.com/rust-lang/rust/issues/125735
>>
>> Once that's implemented we can use a `cfg` to disable this 
>> implementation on new versions that provide it.
>> (Assuming we use the same API)
>>
> 
> Sounds good to me, thanks!

From reading the RFC It seems that `UnsafePinned<T>` should contain a `T`
directly instead of `UnsafeCell<T>` so I should probably also change my 
version it to match.

Cheers
Christian

Re: [PATCH 1/3] rust: add Aliased type

[Boqun Feng]

On Thu, Jan 23, 2025 at 09:18:33PM +0100, Christian Schrefl wrote:
> On 23.01.25 7:25 PM, Boqun Feng wrote:
> > On Thu, Jan 23, 2025 at 07:04:40PM +0100, Christian Schrefl wrote:
> >> Hi Boqun
> >>
> >> On 23.01.25 6:56 PM, Boqun Feng wrote:
> >>> On Thu, Jan 23, 2025 at 11:21:23AM +0100, Christian Schrefl wrote:
> >>> [...]
> >>>>> [1]: https://lore.kernel.org/rust-for-linux/Z407egxOy7oNLpq8@boqun-archlinux/
> >>>>> [2]: https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html#naming
> >>>>
> >>>> I don't particularly care about the name, I mostly used aliased, because that's
> >>>> the name that Alice originally used.
> >>>>
> >>>> `(Always)Shared` seems confusing to me.
> >>>>
> >>>> I guess we can use `UnsafePinned`, but most people won't know what that means,
> >>>
> >>> Hmm.. but doesn't `Aliased` have the same effect, i.e. most people won't
> >>> know what that means? Moreover, people who already knows `UnsafePinned`
> >>> will still take some time to realize "`Aliased` is actually
> >>> `UnsafePinned`
> >>
> >> I guess I'll name it `UnsafePinned` then.
> >>
> >>>
> >>>> also I'm not sure if it's a good Idea to use the same name as a (future) 
> >>>> language type.
> >>>
> >>> The benefit is that we won't re-invent the wheel since `UnsafePinned`
> >>> already does what `Aliased` does here. If we don't have a good name, we
> >>> should use the one that most people are already using. Honestly, at this
> >>> point, I think we should just use the unstable feature unsafe_pinned.
> >>
> >> I think that's not implemented in rustc yet.
> >> At least no implementation is linked in the tracking issue:
> >> https://github.com/rust-lang/rust/issues/125735
> >>
> >> Once that's implemented we can use a `cfg` to disable this 
> >> implementation on new versions that provide it.
> >> (Assuming we use the same API)
> >>
> > 
> > Sounds good to me, thanks!
> 
> From reading the RFC It seems that `UnsafePinned<T>` should contain a `T`
> directly instead of `UnsafeCell<T>` so I should probably also change my 
> version it to match.
> 

Per

	https://github.com/rust-lang/rust/issues/125735#issuecomment-2299183374

, I think you still want to use `UnsafeCell<T>`.

Regards,
Boqun

> Cheers
> Christian
> 

Re: [PATCH 1/3] rust: add Aliased type

[Christian Schrefl]

On 23.01.25 9:24 PM, Boqun Feng wrote:
> On Thu, Jan 23, 2025 at 09:18:33PM +0100, Christian Schrefl wrote:
>> On 23.01.25 7:25 PM, Boqun Feng wrote:
>>> On Thu, Jan 23, 2025 at 07:04:40PM +0100, Christian Schrefl wrote:
>>>> Hi Boqun
>>>>
>>>> On 23.01.25 6:56 PM, Boqun Feng wrote:
>>>>> On Thu, Jan 23, 2025 at 11:21:23AM +0100, Christian Schrefl wrote:
>>>>> [...]
>>>>>>> [1]: https://lore.kernel.org/rust-for-linux/Z407egxOy7oNLpq8@boqun-archlinux/
>>>>>>> [2]: https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html#naming
>>>>>>
>>>>>> I don't particularly care about the name, I mostly used aliased, because that's
>>>>>> the name that Alice originally used.
>>>>>>
>>>>>> `(Always)Shared` seems confusing to me.
>>>>>>
>>>>>> I guess we can use `UnsafePinned`, but most people won't know what that means,
>>>>>
>>>>> Hmm.. but doesn't `Aliased` have the same effect, i.e. most people won't
>>>>> know what that means? Moreover, people who already knows `UnsafePinned`
>>>>> will still take some time to realize "`Aliased` is actually
>>>>> `UnsafePinned`
>>>>
>>>> I guess I'll name it `UnsafePinned` then.
>>>>
>>>>>
>>>>>> also I'm not sure if it's a good Idea to use the same name as a (future) 
>>>>>> language type.
>>>>>
>>>>> The benefit is that we won't re-invent the wheel since `UnsafePinned`
>>>>> already does what `Aliased` does here. If we don't have a good name, we
>>>>> should use the one that most people are already using. Honestly, at this
>>>>> point, I think we should just use the unstable feature unsafe_pinned.
>>>>
>>>> I think that's not implemented in rustc yet.
>>>> At least no implementation is linked in the tracking issue:
>>>> https://github.com/rust-lang/rust/issues/125735
>>>>
>>>> Once that's implemented we can use a `cfg` to disable this 
>>>> implementation on new versions that provide it.
>>>> (Assuming we use the same API)
>>>>
>>>
>>> Sounds good to me, thanks!
>>
>> From reading the RFC It seems that `UnsafePinned<T>` should contain a `T`
>> directly instead of `UnsafeCell<T>` so I should probably also change my 
>> version it to match.
>>
> 
> Per
> 
> 	https://github.com/rust-lang/rust/issues/125735#issuecomment-2299183374
> 
> , I think you still want to use `UnsafeCell<T>`.

Ah thanks for the quick response, I'll keep the `UnsafeCell<T>` then.

> 
> Regards,
> Boqun
> 
>> Cheers
>> Christian
>>

Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Christian Schrefl]

On 19.01.25 11:11 PM, Christian Schrefl wrote:
> When using the Rust miscdevice bindings, you generally embed the
> MiscDeviceRegistration within another struct:
> 
> struct MyDriverData {
>     data: SomeOtherData,
>     misc: MiscDeviceRegistration<MyMiscFile>
> }
> 
> <snip>
>  
>  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 {
>              inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
>                  // SAFETY: The initializer can write to the provided `slot`.
> @@ -79,6 +85,7 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>                  // misc device.
>                  to_result(unsafe { bindings::misc_register(slot) })
>              }),
> +            data <- Aliased::try_pin_init(data),
>              _t: PhantomData,
>          })

After some thought I think `register` needs to be something like:

pub fn register(
    opts: MiscDeviceOptions,
    data: impl PinInit<T::RegistrationData, Error>,
) -> impl PinInit<Self, Error> {
    try_pin_init!(Self {
        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>()) };
            Ok::<(), Error>(())
        }),
        data <- UnsafePinned::try_pin_init(data),
        _t: PhantomData,
    })
    .pin_chain(|slot| {
        // 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.as_raw()) }).map_err(|err| {
            // Drop the Data in case misc_register fails.
            // SAFETY:
            // - We just initialized `data`.
            // - We are about to return `Err(err)`, so it is valid for us to drop `data`.
            unsafe {
                ptr::drop_in_place(slot.data.get());
            }
            err
        })
    })
}

to make sure that `misc_register` is called after data is initialized and to that
`data` will be dropped correctly in case `misc_register` fails.

But I'm not very familiar with `(try_)pin_init!` so this might be unnecessary? 

>      }
> @@ -97,10 +104,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()) };
> @@ -113,6 +128,11 @@ 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 () should be used.
> +    /// This data can be accessed in `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.
> @@ -218,6 +238,9 @@ impl<T: MiscDevice> VtableHelper<T> {
>      // 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`.
> +    // Since this the `MiscDeviceRegistration` struct uses `#[repr(C)]` and the miscdevice is the
> +    // first entry it is guaranteed that the address of the miscdevice is the same as the address
> +    // of the entire `MiscDeviceRegistration` struct.
>      let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() };
>  
>      // SAFETY:
> diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> index 40ad7266c2252e5c0b4e91e501ef9ada2eda3b16..779fcfd64119bdd5b4f8be740f7e8336c652b4d3 100644
> --- a/samples/rust/rust_misc_device.rs
> +++ b/samples/rust/rust_misc_device.rs
> @@ -136,7 +136,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>          };
>  
>          try_pin_init!(Self {
> -            _miscdev <- MiscDeviceRegistration::register(options),
> +            _miscdev <- MiscDeviceRegistration::register(options, ()),
>          })
>      }
>  }
> @@ -156,6 +156,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());
>  
> 

Re: [PATCH 3/3] rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

[Alice Ryhl]

On Thu, Jan 23, 2025 at 6:57 PM Christian Schrefl
<chrisi.schrefl@gmail.com> wrote:
>
> Hi Alice
>
> On 21.01.25 4:40 PM, Alice Ryhl wrote:
> > On Sun, Jan 19, 2025 at 11:11 PM Christian Schrefl
> > <chrisi.schrefl@gmail.com> wrote:
> >>
> >> Share the mutex stored in RustMiscDevice between all instances 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>
> >
> > This change causes all open files to share the same value, instead of
> > it being per-fd.
>
> I know, if that is unwanted I'm fine with dropping this patch,
> it is mostly here to show how patch 2 can be used.

Perhaps instead of changing the per-fd value, we could add a new
shared value? E.g., it could have a counter for the number of open
files.

Alice

Re: [PATCH 3/3] rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

[Greg Kroah-Hartman]

On Fri, Jan 24, 2025 at 08:29:38AM +0100, Alice Ryhl wrote:
> On Thu, Jan 23, 2025 at 6:57 PM Christian Schrefl
> <chrisi.schrefl@gmail.com> wrote:
> >
> > Hi Alice
> >
> > On 21.01.25 4:40 PM, Alice Ryhl wrote:
> > > On Sun, Jan 19, 2025 at 11:11 PM Christian Schrefl
> > > <chrisi.schrefl@gmail.com> wrote:
> > >>
> > >> Share the mutex stored in RustMiscDevice between all instances 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>
> > >
> > > This change causes all open files to share the same value, instead of
> > > it being per-fd.
> >
> > I know, if that is unwanted I'm fine with dropping this patch,
> > it is mostly here to show how patch 2 can be used.
> 
> Perhaps instead of changing the per-fd value, we could add a new
> shared value? E.g., it could have a counter for the number of open
> files.

Counters don't work, sorry (think about dup() for file handles), please,
either make it per-file handle, or a "global" thing for the specific
object, don't attempt to count open/release calls, the vfs does this for
us already.

thanks,

greg k-h

Re: [PATCH 3/3] rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

[Alice Ryhl]

On Fri, Jan 24, 2025 at 9:06 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Jan 24, 2025 at 08:29:38AM +0100, Alice Ryhl wrote:
> > On Thu, Jan 23, 2025 at 6:57 PM Christian Schrefl
> > <chrisi.schrefl@gmail.com> wrote:
> > >
> > > Hi Alice
> > >
> > > On 21.01.25 4:40 PM, Alice Ryhl wrote:
> > > > On Sun, Jan 19, 2025 at 11:11 PM Christian Schrefl
> > > > <chrisi.schrefl@gmail.com> wrote:
> > > >>
> > > >> Share the mutex stored in RustMiscDevice between all instances 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>
> > > >
> > > > This change causes all open files to share the same value, instead of
> > > > it being per-fd.
> > >
> > > I know, if that is unwanted I'm fine with dropping this patch,
> > > it is mostly here to show how patch 2 can be used.
> >
> > Perhaps instead of changing the per-fd value, we could add a new
> > shared value? E.g., it could have a counter for the number of open
> > files.
>
> Counters don't work, sorry (think about dup() for file handles), please,
> either make it per-file handle, or a "global" thing for the specific
> object, don't attempt to count open/release calls, the vfs does this for
> us already.

I mean, it's just for an example, shrug. It could also be another
ioctl that updates the shared value.

Alice

Re: [PATCH 3/3] rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

[Greg Kroah-Hartman]

On Fri, Jan 24, 2025 at 10:42:53AM +0100, Alice Ryhl wrote:
> On Fri, Jan 24, 2025 at 9:06 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Jan 24, 2025 at 08:29:38AM +0100, Alice Ryhl wrote:
> > > On Thu, Jan 23, 2025 at 6:57 PM Christian Schrefl
> > > <chrisi.schrefl@gmail.com> wrote:
> > > >
> > > > Hi Alice
> > > >
> > > > On 21.01.25 4:40 PM, Alice Ryhl wrote:
> > > > > On Sun, Jan 19, 2025 at 11:11 PM Christian Schrefl
> > > > > <chrisi.schrefl@gmail.com> wrote:
> > > > >>
> > > > >> Share the mutex stored in RustMiscDevice between all instances 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>
> > > > >
> > > > > This change causes all open files to share the same value, instead of
> > > > > it being per-fd.
> > > >
> > > > I know, if that is unwanted I'm fine with dropping this patch,
> > > > it is mostly here to show how patch 2 can be used.
> > >
> > > Perhaps instead of changing the per-fd value, we could add a new
> > > shared value? E.g., it could have a counter for the number of open
> > > files.
> >
> > Counters don't work, sorry (think about dup() for file handles), please,
> > either make it per-file handle, or a "global" thing for the specific
> > object, don't attempt to count open/release calls, the vfs does this for
> > us already.
> 
> I mean, it's just for an example, shrug. It could also be another
> ioctl that updates the shared value.

Sure, but the number of times I've seen sample code copied into "real"
code is way too high.  Also, getting people to stop thinking they even
can count the number of open file handles is a good idea as that's an
extremely common "anti-pattern" in way too many drivers even today (i.e.
if you try to count open calls, or even restrict the number of open
calls to attempt some sort of control, you're doing it totally wrong.)

thanks,

greg k-h

Re: [PATCH 3/3] rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

[Alice Ryhl]

On Fri, Jan 24, 2025 at 11:34 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Jan 24, 2025 at 10:42:53AM +0100, Alice Ryhl wrote:
> > On Fri, Jan 24, 2025 at 9:06 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Fri, Jan 24, 2025 at 08:29:38AM +0100, Alice Ryhl wrote:
> > > > On Thu, Jan 23, 2025 at 6:57 PM Christian Schrefl
> > > > <chrisi.schrefl@gmail.com> wrote:
> > > > >
> > > > > Hi Alice
> > > > >
> > > > > On 21.01.25 4:40 PM, Alice Ryhl wrote:
> > > > > > On Sun, Jan 19, 2025 at 11:11 PM Christian Schrefl
> > > > > > <chrisi.schrefl@gmail.com> wrote:
> > > > > >>
> > > > > >> Share the mutex stored in RustMiscDevice between all instances 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>
> > > > > >
> > > > > > This change causes all open files to share the same value, instead of
> > > > > > it being per-fd.
> > > > >
> > > > > I know, if that is unwanted I'm fine with dropping this patch,
> > > > > it is mostly here to show how patch 2 can be used.
> > > >
> > > > Perhaps instead of changing the per-fd value, we could add a new
> > > > shared value? E.g., it could have a counter for the number of open
> > > > files.
> > >
> > > Counters don't work, sorry (think about dup() for file handles), please,
> > > either make it per-file handle, or a "global" thing for the specific
> > > object, don't attempt to count open/release calls, the vfs does this for
> > > us already.
> >
> > I mean, it's just for an example, shrug. It could also be another
> > ioctl that updates the shared value.
>
> Sure, but the number of times I've seen sample code copied into "real"
> code is way too high.  Also, getting people to stop thinking they even
> can count the number of open file handles is a good idea as that's an
> extremely common "anti-pattern" in way too many drivers even today (i.e.
> if you try to count open calls, or even restrict the number of open
> calls to attempt some sort of control, you're doing it totally wrong.)

Fair point. Do you have good ideas for some data we could put in the
data shared by all instances of the file?

Alice

Re: [PATCH 3/3] rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

[Greg Kroah-Hartman]

On Fri, Jan 24, 2025 at 11:39:23AM +0100, Alice Ryhl wrote:
> On Fri, Jan 24, 2025 at 11:34 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Jan 24, 2025 at 10:42:53AM +0100, Alice Ryhl wrote:
> > > On Fri, Jan 24, 2025 at 9:06 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Fri, Jan 24, 2025 at 08:29:38AM +0100, Alice Ryhl wrote:
> > > > > On Thu, Jan 23, 2025 at 6:57 PM Christian Schrefl
> > > > > <chrisi.schrefl@gmail.com> wrote:
> > > > > >
> > > > > > Hi Alice
> > > > > >
> > > > > > On 21.01.25 4:40 PM, Alice Ryhl wrote:
> > > > > > > On Sun, Jan 19, 2025 at 11:11 PM Christian Schrefl
> > > > > > > <chrisi.schrefl@gmail.com> wrote:
> > > > > > >>
> > > > > > >> Share the mutex stored in RustMiscDevice between all instances 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>
> > > > > > >
> > > > > > > This change causes all open files to share the same value, instead of
> > > > > > > it being per-fd.
> > > > > >
> > > > > > I know, if that is unwanted I'm fine with dropping this patch,
> > > > > > it is mostly here to show how patch 2 can be used.
> > > > >
> > > > > Perhaps instead of changing the per-fd value, we could add a new
> > > > > shared value? E.g., it could have a counter for the number of open
> > > > > files.
> > > >
> > > > Counters don't work, sorry (think about dup() for file handles), please,
> > > > either make it per-file handle, or a "global" thing for the specific
> > > > object, don't attempt to count open/release calls, the vfs does this for
> > > > us already.
> > >
> > > I mean, it's just for an example, shrug. It could also be another
> > > ioctl that updates the shared value.
> >
> > Sure, but the number of times I've seen sample code copied into "real"
> > code is way too high.  Also, getting people to stop thinking they even
> > can count the number of open file handles is a good idea as that's an
> > extremely common "anti-pattern" in way too many drivers even today (i.e.
> > if you try to count open calls, or even restrict the number of open
> > calls to attempt some sort of control, you're doing it totally wrong.)
> 
> Fair point. Do you have good ideas for some data we could put in the
> data shared by all instances of the file?

"shared_cookie"?  Read/write and let readers and writers access it.  But
I thought that was what the misc example did already, but I might be
getting confused here, it's been a while since I looked at the code,
sorry.


greg k-h

Re: [PATCH 3/3] rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

[Alice Ryhl]

On Fri, Jan 24, 2025 at 12:22 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Jan 24, 2025 at 11:39:23AM +0100, Alice Ryhl wrote:
> > On Fri, Jan 24, 2025 at 11:34 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Fri, Jan 24, 2025 at 10:42:53AM +0100, Alice Ryhl wrote:
> > > > On Fri, Jan 24, 2025 at 9:06 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Fri, Jan 24, 2025 at 08:29:38AM +0100, Alice Ryhl wrote:
> > > > > > On Thu, Jan 23, 2025 at 6:57 PM Christian Schrefl
> > > > > > <chrisi.schrefl@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Alice
> > > > > > >
> > > > > > > On 21.01.25 4:40 PM, Alice Ryhl wrote:
> > > > > > > > On Sun, Jan 19, 2025 at 11:11 PM Christian Schrefl
> > > > > > > > <chrisi.schrefl@gmail.com> wrote:
> > > > > > > >>
> > > > > > > >> Share the mutex stored in RustMiscDevice between all instances 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>
> > > > > > > >
> > > > > > > > This change causes all open files to share the same value, instead of
> > > > > > > > it being per-fd.
> > > > > > >
> > > > > > > I know, if that is unwanted I'm fine with dropping this patch,
> > > > > > > it is mostly here to show how patch 2 can be used.
> > > > > >
> > > > > > Perhaps instead of changing the per-fd value, we could add a new
> > > > > > shared value? E.g., it could have a counter for the number of open
> > > > > > files.
> > > > >
> > > > > Counters don't work, sorry (think about dup() for file handles), please,
> > > > > either make it per-file handle, or a "global" thing for the specific
> > > > > object, don't attempt to count open/release calls, the vfs does this for
> > > > > us already.
> > > >
> > > > I mean, it's just for an example, shrug. It could also be another
> > > > ioctl that updates the shared value.
> > >
> > > Sure, but the number of times I've seen sample code copied into "real"
> > > code is way too high.  Also, getting people to stop thinking they even
> > > can count the number of open file handles is a good idea as that's an
> > > extremely common "anti-pattern" in way too many drivers even today (i.e.
> > > if you try to count open calls, or even restrict the number of open
> > > calls to attempt some sort of control, you're doing it totally wrong.)
> >
> > Fair point. Do you have good ideas for some data we could put in the
> > data shared by all instances of the file?
>
> "shared_cookie"?  Read/write and let readers and writers access it.  But
> I thought that was what the misc example did already, but I might be
> getting confused here, it's been a while since I looked at the code,
> sorry.

Makes sense to me.

The existing cookie is per `struct file`, but this series is about
making it possible to have data shared by all instances of a
miscdevice. But having two cookies, one that is per `struct file` and
one that is global makes sense to me.

Alice

Re: [PATCH 3/3] rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

[Christian Schrefl]

On 24.01.25 12:37 PM, Alice Ryhl wrote:
> On Fri, Jan 24, 2025 at 12:22 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>>
>> On Fri, Jan 24, 2025 at 11:39:23AM +0100, Alice Ryhl wrote:
>>> On Fri, Jan 24, 2025 at 11:34 AM Greg Kroah-Hartman
>>> <gregkh@linuxfoundation.org> wrote:
>>>>
>>>> On Fri, Jan 24, 2025 at 10:42:53AM +0100, Alice Ryhl wrote:
>>>>> On Fri, Jan 24, 2025 at 9:06 AM Greg Kroah-Hartman
>>>>> <gregkh@linuxfoundation.org> wrote:
>>>>>>
>>>>>> On Fri, Jan 24, 2025 at 08:29:38AM +0100, Alice Ryhl wrote:
>>>>>>> On Thu, Jan 23, 2025 at 6:57 PM Christian Schrefl
>>>>>>> <chrisi.schrefl@gmail.com> wrote:
>>>>>>>>
>>>>>>>> Hi Alice
>>>>>>>>
>>>>>>>> On 21.01.25 4:40 PM, Alice Ryhl wrote:
>>>>>>>>> On Sun, Jan 19, 2025 at 11:11 PM Christian Schrefl
>>>>>>>>> <chrisi.schrefl@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Share the mutex stored in RustMiscDevice between all instances 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>
>>>>>>>>>
>>>>>>>>> This change causes all open files to share the same value, instead of
>>>>>>>>> it being per-fd.
>>>>>>>>
>>>>>>>> I know, if that is unwanted I'm fine with dropping this patch,
>>>>>>>> it is mostly here to show how patch 2 can be used.
>>>>>>>
>>>>>>> Perhaps instead of changing the per-fd value, we could add a new
>>>>>>> shared value? E.g., it could have a counter for the number of open
>>>>>>> files.
>>>>>>
>>>>>> Counters don't work, sorry (think about dup() for file handles), please,
>>>>>> either make it per-file handle, or a "global" thing for the specific
>>>>>> object, don't attempt to count open/release calls, the vfs does this for
>>>>>> us already.
>>>>>
>>>>> I mean, it's just for an example, shrug. It could also be another
>>>>> ioctl that updates the shared value.
>>>>
>>>> Sure, but the number of times I've seen sample code copied into "real"
>>>> code is way too high.  Also, getting people to stop thinking they even
>>>> can count the number of open file handles is a good idea as that's an
>>>> extremely common "anti-pattern" in way too many drivers even today (i.e.
>>>> if you try to count open calls, or even restrict the number of open
>>>> calls to attempt some sort of control, you're doing it totally wrong.)
>>>
>>> Fair point. Do you have good ideas for some data we could put in the
>>> data shared by all instances of the file?
>>
>> "shared_cookie"?  Read/write and let readers and writers access it.  But
>> I thought that was what the misc example did already, but I might be
>> getting confused here, it's been a while since I looked at the code,
>> sorry.
> 
> Makes sense to me.
> 
> The existing cookie is per `struct file`, but this series is about
> making it possible to have data shared by all instances of a
> miscdevice. But having two cookies, one that is per `struct file` and
> one that is global makes sense to me.

I've Implemented one shared (Every instance) and one unique(One for every FD)
data.

Christian

Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Alice Ryhl]

On Fri, Jan 24, 2025 at 12:26 AM Christian Schrefl
<chrisi.schrefl@gmail.com> wrote:
>
> On 19.01.25 11:11 PM, Christian Schrefl wrote:
> > When using the Rust miscdevice bindings, you generally embed the
> > MiscDeviceRegistration within another struct:
> >
> > struct MyDriverData {
> >     data: SomeOtherData,
> >     misc: MiscDeviceRegistration<MyMiscFile>
> > }
> >
> > <snip>
> >
> >  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 {
> >              inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
> >                  // SAFETY: The initializer can write to the provided `slot`.
> > @@ -79,6 +85,7 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> >                  // misc device.
> >                  to_result(unsafe { bindings::misc_register(slot) })
> >              }),
> > +            data <- Aliased::try_pin_init(data),
> >              _t: PhantomData,
> >          })
>
> After some thought I think `register` needs to be something like:
>
> pub fn register(
>     opts: MiscDeviceOptions,
>     data: impl PinInit<T::RegistrationData, Error>,
> ) -> impl PinInit<Self, Error> {
>     try_pin_init!(Self {
>         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>()) };
>             Ok::<(), Error>(())
>         }),
>         data <- UnsafePinned::try_pin_init(data),
>         _t: PhantomData,
>     })
>     .pin_chain(|slot| {
>         // 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.as_raw()) }).map_err(|err| {
>             // Drop the Data in case misc_register fails.
>             // SAFETY:
>             // - We just initialized `data`.
>             // - We are about to return `Err(err)`, so it is valid for us to drop `data`.
>             unsafe {
>                 ptr::drop_in_place(slot.data.get());
>             }
>             err
>         })
>     })
> }
>
> to make sure that `misc_register` is called after data is initialized and to that
> `data` will be dropped correctly in case `misc_register` fails.
>
> But I'm not very familiar with `(try_)pin_init!` so this might be unnecessary?

Using pin_chain is definitely incorrect because it will run the
destructor of MiscDeviceRegistration if the misc_register call fails,
but calling misc_deregister is incorrect in that case.

You should be able to just move the `data <-
UnsafePinned::try_pin_init(data)` line to the top so that the field is
initialized first.

Alice

Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Christian Schrefl]

Hi Alice

On 27.01.25 11:27 AM, Alice Ryhl wrote:
><snip>
>>
>> to make sure that `misc_register` is called after data is initialized and to that
>> `data` will be dropped correctly in case `misc_register` fails.
>>
>> But I'm not very familiar with `(try_)pin_init!` so this might be unnecessary?
> 
> Using pin_chain is definitely incorrect because it will run the
> destructor of MiscDeviceRegistration if the misc_register call fails,
> but calling misc_deregister is incorrect in that case.
> 
> You should be able to just move the `data <-
> UnsafePinned::try_pin_init(data)` line to the top so that the field is
> initialized first.
> 

So if I understand correctly, the following should be correct:

pub fn register(
    opts: MiscDeviceOptions,
    data: impl PinInit<T::RegistrationData, Error>,
) -> impl PinInit<Self, Error> {
    try_pin_init!(Self {
        data <- UnsafePinned::try_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.
            // `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,
    })
}

Sorry I don't know the details of `(try_)pin_init` and the docs only say:
> The fields are initialized in the order that they appear in the initializer. So it is possible
> to read already initialized fields using raw pointers.
> 
> IMPORTANT: You are not allowed to create references to fields of the struct inside of the 
> initializer.

This says its invalid to create references, but as soon as `misc_register` its theoretically
possible that a `open()` call happens and creates a reference to the Registration. I assume
that is fine, because the Registration would be fully initialized at that point, but that's
technically against the Docs.

Also does `try_pin_init!()` automatically drop `data` when `bindings::misc_register` fails?

I couldn't find anything in the Docs about when and what is dropped.

Is there a equivalent to `cargo expand` for the kernel?
It would be nice to be able to look at the code generated by the macros.

Cheers
Christian 

Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Alice Ryhl]

On Mon, Jan 27, 2025 at 2:27 PM Christian Schrefl
<chrisi.schrefl@gmail.com> wrote:
>
> Hi Alice
>
> On 27.01.25 11:27 AM, Alice Ryhl wrote:
> ><snip>
> >>
> >> to make sure that `misc_register` is called after data is initialized and to that
> >> `data` will be dropped correctly in case `misc_register` fails.
> >>
> >> But I'm not very familiar with `(try_)pin_init!` so this might be unnecessary?
> >
> > Using pin_chain is definitely incorrect because it will run the
> > destructor of MiscDeviceRegistration if the misc_register call fails,
> > but calling misc_deregister is incorrect in that case.
> >
> > You should be able to just move the `data <-
> > UnsafePinned::try_pin_init(data)` line to the top so that the field is
> > initialized first.
> >
>
> So if I understand correctly, the following should be correct:
>
> pub fn register(
>     opts: MiscDeviceOptions,
>     data: impl PinInit<T::RegistrationData, Error>,
> ) -> impl PinInit<Self, Error> {
>     try_pin_init!(Self {
>         data <- UnsafePinned::try_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.
>             // `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,
>     })
> }
>
> Sorry I don't know the details of `(try_)pin_init` and the docs only say:
> > The fields are initialized in the order that they appear in the initializer. So it is possible
> > to read already initialized fields using raw pointers.
> >
> > IMPORTANT: You are not allowed to create references to fields of the struct inside of the
> > initializer.
>
> This says its invalid to create references, but as soon as `misc_register` its theoretically
> possible that a `open()` call happens and creates a reference to the Registration. I assume
> that is fine, because the Registration would be fully initialized at that point, but that's
> technically against the Docs.

Creating a reference immediately after misc_register finishes should
be fine. I think that warning is more strict than necessary.

> Also does `try_pin_init!()` automatically drop `data` when `bindings::misc_register` fails?

Yep. On failure, previous fields are cleaned up.

> I couldn't find anything in the Docs about when and what is dropped.
>
> Is there a equivalent to `cargo expand` for the kernel?
> It would be nice to be able to look at the code generated by the macros.

You can expand Rust macros by invoking make on the file with an .i
extension, in exactly the same way as you expand C macros.

Alice

Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Christian Schrefl]

On 27.01.25 2:33 PM, Alice Ryhl wrote:
> On Mon, Jan 27, 2025 at 2:27 PM Christian Schrefl
> <chrisi.schrefl@gmail.com> wrote:
>>
>> Hi Alice
>>
>> On 27.01.25 11:27 AM, Alice Ryhl wrote:
>>> <snip>
>>>>
>>>> to make sure that `misc_register` is called after data is initialized and to that
>>>> `data` will be dropped correctly in case `misc_register` fails.
>>>>
>>>> But I'm not very familiar with `(try_)pin_init!` so this might be unnecessary?
>>>
>>> Using pin_chain is definitely incorrect because it will run the
>>> destructor of MiscDeviceRegistration if the misc_register call fails,
>>> but calling misc_deregister is incorrect in that case.
>>>
>>> You should be able to just move the `data <-
>>> UnsafePinned::try_pin_init(data)` line to the top so that the field is
>>> initialized first.
>>>
>>
>> So if I understand correctly, the following should be correct:
>>
>> pub fn register(
>>     opts: MiscDeviceOptions,
>>     data: impl PinInit<T::RegistrationData, Error>,
>> ) -> impl PinInit<Self, Error> {
>>     try_pin_init!(Self {
>>         data <- UnsafePinned::try_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.
>>             // `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,
>>     })
>> }
>>
>> Sorry I don't know the details of `(try_)pin_init` and the docs only say:
>>> The fields are initialized in the order that they appear in the initializer. So it is possible
>>> to read already initialized fields using raw pointers.
>>>
>>> IMPORTANT: You are not allowed to create references to fields of the struct inside of the
>>> initializer.
>>
>> This says its invalid to create references, but as soon as `misc_register` its theoretically
>> possible that a `open()` call happens and creates a reference to the Registration. I assume
>> that is fine, because the Registration would be fully initialized at that point, but that's
>> technically against the Docs.
> 
> Creating a reference immediately after misc_register finishes should
> be fine. I think that warning is more strict than necessary.
> 
>> Also does `try_pin_init!()` automatically drop `data` when `bindings::misc_register` fails?
> 
> Yep. On failure, previous fields are cleaned up.
> 
>> I couldn't find anything in the Docs about when and what is dropped.
>>
>> Is there a equivalent to `cargo expand` for the kernel?
>> It would be nice to be able to look at the code generated by the macros.
> 
> You can expand Rust macros by invoking make on the file with an .i
> extension, in exactly the same way as you expand C macros.

Thanks!

Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Miguel Ojeda]

On Mon, Jan 27, 2025 at 2:33 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> You can expand Rust macros by invoking make on the file with an .i
> extension, in exactly the same way as you expand C macros.

`.i` for C, `.rsi` for Rust.

Also, Christian: the Rust one currently it works for leaves, e.g.
drivers and samples:

    make LLVM=1 samples/rust/rust_minimal.rsi

Eventually we will add support for others.

I hope that helps!

Cheers,
Miguel