[RFC PATCH v6 0/5] rust: Add drm::JobQueue

[RFC PATCH v6 0/5] rust: Add drm::JobQueue

[Philipp Stanner]

Changes in v6:
  - Rework DmaFence so you can signal fences without dropping them
    (since we'd have to drop in atomic context otherwise)
  - Use synchronize_rcu() for fence dropping for now – with a fast path
    optimization! – instead of call_rcu() because the latter can run in
    atomic context.
  - Add DropWorker for JobQueue which drops jobs and their fences in
    process context.
  - Rework locking design: All JQ API functions can be invoked without
    any driver locks. run_job() is not executed with the JQ lock held,
    thus the driver can sleep.
  - Add a seqno based approach to catch potential races with the
    unlocked call to run_job().
  - Redesign JobQueue::complete_jobs_up_to_seqno() so that by default
    jobs are completed with status == Ok(()). This is done because it
    makes the race-catcher above easier to implement (otherwise we'd
    have to carry the last error around, and if we ever implement batch
    submissions we'd need a list of errors).
  - Since our design assumption is that users will always close their
    ring and drop their JQ if anything goes wrong on the ring, just
    provide one function that signals with an error on teardown:
    JobQueue::teardown().
  - Move the work items out of the locked JobQueueInner, since the work
    queue lock already synchronizes everything. The
    Revocable<JobQueueInner> needs to stay, however, so that the
    SubmitWorker can be safely cancelled on JQ::drop().
  - Fix various logical bugs that I found (code is unfortunately still
    untested).
  - Extend documentation significantly.
  - Formatting.

Changes in v5:
  - Add back the Revocable<JobQueueInner…> from my earliest RFCs. Reason
    is still the same: JQ dropping while dependency fences signal could
    deadlock. Similarly, this solves a dependency kicking of the work
    item again while drop() is running.
  - Do not call run_job() with a lock held, since run_job() might sleep.
    AFAICS, this also makes the proposed XArray solution impossible and
    demands that we use a list.
  - Re-add the earlier credit-count-system, so that run_job() is
    infallible. Simplifies the design quite a bit. (Danilo)
  - Drop jobs in a deferred manner through the work item so that job
    payload data can do atomic-hostile stuff.
  - Because of the point above, I suggest that we stop dropping a
    DriverFence on signal (Danilo's and my idea). It has no real
    advantage, and getting rid of it allows for better controlling what
    drops when through JobQueue. For fence it's only relevant that there
    is an RCU grace period, thus:
  - Replace DriverFence::drop()'s call_rcu() with synchronize_rcu(). We
    would, therefore, demand that everyone who's got a problem with the
    delay drops via work item.

This can still not be tested as I'm blocked by pin-init vs self-ref.
Just FYI.

I would appreciate confirmation of these following thoughts on locking
design:

1.
I believe that callers of jq.new_job() and jq.submit_job() do not need
an outer serialization mutex. Reason is that JQ serializes with its
internal lock. Sequence numbers cannot take over older seqnos because it
is submit_job() who sets the seqno.

2.
I, furthermore, believe that a driver will not need to hold a
driver-lock while calling jq.complete_jobs_up_to_seqno().

IOW, I am suggesting that a driver can always safely use JQ without an
outer lock, and actually *should* do so to avoid potential locking
issues.

Reason:

let driver_stuff = foo.lock();
let current_gpu_seqno = driver_stuff.get_seqno();
drop(driver_stuff);
jq.complete_jobs_up_to_seqno(current_gpu_seqno);

Thus, I believe that it is OK for me to take the JQ lock in
jq.complete_jobs_up_to_seqno().

Please correct me if I'm missing something.

Should I be wrong and a lock inversion could occur, then we could
hypothetically add a fence Signaler object, with which we could signal
fences with a different lock. Similar to how drm_sched does it with the
hardware_fence.


Regards
P.


====

Cover letter from v4:

As the commit messages and code comments detail, progressing JobQueue is
currently somewhat blocked because of an issue with self-referential
PinInit, which Gary generously offered to investigate.

This code compiles, but the example does not because of the
aformentioned issue. Nevertheless, I wanted to provide another RFC here
so that we can move our discussions forward in the meantime, especially
since very much about JobQueue has changed.

Our, now upstreamed, DmaFence abstractions informed some of the notable
changes in JobQueue. Most notably, JobQueue now owns the FenceContext,
Jobs are created on the queue and own the DriverFence. Jobs, again, are
owned by the JobQueue. We hope to enforce correct behavior that way,
having FenceContext, JobQueue and firmware ring all correspond with each
other 1:1:1.

I suspect that a potential deadlock on JQ drop still exists. I
previously solved that with Revocable, which I tend to think will also
be the way to go here.

(One very great news btw is that we almost magically solved a number of
issues with the new DmaFence design – in combination with this JobQueue
design, for the first time it would be possible to fully support the
dma_fence backend_ops. The driver-unload-problem previously had most
users use intermediate fences, like the drm_sched_fence, which means
that callbacks, e.g. from userspace, could not be passed through to the
driver. Now, with FenceContextOps <-> JobQueueOps, we can theoretically
support all of them)

The differences between this draft and Daniel / Tyr's prototype which
probably are most noteworthy are the different lock design
("Philipp"-JobQueue has one big lock, wheres Tyr-JobQueue has 2, 3 if
you count the XArray lock) and the used data structure.

The presented solution uses lists over XArray because:
  1. XArray is semantically more complex and has an additional lock.
  2. An Xarray-as-ringbuffer needs own algorithms for index tracking,
     wrapp around etc. With list you enqueue into a waiting list, and
     for running you move into the running_list.
  3. The memory reservations we need for pre-allocating everything for
     our job-submission path are solved in one go with list, because a
     job simply contains a list head.
  4. Most notably, it is unclear how XArray behaves for ever-increasing
     indices with a sliding window, whereas the list semantic is well
     understood and deterministic.

I obviously don't claim to own all the wisdom in that regard; it's just
that I still propose this solution because these arguments make me
believe that it is the right one.

Since all the list handling is done with iterators, there is currently
no unsafe needed.


I hope we can discuss many things here before we, hopefully soon, can
address the lifetime issue and move to a v1.


This is based on drm-rust-next (10a6623a24a8), plus Danilo's ScopedWork
[1] and my patches [2][3] regarding 'static and lock errors.

Regards,
Philipp

[1] https://lore.kernel.org/rust-for-linux/20260807165252.3849875-1-dakr@kernel.org/
[2] https://lore.kernel.org/rust-for-linux/20260922083631.444614-2-phasta@kernel.org/
[3] https://lore.kernel.org/rust-for-linux/20260924085523.2620704-2-phasta@kernel.org/

Philipp Stanner (5):
  rust: DmaFence: remove static lifetime
  rust: DmaFence: Implement Deref for FenceContext
  rust: DmaFence: Don't drop on signal
  rust: DmaFence: Replace call_rcu() with synchronize_rcu()
  rust: drm: Add JobQueue

 rust/kernel/dma_buf/dma_fence.rs |  88 ++--
 rust/kernel/drm/job_queue.rs     | 700 +++++++++++++++++++++++++++++++
 rust/kernel/drm/mod.rs           |   4 +
 3 files changed, 731 insertions(+), 61 deletions(-)
 create mode 100644 rust/kernel/drm/job_queue.rs

-- 
2.55.0

[RFC PATCH v6 1/5] rust: DmaFence: remove static lifetime

[Philipp Stanner]

(will be upstreamed separately)

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

diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index 18a43e1bb442..eef2b9e93f35 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -291,7 +291,7 @@ fn from(e: AllocError) -> Self {
 ///     }
 /// }
 /// ```
-pub trait FenceCallback: Send + 'static {
+pub trait FenceCallback: Send {
     /// Called when the fence is signaled.
     ///
     /// This is called from the fence signaling path, which may be in interrupt
@@ -310,7 +310,7 @@ pub trait FenceCallback: Send + 'static {
 /// When this object is dropped, the callback is automatically removed if it
 /// hasn't been called yet.
 #[pin_data(PinnedDrop)]
-pub struct FenceCallbackRegistration<T: FenceCallback + 'static> {
+pub struct FenceCallbackRegistration<T: FenceCallback> {
     #[pin]
     callback_foreign: Opaque<bindings::dma_fence_cb>,
     callback: ManuallyDrop<T>,
@@ -326,7 +326,7 @@ impl<T: FenceCallback> FenceCallbackRegistration<T> {
     /// On success the callback is pinned in place and will fire when the fence
     /// signals. On `AlreadySignaled` the callback is returned to the caller so
     /// that owned resources can be reclaimed.
-    pub fn new<'a>(fence: &'a Fence, callback: T) -> impl PinInit<Self, CallbackError<T>> + 'a
+    pub unsafe fn new<'a>(fence: &'a Fence, callback: T) -> impl PinInit<Self, CallbackError<T>> + 'a
     where
         T: 'a,
     {
@@ -693,7 +693,8 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
 ///
 /// let cb_data = CallbackData { };
 /// let waiting_fence = ARef::from(fence.as_fence());
-/// let cb_reg = FenceCallbackRegistration::new(&waiting_fence, cb_data);
+/// // SAFETY: `cb_data`'s content is not forgotten.
+/// let cb_reg = unsafe { FenceCallbackRegistration::new(&waiting_fence, cb_data) };
 /// let cb_reg = KBox::pin_init(cb_reg, GFP_KERNEL)?;
 ///
 /// // TODO signalling guards
-- 
2.55.0

Re: [RFC PATCH v6 1/5] rust: DmaFence: remove static lifetime

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rust: missing `# Safety` documentation for `unsafe fn new` exposing UAF on leak
--

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

rust: DmaFence: remove static lifetime

(will be upstreamed separately)

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index 18a43e1bb442f..eef2b9e93f356 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs

[ ... ]

> @@ -326,7 +326,7 @@ impl<T: FenceCallback> FenceCallbackRegistration<T> {
>      /// On success the callback is pinned in place and will fire when the fence
>      /// signals. On `AlreadySignaled` the callback is returned to the caller so
>      /// that owned resources can be reclaimed.
> -    pub fn new<'a>(fence: &'a Fence, callback: T) -> impl PinInit<Self, CallbackError<T>> + 'a
> +    pub unsafe fn new<'a>(fence: &'a Fence, callback: T) -> impl PinInit<Self, CallbackError<T>> + 'a

[Severity: Medium]
Since this patch removes the 'static bound from FenceCallbackRegistration
and makes FenceCallbackRegistration::new() unsafe to prevent callbacks from
outliving their captured references, does this function also need a
/// # Safety documentation block?

According to the Rust coding guidelines, unsafe functions must document
their safety preconditions. Without it, callers aren't explicitly informed
of their obligation to prevent leaks (e.g. via core::mem::forget) which
could bypass the Drop implementation and leave a dangling callback pointer
that eventually causes a use-after-free when the fence signals.

>      where
>          T: 'a,
>      {

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

[RFC PATCH v6 2/5] rust: DmaFence: Implement Deref for FenceContext

[Philipp Stanner]

A FenceContext carries data, which the user should be able to access.

Implement Deref for FenceContext.

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

diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index eef2b9e93f35..0028124e6aa6 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -86,6 +86,14 @@ pub struct FenceContext<T: FenceContextOps + Send + Sync> {
     data: T,
 }
 
+impl<T: Send + Sync + FenceContextOps> Deref for FenceContext<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        &self.data
+    }
+}
+
 impl<'a, T: Send + Sync + FenceContextOps> FenceContext<T> {
     // This can later be extended as a vtable in case other parties need support
     // for the more "exotic" callbacks.
-- 
2.55.0

Re: [RFC PATCH v6 2/5] rust: DmaFence: Implement Deref for FenceContext

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] rust: missing #[inline] on Deref::deref in abstraction
--

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

rust: DmaFence: Implement Deref for FenceContext

This commit implements the Deref trait for FenceContext. This allows users
to easily access the data carried by the context.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index eef2b9e93f356..0028124e6aa64 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs
[ ... ]
> @@ -86,6 +86,14 @@ pub struct FenceContext<T: FenceContextOps + Send + Sync> {
>      data: T,
>  }
>  
> +impl<T: Send + Sync + FenceContextOps> Deref for FenceContext<T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {

[Severity: Low]
This isn't a bug, but should this method be marked with the #[inline]
attribute?

The Rust subsystem guidelines recommend that small functions within
abstractions (such as the kernel crate) be annotated with #[inline]. Since
this deref implementation is a trivial getter returning a reference to a
struct field, omitting the attribute might lead to unnecessary function call
overhead when used frequently.

> +        &self.data
> +    }
> +}

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

[RFC PATCH v6 3/5] 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index 0028124e6aa6..b67121c7abd4 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -806,7 +806,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
-- 
2.55.0

Re: [RFC PATCH v6 3/5] 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] dma_fence: counter underflow in DriverFence::signal leads to panic or UAF
--

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

rust: DmaFence: Don't drop on signal

This commit changes the DriverFence::signal method to take a shared reference
instead of consuming the object. This allows multiple or concurrent calls to
signal() on the same fence object, avoiding unnecessary drops in atomic
contexts.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index 0028124e6aa64..b67121c7abd41 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs
> @@ -806,23 +806,23 @@ 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
[ ... ]
>              // 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 signal() now takes a shared reference and can be called multiple times,
will this unconditional decrement of nr_of_unsignaled_fences cause an
underflow?

When called repeatedly for the same fence, this decrement executes outside
the if !signaled block and will occur on every call.

This could cause the counter to artificially reach 0 and bypass the panic
check in FenceContext::drop(), potentially allowing the context to be freed
while unsignaled fences still exist.

Could this result in a use-after-free when those fences are later queried by
the C backend?

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

Re: [RFC PATCH v6 3/5] rust: DmaFence: Don't drop on signal

[Alice Ryhl]

On Fri, Oct 16, 2026 at 10:57:22AM +0200, Philipp Stanner wrote:
> 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>

This seems reasonable to me, though sashiko makes a good point about
being more careful when updating the fence counter.

Alice

[RFC PATCH v6 4/5] 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 | 69 ++++++--------------------------
 1 file changed, 13 insertions(+), 56 deletions(-)

diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index b67121c7abd4..9efb84c00b6a 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -42,7 +42,8 @@
             Atomic,
             Relaxed, //
         },
