Re: [PATCH 3/3] rust: devres: fix race in Devres::drop()

["Benno Lossin" <lossin@kernel.org>]

On Thu Jun 12, 2025 at 12:26 PM CEST, Danilo Krummrich wrote:
> On Thu, Jun 12, 2025 at 10:13:29AM +0200, Benno Lossin wrote:
>> On Tue Jun 3, 2025 at 10:48 PM CEST, Danilo Krummrich wrote:
>> > In Devres::drop() we first remove the devres action and then drop the
>> > wrapped device resource.
>> >
>> > The design goal is to give the owner of a Devres object control over when
>> > the device resource is dropped, but limit the overall scope to the
>> > corresponding device being bound to a driver.
>> >
>> > However, there's a race that was introduced with commit 8ff656643d30
>> > ("rust: devres: remove action in `Devres::drop`"), but also has been
>> > (partially) present from the initial version on.
>> >
>> > In Devres::drop(), the devres action is removed successfully and
>> > subsequently the destructor of the wrapped device resource runs.
>> > However, there is no guarantee that the destructor of the wrapped device
>> > resource completes before the driver core is done unbinding the
>> > corresponding device.
>> >
>> > If in Devres::drop(), the devres action can't be removed, it means that
>> > the devres callback has been executed already, or is still running
>> > concurrently. In case of the latter, either Devres::drop() wins revoking
>> > the Revocable or the devres callback wins revoking the Revocable. If
>> > Devres::drop() wins, we (again) have no guarantee that the destructor of
>> > the wrapped device resource completes before the driver core is done
>> > unbinding the corresponding device.
>> 
>> I don't understand the exact sequence of events here. Here is what I got
>> from your explanation:
>> 
>> * the driver created a `Devres<T>` associated to their device.
>> * their physical device gets disconnected and thus the driver core
>>   starts unbinding the device.
>> * simultaneously, the driver drops the `Devres<T>` (eg because the
>>   driver initiated the physical removal)
>> * now `devres_callback` is being called from both `Devres::Drop` (which
>>   calls `Devres::remove_action`) and from the driver core.
>> * they both call `inner.data.revoke()`, but only one wins, in our
>>   example `Devres::drop`.
>> * but now the driver core has finished running `devres_callback` and
>>   finalizes unbinding the device, even though the `Devres` still exists
>>   though is almost done being dropped.
>
> Your "almost done being dropped" is close, actually Devres::drop() may or may
> not be done calling Revocable::revoke(), i.e. drop_in_place() of the data.
>
> CPU0						CPU1
>
> Devres::drop() {				devres_callback() {
> 	self.data.revoke() {				this.data.revoke() {
> 		is_available.swap() == true
> 								is_available.swap == false
> 							}
> 						}
>
> 						// [...]
> 						// driver fully unbound
> 		drop_in_place() {
> 			pci_iounmap()
> 			pci_release_region()
> 		}
> 	}
> }

Ahh I missed that the splitting point could be inside of `revoke`.

What are those `pci_iounmap` and `pci_release_region` calls? Ah maybe
they are from a `T` that expects the driver to still be bound when it is
being dropped? That would make a lot of sense.

If I'm correct, do you mind adding the above example execution to the
commit message? Including the information that the type `T` is allowed
to expect that the driver is still bound while being dropped. That would
make it obvious to me what the issue is.

> This means that we have to ensure that the revoke() in Devres::drop() is
> completed before devres_callback() completes, in case they race.

Agreed :)

---
Cheers,
Benno