Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues: - [High] Missing NULL check for `bin_job` and `render_job` in IRQ handler causes kernel panic on spurious interrupts. - [High] `vc4_v3d_get_bin_slot` prematurely returns `-ENOMEM` when bin slots are exhausted but the hardware is momentarily idle. - [High] Unbalanced `vc4_bo_dec_usecnt()` in error path during BO lookup causes usecnt underflow. - [Medium] `vc4_save_hang_state()` fails to capture BOs for the active bin job, capturing only the render job's BOs. - [Medium] Unconditional `vc4_perfmon_stop` in IRQ handlers breaks performance monitoring for pipelined or concurrent jobs. Pre-existing issues: - [High] An off-by-one error in shader record validation allows userspace to trigger an out-of-bounds read past the end of the `render->bo` array. -- --- Patch [7]: [PATCH v3 7/8] drm/vc4: Switch to DRM GPU scheduler --- commit 873ba817e912575f3a2ceffbc30604b80b1e73ac Author: Maíra Canal <[email protected]> drm/vc4: Switch to DRM GPU scheduler Wire up the DRM GPU scheduler to the VC4 submission path, replacing the legacy hand-rolled job queue implementation. The scheduler now handles job ordering, dependency tracking, and timeout detection. > diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c > --- a/drivers/gpu/drm/vc4/vc4_irq.c > +++ b/drivers/gpu/drm/vc4/vc4_irq.c > @@ -225,16 +136,42 @@ vc4_irq(int irq, void *arg) > if (intctl & V3D_INT_FLDONE) { > + struct vc4_bin_job *bin_job; > + struct vc4_fence *fence; > + > spin_lock(&vc4->job_lock); > - vc4_irq_finish_bin_job(dev); > + bin_job = vc4->bin_job; > + vc4->bin_job = NULL; > spin_unlock(&vc4->job_lock); > + > + fence = to_vc4_fence(bin_job->base.irq_fence); [Severity: High] Could this cause a NULL pointer dereference on spurious interrupts? If the hardware generates a spurious V3D_INT_FLDONE interrupt, or if the interrupt fires after the job state has been reset (e.g., due to a timeout or GPU reset), vc4->bin_job will be NULL. Unconditionally dereferencing bin_job->base.irq_fence here would lead to an immediate kernel panic in the interrupt handler. > + > + vc4_perfmon_stop(vc4, bin_job->base.perfmon, true); [Severity: Medium] Is it safe to unconditionally stop the perfmon here? Under the legacy implementation, the driver left the perfmon running across the gap between a job's bin and render phases, only stopping it if the next job didn't use the same perfmon. By calling vc4_perfmon_stop() at the end of every bin job, the hardware counters might be prematurely disabled while the corresponding render job is still actively executing, which would corrupt the gathered statistics. > + > + trace_vc4_bcl_end_irq(dev, fence->seqno); > + > + dma_fence_signal(&fence->base); > + > status = IRQ_HANDLED; > } > > if (intctl & V3D_INT_FRDONE) { > + struct vc4_render_job *render_job; > + struct vc4_fence *fence; > + > spin_lock(&vc4->job_lock); > - vc4_irq_finish_render_job(dev); > + render_job = vc4->render_job; > + vc4->render_job = NULL; > spin_unlock(&vc4->job_lock); > + > + fence = to_vc4_fence(render_job->base.irq_fence); [Severity: High] Similar to the FLDONE case above, is it possible for render_job to be NULL here if a spurious V3D_INT_FRDONE interrupt occurs, leading to a panic? > diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c > --- a/drivers/gpu/drm/vc4/vc4_v3d.c > +++ b/drivers/gpu/drm/vc4/vc4_v3d.c > @@ -194,21 +193,20 @@ int vc4_v3d_get_bin_slot(struct vc4_dev *vc4) > return slot; > } > > - /* Couldn't find an open slot. Wait for render to complete > + /* Couldn't find an open slot. Wait for render to complete > * and try again. > */ > - exec = vc4_last_render_job(vc4); > - if (exec) > - seqno = exec->seqno; > + fence = NULL; > + if (vc4->render_job) > + fence = dma_fence_get(vc4->render_job->base.done_fence); > spin_unlock_irqrestore(&vc4->job_lock, irqflags); [Severity: High] Will this prematurely return -ENOMEM when the hardware is momentarily idle? Bin slots are allocated during vc4_submit_cl_ioctl before jobs are queued in the DRM scheduler. If the hardware is momentarily idle while jobs are waiting in the DRM scheduler queue holding bin slots, vc4->render_job will be NULL. This bypasses the wait and immediately returns -ENOMEM, which could cause spurious ioctl failures under load. > diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c > --- a/drivers/gpu/drm/vc4/vc4_gem.c > +++ b/drivers/gpu/drm/vc4/vc4_gem.c > @@ -168,21 +150,16 @@ vc4_save_hang_state(struct drm_device *dev) > spin_lock_irqsave(&vc4->job_lock, irqflags); > - exec[0] = vc4_first_bin_job(vc4); > - exec[1] = vc4_first_render_job(vc4); > - if (!exec[0] && !exec[1]) > + bin_job = vc4->bin_job; > + render_job = vc4->render_job; > + if (!bin_job && !render_job) > goto err_free_state; > > - /* Get the bos from both binner and renderer into hang state. */ > + /* Get the BOs from the render job into hang state. */ > state->bo_count = 0; > - for (i = 0; i < 2; i++) { > - if (!exec[i]) > - continue; > - > - unref_list_count = 0; > - list_for_each_entry(bo, &exec[i]->unref_list, unref_head) > - unref_list_count++; > - state->bo_count += exec[i]->bo_count + unref_list_count; > + if (render_job) { > + unref_list_count = list_count_nodes(&render_job->unref_list); > + state->bo_count += render_job->bo_count + unref_list_count; > } [Severity: Medium] Does this unintentionally skip capturing BOs for the active bin job? Because the new job structures moved BO tracking exclusively into vc4_render_job, this only iterates over the bo_count and unref_list of vc4->render_job. If a hang happens during a binning operation while the render queue is idle, it seems no BOs will be captured at all. > diff --git a/drivers/gpu/drm/vc4/vc4_validate.c > b/drivers/gpu/drm/vc4/vc4_validate.c > --- a/drivers/gpu/drm/vc4/vc4_validate.c > +++ b/drivers/gpu/drm/vc4/vc4_validate.c > @@ -798,12 +801,12 @@ validate_gl_shader_rec(struct drm_device *dev, > exec->shader_rec_size -= packet_size; > > for (i = 0; i < shader_reloc_count; i++) { > - if (src_handles[i] > exec->bo_count) { > + if (src_handles[i] > render->bo_count) { > DRM_DEBUG("Shader handle %d too big\n", src_handles[i]); > return -EINVAL; > } > > - bo[i] = to_drm_gem_dma_obj(exec->bo[src_handles[i]]); > + bo[i] = to_drm_gem_dma_obj(render->bo[src_handles[i]]); [Severity: High] This is a pre-existing issue, but does this bounds check contain an off-by-one error? Since the render->bo array has bo_count elements, valid indices are from 0 to bo_count - 1. If userspace provides an index exactly equal to bo_count, the check incorrectly succeeds. The kernel then dereferences render->bo[bo_count], resulting in an out-of-bounds read past the end of the array. > diff --git a/drivers/gpu/drm/vc4/vc4_submit.c > b/drivers/gpu/drm/vc4/vc4_submit.c > --- /dev/null > +++ b/drivers/gpu/drm/vc4/vc4_submit.c [ ... ] > +fail_dec_usecnt: > + /* Decrease usecnt on acquired objects */ > + for (i--; i >= 0; i--) > + vc4_bo_dec_usecnt(to_vc4_bo(job->bo[i])); [Severity: High] Are the reference counts getting underflowed during the error path? If an increment fails, the error path loops backwards decrementing the usecnt. However, because the BO array remains fully populated in the job struct, the subsequent failure cleanup in vc4_render_job_free() iterates over the entire array again: vc4_submit.c:vc4_render_job_free() { if (job->bo) { for (int i = 0; i < job->bo_count; i++) { struct vc4_bo *bo = to_vc4_bo(job->bo[i]); vc4_bo_dec_usecnt(bo); ... } } } This would double-decrement the BOs that were unwound here. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