-        rcu::rcu_barrier, //
+        rcu::rcu_barrier,
+        rcu::synchronize_rcu, //
     }, //
 };
 
@@ -150,7 +151,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(),
@@ -601,8 +601,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
@@ -973,59 +971,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();
+
         // 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) };
 
-        // 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: 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.55.0

Re: [RFC PATCH v6 4/5] 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] mm: memory leak of T::FenceDataType in DriverFence::drop due to dropping NonNull wrapper
- [High] locking: synchronize_rcu() in DriverFence::drop causes sleep-in-atomic
- [High] dma_fence: UAF of FenceContext strings via outliving Fence consumers
- [Low] doc: outdated safety comments referencing removed call_rcu()
--

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

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

This commit replaces the use of call_rcu() with synchronize_rcu() for dropping
DriverFence data. It removes the raw C callback and drops the data in-place
using drop_in_place().

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index b67121c7abd41..9efb84c00b6a2 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs

[ ... ]

> @@ -973,59 +971,18 @@ fn drop(&mut self) {
>              return;
>          }

[Severity: Low]
Does this patch leave outdated safety comments in DriverFence::drop()?

The deferred call_rcu() has been removed from DriverFence::drop() and replaced
with synchronize_rcu(), but multiple SAFETY comments still reference it:

    // SAFETY: `guard` is valid until the `call_rcu()` below.
    let signaled: bool = unsafe { bindings::dma_fence_test_signaled_flag(guard.as_raw()) };

