Message-ID: <e8fac6ba-41fe-4695-bd30-b1e6c3235815@gmail.com>
Date: Wed, 14 Jan 2026 19:50:49 +0000
From: Igor Korotin <igor.korotin.linux@...il.com>
To: Danilo Krummrich <dakr@...nel.org>, gregkh@...uxfoundation.org,
 rafael@...nel.org, ojeda@...nel.org, boqun.feng@...il.com, gary@...yguo.net,
 bjorn3_gh@...tonmail.com, lossin@...nel.org, a.hindborg@...nel.org,
 aliceryhl@...gle.com, tmgross@...ch.edu, david.m.ertman@...el.com,
 ira.weiny@...el.com, leon@...nel.org, bhelgaas@...gle.com,
 kwilczynski@...nel.org, wsa+renesas@...g-engineering.com
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
 linux-pci@...r.kernel.org, linux-usb@...r.kernel.org,
 linux-i2c@...r.kernel.org
Subject: Re: [PATCH 0/6] Address race condition with Device::drvdata()

On 1/7/2026 10:34 AM, Danilo Krummrich wrote:
> Currently, the driver's device private data is allocated and initialized
> from driver core code called from bus abstractions after the driver's
> probe() callback returned the corresponding initializer.
> 
> Similarly, the driver's device private data is dropped within the
> remove() callback of bus abstractions after calling the remove()
> callback of the corresponding driver.
> 
> However, commit 6f61a2637abe ("rust: device: introduce
> Device::drvdata()") introduced an accessor for the driver's device
> private data for a Device<Bound>, i.e. a device that is currently bound
> to a driver.
> 
> Obviously, this is in conflict with dropping the driver's device private
> data in remove(), since a device can not be considered to be fully
> unbound after remove() has finished:
> 
> We also have to consider registrations guarded by devres - such as IRQ
> or class device registrations - which are torn down after remove() in
> devres_release_all().
> 
> Thus, it can happen that, for instance, a class device or IRQ callback
> still calls Device::drvdata(), which then runs concurrently to remove()
> (which sets dev->driver_data to NULL and drops the driver's device
> private data), before devres_release_all() started to tear down the
> corresponding registration. This is because devres guarded registrations
> can, as expected, access the corresponding Device<Bound> that defines
> their scope.
> 
> In C it simply is the driver's responsibility to ensure that its device
> private data is freed after e.g. an IRQ registration is unregistered.
> 
> Typically, C drivers achieve this by allocating their device private data
> with e.g. devm_kzalloc() before doing anything else, i.e. before e.g.
> registering an IRQ with devm_request_threaded_irq(), relying on the
> reverse order cleanup of devres [1].
> 
> Technically, we could do something similar in Rust. However, the
> resulting code would be pretty messy:
> 
> In Rust we have to differentiate between allocated but uninitialized
> memory and initialized memory in the type system. Thus, we would need to
> somehow keep track of whether the driver's device private data object
> has been initialized (i.e. probe() was successful and returned a valid
> initializer for this memory) and conditionally call the destructor of
> the corresponding object when it is freed.
> 
> This is because we'd need to allocate and register the memory of the
> driver's device private data *before* it is initialized by the
> initializer returned by the driver's probe() callback, because the
> driver could already register devres guarded registrations within
> probe() outside of the driver's device private data initializer.
> 
> Luckily there is a much simpler solution: Instead of dropping the
> driver's device private data at the end of remove(), we just drop it
> after the device has been fully unbound, i.e. after all devres callbacks
> have been processed.
> 
> For this, we introduce a new post_unbind() callback private to the
> driver-core, i.e. the callback is neither exposed to drivers, nor to bus
> abstractions.
> 
> This way, the driver-core code can simply continue to conditionally
> allocate the memory for the driver's device private data when the
> driver's initializer is returned from probe() - no change needed - and
> drop it when the driver-core code receives the post_unbind() callback.
> 
> --
> 
> Dependency wise we need a common Driver trait that describes the layout of a
> specific driver structure, such as struct pci_driver or struct platform_driver.
> Additional to this specific driver type (which was previously the associated
> type RegType of the RegistrationOps) it provides the offset to the embedded
> struct device_driver and the type of the driver's device private data.
> 
> This patch series contains two additional dependencies:
> 
>    (1) A fix for i2c::Driver::shutdown() to not free the driver's device
>        private data at all, which otherwise causes the exact same bug, and
>        is not necessary in the first place anyways.
> 
>    (2) Add the auxiliary::Driver::unbind() callback. Strictly speaking,
>        this is not a dependency, but without this patch the main fix of this
>        series leaves the remove() callback of the auxiliary bus
>        abstraction with either dead code or quite some code removed;
>        code that we would otherwise add back immediately afterwards.
> 
> --
> 
> [1] In fact, the cleanup ordering of devres is a separate challenge in
>      Rust, since it is technically unsound to rely on the driver to pick
>      the correct order. I am already working on a solution for this;
>      luckily this also has some synergies with optimizing the required
>      synchronize_rcu() calls required by the Rust Devres container
>      structure down to exactly one per driver unbind.
> 
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=driver/post_unbind
> 
> Danilo Krummrich (6):
>    rust: i2c: do not drop device private data on shutdown()
>    rust: auxiliary: add Driver::unbind() callback
>    rust: driver: introduce a common Driver trait
>    rust: driver: add DEVICE_DRIVER_OFFSET to the Driver trait
>    rust: driver: add DriverData type to the generic Driver trait
>    rust: driver: drop device private data post unbind
> 
>   drivers/base/dd.c             |  4 ++
>   include/linux/device/driver.h | 11 +++++
>   rust/kernel/auxiliary.rs      | 41 +++++++++++++----
>   rust/kernel/device.rs         | 20 ++++----
>   rust/kernel/driver.rs         | 86 ++++++++++++++++++++++++++++-------
>   rust/kernel/i2c.rs            | 31 ++++++++-----
>   rust/kernel/pci.rs            | 27 +++++++----
>   rust/kernel/platform.rs       | 27 +++++++----
>   rust/kernel/usb.rs            | 27 +++++++----
>   9 files changed, 203 insertions(+), 71 deletions(-)
> 
> 
> base-commit: 8510ef5e3cfbd7d59a16845f85cd0194a8689761

For the I2C parts: Acked-by: Igor Korotin <igor.korotin.linux@...il.com>

Thanks
Igor

Powered by blists - more mailing lists