[PATCH 0/2] rust: Adjust dma_buf::DriverFence::drop()

[PATCH 0/2] rust: Adjust dma_buf::DriverFence::drop()

[Philipp Stanner]

While moving our collaborative drm::JobQueue draft forward, we
discovered that some changes to DmaFence are necessary so that the
design fits into various requirements regarding atomic context.

This mini-series is a requirement for upstreaming drm::JobQueue:

https://lore.kernel.org/rust-for-linux/20261016085724.1031766-2-phasta@kernel.org/

This is tested with current JobQueue and a dummy driver.

Philipp Stanner (2):
  rust: DmaFence: Don't drop on signal
  rust: DmaFence: Replace call_rcu() with synchronize_rcu()

 rust/kernel/dma_buf/dma_fence.rs | 100 ++++++++++---------------------
 1 file changed, 32 insertions(+), 68 deletions(-)


base-commit: 9fe580fd90e20c8432832c5f9a7e92b049ae9af6
-- 
2.56.0

[PATCH 1/2] rust: DmaFence: Don't drop on signal

[Philipp Stanner]

The original design for DmaFence intended to comply as strictly as
possible with the "dma_fence contract", which, according to some, says
that a fence must only be signaled once. Thus, the implementation was
made so that a DriverFence drops on signal.

While extending JobQueue's capabilities, it was discovered that this
characteristic causes - unnecessary - trouble: fence-data must be
dropped in an RCU-deferred manner because the C backend demands that.
However, fences are often signaled in atomic context (e.g., interrupt
handler) where you don't want to drop, but signal as quickly as
possible.

Actually, there is no real hard reason to enforce that a fence can only
get signaled once, because the C backend is robust against multiple
signal attempts.

What is decisive is that a) all fences signal and b) they only drop no
earlier than 1 RCU grace period after signaling.

Don't drop a DriverFence on signal().

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 rust/kernel/dma_buf/dma_fence.rs | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index a1d851656562..3022bdb5f7c3 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -805,7 +805,7 @@ pub fn as_fence(&self) -> &Fence {
     }
 
     /// Signal the fence. This will invoke all registered callbacks.
-    pub fn signal(self, res: Result) {
+    pub fn signal(&self, res: Result) {
         let fence = self.as_fence().lock();
 
         // SAFETY: `fence` is valid because `self` is valid. The lock must be
@@ -816,14 +816,15 @@ pub fn signal(self, res: Result) {
                 // must not have been signaled yet, which we check directly above.
                 unsafe { bindings::dma_fence_set_error(fence.as_raw(), err.to_errno()) };
             }
+
+            // SAFETY: `self.data` is valid because `self` is valid.
+            let fctx = unsafe { self.data.as_ref().fctx };
+            let _ = fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed);
+
             // SAFETY: `fence` is valid because `self` is valid. The lock must
             // be held, which we acquired above.
             unsafe { bindings::dma_fence_signal_locked(fence.as_raw()) };
         }
-
-        // SAFETY: `self.data` is valid because `self` is valid.
-        let fctx = unsafe { self.data.as_ref().fctx };
-        let _ = fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed);
     }
 }
 
-- 
2.56.0

[PATCH 2/2] rust: DmaFence: Replace call_rcu() with synchronize_rcu()

[Philipp Stanner]

The C dma_fence contract demands that a fence's data disappears no
earlier than 1 RCU grace period after the DriverFence was signaled.

So far, we achieved this with dropping the DriverFence's data in a
deferred manner with a manual, raw binding call to call_rcu().

However, this call_rcu() does not solve one problem: It is conceivable
that some driver data must not drop in atomic context; but call_rcu()'s
payload *can* be executed in atomic context.

Moreover, the current developments in drm::JobQueue, where a DriverFence
is always coupled 1:1 with a Job, allows for dropping job and fence
through a work item.

The undesirable blocking behavior of synchronize_rcu() would, thus, not
be annoying any thread anymore. The direct users of DmaFence could
achieve the same by dropping their DriverFences through work items.

The only conceivable other solution, queue_rcu_work(), would, for many
users, schedule a work_item from another work_item, which seems
undesirable.

Additionally, using the existing synchronize_rcu() abstraction has the
advantage of us getting rid of a raw bindings call.