Could these comments be updated to prevent confusion during future maintenance?

>  
> +        // 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]
DriverFence implements Send + Sync, and Drop is called automatically when
variables go out of scope. Can this synchronize_rcu() be called from atomic
contexts like spinlock critical sections, interrupt handlers, or RCU read-side
critical sections?

Because synchronize_rcu() blocks and waits for a grace period, wouldn't dropping
a DriverFence in these contexts cause a might_sleep() lockdep warning or a
system deadlock on PREEMPT_RT?

[Severity: High]
This problem wasn't introduced by this patch, but does FenceContext have a
use-after-free issue with its strings?

The safety comment in get_driver_name() assumes it is only called on unsignaled
fences:

rust/kernel/dma_buf/dma_fence.rs:get_driver_name() {
    // SAFETY: The C backend only invokes this callback with `ptr` pointing
    // to a valid, unsignaled `bindings::dma_fence`.
    // ...
    let fctx = unsafe { Self::from_raw_fence(ptr) };

    fctx.driver_name.as_char_ptr()
}

However, sync_fill_fence_info() accesses the driver name when userspace queries
the sync_file info, and it calls get_driver_name() regardless of the signaled
state.

A consumer holding a sync_file fd can retain a reference to the Fence long after
the driver signals it and drops the FenceContext.

Furthermore, by replacing call_rcu() with synchronize_rcu() in
DriverFence::drop(), doesn't this completely defeat the rcu_barrier() in
FenceContext::drop() which was meant to wait for these tear-downs?

rust/kernel/dma_buf/dma_fence.rs:FenceContext::drop() {
    // Wait for the RCU callbacks in `DriverFence::drop`.
    rcu_barrier();
}

>  
>          // 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 actually drop the underlying data, or does it just drop the NonNull
pointer itself?

Since self.data is of type NonNull<DriverFenceData<'a, T>>, &raw mut self.data
produces a pointer to the NonNull wrapper (*mut NonNull<...>).

Because NonNull is Copy and has no Drop implementation, calling drop_in_place()
on this pointer is a no-op.

Will this permanently leak the actual user payload T::FenceDataType located at
(*self.data.as_ptr()).data?

> -        // 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: The `synchronize_rcu()` above ensures all accessors are gone.
> +        unsafe { bindings::dma_fence_put(self.as_raw()) };
>      }
>  }

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

[RFC PATCH v6 5/5] rust: drm: Add JobQueue

[Philipp Stanner]

JobQueue is a pure Rust component that handles job submissions for GPUs
with firmware scheduling, i.e., GPUs that support one exclusive ring per
execution context.

JobQueue's primary task is dependency handling, where the user can set
dependencies, each in the form of a `dma_buf::Fence`, on a job and then
submit it to the queue.

Jobs are being executed in order, following the FIFO principle. Only
jobs whose dependencies are all fullfilled get executed, otherwise the
JobQueue pauses.

A core design principle, just like with DmaFence, is to avoid all
allocations in job submission paths, since this runs danger of
deadlocking.

Another design principle, new when compared to the older drafts, is the
ownership relationship with fences:

  1. The JobQueue owns the FenceContext exclusively.
  2. Jobs are created on the JobQueue, ensuring correct fence sequence
     number ordering.
  3. Jobs own the respective DriverFence.
  4. Jobs are owned by the JobQueue.

Consequently, a driver can only interact with fences and FenceContext
indirectly through the JobQueue, and can also only complete jobs (and,
hence, signal fences) through the JobQueue.

Unfortunately, since jobs need a reference to the JobQueue for their
fence dependency objects (DependencyWaker), this code can currently not
be used in this form because some work on self-referential PinInit is
required. See the respective comments in the Example.

When a job shall be run, both its JobQueue::data and Job::data get
borrowed back to it, together with the sequence number of this job, so
that the driver can perform the appropriate interaction with the
hardware.

The submit_worker runs all ready jobs based on a credit count system, so
that run_job() can be infallible. The same worker drops jobs (and their
fences) that are already completed. Should this proof to be a
performance issue, a second worker could trivially be added for this.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 rust/kernel/drm/job_queue.rs | 700 +++++++++++++++++++++++++++++++++++
 rust/kernel/drm/mod.rs       |   4 +
 2 files changed, 704 insertions(+)
 create mode 100644 rust/kernel/drm/job_queue.rs

