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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z1bPYb0nDcUN7SKK@pollux.localdomain>
Date: Mon, 9 Dec 2024 12:07:13 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Trevor Gross <tmgross@...ch.edu>, Lee Jones <lee@...nel.org>,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] rust: miscdevice: access the `struct miscdevice`
 from fops->open()

On Mon, Dec 09, 2024 at 07:27:47AM +0000, 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, only the open call is given
> access to it. To print from other calls, they should take a refcount on
> the device to keep it alive.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> ---
>  rust/kernel/miscdevice.rs | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index 0cb79676c139..c5af1d5ec4be 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -104,7 +104,7 @@ pub trait MiscDevice {
>      /// 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>;

How is the user of this abstraction supposed to access the underlying struct
miscdevice e.g. from other fops? AFAICS, there is no way for the user to store a
device pointer / reference in their driver private data.

I also think it's a bit weird to pass the registration structure in open() to
access the device.

I think we need an actual representation of a struct miscdevice, i.e.
`misc::Device`.

We can discuss whether we want to implement it like I implemented `pci::Device`
and `platform::Device`, i.e. as an `ARef<device::Device>` or if we do it like
you proposed, but I think things should be aligned.

>  
>      /// Called when the misc device is released.
>      fn release(device: Self::Ptr, _file: &File) {
> @@ -190,14 +190,27 @@ impl<T: MiscDevice> VtableHelper<T> {
>          return ret;
>      }
>  
> +    // SAFETY: The opwn call of a file can access the private data.
> +    let misc_ptr = unsafe { (*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.
> +    // * The file is valid for the duration of the `T::open` call.
>      // * 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(file) };
> +
> +    let ptr = match T::open(file, misc) {
>          Ok(ptr) => ptr,
>          Err(err) => return err.to_errno(),
>      };
>  
> +    // This overwrites the private data from above. It makes sense to not hold on to the misc
> +    // pointer since the `struct miscdevice` can get unregistered as soon as we return from this
> +    // call, so the misc pointer might be dangling on future file operations.
> +    //
>      // SAFETY: The open call of a file owns the private data.
>      unsafe { (*file).private_data = ptr.into_foreign().cast_mut() };
>  
> 
> -- 
> 2.47.1.545.g3c1d2e2a6a-goog
> 
>