[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