diff --git a/rust/kernel/drm/job_queue.rs b/rust/kernel/drm/job_queue.rs
new file mode 100644
index 000000000000..66f98ee3df70
--- /dev/null
+++ b/rust/kernel/drm/job_queue.rs
@@ -0,0 +1,700 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2026 Red Hat Inc.
+ * Author: Philipp Stanner <pstanner@redhat.com>
+ */
+
+//! DRM's job dependency manager and load balancer for GPUs with firmware
+//! scheduling, i.e., GPUs that can spawn one ring exclusive per user context.
+//!
+//! JobQueue abstracts the entire handling of [`dma_buf::FenceContext`] and its
+//! associated [`dma_buf::DriverFence`]s for the user.
+//!
+//! In this design, a JobQueue is always the owner of a [`dma_buf::FenceContext`]
+//! which represents the timeline on the associated GPU ring. Jobs created on
+//! this JobQueue always contain a [`dma_buf::DriverFence`] from this JobQueue,
+//! where the fence sequence number corresponds with the job sequence number,
+//! and the fence's generic payload data is also the job's payload data. This
+//! allows for the dma_fence backend_ops callbacks interacting with the driver
+//! through JobQueue. The trait [`JobQueueOps`] is the interface between driver
+//! and fence backend_ops.
+//!
+//! [`Job`]s created on the [`JobQueue`] take over ownership of the user's data
+//! and prepare all memory allocations. A later job submission sets up the fence
+//! and performs no fallible operations.
+//!
+//! Before jobs are submitted to a queue, dependencies in the form of
+//! [`dma_buf::Fence`] can be set on them. Jobs are run by the JobQueue in
+//! process context through a work item in FIFO order. Only jobs whose
+//! dependency fences have all been signaled are run. If the front job's
+//! dependency is not yet fullfilled, the JobQueue pauses until that's the case.
+//!
+//! JobQueue implements a credit count system, where the queue itself gets
+//! assigned a capacity by the user, and jobs get assigned individual cost. Jobs
+//! get only pushed if there is enough capacity; if not, running stops until
+//! that is the case. These credits must accurately represent the capacity on
+//! the GPU ring the queue was created for.
+//!
+//! The driver tells the JobQueue which jobs are finished through
+//! [`JobQueue::complete_jobs_up_to_seqno()`]. This function is typically
+//! invoked through the driver's GPU interrupt handler.
+//!
+//! Since jobs might contain data that must not drop in atomic context, and
+//! since jobs' [`dma_buf::DriverFence`]s perform a call to
+//! [`rcu::synchronize_rcu()`], they are dropped through a work item if [`Drop`]
+//! is implemented for the job's payload data.
+//!
+//! From JobQueue's perspective, it is not necessary to call any of its API
+//! functions with a driver lock held; this is actually discouraged to reduce
+//! the probability of deadlocks. All JobQueue API functions are properly
+//! ordered using the internal lock.
+//!
+//! JobQueue tells drivers to run jobs through [`JobQueueOps::run_job`], which
+//! is called without the JobQueue's spinlock being held, i.e., the driver is
+//! permitted to perform sleeping operations in the callback. JobQueue handles
+//! races between `run_job` and [`JobQueueOps::complete_jobs_up_to_seqno()`].
+//!
+//! This is pure Rust code that does not directly abstract C.
+
+use crate::prelude::*;
+
+use core::{
+    convert::Infallible,
+    ops::Deref, //
+};
+
+use kernel::{
+    dma_buf::dma_fence::{
+        DriverFence,
+        DriverFenceAllocation,
+        Fence,
+        FenceCallback,
+        FenceCallbackRegistration,
+        FenceContext,
+        FenceContextOps, //
+    },
+    list::*,
+    revocable::Revocable,
+    str::CString,
+    sync::{
+        aref::ARef,
+        atomic::{
+            Atomic,
+            Relaxed, //
+        }, //
+        new_spinlock_irq,
+        SpinLockIrq,
+    }, //
+    workqueue::{
+        self,
+        new_scoped_work,
+        //        ScopedQueue,
+        ScopedWork,
+        ScopedWorkItem,
+        ScopedWorkRef,
+    }, //
+};
+
+struct DependencyWaker<'b, T: JobQueueOps + Send + Sync> {
+    nr_of_deps: &'b Atomic<u32>,
+    jq: &'b JobQueue<'b, T>,
+}
+
+impl<'b, T: JobQueueOps + Send + Sync> FenceCallback for DependencyWaker<'b, T> {
+    fn on_signal(&mut self) {
+        // Last dependency kicks the worker.
+        if self.nr_of_deps.fetch_sub(1, Relaxed) == 1 {
+            // The drop-order of JobQueue ensures that all DependencyWakers are
+            // cancelled before the JobQueue itself is dropped.
+            self.jq.check_start_submit_worker();
+        }
+    }
+}
+
+/// A JobQueue job before it has been submitted.
+///
+/// This is only useful for registering dependencies on and for submitting it.
+pub struct Job<'a, T: JobQueueOps + Send + Sync> {
+    inner: ListArc<JobInternal<'a, T>>,
+}
+
+impl<'a, T: JobQueueOps + Send + Sync> Job<'a, T> {
+    /// Add a dependency for the job.
+    ///
+    /// A JobQueue will only push a job once `fence` has been signaled.
+    pub fn add_dependency(&'a mut self, fence: &Fence) -> Result {
+        let nr_of_deps = &raw const self.inner.nr_of_deps;
+
+        let waker = DependencyWaker {
+            // SAFETY: `nr_of_deps` lives as long as the job.
+            // TODO RFC: Is there a better way to do it? The mutable-immutable references
+            // otherwise run into conflict when `nr_of_deps` is used below.
+            nr_of_deps: unsafe { &*nr_of_deps },
+            jq: self.inner.jq,
+        };
+
+        let job = &mut self.inner;
+
+        job.dependencies().reserve(1, GFP_KERNEL)?;
+
+        // If the registration below gets registered successfully, a signaling
+        // fence will decrement the atomic. That could race. Thus, increment in
+        // advance. Ordering is ensured thanks to the dma_fence backend.
+        job.nr_of_deps.fetch_add(1, Relaxed);
+
+        // SAFETY: The `registration` is not forgotten. It drops with JobQueue.
+        let registration = unsafe { FenceCallbackRegistration::new(fence, waker) };
+        match KBox::pin_init(registration, GFP_KERNEL) {
+            Err(e) => {
+                let _ = job.nr_of_deps.fetch_sub(1, Relaxed);
+                if e == ENOENT {
+                    // Was already signaled. No reason to bother the caller.
+                    return Ok(());
+                }
+                return Err(e);
+            }
+            Ok(dep) => {
+                if let Err(e) = job.dependencies().push_within_capacity(dep) {
+                    // Impossible because of the reservation.
+                    pr_err!("Job::add_dependency(): reserved memory not available.\n");
+                    return Err(e.into());
+                }
+            }
+        }
+
+        Ok(())
+    }
+}
+
+#[pin_data]
+#[repr(C)]
+struct JobInternal<'b, T: JobQueueOps + Send + Sync> {
+    #[pin]
+    links: ListLinks,
+    cost: u32,
+    fence_allocation: ListArcField<Option<DriverFenceAllocation<'b, DriverDataWrapper<T>>>, 0>,
+    fence: ListArcField<Option<DriverFence<'b, DriverDataWrapper<T>>>>,
+    dependencies: ListArcField<KVec<Pin<KBox<FenceCallbackRegistration<DependencyWaker<'b, T>>>>>>,
+    nr_of_deps: Atomic<u32>,
+    jq: &'b JobQueue<'b, T>,
+}
+
+impl_list_arc_safe! {
+    impl{'b, T: JobQueueOps + Send + Sync} ListArcSafe<0> for JobInternal<'b, T> { untracked; }
+}
+impl_list_item! {
+    impl{'b, T: JobQueueOps + Send + Sync} ListItem<0> for JobInternal<'b, T> { using ListLinks { self.links }; }
+}
+
+impl<'b, T: JobQueueOps + Send + Sync> JobInternal<'b, T> {
+    //    const LIST_JOB_ID: u64 = 0xe7e445f2bb9e97fc;
+
+    kernel::list::define_list_arc_field_getter! {
+        pub(crate) fn fence_allocation(&mut self<0>) -> &mut Option<DriverFenceAllocation<'b, DriverDataWrapper<T>>> { fence_allocation }
+        pub(crate) fn fence(&mut self<0>) -> &mut Option<DriverFence<'b, DriverDataWrapper<T>>> { fence }
+        pub(crate) fn dependencies(&mut self<0>) -> &mut KVec<Pin<KBox<FenceCallbackRegistration<DependencyWaker<'b, T>>>>> { dependencies }
+    }
+}
+
+#[pin_data]
+struct SubmitWorker<'a, T: JobQueueOps + Send + Sync> {
+    #[pin]
+    jq: &'a JobQueue<'a, T>,
+}
+
+impl<T: JobQueueOps + Send + Sync> ScopedWorkItem for SubmitWorker<'_, T> {
+    fn run(work: &ScopedWorkRef<Self>) {
+        // Prevent work-item UAF if the JQ is trying to drop right now.
+        let Some(jq_accessor) = work.jq.inner.try_access() else {
+            return;
+        };
+
+        let mut jq = jq_accessor.lock();
+
+        while let Some(mut job) = jq.as_mut().project().waiting_jobs.pop_front() {
+            // While we didn't hold the lock below for `run_job()`, it could be
+            // that a new job entered the waiting list which has an unfulfilled
+            // dependency.
+            if job.nr_of_deps.load(Relaxed) > 0 {
+                jq.waiting_jobs.push_front(job);
+                break;
+            }
+            let new_capacity = jq.capacity as i64 - i64::from(job.cost);
+            if new_capacity < 0 {
+                jq.waiting_jobs.push_front(job);
+                break;
+            }
+            *jq.as_mut().project().capacity = new_capacity as u64;
+
+            let seqno = job
+                .fence()
+                .as_ref()
+                .expect("fence not present")
+                .as_fence()
+                .seqno();
+            let runnable_job = JobRunnable {
+                seqno,
+                data: job.fence().as_ref().expect("fence not present").deref(),
+            };
+
+            let jq_data = work.jq.fctx.deref().deref();
+
+            drop(jq); // Release lock.
+            T::run_job(jq_data, runnable_job);
+            jq = jq_accessor.lock(); // Retake lock.
+
+            // run_job() above needs to be called without lock protection. Thus,
+            // it is possible that the driver instructed completion right before
+            // we could re-take the lock.
+            if jq.last_completed >= seqno {
+                if let Some(fence) = job.fence() {
+                    fence.signal(Ok(()));
+                } else {
+                    panic!("fence not present.");
+                }
+
+                // We drop this job here directly, from work_item context.
+            } else {
+                jq.running_jobs.push_back(job);
+            }
+        }
+    }
+}
+
+#[pin_data]
+struct DropWorker<'a, T: JobQueueOps + Send + Sync> {
+    #[pin]
+    jq: &'a JobQueue<'a, T>,
+}
+
+impl<T: JobQueueOps + Send + Sync> ScopedWorkItem for DropWorker<'_, T> {
+    fn run(work: &ScopedWorkRef<Self>) {
+        // Contrary to the SubmitWorker, this worker would probably never need
+        // this revocable guard, since the worker is cancelled synchronously
+        // once the JQ drops. It cannot be restarted when JQ is dropping.
+        // But the inner JQ needs to be accessed through Revocable anyways, so
+        // we get more robustness for free.
+        let Some(jq_accessor) = work.jq.inner.try_access() else {
+            return;
+        };
+
+        let mut jq = jq_accessor.lock();
+
+        while let Some(_job) = jq.drop_list.pop_front() {
+            // Droppy-dy-drop
+        }
+    }
+}
+
+#[pin_data]
+struct JobQueueInner<'a, T: JobQueueOps + Send + Sync> {
+    /// Fence sequence number of the last job the user has completed.
+    #[pin]
+    /// List of jobs waiting to be run on the hardware.
+    waiting_jobs: List<JobInternal<'a, T>>,
+    #[pin]
+    /// Jobs that are currently executing on the hardware.
+    running_jobs: List<JobInternal<'a, T>>,
+    /// The queue's capacity.
+    capacity: u64,
+    /// The sequence number of the last completed job.
+    last_completed: u64,
+    #[pin]
+    /// Jobs that have finished executing and whose payload data `T` implements
+    /// `Drop`. These jobs are dropped in process context, which also allows for
+    /// their [`dma_buf::DriverFence`]s to implement the necessary RCU grace
+    /// period delay.
+    ///
+    /// If `Drop` is not implemented, this list is unused and the elements drop
+    /// directly in [`JobQueue::complete_jobs_up_to_seqno`].
+    ///
+    /// Should the JobQueue drop and the [`DropWorker`] not yet have dropped all
+    /// jobs, that is no problem because the JobQueue itself drops in process
+    /// context.
+    drop_list: List<JobInternal<'a, T>>,
+}
+
+/// A JobQueue.
+///
+/// When you drop your JobQueue, it might still attempt for one RCU grace period
+/// to submit more jobs via the [`JobQueueOps::run_job`] callback into your driver.
+///
+/// Dropping a JobQueue results in all remaining waiting and running jobs'
+/// fences being signaled with ECANCELED.
+///
+/// A JobQueue must only ever drop in process context.
+///
+/// # Examples
+///
+/// ```
+/// use core::ops::Deref;
+///
+/// use kernel::{
+///     str::CString,
+///     dma_buf::*,
+///     drm::{
+///         JobQueue,
+///         JobQueueOps,
+///         JobRunnable,
+///     },
+/// };
+///
+/// let driver_name = CString::try_from_fmt(fmt!("dummy_driver"))?;
+/// let timeline_name = CString::try_from_fmt(fmt!("dummy_timeline"))?;
+///
+/// #[pin_data]
+/// struct FenceContextData {}
+///
+/// impl FenceContextData {
+///     fn new() -> impl PinInit<Self> {
+///         pin_init!(Self {})
+///     }
+/// }
+///
+/// impl FenceContextOps for FenceContextData {
+///     type FenceDataType = ();
+/// }
+///
+/// let fctx_data = FenceContextData::new();
+///
+/// let fctx = KBox::pin_init(
+///     FenceContext::new(0, c"dependency_driver", c"dependency_timeline", fctx_data),
+///     GFP_KERNEL
+/// )?;
+///
+/// let dependency = fctx.new_fence_allocation(())?.new_fence();
+///
+/// #[pin_data]
+/// struct JqData {}
+///
+/// impl JqData {
+///     fn new() -> impl PinInit<Self> {
+///         pin_init!(Self {})
+///     }
+/// }
+///
+/// struct JobData {
+///     data: CString,
+/// }
+///
+/// impl JobQueueOps for JqData {
+///     type JobDataType = JobData;
+///
+///     fn run_job<'a>(&self, job: JobRunnable<'a, JobData>) {
+///
+///     }
+/// }
+///
+/// let jq_data = JqData::new();
+/// let jq = KBox::pin_init(JobQueue::new(0, driver_name, timeline_name, 1000, jq_data), GFP_KERNEL)?;
+///
+/// struct Callback {}
+///
+/// impl FenceCallback for Callback {
+///     fn on_signal(&mut self) {
+///
+///     }
+/// }
+///
+/// let data = CString::try_from_fmt(fmt!("hello there!"))?;
+/// let data = JobData { data };
+///
+/// // TODO RFC:
+/// // This is the problem where self-references come into play.
+/// // Jobs need a reference to the JobQueue due to their DependencyWaker
+/// // callbacks. At the same time, the JobQueue owns the jobs.
+/// // So the following outcommented code would not compile.
+/// // Gary is working on a solution for that.
+///
+/// //let job1 = jq.new_job(data)?;
+/// // job1.add_dependency(dependency.as_fence())?;
+/// //let job2 = jq.new_job(data)?;
+/// //jq.submit(job1);
+/// //jq.submit(job2);
+///
+/// //drop(job);
+/// //drop(jq);
+///
+/// Ok::<(), Error>(())
+/// ```
+#[pin_data(PinnedDrop)]
+pub struct JobQueue<'a, T: JobQueueOps + Send + Sync> {
+    #[pin]
+    drop_worker: ScopedWork<DropWorker<'a, T>>,
+    #[pin]
+    // Revocable to prevent work-item UAF on `inner`.
+    inner: Revocable<SpinLockIrq<JobQueueInner<'a, T>>>,
+    #[pin]
+    fctx: FenceContext<DriverDataWrapper<T>>,
+    #[pin]
+    submit_worker: ScopedWork<SubmitWorker<'a, T>>,
+}
+
+// SAFETY: The JobQueue is locked and pinned.
+unsafe impl<T: JobQueueOps + Send + Sync> Sync for JobQueue<'_, T> {}
+// SAFETY: The JobQueue is locked and pinned.
+unsafe impl<T: JobQueueOps + Send + Sync> Send for JobQueue<'_, T> {}
+
+/// Object passed to the driver to run a job.
+pub struct JobRunnable<'a, T> {
+    /// The job's sequence number.
+    pub seqno: u64,
+    /// The associated job data.
+    pub data: &'a T,
+}
+
+/// Ops for the JobQueue.
+pub trait JobQueueOps {
+    /// The type of the data passed over in [`JobQueue::new_job`].
+    type JobDataType: Send + Sync;
+
+    /// Instructs the driver to run this job.
+    ///
+    /// JobQueue calls this callback from process context without any locks
+    /// held, thus, the driver is permitted to sleep in its implementation.
+    ///
+    /// The driver must be aware that JobQueue trusts on its credit count system
+    /// for this operation to be infallible.
+    ///
+    /// This callback might be called while the driver is in the process of
+    /// dropping the JobQueue, but will never be called afterwards.
+    fn run_job<'a>(&self, job: JobRunnable<'a, Self::JobDataType>);
+}
+
+#[pin_data]
+struct DriverDataWrapper<T: JobQueueOps + Send + Sync> {
+    #[pin]
+    data: T,
+}
+
+impl<T: JobQueueOps + Send + Sync> Deref for DriverDataWrapper<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        &self.data
+    }
+}
+
+impl<T: JobQueueOps + Send + Sync> DriverDataWrapper<T> {
+    fn new<E>(data: impl PinInit<T, E>) -> impl PinInit<Self, E>
+    where
+        Error: From<E>,
+        E: From<Infallible>,
+    {
+        try_pin_init!(Self {
+            data <- data,
+        }? E)
+    }
+}
+
+impl<T: JobQueueOps + Send + Sync> FenceContextOps for DriverDataWrapper<T> {
+    type FenceDataType = T::JobDataType;
+}
+
+impl<'a, T: JobQueueOps + Send + Sync> JobQueue<'a, T> {
+    /// Create a new JobQueue.
+    pub fn new<E>(
+        initial_seqno: u64,
+        // TODO: pass these as &CStr. Figure out how to get the lifetimes for PinInit right.
+        driver_name: CString,
+        timeline_name: CString,
+        capacity: u64,
+        data: impl PinInit<T, E>,
+    ) -> impl PinInit<Self, Error>
+    where
+        Error: From<E>,
+        E: From<Infallible>,
+    {
+        try_pin_init!(&this in Self {
+            inner <- Revocable::new(new_spinlock_irq!(try_pin_init!(JobQueueInner {
+                capacity,
+                last_completed: 0,
+                waiting_jobs: List::new(),
+                running_jobs: List::new(),
+                drop_list: List::new(),
+                // SAFETY: `this` is valid. The lifetimes ensure that the reference remains valid.
+            }))),
+            fctx <- FenceContext::new(initial_seqno, &driver_name, &timeline_name, DriverDataWrapper::new(data)),
+            // SAFETY: `this` is valid. The lifetimes ensure that the reference remains valid.
+            submit_worker <- new_scoped_work!("JobQueueSubmitWorker", Ok(SubmitWorker { jq: unsafe { &*this.as_ptr() } })),
+            // SAFETY: `this` is valid. The lifetimes ensure that the reference remains valid.
+            drop_worker <- new_scoped_work!("JobQueueDropWorker", Ok(DropWorker { jq: unsafe { &*this.as_ptr() } })),
+        })
+    }
+
+    /// Create a new job.
+    ///
+    /// The `cost` specified here is what JobQueue subtracts from its capacity
+    /// to determine whether a job is runnable, i.e., has space on the GPU ring.
+    ///
+    /// The `data` contained by a job will be borrowed back to the driver once
+    /// the jobs shall be run through [`JobQueueOps::run_job`].
+    pub fn new_job(&'a self, cost: u32, data: T::JobDataType) -> Result<Job<'a, T>> {
+        let fence_allocation = self.fctx.new_fence_allocation(data)?;
+        let job = try_pin_init!(JobInternal {
+            links <- ListLinks::new(),
+            cost,
+            fence_allocation: ListArcField::new(Some(fence_allocation)),
+            fence: ListArcField::new(None),
+            nr_of_deps: Atomic::new(0),
+            dependencies <- ListArcField::new(KVec::new()),
+            jq: &self,
+        });
+
+        let inner = ListArc::pin_init(job, GFP_KERNEL)?;
+
+        Ok(Job { inner })
+    }
+
+    // TODO RFC the lifetime ensures that only jobs can be submitted that were
+    // actually created on this JQ, right?
+    /// Submit a job that has previously been created with [`JobQueue::new_job()`].
+    ///
+    /// Returns a [`dma_buf::Fence`] on which the user can register callbacks.
+    /// The fence will be signaled once the job is completed through
+    /// [`JobQueue::complete_jobs_up_to_seqno`], which can happen immediately
+    /// after the job has been submitted.
+    ///
+    /// This function does all fence initialization under the JobQueue lock in
+    /// one go, including setting of the fence sequence number. Thus, it is
+    /// typically unnecessary (and advaised against) to call this function with
+    /// a driver lock held.
+    pub fn submit(&self, job: Job<'a, T>) -> ARef<Fence> {
+        // unwrap() can't fire because `self` is valid.
+        let jq = self.inner.try_access().unwrap();
+        // The lock orders setting of the sequence number, so no driver lock is needed.
+        let mut jq = jq.lock();
+        let mut job = job.inner;
+        let fence = job
+            .fence_allocation()
+            .take()
+            .expect("fence_allocation not present")
+            .new_fence();
+
+        let ret_fence = ARef::from(fence.as_fence());
+
+        *job.fence() = Some(fence);
+
+        jq.waiting_jobs.push_back(job);
+        drop(jq);
+        self.check_start_submit_worker();
+
+        ret_fence
+    }
+
+    fn check_start_submit_worker(&self) {
+        // TODO this shouldn't be the system wq
+        // SAFETY: The worker is carried by the JobQueue and, thus, cannot be
+        // forgotten.
+        let _ = unsafe { workqueue::system_dfl().enqueue_scoped(&self.submit_worker) };
+    }
+
+    fn check_start_drop_worker(&self) {
+        // TODO this shouldn't be the system wq
+        // SAFETY: The worker is carried by the JobQueue and, thus, cannot be
+        // forgotten.
+        let _ = unsafe { workqueue::system_dfl().enqueue_scoped(&self.drop_worker) };
+    }
+
+    /// Complete all the jobs in the queue's running_list, up to `seqno`.
+    ///
+    /// This signals the jobs' fences and schedules dropping of both the [`Job`]
+    /// and its [`DriverFence`].
+    ///
+    /// This signals the associated fences with the `Ok(())` status.
+    ///
+    /// This function tells the implementation that there is now more capacity
+    /// for more jobs to be pushed into the GPU ring.
+    // TODO RFC: should seqno be inclusive? [0,seqno] <-> [0,seqno[ ?
+    pub fn complete_jobs_up_to_seqno(&self, seqno: u64) {
+        let needs_drop = core::mem::needs_drop::<T::JobDataType>();
+        // unwrap() can't fire because `self` is valid.
+        let jq = self.inner.try_access().unwrap();
+        let mut jq_locked = jq.lock();
+        let mut jq = jq_locked.as_mut().project();
+
+        if seqno < *jq.last_completed {
+            return;
+        }
+
+        while let Some(mut job) = jq.running_jobs.pop_front() {
+            if job
+                .fence()
+                .as_mut()
+                .expect("fence not present")
+                .as_fence()
+                .seqno()
+                > seqno
+            {
+                jq.running_jobs.push_front(job);
+                break;
+            }
+
+            *jq.capacity += u64::from(job.cost);
+
+            // This enables the SubmitWorker to complete a job that is not yet
+            // in 'running_jobs', in case of a race against the lock.
+            *jq.last_completed = seqno;
+
+            if let Some(fence) = job.fence() {
+                fence.signal(Ok(()));
+            } else {
+                panic!("fence not present.");
+            }
+
+            if needs_drop {
+                jq.drop_list.push_back(job);
+            }
+            // Otherwise, job is dropped here directly. Its fence won't perform
+            // an RCU grace period wait.
+        }
+
+        drop(jq_locked);
+
+        self.check_start_submit_worker();
+        if needs_drop {
+            self.check_start_drop_worker();
+        }
+    }
+
+    /// Tear the JobQueue down. Complete all pending jobs and signal their
+    /// fences with `error`.
+    ///
+    /// Only use this if you want to tear the JobQueue down in an error path and
+    /// want to specify the error.
+    ///
+    /// You can also just drop the JobQueue, which would result in all remaining
+    /// jobs' fences being signaled with ECANCELED.
+    ///
+    /// Note that this function has a small race window against the
+    /// `SubmitWorker` and its [`JobQueueOps::run_job`] callback, where the
+    /// latter might theoretically succeed at pushing more jobs to the driver.
+    /// The driver likely will want to ignore such attempts to push more jobs to
+    /// the GPU ring. Besides, the race merely results in these subsequent
+    /// jobs being signaled with ECANCELED on drop, thus, maybe differing from
+    /// the `error` provided here.
+    pub fn teardown<E>(self, error: Error)
+    where
+        Error: From<E>,
+    {
+        let jq = self.inner.try_access().unwrap();
+        let mut jq = jq.lock();
+
+        while let Some(mut job) = jq.running_jobs.pop_front() {
+            if let Some(fence) = job.fence() {
+                fence.signal(Err(error));
+            } else {
+                panic!("fence not present.");
+            }
+        }
+    }
+}
+
+#[pinned_drop]
+impl<T: JobQueueOps + Send + Sync> PinnedDrop for JobQueue<'_, T> {
+    fn drop(self: Pin<&mut Self>) {
+        // Revoke to prevent still existing DependencyWakers from kicking the
+        // SubmitWorker again before we could deregister them.
+        let _ = self.inner.revoke();
+    }
+}
diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
index fd6ed35bc35a..f2b7fbbce5bb 100644
--- a/rust/kernel/drm/mod.rs
+++ b/rust/kernel/drm/mod.rs
@@ -8,6 +8,7 @@
 pub mod gem;
 pub mod gpuvm;
 pub mod ioctl;
