Re: [PATCH 0/6] Address race condition with Device::drvdata()

["Danilo Krummrich" <dakr@kernel.org>]

On Mon Jan 12, 2026 at 4:34 PM CET, Alice Ryhl wrote:
> On Wed, Jan 07, 2026 at 05:40:20PM +0100, Danilo Krummrich wrote:
>> On Wed Jan 7, 2026 at 4:51 PM CET, Alice Ryhl wrote:
>> > If a &Device<Bound> lets you access a given value, then we must not
>> > destroy that value until after the last &Device<Bound> has expired.
>> >
>> > A &Device<Bound> lets you access the driver private data. And a
>> > &Device<Bound> lets you access the contents of a Devres<T>.
>> >
>> > Thus, the last &Device<Bound> must expire before we destroy driver
>> > private data or values inside of Devres<T>. Etc.
>> 
>> Yes, the last &Device<Bound> must expire before we destroy the device private
>> data. This is exactly what is achieved by this patch. The device private data is
>> destroyed after all devres callbacks have been processed, which guarantees that
>> there can't be any contexts left that provide a &Device<Bound>.
>> 
>> As for the values inside of a Devres<T>, this is exactly what I refer to in my
>> paragraph above talking about the unsoundness of the devres cleanup ordering in
>> Rust.
>> 
>> I also mention that I'm already working on a solution and it is in fact pretty
>> close to the solution you propose below, i.e. a generic mechanism to support
>> multiple devres domains (which I also see advantages for in C code).
>> 
>> As mentioned, this will also help with getting the required synchronize_rcu()
>> calls down to exactly one per device unbind.
>> 
>> Technically, we could utilize such a devres domain for dropping the device
>> private data, but there is no need to have a separate domain just for this, we
>> already have a distinct place for dropping and freeing the device private data
>> after the device has been fully unbound, which is much simpler than a separate
>> devres domain.
>> 
>> Now, you may argue we don't need a separate devres domain, and that we could use
>> the non-early devres domain. However, this would have the following implication:
>> 
>> In the destructor of the device private data, drivers could still try to use
>> device resources stored in the device private data through try_access(), which
>> may or may not succeed depending on whether the corresponding Devres<T>
>> containers are part of the device private data initializer or whether they have
>> been allocated separately.
>> 
>> Or in other words it would leave room for drivers to abuse this behavior.
>> 
>> Therefore, the desired order is:
>> 
>>   1. Driver::unbind() (A place for drivers to tear down the device;
>>      registrations are up - unless explicitly revoked by the driver (this is a
>>      semantic choice) - and device resources are accessible.)
>> 
>>   2. devm_early_* (Drop all devres guarded registrations.)
>> 
>>   3. No more &Device<Bound> left.
>> 
>>   4. devm_* (Drop all device resources.)
>> 
>>   5. No more device resources left.
>> 
>>   6. Drop and free device private data. (try_access() will never succeed in the
>>      destructor of the device private data.
>
> so your private data is just the first devres resource ;)

Correct, that would work as well. However, I have a paragraph in the cover
letter explaining why this implementation is not desirable, i.e. more error
prone implementation and more explicit handling required by bus code.

> Ok. I'm worried that when you fix Devres, you have to undo changes you
> made here. But I guess that's not the end of the world (and maybe you
> don't have to).

For the reasons above this will remain as is even with the Devres rework. With a
separate devres stage it would become less error prone and we could also avoid
bus code involvement, but it would still be more complicated than a simple
callback.

> Concept SGTM. I have not yet reviewed patches in details, but
>
> Acked-by: Alice Ryhl <aliceryhl@google.com>

Thanks for taking a look! :)