Skip to contentSkip to conversation

rust: devres: fix race between concurrent revokers

PATCH Applied Danilo Krummrich 3 participants 7 messages Jun 28 – Jul 13

Subsystems (3)
Stable branch Driver core Rust
lore
Share & tools
lore

Danilo Krummrich

author maintainer
#0 lore raw

PATCH 1/1 rust: devres: fix race between concurrent revokers

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

The driver core's devres_release_all() calls Revocable::revoke() via the release 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. The release callback signals the Completion after revoke() finishes, and Devres::drop() waits for it when it loses the is_available swap. This ensures the wrapped object is fully torn down before Devres::drop() returns.

Cc [email protected]
Reported-by Sashiko <[email protected]>
Closes https://lore.kernel.org/dri-devel/[email protected]/ open here
Fixes 05aa6fb1c21d ("rust: scatterlist: Add abstraction for sg_table")
Signed-off-by Danilo Krummrich <[email protected]>
---
 rust/kernel/devres.rs | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)
rust/kernel/devres.rs +16 -2
index 11ce500e9b76..11d862f1e6de 100644
@@ -21,7 +21,8 @@
     sync::{
         aref::ARef,
         rcu,
-        Arc, //
+        Arc,
+        Completion, //
     },
     types::{
         ForeignOwnable,

@@ -37,6 +38,8 @@ struct Inner<T> {
     node: Opaque<bindings::devres_node>,
     #[pin]
     data: Revocable<T>,
+    #[pin]
+    revocation: Completion,
 }
 
 /// This abstraction is meant to be used by subsystems to containerize [`Device`] bound resources to

@@ -53,6 +56,10 @@ struct Inner<T> {
 /// After the [`Devres`] has been unbound it is not possible to access the encapsulated resource
 /// anymore.
 ///
+/// When a [`Devres`] is dropped, it is guaranteed that `T` has been fully dropped by the time
+/// [`Devres::drop`] returns, even if a concurrent revocation through the release callback is in
+/// progress.
+///
 /// [`Devres`] users should make sure to simply free the corresponding backing resource in `T`'s
 /// [`Drop`] implementation.
 ///

@@ -217,6 +224,7 @@ pub fn new<E>(dev: &Device<Bound>, data: impl PinInit<T, E>) -> Result<Self>
                     };
                 }),
                 data <- Revocable::new(data),
+                revocation <- Completion::new(),
             }),
             GFP_KERNEL,
         )?;

@@ -254,7 +262,9 @@ fn data(&self) -> &Revocable<T> {
         // SAFETY: `inner` is a valid `Inner<T>` pointer.
         let inner = unsafe { &*inner };
 
-        inner.data.revoke();
+        if inner.data.revoke() {
+            inner.revocation.complete_all();
+        }
     }
 
     #[allow(clippy::missing_safety_doc)]

@@ -361,6 +371,10 @@ fn drop(&mut self) {
                 // this additional reference count.
                 drop(unsafe { Arc::from_raw(Arc::as_ptr(&self.inner)) });
             }
+        } else {
+            // The release callback is concurrently revoking; wait for it to finish
+            // `drop_in_place()` of the wrapped object before returning.
+            self.inner.revocation.wait_for_completion();
         }
     }
 }

base-commit: 0716f9b9338a86dd27796e00ed0fd560c653323a

Signature · 1 line
2.54.0
author maintainer
#1 lore raw

PATCH 1/1 rust: devres: ensure revocation is complete before device finishes unbinding

Now that the revocation Completion is in place, also address the symmetric case. When Devres::drop() wins the is_available swap and the devres callback loses, the callback returns to devres_release_all() without waiting. This means device unbinding can complete while Devres::drop() is still executing drop_in_place() on another CPU, which is a problem if T's destructor accesses device state.

Make the synchronization bidirectional. Whichever side performs drop_in_place() signals the Completion, and the other side waits.

This does not reintroduce the nested Devres deadlock fixed by commit ba268514ea14 ("rust: devres: fix race condition due to nesting"), because that deadlock was caused by drop waiting for the release callback to return (the old 'devm' Completion). Here, both sides only wait for drop_in_place() to finish, which completes within the current call chain. The Arc<Inner<T>> keeps the Inner allocation alive independently.

Cc [email protected]
Fixes ba268514ea14 ("rust: devres: fix race condition due to nesting")
Signed-off-by Danilo Krummrich <[email protected]>
---
 rust/kernel/devres.rs | 7 +++++++
 1 file changed, 7 insertions(+)
rust/kernel/devres.rs +7 -0
index 11d862f1e6de..f112c7e8bc3b 100644
@@ -264,6 +264,11 @@ fn data(&self) -> &Revocable<T> {
 
         if inner.data.revoke() {
             inner.revocation.complete_all();
+        } else {
+            // Devres::drop() is concurrently revoking; wait for it to finish `drop_in_place()`
+            // before returning to `devres_release_all()`, ensuring `T` is fully torn down before
+            // the device finishes unbinding.
+            inner.revocation.wait_for_completion();
         }
     }
 

