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

[sashiko-bot@kernel.org]

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