Providing access to the underlying `struct miscdevice` is useful for
various reasons. For example, this allows you access the miscdevice's
internal `struct device` for use with the `dev_*` printing macros.

Note that since the underlying `struct miscdevice` could get freed at
any point after the fops->open() call (if misc_deregister is called),
only the open call is given access to it. To use `dev_*` printing macros
from other fops hooks, take a refcount on `miscdevice->this_device` to
keep it alive. See the linked thread for further discussion on the
lifetime of `struct miscdevice`.

Link: https://lore.kernel.org/r/2024120951-botanist-exhale-4845@gregkh
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/miscdevice.rs | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index 0cb79676c139..75a9d26c8001 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -97,14 +97,14 @@ fn drop(self: Pin<&mut Self>) {
 
 /// Trait implemented by the private data of an open misc device.
 #[vtable]
-pub trait MiscDevice {
+pub trait MiscDevice: Sized {
     /// What kind of pointer should `Self` be wrapped in.
     type Ptr: ForeignOwnable + Send + Sync;
 
     /// Called when the misc device is opened.
     ///
     /// The returned pointer will be stored as the private data for the file.
-    fn open(_file: &File) -> Result<Self::Ptr>;
+    fn open(_file: &File, _misc: &MiscDeviceRegistration<Self>) -> Result<Self::Ptr>;
 
     /// Called when the misc device is released.
     fn release(device: Self::Ptr, _file: &File) {
@@ -182,24 +182,38 @@ impl<T: MiscDevice> VtableHelper<T> {
 /// The file must be associated with a `MiscDeviceRegistration<T>`.
 unsafe extern "C" fn fops_open<T: MiscDevice>(
     inode: *mut bindings::inode,
-    file: *mut bindings::file,
+    raw_file: *mut bindings::file,
 ) -> c_int {
     // SAFETY: The pointers are valid and for a file being opened.
-    let ret = unsafe { bindings::generic_file_open(inode, file) };
+    let ret = unsafe { bindings::generic_file_open(inode, raw_file) };
     if ret != 0 {
         return ret;
     }
 
+    // SAFETY: The open call of a file can access the private data.
+    let misc_ptr = unsafe { (*raw_file).private_data };
+
+    // SAFETY: This is a miscdevice, so `misc_open()` set the private data to a pointer to the
+    // associated `struct miscdevice` before calling into this method. Furthermore, `misc_open()`
+    // ensures that the miscdevice can't be unregistered and freed during this call to `fops_open`.
+    let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() };
+
     // SAFETY:
-    // * The file is valid for the duration of this call.
+    // * This underlying file is valid for (much longer than) the duration of `T::open`.
     // * There is no active fdget_pos region on the file on this thread.
-    let ptr = match T::open(unsafe { File::from_raw_file(file) }) {
+    let file = unsafe { File::from_raw_file(raw_file) };
+
+    let ptr = match T::open(file, misc) {
         Ok(ptr) => ptr,
         Err(err) => return err.to_errno(),
     };
 
-    // SAFETY: The open call of a file owns the private data.
-    unsafe { (*file).private_data = ptr.into_foreign().cast_mut() };
+    // This overwrites the private data with the value specified by the user, changing the type of
+    // this file's private data. All future accesses to the private data is performed by other
+    // fops_* methods in this file, which all correctly cast the private data to the new type.
+    //
+    // SAFETY: The open call of a file can access the private data.
+    unsafe { (*raw_file).private_data = ptr.into_foreign().cast_mut() };
 
     0
 }

-- 
2.47.1.613.gc27f4b7a9f-goog

On Tue, 10 Dec 2024, Alice Ryhl wrote:

> Providing access to the underlying `struct miscdevice` is useful for
> various reasons. For example, this allows you access the miscdevice's
> internal `struct device` for use with the `dev_*` printing macros.
> 
> Note that since the underlying `struct miscdevice` could get freed at
> any point after the fops->open() call (if misc_deregister is called),
> only the open call is given access to it. To use `dev_*` printing macros
> from other fops hooks, take a refcount on `miscdevice->this_device` to
> keep it alive. See the linked thread for further discussion on the
> lifetime of `struct miscdevice`.
> 
> Link: https://lore.kernel.org/r/2024120951-botanist-exhale-4845@gregkh
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/miscdevice.rs | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)

Reviewed-by: Lee Jones <lee@kernel.org>

> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index 0cb79676c139..75a9d26c8001 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -97,14 +97,14 @@ fn drop(self: Pin<&mut Self>) {
>  
>  /// Trait implemented by the private data of an open misc device.
>  #[vtable]
> -pub trait MiscDevice {
> +pub trait MiscDevice: Sized {
>      /// What kind of pointer should `Self` be wrapped in.
>      type Ptr: ForeignOwnable + Send + Sync;
>  
>      /// Called when the misc device is opened.
>      ///
>      /// The returned pointer will be stored as the private data for the file.
> -    fn open(_file: &File) -> Result<Self::Ptr>;
> +    fn open(_file: &File, _misc: &MiscDeviceRegistration<Self>) -> Result<Self::Ptr>;
>  
>      /// Called when the misc device is released.
>      fn release(device: Self::Ptr, _file: &File) {
> @@ -182,24 +182,38 @@ impl<T: MiscDevice> VtableHelper<T> {
>  /// The file must be associated with a `MiscDeviceRegistration<T>`.
>  unsafe extern "C" fn fops_open<T: MiscDevice>(
>      inode: *mut bindings::inode,
> -    file: *mut bindings::file,
> +    raw_file: *mut bindings::file,
>  ) -> c_int {
>      // SAFETY: The pointers are valid and for a file being opened.
> -    let ret = unsafe { bindings::generic_file_open(inode, file) };
> +    let ret = unsafe { bindings::generic_file_open(inode, raw_file) };
>      if ret != 0 {
>          return ret;
>      }
>  
> +    // SAFETY: The open call of a file can access the private data.
> +    let misc_ptr = unsafe { (*raw_file).private_data };
> +
> +    // SAFETY: This is a miscdevice, so `misc_open()` set the private data to a pointer to the
> +    // associated `struct miscdevice` before calling into this method. Furthermore, `misc_open()`
> +    // ensures that the miscdevice can't be unregistered and freed during this call to `fops_open`.
> +    let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() };
> +
>      // SAFETY:
> -    // * The file is valid for the duration of this call.
> +    // * This underlying file is valid for (much longer than) the duration of `T::open`.
>      // * There is no active fdget_pos region on the file on this thread.
> -    let ptr = match T::open(unsafe { File::from_raw_file(file) }) {
> +    let file = unsafe { File::from_raw_file(raw_file) };
> +
> +    let ptr = match T::open(file, misc) {
>          Ok(ptr) => ptr,
>          Err(err) => return err.to_errno(),
>      };
>  
> -    // SAFETY: The open call of a file owns the private data.
> -    unsafe { (*file).private_data = ptr.into_foreign().cast_mut() };
> +    // This overwrites the private data with the value specified by the user, changing the type of
> +    // this file's private data. All future accesses to the private data is performed by other
> +    // fops_* methods in this file, which all correctly cast the private data to the new type.
> +    //
> +    // SAFETY: The open call of a file can access the private data.
> +    unsafe { (*raw_file).private_data = ptr.into_foreign().cast_mut() };
>  
>      0
>  }
> 
> -- 
> 2.47.1.613.gc27f4b7a9f-goog
> 

-- 
Lee Jones [李琼斯]

On Wed, 11 Dec 2024, Lee Jones wrote:

> On Tue, 10 Dec 2024, Alice Ryhl wrote:
> 
> > Providing access to the underlying `struct miscdevice` is useful for
> > various reasons. For example, this allows you access the miscdevice's
> > internal `struct device` for use with the `dev_*` printing macros.
> > 
> > Note that since the underlying `struct miscdevice` could get freed at
> > any point after the fops->open() call (if misc_deregister is called),
> > only the open call is given access to it. To use `dev_*` printing macros
> > from other fops hooks, take a refcount on `miscdevice->this_device` to
> > keep it alive. See the linked thread for further discussion on the
> > lifetime of `struct miscdevice`.
> > 
> > Link: https://lore.kernel.org/r/2024120951-botanist-exhale-4845@gregkh
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  rust/kernel/miscdevice.rs | 30 ++++++++++++++++++++++--------
> >  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> Reviewed-by: Lee Jones <lee@kernel.org>

Tested-by: Lee Jones <lee@kernel.org>

-- 
Lee Jones [李琼斯]