Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Integer truncation on 32-bit platforms breaks infinite waits. - [High] xa_alloc_cyclic() error path causes data corruption by erasing valid fences. - [High] GFP_KERNEL memory allocation under dma_resv locks causes deadlocks. - [Medium] Legitimate negative error codes from dma_fence_wait_timeout are masked to -ETIME. --
--- Patch [6]: [PATCH v3 6/8] drm/vc4: Add per-file descriptor seqno tracking --- commit fcd8c204d7bfbab0abf15f0e12e9b8db05263bcc Author: Maíra Canal <[email protected]> drm/vc4: Add per-file descriptor seqno tracking Implement per-file descriptor seqno tracking using an xarray, allowing userspace to wait on specific job completions via vc4_wait_seqno_ioctl. > diff --git a/drivers/gpu/drm/vc4/vc4_submit.c > b/drivers/gpu/drm/vc4/vc4_submit.c > index 5e839db2ba4d3..0e43f31fb813f 100644 > --- a/drivers/gpu/drm/vc4/vc4_submit.c > +++ b/drivers/gpu/drm/vc4/vc4_submit.c [ ... ] > @@ -232,6 +232,57 @@ vc4_get_bcl(struct drm_device *dev, struct vc4_exec_info > *exec) > return ret; > } > > +int > +vc4_wait_seqno_ioctl(struct drm_device *dev, void *data, > + struct drm_file *file_priv) > +{ > + struct vc4_file *vc4_priv = file_priv->driver_priv; > + struct vc4_dev *vc4 = to_vc4_dev(dev); > + struct drm_vc4_wait_seqno *args = data; > + unsigned long timeout_jiffies = nsecs_to_jiffies(args->timeout_ns); [Severity: High] On 32-bit architectures, could this truncate the 64-bit timeout_ns? If userspace passes ~0ULL for an infinite wait, this would truncate to a 32-bit unsigned long, which then evaluates to a negative signed long when passed to dma_fence_wait_timeout(), potentially causing the wait to fail instantly. > + unsigned long start = jiffies; > + struct dma_fence *fence; > + long ret; > + [ ... ] > + if (!fence) > + return 0; > + > + trace_vc4_wait_for_seqno_begin(dev, args->seqno, args->timeout_ns); > + ret = dma_fence_wait_timeout(fence, true, timeout_jiffies); > + trace_vc4_wait_for_seqno_end(dev, args->seqno); > + > + dma_fence_put(fence); > + > + if (ret == -ERESTARTSYS) { > + u64 delta = jiffies_to_nsecs(jiffies - start); > + > + if (args->timeout_ns >= delta) > + args->timeout_ns -= delta; > + else > + args->timeout_ns = 0; > + > + return ret; > + } > + > + return ret > 0 ? 0 : -ETIME; [Severity: Medium] Does this mask legitimate negative error codes returned by dma_fence_wait_timeout()? If dma_fence_wait_timeout() returns an error like -EINVAL for invalid timeouts or -EDEADLK, this translates it to -ETIME, which might mislead userspace debugging and error handling. > +} > + [ ... ] > @@ -290,6 +341,10 @@ vc4_render_job_free(struct kref *ref) > vc4->bin_alloc_used &= ~job->bin_slots; > spin_unlock_irqrestore(&vc4->job_lock, irqflags); > > + if (job->seqno) > + xa_erase(&job->file->seqno_xa, job->seqno); [Severity: High] Could this unconditionally erase a valid fence belonging to a concurrent submission if xa_alloc_cyclic() previously failed? In vc4_submit_cl_ioctl(), xa_alloc_cyclic() can populate render->seqno with a proposed ID before returning -ENOMEM. The error path would then call this function, which erases the ID from the xarray. If another thread successfully used that same ID, its completion fence would be incorrectly removed. > + > + vc4_file_put(job->file); > vc4_job_free(ref); > } > [ ... ] > @@ -547,11 +611,20 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data, > goto fail_exec; > > scoped_guard(mutex, &vc4->sched_lock) { > - ret = vc4_push_jobs(&exec); > + ret = xa_alloc_cyclic(&vc4_priv->seqno_xa, &render->seqno, > + NULL, xa_limit_32b, &vc4_priv->next_seqno, > + GFP_KERNEL); [Severity: High] Is it safe to use GFP_KERNEL here and in xa_store() while dma_resv locks are held? vc4_submit_cl_ioctl() calls vc4_lock_bo_reservations() before reaching this code. Using GFP_KERNEL allows direct memory reclaim, which could invoke DRM shrinkers. If a shrinker attempts to acquire the same dma_resv lock to evict a buffer object, it could result in an AB-BA deadlock. > + if (ret < 0) > + goto fail_exec; > + > + ret = vc4_push_jobs(vc4_priv, &exec); > } > -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
