Re: [PATCH v3 2/3] rust: miscdevice: access the `struct miscdevice` from fops->open()

[Lee Jones <lee@kernel.org>]

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 [李琼斯]