Replace DriverFence::drop()'s call_rcu() with synchronize_rcu().

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 rust/kernel/dma_buf/dma_fence.rs | 89 ++++++++++----------------------
 1 file changed, 26 insertions(+), 63 deletions(-)

diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index 3022bdb5f7c3..ad9cebf8428c 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -42,7 +42,10 @@
             Atomic,
             Relaxed, //
         },
-        rcu::rcu_barrier, //
+        rcu::{
+            rcu_barrier,
+            synchronize_rcu, //
+        }, //
     }, //
 };
 
@@ -65,7 +68,7 @@ pub struct FenceContext<T: FenceContextOps + Send + Sync> {
     /// The sequence number for the next fence created.
     seqno: Atomic<u64>,
     // The name parameters can be accessed by the dma_fence backend_ops. UAF
-    // errors are prevented by the `call_rcu()` in `drop_driver_fence_data()`.
+    // errors are prevented by the `synchronize_rcu()` in `drop_driver_fence_data()`.
     /// The name of the driver this FenceContext's fences belong to.
     driver_name: CString,
     /// The name of the timeline this FenceContext's fences belong to.
@@ -142,7 +145,6 @@ pub fn new_fence_allocation(
         data: T::FenceDataType,
     ) -> Result<DriverFenceAllocation<'_, T>> {
         let fence_data = DriverFenceData {
-            rcu_head: Default::default(),
             // `inner` remains uninitialized until a `DriverFence` takes over.
             inner: Fence {
                 inner: Opaque::uninit(),
@@ -600,8 +602,6 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
     // necessary so that the C backend can free the allocation (coming from our
     // Rust code) with kfree_rcu().
     inner: Fence,
-    /// Callback head for dropping this in a deferred manner through RCU.
-    rcu_head: bindings::callback_head,
     /// Reference to access the FenceContext.
     fctx: &'a FenceContext<T>,
     /// The API user's data. It is essential that the data only performs
@@ -644,6 +644,10 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
 ///
 /// In the unlikely case of such violations, a panic is thrown.
 ///
+/// For requirements imposed by the C backend, a [`DriverFence`] might perform
+/// a call to [`rcu::synchronize_rcu`] when it drops. Thus, avoid dropping a
+/// [`DriverFence`] in atomic context.
+///
 /// # Examples
 ///
 /// ```
@@ -940,15 +944,15 @@ fn drop(&mut self) {
         // dma_fence_is_signaled_locked() because the C backend wants to get rid
         // of the latter.
 
-        // SAFETY: `guard` is valid until the `call_rcu()` below.
+        // SAFETY: `guard` is valid.
         let signaled: bool = unsafe { bindings::dma_fence_test_signaled_flag(guard.as_raw()) };
         if !signaled {
             pr_err!("DriverFence drops unsignaled. Danger of memory corruption!\n");
-            // SAFETY: `guard` is valid until the `call_rcu()` below. The fence
-            // must not have been signaled yet, which we check directly above.
+            // SAFETY: `guard` is valid. The fence must not have been signaled
+            // yet, which we check directly above.
             unsafe { bindings::dma_fence_set_error(guard.as_raw(), ECANCELED.to_errno()) };
-            // SAFETY: `guard` is valid until the `call_rcu()` below. The lock
-            // must be held, which we acquired above.
+            // SAFETY: `guard` is valid. The lock must be held, which we
+            // acquired above.
             unsafe { bindings::dma_fence_signal_locked(guard.as_raw()) };
 
             // SAFETY: `self.data` is valid because `self` is valid.
@@ -973,59 +977,18 @@ fn drop(&mut self) {
             return;
         }
 
-        // SAFETY: Valid because `self` is valid.
-        let rcu_head_ptr = unsafe { &raw mut (*self.data.as_ptr()).rcu_head };
+        // Make sure none of the fence backend_ops can access data anymore.
+        //
+        // TODO:
+        // This would not be necessary if the C dma_fence backend were using a
+        // spinlock to properly synchronize its signaled state. Fix it in C and
+        // then remove synchronize_rcu().
+        synchronize_rcu();
 
-        // SAFETY: `call_rcu()` is always safe to be called. `rcu_head_ptr` was
-        // created validly above. The module must perform a `synchronize_rcu()`
-        // or `rcu_barrier()` call to guard against module unload.
-        unsafe { bindings::call_rcu(rcu_head_ptr, Some(drop_driver_fence_data::<T>)) };
+        // SAFETY: Valid because `self` is valid.
+        unsafe { drop_in_place(&raw mut self.data) };
+
+        // SAFETY: The `synchronize_rcu()` above ensures all accessors are gone.
+        unsafe { bindings::dma_fence_put(self.as_raw()) };
     }
 }
-
-// TODO:
-// The entire call_rcu() mechanism in the drop above and the code below would be
-// unnecessary if C's dma_fence_signal() could be reworked in a way that after it
-// ran, the caller knows that no fence_ops callbacks can be running anymore.
-// In other words, if the dma_fence backend would use its spinlock for full
-// synchronization.
-//
-// Then we could move the drop_in_place() and dma_fence_put() upwards into the
-// drop() implementation and call it a day.
-
-/// Finally really drop this `DriverFence<T>`
-///
-/// # Safety
-///
-/// `head` references the `rcu_head` field of an `DriverFenceData<T>`. All
-/// accessors to that `DriverFenceData<T>` must be gone by now. This must be
-/// ensured by signalling the associated `DriverFence<T>` and then waiting
-/// for a grace period until calling this function here.
-unsafe extern "C" fn drop_driver_fence_data<T: Send + Sync + FenceContextOps>(
-    head: *mut bindings::callback_head,
-) {
-    // SAFETY: Caller provides a pointer to the `rcu_head` field of a `DriverFenceData<C>`.
-    let fence_data = unsafe { container_of!(head, DriverFenceData<'_, T>, rcu_head) };
-
-    // SAFETY: `fence_data` was created validly above. All the fence's data will
-    // only drop below, but the raw pointer to the raw C `dma_fence` remains
-    // valid because the reference count is only decremented at the end of the
-    // function.
-    let fence = unsafe { (*fence_data).inner.inner.get() };
-
-    // SAFETY: `fence_data` was created validly above. The user has already
-    // dropped the only conventional accessor to the user data, the `DriverFence`,
-    // one grace period ago. All accessors are gone now.
-    unsafe { drop_in_place(&raw mut (*fence_data).data) };
-
-    // The inner `Fence` explicitly does not get dropped because there may be
-    // many more users / consumers, each holding their own reference.
-
-    // SAFETY: Once a `DriverFence` is initialized, the inner `fence` is valid
-    // and initialized. It is valid until the refcount drops to 0, which can
-    // earliest happen once we drop the `DriverFence`'s reference here.
-    unsafe { bindings::dma_fence_put(fence) };
-
-    // The actual memory the data associated with a `DriverFence` lives in
-    // gets freed by the C dma_fence backend once the fence's refcount reaches 0.
-}
-- 
2.56.0

Re: [PATCH 1/2] rust: DmaFence: Don't drop on signal

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rust: dma_fence: premature unsignaled count decrement in signal()
--

commit cc3ae2917828ada16cb77f317557169874bb144b
Author: Philipp Stanner <phasta@kernel.org>

rust: DmaFence: Don't drop on signal

This commit changes DriverFence::signal() in rust/kernel/dma_buf/dma_fence.rs
to take a shared reference instead of consuming self, allowing fences to be
signaled without immediately dropping the DriverFence.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index a1d851656562d..3022bdb5f7c3e 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs
> @@ -805,7 +805,7 @@ pub fn as_fence(&self) -> &Fence {
>      }
>  
>      /// Signal the fence. This will invoke all registered callbacks.
> -    pub fn signal(self, res: Result) {
> +    pub fn signal(&self, res: Result) {
>          let fence = self.as_fence().lock();
>  
>          // SAFETY: `fence` is valid because `self` is valid. The lock must be
> @@ -816,14 +816,15 @@ pub fn signal(self, res: Result) {
>                  // must not have been signaled yet, which we check directly above.
>                  unsafe { bindings::dma_fence_set_error(fence.as_raw(), err.to_errno()) };
>              }
> +
> +            // SAFETY: `self.data` is valid because `self` is valid.
> +            let fctx = unsafe { self.data.as_ref().fctx };
> +            let _ = fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed);
> +
>              // SAFETY: `fence` is valid because `self` is valid. The lock must
>              // be held, which we acquired above.
>              unsafe { bindings::dma_fence_signal_locked(fence.as_raw()) };
>          }
> -
> -        // SAFETY: `self.data` is valid because `self` is valid.
> -        let fctx = unsafe { self.data.as_ref().fctx };
> -        let _ = fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed);
>      }
>  }

[Severity: High]
Since DriverFence::signal() no longer consumes self and no longer runs
DriverFence::drop(), can decrementing fctx.nr_of_unsignaled_fences here allow
FenceContext::drop() to free FenceContext too early?

In FenceContext::drop(), nr_of_unsignaled_fences is checked before calling
rcu_barrier() to wait for the RCU callbacks queued by DriverFence::drop():

rust/kernel/dma_buf/dma_fence.rs:FenceContext::drop() {
    if self.nr_of_unsignaled_fences.load(Relaxed) != 0 {
        panic!("Forgotten fences in FenceContext.");
    }
    ...
    // Wait for the RCU callbacks in `DriverFence::drop`.
    rcu_barrier();
}

Because DriverFence::signal() now takes &self and does not drop the fence,
DriverFence::drop() has not queued call_rcu(rcu_head_ptr,
Some(drop_driver_fence_data::<T>)) when signal() returns.

If a DriverFence is signaled via DriverFence::signal() and then forgotten in
safe Rust via core::mem::forget() (or held in foreign storage via
ForeignOwnable::into_foreign()) when FenceContext is dropped,
FenceContext::drop() will observe nr_of_unsignaled_fences == 0 without
panicking. It then runs rcu_barrier(), which returns without waiting for an
RCU grace period after signaling because no RCU callback was ever queued by
DriverFence::drop(). Concurrent RCU readers in dma_fence_driver_name() or
dma_fence_timeline_name() that raced with signaling could then read freed
fctx.driver_name and fctx.timeline_name memory, or a later
DriverFence::drop() on a foreign-owned fence could access a dangling fctx
reference and queue call_rcu() after FenceContext::drop() has already
completed.

Additionally, fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed) now runs
before bindings::dma_fence_signal_locked(fence.as_raw()). During
dma_fence_signal_locked(), trace_dma_fence_signaled() invokes
FenceContext::get_driver_name() and FenceContext::get_timeline_name(), which
dereference fctx. If a foreign-owned DriverFence is signaled via
DriverFence::borrow().signal() concurrently with FenceContext::drop(), could
FenceContext::drop() see nr_of_unsignaled_fences == 0 and free FenceContext
before dma_fence_signal_locked() even finishes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102150617.527772-2-phasta@kernel.org?part=1

Re: [PATCH 2/2] rust: DmaFence: Replace call_rcu() with synchronize_rcu()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] rust: dma_fence: leak of FenceDataType in DriverFence::drop()
- [High] rust: dma_fence: missing RCU grace period before FenceContext drop
- [Low] rust: dma_fence: stale atomic-context Drop docs on FenceDataType
- [Low] rust: dma_fence: broken intra-doc link to rcu::synchronize_rcu
--

