[PATCH 2/2] rust: revocable: fix race between concurrent revokers

[PATCH 2/2] rust: revocable: fix race between concurrent revokers

[Danilo Krummrich]

There is a potential race condition when two paths try to revoke a
Revocable concurrently.

It can happen with e.g. Devres, where the driver core's
devres_release_all() calls Revocable::revoke() via the devres callback,
while Devres::drop() calls revoke_nosync() on another CPU.

The revoker that does not claim the is_available swap returns
immediately, but the revoker that did may still be executing
drop_in_place() on the inner data. This can cause a use-after-free when
the other revoker's caller proceeds to drop adjacent resources that
drop_in_place() still references (e.g., Devres<DmaMappedSgt> racing with
SGTable freeing the backing sg_table and pages).

Fix this by adding a Completion to Revocable. The revoker that claims
the swap signals the Completion after drop_in_place() finishes, and any
concurrent revoker waits for it before returning. This ensures the
wrapped object is fully torn down before either path continues.

If needed, a revoke_no_wait() variant that does not wait for concurrent
revocations to complete can be added in the future.

Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/dri-devel/20260612202841.2577C1F000E9@smtp.kernel.org/
Suggested-by: Gary Guo <gary@garyguo.net>
Fixes: 05aa6fb1c21d ("rust: scatterlist: Add abstraction for sg_table")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/revocable.rs | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index 0f4ae673256d..6d9d6ecccba1 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -7,7 +7,15 @@
 
 use pin_init::Wrapper;
 
