linux-kernel - Re: [PATCH v3 1/2] rust: miscdevice: add additional data to MiscDeviceRegistration

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: <682b758e.e90a0220.96106.9ada@mx.google.com>
Date: Mon, 19 May 2025 11:16:44 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Christian Schrefl <chrisi.schrefl@...il.com>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...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>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Lee Jones <lee@...nel.org>,
	Daniel Almeida <daniel.almeida@...labora.com>,
	Gerald Wisböck <gerald.wisboeck@...ther.ink>,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] rust: miscdevice: add additional data to
 MiscDeviceRegistration

On Sat, May 17, 2025 at 03:42:29PM +0200, Danilo Krummrich wrote:
> On Sat, May 17, 2025 at 01:33:49PM +0200, Christian Schrefl wrote:
> > +pub struct MiscDeviceRegistration<T: MiscDevice> {
> >      #[pin]
> >      inner: Opaque<bindings::miscdevice>,
> > +    #[pin]
> > +    data: UnsafePinned<T::RegistrationData>,
> >      _t: PhantomData<T>,
> >  }
> 
> I recommend not to store data within a Registration type itself.
> 
> I know that this is designed with the focus on using misc device directly from
> the module scope; and in this context it works great.
> 
> However, it becomes quite suboptimal when used from a driver scope. For
> instance, if the misc device is registered within a platform driver's probe()
> function.
> 
> I know this probably isn't supported yet. At least, I assume it isn't supported
> "officially", given that the abstraction does not provide an option to set a
> parent device. Yet I think we should consider it.
> 
> The reason this is suboptimal is that, from the callbacks of a misc device we
> may want to access device resources from the platform device.
> 
> Since device resources have to be protected with Devres, we'd need to access
> them with Revocable::try_access_with() for instance.
> 
> However, it would be much better if we had proof that the parent device of the
> misc device (i.e. the platform device) is bound (i.e. provide a &Device<Bound>)
> and hence are able to access device resources directly.
> 
> The only way to prove this, is to prove that the misc device registration is
> guaranteed to be removed when the parent device (i.e. the platform driver) is
> unbound.
> 
> And this we can only prove if we wrap MiscDeviceRegistration itself in a
> Devres; we don't want MiscDeviceRegistration to out-live the driver it was
> registered by anyways, so that's a free optimization.
> 
> If the data above is stored directly in the MiscDeviceRegistration however it
> means that we can only access it through a Devres<MiscDeviceRegistration>, which
> would be annoying.
> 
> To be fair, storing data in MiscDeviceRegistration is not the main issue of why
> this is suboptimal in driver, but it adds to the problem.
> 
> In general, the design of MiscDeviceRegistration is a bit suboptimal to be used
> within drivers. For drivers it works much better when the Registration type
> really *only* represents the state of a thing being registered, such that we can
> guard it with Devres *without* any downsides or additional complexity. One
> example for that would be the drm::driver::Registration [1].
> 
> If we want misc device to work optimally with drivers as well, we need to split
> things in two types: `misc::Device`:
> 
> 	struct Device<T: MiscDevice> {
> 	   #[pin]
> 	   misc: Opaque<bindings::miscdevice>,
> 	   #[pin]
> 	   data: UnsafePinned<T::RegistrationData>,
> 	   _t: PhantomData<T>,
> 	}
> 
> and `misc::Registration`:
> 
> 	struct Registration(ARef<misc::Device>);
> 
> 
> and make the `misc::Device` own the data, not the `misc::Registration`.
> 
> This way we can wrap misc::Registration into a Devres, with all guarantees it
> gives us and an no downsides.
> 
> I'm not saying that I want to block this patch, especially given that using the
> misc device abstraction doesn't seem to be supported to be used from drivers,
> but please understand that the design of the misc device abstraction, while it
> works fine for the module scope, really is sub-optimal for the use within
> drivers and hence should be re-worked.
> 
> Can we please either do the re-work right away or add a proper TODO?
> 

Well, I'd say we do the re-work right away, because I don't see any
other work depends on this right now. Let's do the right thing.

Regards,
Boqun

> [1] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/drm/driver.rs?ref_type=heads#L121