linux-kernel - Re: [PATCH 6/7] rust: miscdevice: expose the parent device as &Device<Bound>

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: <aDreGUcvyR4kjMGl@pollux>
Date: Sat, 31 May 2025 12:46:49 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <lossin@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, ojeda@...nel.org,
	alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net,
	bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	a.hindborg@...nel.org, aliceryhl@...gle.com, tmgross@...ch.edu,
	chrisi.schrefl@...il.com, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/7] rust: miscdevice: expose the parent device as
 &Device<Bound>

On Sat, May 31, 2025 at 10:27:44AM +0200, Benno Lossin wrote:
> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> > @@ -227,11 +229,21 @@ fn drop(self: Pin<&mut Self>) {
> >      }
> >  }
> >  
> > +/// The arguments passed to the file operation callbacks of a [`MiscDeviceRegistration`].
> > +pub struct MiscArgs<'a, T: MiscDevice> {
> > +    /// The [`Device`] representation of the `struct miscdevice`.
> > +    pub device: &'a Device,
> > +    /// The parent [`Device`] of [`Self::device`].
> > +    pub parent: Option<&'a Device<Bound>>,
> > +    /// The `RegistrationData` passed to [`MiscDeviceRegistration::register`].
> > +    pub data: &'a T::RegistrationData,
> 
> Here I would also just use `T`, remove the `MiscDevice` bound and then
> use `MiscArgs<'_, Self::RegistrationData>` below.

It has the disadvantage that the documentation of the `data` field above needs
to be much more vague, since we can't claim that it's the `RegistrationData`
passed to `MiscDeviceRegistration::register` anymore -- given that, I'm not sure
it's worth changing.

> > +}
> > +
> >  /// Trait implemented by the private data of an open misc device.
> >  #[vtable]
> >  pub trait MiscDevice: Sized {
> >      /// What kind of pointer should `Self` be wrapped in.
> > -    type Ptr: ForeignOwnable + Send + Sync;
> > +    type Ptr: Send + Sync;
> 
> There is no info about this change in the commit message. Why are we
> changing this? This seems a bit orthogonal to the other change, maybe do
> it in a separate patch?

It's a consequence of the implementation:

A `Ptr` instance is created in the misc device's file operations open() callback
and dropped in the fops release() callback.

Previously, this was stored in the private data pointer of the struct file that
is passed for every file operation in open().

Also note that when open is called the private data pointer in a struct file
points to the corresponding struct miscdevice.

With this patch, we keep the pointer to the struct miscdevice in the private
data pointer of struct file, but instead store the `Ptr` instance in
`RawDeviceRegistration::private`.

Subsequently, ForeignOwnable is not a required trait anymore.

We need this in order to keep access to the `RawDeviceRegistration` throughout
file operations to be able to pass the misc device's parent as &Device<Bound>
through the `MiscArgs` above.

> Also `Ptr` doesn't make much sense for the name, since now that the
> `ForeignOwnable` bound is gone, I could set this to `Self` and then have
> access to `&Self` in the callbacks.

We can't make it `Self`, it might still be some pointer type, require pin-init,
etc. So, it has to be a generic type.

But, I agree that we should not name it `Ptr`, probably should never have been
named `Ptr`, but `Data`, `Private` or similar.

> Would that also make sense to use as a general change? So don't store
> `Self::Ptr`, but `Self` directly?

I think it can't be `Self`, see above.