@@ -364,6 +369,8 @@ fn drop(&mut self) {
         // SAFETY: When `drop` runs, it is guaranteed that nobody is accessing the revocable data
         // anymore, hence it is safe not to wait for the grace period to finish.
         if unsafe { self.data().revoke_nosync() } {
+            self.inner.revocation.complete_all();
+
             // We revoked `self.data` before devres did, hence try to remove it.
             if self.remove_node() {
                 // SAFETY: In `Self::new` we have taken an additional reference count of `self.data`

base-commit: 6cb8c4e26a3684c9df382a350f06bfbe2a197e5e

Signature · 1 line
2.54.0
reviewer Reviewed-by on v1 1/1
#2 lore raw

Re: [PATCH] rust: devres: ensure revocation is complete before device finishes unbinding

Quoted 18 lines · Danilo Krummrich

Now that the revocation Completion is in place, also address the symmetric case. When Devres::drop() wins the is_available swap and the devres callback loses, the callback returns to devres_release_all() without waiting. This means device unbinding can complete while Devres::drop() is still executing drop_in_place() on another CPU, which is a problem if T's destructor accesses device state.

Make the synchronization bidirectional. Whichever side performs drop_in_place() signals the Completion, and the other side waits.

This does not reintroduce the nested Devres deadlock fixed by commit ba268514ea14 ("rust: devres: fix race condition due to nesting"), because that deadlock was caused by drop waiting for the release callback to return (the old 'devm' Completion). Here, both sides only wait for drop_in_place() to finish, which completes within the current call chain. The Arc<Inner<T>> keeps the Inner allocation alive independently.

Quoted 3 lines
Cc [email protected]
Fixes ba268514ea14 ("rust: devres: fix race condition due to nesting")
Signed-off-by Danilo Krummrich <[email protected]>
Reviewed-by Alice Ryhl <[email protected]>
reviewer Reviewed-by on v1 1/1
#3 lore raw

Re: [PATCH] rust: devres: ensure revocation is complete before device finishes unbinding

In reply to Danilo Krummrich
Quoted 18 lines · Danilo Krummrich

Now that the revocation Completion is in place, also address the symmetric case. When Devres::drop() wins the is_available swap and the devres callback loses, the callback returns to devres_release_all() without waiting. This means device unbinding can complete while Devres::drop() is still executing drop_in_place() on another CPU, which is a problem if T's destructor accesses device state.

Make the synchronization bidirectional. Whichever side performs drop_in_place() signals the Completion, and the other side waits.

This does not reintroduce the nested Devres deadlock fixed by commit ba268514ea14 ("rust: devres: fix race condition due to nesting"), because that deadlock was caused by drop waiting for the release callback to return (the old 'devm' Completion). Here, both sides only wait for drop_in_place() to finish, which completes within the current call chain. The Arc<Inner<T>> keeps the Inner allocation alive independently.

Quoted 3 lines
Cc [email protected]
Fixes ba268514ea14 ("rust: devres: fix race condition due to nesting")
Signed-off-by Danilo Krummrich <[email protected]>
Reviewed-by Gary Guo <[email protected]>
Quoted 3 lines
---
 rust/kernel/devres.rs | 7 +++++++
 1 file changed, 7 insertions(+)
reviewer Reviewed-by on v1 1/1
#4 lore raw
In reply to Danilo Krummrich
Quoted 19 lines · Danilo Krummrich

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

The driver core's devres_release_all() calls Revocable::revoke() via the release 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. The release callback signals the Completion after revoke() finishes, and Devres::drop() waits for it when it loses the is_available swap. This ensures the wrapped object is fully torn down before Devres::drop() returns.

Quoted 5 lines
Cc [email protected]
Reported-by Sashiko <[email protected]>
Closes https://lore.kernel.org/dri-devel/[email protected]/ open here
Fixes 05aa6fb1c21d ("rust: scatterlist: Add abstraction for sg_table")
Signed-off-by Danilo Krummrich <[email protected]>
Reviewed-by Alice Ryhl <[email protected]>
reviewer Reviewed-by on v1 1/1
#5 lore raw
In reply to Danilo Krummrich
Quoted 19 lines · Danilo Krummrich

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

The driver core's devres_release_all() calls Revocable::revoke() via the release 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. The release callback signals the Completion after revoke() finishes, and Devres::drop() waits for it when it loses the is_available swap. This ensures the wrapped object is fully torn down before Devres::drop() returns.

Quoted 5 lines
Cc [email protected]
Reported-by Sashiko <[email protected]>
Closes https://lore.kernel.org/dri-devel/[email protected]/ open here
Fixes 05aa6fb1c21d ("rust: scatterlist: Add abstraction for sg_table")
Signed-off-by Danilo Krummrich <[email protected]>
Reviewed-by Gary Guo <[email protected]>
Quoted 3 lines
---
 rust/kernel/devres.rs | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)
author maintainer Applied
#6 lore raw
In reply to Danilo Krummrich
Quoted 1 line · Danilo Krummrich

[PATCH] rust: devres: fix race between concurrent revokers

Applied, thanks!

Branch: driver-core-testing
Tree: git://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git

[1/2] rust: devres: fix race between concurrent revokers

      commit: acc516dfa197
[2/2] rust: devres: ensure revocation is complete before device finishes unbinding
      commit: a10639966fd7

The patches will appear in the next linux-next integration (typically within 24 hours on weekdays).

The patches are in the driver-core-testing branch and will be promoted to driver-core-next after validation.