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

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

[Danilo Krummrich]

Fix a race condition caused by not considering that Revocable does not guarantee
the wrapped object has been fully dropped by the time revoke() or
revoke_nosync() returns false.

There is currently no actual bug caused by this, but it causes SGTable and
Lyude's patch in [1] to be unsound.

Since Completion::new() is infallible but Revocable::new() is generic over the
error type, the first patch adds PinInit::map_err() to bridge the error type
mismatch.

This can either go separately or together with Lyude's patch. Both cases are
unlikely to ever hit this problem, so it should be fine either way.

[1] https://lore.kernel.org/dri-devel/20260612194436.585385-5-lyude@redhat.com/

Danilo Krummrich (2):
  pin-init: add PinInit::map_err() for error type conversion
  rust: revocable: fix race between concurrent revokers

 rust/kernel/revocable.rs | 23 ++++++++++++++++++---
 rust/pin-init/src/lib.rs | 44 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 3 deletions(-)


base-commit: 66affa37cfac0aec061cc4bcf4a065b0c52f7e19
-- 
2.54.0

[PATCH 1/2] pin-init: add PinInit::map_err() for error type conversion

[Danilo Krummrich]

Add a map_err() method to PinInit that converts the error type of an
initializer. This is useful when combining initializers with different
error types, for example when an infallible initializer (error type
Infallible) needs to be used in a context that expects a different error
type.

The MapErr wrapper stores the original initializer and mapping function,
implementing PinInit and Init with the appropriate trait bounds on the
original initializer.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/pin-init/src/lib.rs | 44 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/rust/pin-init/src/lib.rs b/rust/pin-init/src/lib.rs
index fd40c8f244a1..deaf65147e99 100644
--- a/rust/pin-init/src/lib.rs
+++ b/rust/pin-init/src/lib.rs
@@ -949,6 +949,18 @@ fn pin_chain<F>(self, f: F) -> ChainPinInit<Self, F, T, E>
     {
         ChainPinInit(self, f, __internal::PhantomInvariant::new())
     }
+
+    /// Convert the error type of this pin-initializer.
+    ///
+    /// This is useful when combining initializers with different error types, for example
+    /// when an infallible initializer (error type [`Infallible`]) needs to be used in a context
+    /// that expects a different error type.
+    fn map_err<E2, F>(self, f: F) -> MapErr<Self, F, T, E>
+    where
+        F: FnOnce(E) -> E2,
+    {
+        MapErr(self, f, __internal::PhantomInvariant::new())
+    }
 }
 
 /// An initializer returned by [`PinInit::pin_chain`].
@@ -975,6 +987,38 @@ unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
     }
 }
 
+/// An initializer returned by [`PinInit::map_err`].
+pub struct MapErr<I, F, T: ?Sized, E>(I, F, __internal::PhantomInvariant<(E, T)>);
+
+// SAFETY: The `__pinned_init` function is implemented such that it
+// - returns `Ok(())` on successful initialization,
+// - returns `Err(err)` on error and in this case `slot` will be dropped.
+// - considers `slot` pinned.
+unsafe impl<T: ?Sized, E, E2, I, F> PinInit<T, E2> for MapErr<I, F, T, E>
+where
+    I: PinInit<T, E>,
+    F: FnOnce(E) -> E2,
+{
+    unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E2> {
+        // SAFETY: All requirements fulfilled since this function is `__pinned_init`.
+        unsafe { self.0.__pinned_init(slot).map_err(self.1) }
+    }
+}
+
+// SAFETY: The `__init` function is implemented such that it
+// - returns `Ok(())` on successful initialization,
+// - returns `Err(err)` on error and in this case `slot` will be dropped.
+unsafe impl<T: ?Sized, E, E2, I, F> Init<T, E2> for MapErr<I, F, T, E>
+where
+    I: Init<T, E>,
+    F: FnOnce(E) -> E2,
+{
+    unsafe fn __init(self, slot: *mut T) -> Result<(), E2> {
+        // SAFETY: All requirements fulfilled since this function is `__init`.
+        unsafe { self.0.__init(slot).map_err(self.1) }
+    }
+}
+
 /// An initializer for `T`.
 ///
 /// To use this initializer, you will need a suitable memory location that can hold a `T`. This can
-- 
2.54.0

[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