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: <aCiSRZjOETsD8MhX@pollux>
Date: Sat, 17 May 2025 15:42:29 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Christian Schrefl <chrisi.schrefl@...il.com>
Cc: 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>,
	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 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?

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