+pub mod job_queue;
 
 pub use self::device::Device;
 pub use self::device::DeviceContext;
@@ -20,6 +21,9 @@
 pub use self::driver::DriverInfo;
 pub use self::driver::Registration;
 pub use self::file::File;
+pub use self::job_queue::JobQueue;
+pub use self::job_queue::JobQueueOps;
+pub use self::job_queue::JobRunnable;
 
 pub(crate) mod private {
     pub trait Sealed {}
-- 
2.55.0

Re: [RFC PATCH v6 5/5] rust: drm: Add JobQueue

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] drm: race in JobQueue completion misses updates and leaks capacity
- [High] drm: sleeping in atomic context due to dropping jobs under SpinLockIrq
- [High] drm: JobQueue fails to signal waiting jobs on drop and teardown causing hangs
- [High] drm: UAF in JobQueue due to incorrect field drop order
- [High] drm: job_queue: oversized job permanently deadlocks FIFO queue
- [Medium] drm: JobQueue::teardown takes self by value making it uncallable for PinnedDrop type
- [Medium] drm: Job::add_dependency causes a permanent mutable borrow making Job unusable
--

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

rust: drm: Add JobQueue

JobQueue is a pure Rust component that handles job submissions for GPUs
with firmware scheduling, i.e., GPUs that support one exclusive ring per
execution context.