commit 5b8baa0550a0166f96a31017e673de763e929c7c
Author: Philipp Stanner <phasta@kernel.org>

rust: DmaFence: Replace call_rcu() with synchronize_rcu()

This commit replaces the deferred call_rcu() callback in DriverFence::drop()
with a synchronous synchronize_rcu() call so that driver fence payload data is
not dropped in atomic context.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index 3022bdb5f7c3e..ad9cebf8428c0 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs
> @@ -42,7 +42,10 @@
>              Atomic,
>              Relaxed, //
>          },
> -        rcu::rcu_barrier, //
> +        rcu::{
> +            rcu_barrier,
> +            synchronize_rcu, //
> +        }, //
>      }, //
>  };
>  
> @@ -65,7 +68,7 @@ pub struct FenceContext<T: FenceContextOps + Send + Sync> {
>      /// The sequence number for the next fence created.
>      seqno: Atomic<u64>,
>      // The name parameters can be accessed by the dma_fence backend_ops. UAF
> -    // errors are prevented by the `call_rcu()` in `drop_driver_fence_data()`.
> +    // errors are prevented by the `synchronize_rcu()` in `drop_driver_fence_data()`.
>      /// The name of the driver this FenceContext's fences belong to.
>      driver_name: CString,
>      /// The name of the timeline this FenceContext's fences belong to.

[ ... ]

> @@ -600,8 +602,6 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
>      // necessary so that the C backend can free the allocation (coming from our
>      // Rust code) with kfree_rcu().
>      inner: Fence,
> -    /// Callback head for dropping this in a deferred manner through RCU.
> -    rcu_head: bindings::callback_head,
>      /// Reference to access the FenceContext.
>      fctx: &'a FenceContext<T>,
>      /// The API user's data. It is essential that the data only performs

[Severity: Low]
Since DriverFence::drop() now drops the payload synchronously after
synchronize_rcu() instead of inside a call_rcu() callback, should the doc
comments on DriverFenceData::data and FenceContext::new_fence_allocation() be
updated to remove the requirement that T::FenceDataType::drop() only perform
operations legal in atomic context?

> @@ -644,6 +644,10 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
>  ///
>  /// In the unlikely case of such violations, a panic is thrown.
>  ///
> +/// For requirements imposed by the C backend, a [`DriverFence`] might perform
> +/// a call to [`rcu::synchronize_rcu`] when it drops. Thus, avoid dropping a
> +/// [`DriverFence`] in atomic context.

[Severity: Low]
Because synchronize_rcu is imported directly from kernel::sync::rcu rather
than importing the rcu module, will the intra-doc link to rcu::synchronize_rcu
fail to resolve in rustdoc unless written as synchronize_rcu?

[ ... ]

> @@ -973,59 +977,18 @@ fn drop(&mut self) {
>              return;
>          }
>  
> +        // Make sure none of the fence backend_ops can access data anymore.
> +        //
> +        // TODO:
> +        // This would not be necessary if the C dma_fence backend were using a
> +        // spinlock to properly synchronize its signaled state. Fix it in C and
> +        // then remove synchronize_rcu().
> +        synchronize_rcu();

[Severity: High]
Can this cause a use-after-free on FenceContext (and its driver_name and
timeline_name fields) if a concurrent RCU reader is in dma_fence_driver_name()
or dma_fence_timeline_name() when the last fence is signaled and FenceContext
is dropped?

In DriverFence::drop(), if !core::mem::needs_drop::<T::FenceDataType>() is
true, the function returns early before calling synchronize_rcu():

    if !core::mem::needs_drop::<T::FenceDataType>() {
        unsafe { bindings::dma_fence_put(self.as_raw()) };
        return;
    }

Similarly, if a signaled DriverFence is forgotten or converted via
ForeignOwnable::into_foreign() after DriverFence::signal() has already
decremented nr_of_unsignaled_fences, DriverFence::drop() does not run before
FenceContext can be dropped.

In FenceContext::drop(), the code calls rcu_barrier() to wait for callbacks
from DriverFence::drop():

    // Wait for the RCU callbacks in `DriverFence::drop`.
    rcu_barrier();

However, DriverFence::drop() no longer queues any call_rcu() callbacks, and
rcu_barrier() only waits for pending call_rcu() callbacks rather than waiting
for an RCU grace period. Should FenceContext::drop() call synchronize_rcu()
instead of rcu_barrier() (and should the comment in FenceContext be updated to
no longer refer to the removed drop_driver_fence_data() function)?

>  
>          // SAFETY: Valid because `self` is valid.
> -        let rcu_head_ptr = unsafe { &raw mut (*self.data.as_ptr()).rcu_head };
> +        unsafe { drop_in_place(&raw mut self.data) };

[Severity: High]
Does this leak the T::FenceDataType payload when DriverFence is dropped?

On DriverFence, self.data has type NonNull<DriverFenceData<'a, T>>:

    pub struct DriverFence<'a, T: Send + Sync + FenceContextOps> {
        data: NonNull<DriverFenceData<'a, T>>,
    }

While the user payload is the data field inside DriverFenceData:

    struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
        inner: Fence,
        fctx: &'a FenceContext<T>,
        #[pin]
        data: T::FenceDataType,
    }

Passing &raw mut self.data to drop_in_place() drops the NonNull pointer itself
(which is a no-op) rather than (*self.data.as_ptr()).data, and the subsequent
dma_fence_put() lets C free the allocation via kfree_rcu() without ever
running the destructor of T::FenceDataType. Should this be:

    unsafe { drop_in_place(&raw mut (*self.data.as_ptr()).data) };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102150617.527772-2-phasta@kernel.org?part=2