-use crate::{bindings, prelude::*, sync::rcu, types::Opaque};
+use crate::{
+    bindings,
+    prelude::*,
+    sync::{
+        rcu,
+        Completion, //
+    },
+    types::Opaque,
+};
 use core::{
     marker::PhantomData,
     ops::Deref,
@@ -67,6 +75,8 @@
 pub struct Revocable<T> {
     is_available: AtomicBool,
     #[pin]
+    revocation: Completion,
+    #[pin]
     data: Opaque<T>,
 }
 
@@ -85,6 +95,7 @@ impl<T> Revocable<T> {
     pub fn new<E>(data: impl PinInit<T, E>) -> impl PinInit<Self, E> {
         try_pin_init!(Self {
             is_available: AtomicBool::new(true),
+            revocation <- Completion::new().map_err(|e| match e {}),
             data <- Opaque::pin_init(data),
         }? E)
     }
@@ -168,6 +179,10 @@ unsafe fn revoke_internal<const SYNC: bool>(&self) -> bool {
             // SAFETY: We know `self.data` is valid because only one CPU can succeed the
             // `compare_exchange` above that takes `is_available` from `true` to `false`.
             unsafe { drop_in_place(self.data.get()) };
+
+            self.revocation.complete_all();
+        } else {
+            self.revocation.wait_for_completion();
         }
 
         revoke
@@ -179,7 +194,8 @@ unsafe fn revoke_internal<const SYNC: bool>(&self) -> bool {
     /// expecting that there are no concurrent users of the object.
     ///
     /// Returns `true` if `&self` has been revoked with this call, `false` if it was revoked
-    /// already.
+    /// already. In the latter case, this function waits for the concurrent revocation to complete
+    /// before returning.
     ///
     /// # Safety
     ///
@@ -200,7 +216,8 @@ pub unsafe fn revoke_nosync(&self) -> bool {
     /// function waits for the concurrent access to complete before dropping the wrapped object.
     ///
     /// Returns `true` if `&self` has been revoked with this call, `false` if it was revoked
-    /// already.
+    /// already. In the latter case, this function waits for the concurrent revocation to complete
+    /// before returning, ensuring the wrapped object has been fully dropped.
     pub fn revoke(&self) -> bool {
         // SAFETY: By passing `true` we ask `revoke_internal` to wait for the grace period to
         // finish.
-- 
2.54.0

Re: [PATCH 2/2] rust: revocable: fix race between concurrent revokers

[Boqun Feng]

On Thu, Jun 18, 2026 at 09:32:59PM +0200, Danilo Krummrich wrote:
> There is a potential race condition when two paths try to revoke a
> Revocable concurrently.
> 
> It can happen with e.g. Devres, where the driver core's
> devres_release_all() calls Revocable::revoke() via the devres callback,
> while Devres::drop() calls revoke_nosync() on another CPU.
> 
> The revoker that does not claim the is_available swap returns
> immediately, but the revoker that did may still be executing
> drop_in_place() on the inner data. This can cause a use-after-free when
> the other revoker's caller proceeds to drop adjacent resources that
> drop_in_place() still references (e.g., Devres<DmaMappedSgt> racing with
> SGTable freeing the backing sg_table and pages).
> 
> Fix this by adding a Completion to Revocable. The revoker that claims
> the swap signals the Completion after drop_in_place() finishes, and any
> concurrent revoker waits for it before returning. This ensures the
> wrapped object is fully torn down before either path continues.
> 

I'm not sure this issue is a Revocable issue or even Devres issue,
because normally if you have a

struct Foo {
    revoke: Revocable<T>
    data: U
}

and if `revoke` referenced `data`, then `T` will have a refcount on `U`
(of course this requires `U` to be a `Arc` or `ARef`, not very
effecient, but correct). And we won't have this issue because either
revoker will be responsible for the finally drop.

This issue happens particularly when we want to save the extra refcount
(and indirect reference), and I think this is the issue that `Foo`
should handle instead of `Revocable`. So maybe we should move the fix
into `Devres` layer? Thoughts?

(I'm still hoping there could be some lightweight usage of Revocable
other than Devres, hence the ask.)

Regards,
Boqun

> If needed, a revoke_no_wait() variant that does not wait for concurrent
> revocations to complete can be added in the future.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/dri-devel/20260612202841.2577C1F000E9@smtp.kernel.org/
> Suggested-by: Gary Guo <gary@garyguo.net>
> Fixes: 05aa6fb1c21d ("rust: scatterlist: Add abstraction for sg_table")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
[...]

Re: [PATCH 2/2] rust: revocable: fix race between concurrent revokers

[Danilo Krummrich]

On Thu Jun 18, 2026 at 11:35 PM CEST, Boqun Feng wrote:
> This issue happens particularly when we want to save the extra refcount
> (and indirect reference), and I think this is the issue that `Foo`
> should handle instead of `Revocable`. So maybe we should move the fix
> into `Devres` layer? Thoughts?
>
> (I'm still hoping there could be some lightweight usage of Revocable
> other than Devres, hence the ask.)

I agree that a "lightweight" usage of Revocable is reasonable, and we can still
have that; nothing prevents that (see below).

We could also turn it around and have revoke_wait() and make no wait the
default, but I think it is a bit of a footgun.

Another alternative would be a new type over Revocable, which may be a bit
cleaner. (Although in that case I can also just move it into Devres for now, as
it is the sole user of Revocable anyway.)

>> If needed, a revoke_no_wait() variant that does not wait for concurrent
>> revocations to complete can be added in the future.

Re: [PATCH 2/2] rust: revocable: fix race between concurrent revokers

[Boqun Feng]

On Fri, Jun 19, 2026 at 12:24:12AM +0200, Danilo Krummrich wrote:
> On Thu Jun 18, 2026 at 11:35 PM CEST, Boqun Feng wrote:
> > This issue happens particularly when we want to save the extra refcount
> > (and indirect reference), and I think this is the issue that `Foo`
> > should handle instead of `Revocable`. So maybe we should move the fix
> > into `Devres` layer? Thoughts?
> >
> > (I'm still hoping there could be some lightweight usage of Revocable
> > other than Devres, hence the ask.)
> 
> I agree that a "lightweight" usage of Revocable is reasonable, and we can still
> have that; nothing prevents that (see below).
> 
> We could also turn it around and have revoke_wait() and make no wait the
> default, but I think it is a bit of a footgun.
> 
> Another alternative would be a new type over Revocable, which may be a bit
> cleaner. (Although in that case I can also just move it into Devres for now, as
> it is the sole user of Revocable anyway.)
> 
> >> If needed, a revoke_no_wait() variant that does not wait for concurrent
> >> revocations to complete can be added in the future.

I'm worried about the space cost of this fix on Revocable as well. So a
new type or moving it into Devres feels better to me.

Regards,
Boqun