> diff --git a/rust/kernel/drm/job_queue.rs b/rust/kernel/drm/job_queue.rs
> new file mode 100644
> index 0000000000000..66f98ee3df70b
> --- /dev/null
> +++ b/rust/kernel/drm/job_queue.rs
[ ... ]
> +impl<'a, T: JobQueueOps + Send + Sync> Job<'a, T> {
> +    /// Add a dependency for the job.
> +    ///
> +    /// A JobQueue will only push a job once `fence` has been signaled.
> +    pub fn add_dependency(&'a mut self, fence: &Fence) -> Result {

[Severity: Medium]
Can this cause a permanent mutable borrow of the job?

The method signature uses &'a mut self, where 'a is the lifetime parameter
of the Job struct itself. By mutably borrowing the job for its entire
remaining lifetime, it appears any subsequent attempt to call another method
on this job or pass it to JobQueue::submit(job) will result in a compile-time
error stating the value is already borrowed.

> +        let nr_of_deps = &raw const self.inner.nr_of_deps;
[ ... ]
> +impl<T: JobQueueOps + Send + Sync> ScopedWorkItem for SubmitWorker<'_, T> {
> +    fn run(work: &ScopedWorkRef<Self>) {
> +        // Prevent work-item UAF if the JQ is trying to drop right now.
> +        let Some(jq_accessor) = work.jq.inner.try_access() else {
> +            return;
> +        };
> +
> +        let mut jq = jq_accessor.lock();
> +
> +        while let Some(mut job) = jq.as_mut().project().waiting_jobs.pop_front() {
> +            // While we didn't hold the lock below for `run_job()`, it could be
> +            // that a new job entered the waiting list which has an unfulfilled
> +            // dependency.
> +            if job.nr_of_deps.load(Relaxed) > 0 {
> +                jq.waiting_jobs.push_front(job);
> +                break;
> +            }
> +            let new_capacity = jq.capacity as i64 - i64::from(job.cost);
> +            if new_capacity < 0 {
> +                jq.waiting_jobs.push_front(job);
> +                break;
> +            }

[Severity: High]
Could this permanently stall the FIFO queue if an oversized job is submitted?

Since there doesn't appear to be validation in new_job rejecting a job with
a cost larger than the queue's total capacity, such a job could enter the
system. If capacity - cost < 0 for an oversized job, the worker pushes it
back to the front of the list and breaks. Because it sits at the front, no
subsequent jobs can run, and the queue capacity will never grow large enough
to run it, effectively deadlocking the queue.

> +            *jq.as_mut().project().capacity = new_capacity as u64;
> +
> +            let seqno = job
> +                .fence()
> +                .as_ref()
> +                .expect("fence not present")
> +                .as_fence()
> +                .seqno();
> +            let runnable_job = JobRunnable {
> +                seqno,
> +                data: job.fence().as_ref().expect("fence not present").deref(),
> +            };
> +
> +            let jq_data = work.jq.fctx.deref().deref();
> +
> +            drop(jq); // Release lock.
> +            T::run_job(jq_data, runnable_job);
> +            jq = jq_accessor.lock(); // Retake lock.
> +
> +            // run_job() above needs to be called without lock protection. Thus,
> +            // it is possible that the driver instructed completion right before
> +            // we could re-take the lock.
> +            if jq.last_completed >= seqno {

[Severity: High]
Is it possible for this check to fail if a job completion races with the worker?

If the driver's interrupt handler calls complete_jobs_up_to_seqno(seqno)
while the lock is dropped during T::run_job, it will try to iterate over
running_jobs. Since the SubmitWorker hasn't pushed this job to
running_jobs yet, the loop in complete_jobs_up_to_seqno() is skipped,
and jq.last_completed isn't updated.

When SubmitWorker reacquires the lock, jq.last_completed >= seqno would
evaluate to false, and it would push the already-completed job into
running_jobs. This job would remain there forever, leaking its capacity
and hanging.

> +                if let Some(fence) = job.fence() {
> +                    fence.signal(Ok(()));
> +                } else {
> +                    panic!("fence not present.");
> +                }
> +
> +                // We drop this job here directly, from work_item context.
> +            } else {

[Severity: High]
Does dropping the job here violate atomic context constraints?

The JobQueue protects its inner state with a SpinLockIrq, meaning the CPU
is in atomic context here. The job implicitly drops at the end of the if
block while the spinlock guard is still held. Since dropping the job's
DriverFence involves an RCU grace period delay (typically calling
synchronize_rcu()), calling this sleeping function under a spinlock can
lead to a scheduling while atomic panic.

> +                jq.running_jobs.push_back(job);
> +            }
> +        }
> +    }
> +}
> +
> +#[pin_data]
> +struct DropWorker<'a, T: JobQueueOps + Send + Sync> {
> +    #[pin]
> +    jq: &'a JobQueue<'a, T>,
> +}
> +
> +impl<T: JobQueueOps + Send + Sync> ScopedWorkItem for DropWorker<'_, T> {
> +    fn run(work: &ScopedWorkRef<Self>) {
[ ... ]
> +        let mut jq = jq_accessor.lock();
> +
> +        while let Some(_job) = jq.drop_list.pop_front() {
> +            // Droppy-dy-drop
> +        }

[Severity: High]
Similar to the issue above, does this drop the job in atomic context?

The _job is dropped at the end of the while loop block while jq
(the SpinLockIrq guard) is still held. This seems like it will call
synchronize_rcu() via the DriverFence drop logic, which sleeps and could
cause a panic.

> +    }
> +}
[ ... ]
> +#[pin_data(PinnedDrop)]
> +pub struct JobQueue<'a, T: JobQueueOps + Send + Sync> {
> +    #[pin]
> +    drop_worker: ScopedWork<DropWorker<'a, T>>,
> +    #[pin]
> +    // Revocable to prevent work-item UAF on `inner`.
> +    inner: Revocable<SpinLockIrq<JobQueueInner<'a, T>>>,
> +    #[pin]
> +    fctx: FenceContext<DriverDataWrapper<T>>,
> +    #[pin]
> +    submit_worker: ScopedWork<SubmitWorker<'a, T>>,
> +}

