Re: [RFC PATCH v5 6/6] rust: drm: Add JobQueue

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] drm/job_queue: recursive spinlock deadlock when signaling intra-queue dependencies
- [High] drm/job_queue: job is permanently lost when queue capacity is exceeded
- [High] drm/job_queue: submit_worker completely ignores dependencies and runs unready jobs
- [High] drm/job_queue: synchronous job completion is permanently missed during run_job
- [High] drm: JobQueue runs run_job in atomic context due to RevocableGuard
- [Medium] drm/job_queue: capacity is leaked when SubmitWorker cleans up signaled jobs
- [Medium] drm/job_queue: JobQueue API is unusable due to borrow checker conflict
--

commit 4ab6c27823ae4ac383ac1e5112edd966e2143dbd
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..b5c7f4ecb3f46
> --- /dev/null
> +++ b/rust/kernel/drm/job_queue.rs
> @@ -0,0 +1,525 @@
> +// SPDX-License-Identifier: GPL-2.0

[ ... ]

> +impl<T: JobQueueOps + Send + Sync> ScopedWorkItem for SubmitWorker<'_, T> {
> +    fn run(work: &ScopedWorkRef<Self>) {
> +        let Some(jq_accessor) = work.jq.inner.try_access() else {
> +            return;
> +        };

[Severity: High]
The JobQueue documentation explicitly states that jobs are submitted in
process context, which normally permits sleeping. However, does this
try_access() call enter an atomic context?

When try_access() returns a RevocableGuard, it intrinsically holds
rcu::read_lock(). Because the jq_accessor guard remains alive across the
subsequent call to T::run_job(), the driver callback is executed entirely
within an RCU read-side critical section, where sleeping is forbidden.

Will this regression cause a "scheduling while atomic" panic if a driver relies
on the documented process context guarantee to sleep, wait on mutexes, or use
GFP_KERNEL allocations inside run_job?

> +
> +        let mut jq = jq_accessor.lock();
> +
> +        while let Some(mut job) = jq.as_mut().project().waiting_jobs.pop_front() {
> +            let new_capacity = jq.capacity as i64 - job.cost as i64;

[Severity: High]
Does this bypass the dependency system? JobQueue::submit() pushes jobs
unconditionally to waiting_jobs, and this loop iterates over them and executes
them as long as there is sufficient queue capacity.

However, it never checks if job.nr_of_deps == 0 before executing the job. Will
this regression result in jobs being sent to the hardware immediately,
regardless of pending dependency fences?

> +            if new_capacity < 0 {
> +                break;
> +            }

[Severity: High]
When this loop encounters a job that exceeds the available capacity, it executes
a break to exit. Because the job was already removed from the waiting_jobs list
via pop_front(), breaking the loop drops the local job variable, destroying it.

Since the job is never pushed back onto waiting_jobs, its hardware operations
will be lost and its completion fence will never signal. Will this regression
cause a permanent hang for anything depending on it?

> +            *jq.as_mut().project().capacity = new_capacity as u64;
> +
> +            let runnable_job = JobRunnable {
> +                seqno: job.fence().as_ref().expect("fence not present").as_fence().seqno(),
> +                data: job.fence().as_ref().expect("fence not present").deref(),
> +            };
> +
> +            let jq_data = work.jq.fctx.deref().deref();
> +
> +            drop(jq);
> +            T::run_job(jq_data, runnable_job);
> +            // The driver could now have taken the lock here and try to complete
> +            // this very job. It would then not get signaled because it was not
> +            // yet in the running list. However, since the driver completes jobs
> +            // by sequence number, its subsequent calls to jq.complete_jobs_up_to_seqno()
> +            // will signal this job, because then it will be in the running_jobs
> +            // list.
> +            // We accept this race because it makes the implementation much
> +            // simpler and avoids locking issues. Otherwise, complete_jobs_up_to_seqno()
> +            // would need a distinct signaler-object-list to avoid races between
> +            // this work item and said function.
> +            jq = jq_accessor.lock();
> +
> +            jq.as_mut().project().running_jobs.push_back(job);

[Severity: High]
The code drops the queue lock before calling T::run_job(). If the driver
completes the job immediately (e.g. via synchronous execution or a very fast
hardware interrupt) and calls complete_jobs_up_to_seqno(), the completion
handler will search running_jobs.

Since the worker hasn't yet pushed the job to running_jobs here, the handler
will completely miss it. If this is the last job in a burst, and no subsequent
jobs trigger completion, will this regression leave the job hanging forever in
running_jobs without its fence signaling?

> +        }
> +
> +        // Now clean up all already-signaled jobs, dropping their fences.
> +        let mut jq = jq.as_mut().project();
> +
> +        let mut cursor = jq.running_jobs.cursor_front();
> +
> +        while let Some(job) = cursor.peek_next() {
> +            let mut job = job.remove();
> +
> +            if !job.fence().as_mut().expect("fence not present").as_fence().is_signaled() {
> +                jq.running_jobs.push_front(job);
> +                // Jobs complete in order. No point in checking the rest.
> +                break;
> +            }

[Severity: Medium]
This cleanup loop iterates over running_jobs and removes any job whose fence
is signaled. Unlike the standard completion path in
complete_jobs_up_to_seqno() which restores capacity via
*jq.capacity += job.cost, this loop drops the job without updating jq.capacity.

Will this regression cause a permanent reduction in the queue's tracked capacity?

[ ... ]

> +    /// Create a new job.
> +    pub fn new_job(&'a self, cost: u32, data: T::JobDataType) -> Result<Job<'a, T>> {

[Severity: Medium]
This borrows the JobQueue immutably (&'a self) for the lifetime of the
returned Job. Later, JobQueue::submit takes a mutable reference (&mut self) and
the Job.

Because self cannot be borrowed mutably while the job's immutable borrow is
active, this creates a strict compile error. While the documentation comment
acknowledges this problem, does this regression make the API structurally
impossible to call from driver code?

[ ... ]

> +    pub fn complete_jobs_up_to_seqno(&self, seqno: u64, status: Result) {
> +        // 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();
> +
> +        let mut cursor = jq.running_jobs.cursor_front();
> +
> +        while let Some(job) = cursor.peek_next() {
> +            let mut job = job.remove();
> +
> +            if job.fence().as_mut().expect("fence not present").as_fence().seqno() > seqno {
> +                jq.running_jobs.push_front(job);
> +                break;
> +            }
> +
> +            *jq.capacity += job.cost as u64;
> +
> +            if let Some(fence) = job.fence() {
> +                fence.signal(status);

[Severity: Critical]
This function acquires the queue's SpinLockIrq (jq.lock()) and calls
fence.signal() on completed jobs. If another job in the same queue was
configured to depend on this fence, DependencyWaker::on_signal() is executed
synchronously by the dma-fence subsystem.

Since on_signal() then attempts to acquire the exact same queue spinlock via
jq.deref().lock(), will this regression result in a guaranteed recursive
spinlock deadlock on the same CPU?

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