linux-kernel - Re: [PATCH 2/8] rust: device: introduce Device::drvdata()

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: <aQIPvaFJIXySV-Q5@google.com>
Date: Wed, 29 Oct 2025 12:59:41 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com, 
	kwilczynski@...nel.org, david.m.ertman@...el.com, ira.weiny@...el.com, 
	leon@...nel.org, acourbot@...dia.com, ojeda@...nel.org, alex.gaynor@...il.com, 
	boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com, 
	lossin@...nel.org, a.hindborg@...nel.org, tmgross@...ch.edu, 
	pcolberg@...hat.com, rust-for-linux@...r.kernel.org, 
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/8] rust: device: introduce Device::drvdata()

On Tue, Oct 21, 2025 at 12:34:24AM +0200, Danilo Krummrich wrote:
> In C dev_get_drvdata() has specific requirements under which it is valid
> to access the returned pointer. That is, drivers have to ensure that
> 
>   (1) for the duration the returned pointer is accessed the driver is
>       bound and remains to be bound to the corresponding device,
> 
>   (2) the returned void * is treated according to the driver's private
>       data type, i.e. according to what has been passed to
>       dev_set_drvdata().
> 
> In Rust, (1) can be ensured by simply requiring the Bound device
> context, i.e. provide the drvdata() method for Device<Bound> only.
> 
> For (2) we would usually make the device type generic over the driver
> type, e.g. Device<T: Driver>, where <T as Driver>::Data is the type of
> the driver's private data.
> 
> However, a device does not have a driver type known at compile time and
> may be bound to multiple drivers throughout its lifetime.
> 
> Hence, in order to be able to provide a safe accessor for the driver's
> device private data, we have to do the type check on runtime.
> 
> This is achieved by letting a driver assert the expected type, which is
> then compared to a type hash stored in struct device_private when
> dev_set_drvdata() is called.
> 
> Example:
> 
> 	// `dev` is a `&Device<Bound>`.
> 	let data = dev.drvdata::<SampleDriver>()?;
> 
> There are two aspects to note:
> 
>   (1) Technically, the same check could be achieved by comparing the
>       struct device_driver pointer of struct device with the struct
>       device_driver pointer of the driver struct (e.g. struct
>       pci_driver).
> 
>       However, this would - in addition the pointer comparison - require
>       to tie back the private driver data type to the struct
>       device_driver pointer of the driver struct to prove correctness.
> 
>       Besides that, accessing the driver struct (stored in the module
>       structure) isn't trivial and would result into horrible code and
>       API ergonomics.
> 
>   (2) Having a direct accessor to the driver's private data is not
>       commonly required (at least in Rust): Bus callback methods already
>       provide access to the driver's device private data through a &self
>       argument, while other driver entry points such as IRQs,
>       workqueues, timers, IOCTLs, etc. have their own private data with
>       separate ownership and lifetime.
> 
>       In other words, a driver's device private data is only relevant
>       for driver model contexts (such a file private is only relevant
>       for file contexts).
> 
> Having that said, the motivation for accessing the driver's device
> private data with Device<Bound>::drvdata() are interactions between
> drivers. For instance, when an auxiliary driver calls back into its
> parent, the parent has to be capable to derive its private data from the
> corresponding device (i.e. the parent of the auxiliary device).
> 
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>

Are you going to open that docs PR to the Rust compiler about the size
of TypeID that we talked about? :)

Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>

> +// Compile-time checks.
> +const _: () = {
> +    // Assert that we can `read()` / `write()` a `TypeId` instance from / into `struct driver_type`.
> +    static_assert!(core::mem::size_of::<bindings::driver_type>() == core::mem::size_of::<TypeId>());
> +};

You don't need the "const _: ()" part. See the definition of
static_assert! to see why.

Also, I would not require equality. The Rust team did not think that it
would ever increase in size, but it may decrease.

>  /// The core representation of a device in the kernel's driver model.
>  ///
>  /// This structure represents the Rust abstraction for a C `struct device`. A [`Device`] can either
> @@ -198,12 +204,29 @@ pub unsafe fn as_bound(&self) -> &Device<Bound> {
>  }
>  
>  impl Device<CoreInternal> {
> +    fn type_id_store<T: 'static>(&self) {

This name isn't great. How about "set_type_id()" instead?

Alice