[Severity: High]
Does the declaration order of submit_worker introduce a use-after-free risk?

Rust drops struct fields in declaration order (top-to-bottom). Because
submit_worker is declared after inner and fctx, its work item will be
canceled and dropped last. If SubmitWorker::run executes in the narrow
window after inner and fctx are destroyed but before submit_worker
is dropped, it will try to access work.jq.inner and work.jq.fctx,
resulting in a use-after-free.

[ ... ]
> +    pub fn complete_jobs_up_to_seqno(&self, seqno: u64) {
> +        let needs_drop = core::mem::needs_drop::<T::JobDataType>();
> +        // unwrap() can't fire because `self` is valid.
> +        let jq = self.inner.try_access().unwrap();
> +        let mut jq_locked = jq.lock();
> +        let mut jq = jq_locked.as_mut().project();
> +
> +        if seqno < *jq.last_completed {
> +            return;
> +        }
> +
> +        while let Some(mut job) = jq.running_jobs.pop_front() {
> +            if job
> +                .fence()
> +                .as_mut()
> +                .expect("fence not present")
> +                .as_fence()
> +                .seqno()
> +                > seqno
> +            {
> +                jq.running_jobs.push_front(job);
> +                break;
> +            }
> +
> +            *jq.capacity += u64::from(job.cost);
> +
> +            // This enables the SubmitWorker to complete a job that is not yet
> +            // in 'running_jobs', in case of a race against the lock.
> +            *jq.last_completed = seqno;

[Severity: High]
Is it intentional that these capacity and state updates are inside the loop?

If SubmitWorker::run is currently executing the job and hasn't yet pushed it
to running_jobs, running_jobs.pop_front() will return None. This skips
the loop entirely, so capacity is not restored and last_completed is not
updated. When SubmitWorker reacquires the lock, it won't see the completion.

[ ... ]
> +    pub fn teardown<E>(self, error: Error)
> +    where
> +        Error: From<E>,
> +    {

[Severity: Medium]
Will this function compile given self is passed by value?

JobQueue uses #[pin_data(PinnedDrop)] and contains structurally pinned
!Unpin fields like ScopedWork. Because it is !Unpin, it must be accessed
through pinned pointers. Taking self by value requires moving the JobQueue
out of its pinned pointer, which the Rust compiler rejects for !Unpin types.
It seems this method is uncallable dead code.

> +        let jq = self.inner.try_access().unwrap();
> +        let mut jq = jq.lock();
> +
> +        while let Some(mut job) = jq.running_jobs.pop_front() {
> +            if let Some(fence) = job.fence() {
> +                fence.signal(Err(error));
> +            } else {
> +                panic!("fence not present.");
> +            }
> +        }
> +    }

[Severity: High]
Does teardown silently drop waiting jobs without signaling them?

While it signals fences for running_jobs, it ignores waiting_jobs.
Since dma_fence instances do not signal themselves on drop, tasks waiting
on these fences will hang indefinitely.

> +}
> +
> +#[pinned_drop]
> +impl<T: JobQueueOps + Send + Sync> PinnedDrop for JobQueue<'_, T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        // Revoke to prevent still existing DependencyWakers from kicking the
> +        // SubmitWorker again before we could deregister them.
> +        let _ = self.inner.revoke();
> +    }
> +}

[Severity: High]
Does this drop implementation fulfill the documentation's guarantee?

The module documentation states: "Dropping a JobQueue results in all
remaining waiting and running jobs' fences being signaled with ECANCELED."
However, this drop logic does not iterate over waiting_jobs or
running_jobs to signal them. The fences will just be dropped, which will
cause any waiting tasks to hang indefinitely.

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