Message-ID: <aD-EiRChuScS5TK-@tardis.local>
Date: Tue, 3 Jun 2025 16:26:01 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, ojeda@...nel.org,
	alex.gaynor@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	benno.lossin@...ton.me, a.hindborg@...nel.org, aliceryhl@...gle.com,
	tmgross@...ch.edu, chrisi.schrefl@...il.com,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] rust: devres: fix race in Devres::drop()

On Tue, Jun 03, 2025 at 10:48:52PM +0200, 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.
> 
> Depending on the specific device resource, this can potentially lead to
> user-after-free bugs.
> 

This all sounds reasonable, one question though: it seems to me the
problem exists only for the device resources that expect the device
being bounded, so hypothetically if the device resources can be
programmed against unbound devices, then the current behavior should be
fine? For example, in your case, you want free_irq() to happen before
the device becomes unbound, which is of course reasonable, but it sounds
more like a design choice (or what device model we want to use), because
hypothetically you can program an irq that still works even if the
device is unbound, no?

Again this sounds reasonable to me, just want to check my understanding
here.

Regards,
Boqun

> In order to fix this, implement the following logic.
> 
> In the devres callback, we're always good when we get to revoke the
> device resource ourselves, i.e. Revocable::revoke() returns true.
> 
> If Revocable::revoke() returns false, it means that Devres::drop(),
> concurrently, already drops the device resource and we have to wait for
> Devres::drop() to signal that it finished dropping the device resource.
> 
> Note that if we hit the case where we need to wait for the completion of
> Devres::drop() in the devres callback, it means that we're actually
> racing with a concurrent Devres::drop() call, which already started
> revoking the device resource for us. This is rather unlikely and means
> that the concurrent Devres::drop() already started doing our work and we
> just need to wait for it to complete it for us. Hence, there should not
> be any additional overhead from that.
> 
> (Actually, for now it's even better if Devres::drop() does the work for
> us, since it can bypass the synchronize_rcu() call implied by
> Revocable::revoke(), but this goes away anyways once I get to implement
> the split devres callback approach, which allows us to first flip the
> atomics of all registered Devres objects of a certain device, execute a
> single synchronize_rcu() and then drop all revocable objects.)
> 
> In Devres::drop() we try to revoke the device resource. If that is *not*
> successful, it means that the devres callback already did and we're good.
> 
> Otherwise, we try to remove the devres action, which, if successful,
> means that we're good, since the device resource has just been revoked
> by us *before* we removed the devres action successfully.
> 
> If the devres action could not be removed, it means that the devres
> callback must be running concurrently, hence we signal that the device
> resource has been revoked by us, using the completion.
> 
> This makes it safe to drop a Devres object from any task and at any point
> of time, which is one of the design goals.
> 
[...]

Powered by blists - more mailing lists