Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
New issues: - [Critical] `vc4_sched_init()` is never called, leading to a zero-initialized scheduler and a kernel panic. - [High] Unsigned enum used as a reverse loop counter causes an infinite loop and out-of-bounds access. - [High] Dereferencing `bin->render` without checking for NULL causes a kernel panic. - [High] Race condition between concurrent multi-queue timeout handlers causes Use-After-Free. - [High] Missing `dma_fence_put()` for `irq_fence` causes a steady memory leak. - [High] GFP_KERNEL memory allocations during GPU reset recovery cause deadlocks. - [Low] Missing definition of `vc4_job_cleanup` causing build failure. Pre-existing issues: - [High] Infinite loop in a command list extends the GPU timeout indefinitely (DoS). - [High] Hardware registers are accessed without guaranteeing the device is successfully powered on. -- --- Patch [3]: [PATCH v3 3/8] drm/vc4: Add DRM GPU scheduler infrastructure and job structures --- commit 823d1127ab669811483f871897f29bae5ea3a18f Author: Maíra Canal <[email protected]> drm/vc4: Add DRM GPU scheduler infrastructure and job structures Introduce struct vc4_job as the new base structure for GPU jobs, embedding struct drm_sched_job for use with the DRM GPU scheduler. Add core scheduler infrastructure in vc4_sched.c, implementing queue operations, timeout handling, and job cleanup. > diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c > --- a/drivers/gpu/drm/vc4/vc4_drv.c > +++ b/drivers/gpu/drm/vc4/vc4_drv.c [ ... ] > @@ -155,15 +158,33 @@ static int vc4_open(struct drm_device *dev, struct > drm_file *file) > return -ENOMEM; > vc4file->dev = vc4; > > + for (q = 0; q < VC4_MAX_QUEUES; q++) { > + sched = &vc4->queue[q].sched; > + ret = drm_sched_entity_init(&vc4file->sched_entity[q], > + DRM_SCHED_PRIORITY_NORMAL, &sched, > + 1, NULL); [Severity: Critical] Is it intentional that the DRM schedulers are initialized against a zero-initialized structure? I see vc4_sched_init() is implemented in vc4_sched.c but it never seems to be called during driver setup. If vc4->queue[q].sched is completely zero-initialized, doesn't opening and then closing a file trigger a NULL pointer dereference in vc4_close() when calling drm_sched_entity_destroy() due to the uninitialized wait queues? > + if (ret) > + goto err_sched; > + } > + > vc4_perfmon_open_file(vc4file); > file->driver_priv = vc4file; > + > return 0; > + > +err_sched: > + for (q--; q >= 0; q--) > + drm_sched_entity_destroy(&vc4file->sched_entity[q]); [Severity: High] Will this loop ever terminate on failure? Since q is of type enum vc4_queue, the compiler will likely treat it as an unsigned integer type. If the first allocation fails (q = 0), wouldn't q-- underflow to the maximum unsigned value, making q >= 0 always true and leading to an out-of-bounds access and infinite loop? > diff --git a/drivers/gpu/drm/vc4/vc4_fence.c b/drivers/gpu/drm/vc4/vc4_fence.c > --- a/drivers/gpu/drm/vc4/vc4_fence.c > +++ b/drivers/gpu/drm/vc4/vc4_fence.c > @@ -23,6 +23,24 @@ > > #include "vc4_drv.h" > > +struct dma_fence *vc4_fence_create(struct vc4_dev *vc4, enum vc4_queue q) > +{ > + struct vc4_queue_state *queue = &vc4->queue[q]; > + struct vc4_fence *fence; > + > + fence = kzalloc_obj(*fence); [Severity: High] Can this memory allocation cause a deadlock during GPU reset? When the GPU hangs, the timeout handler calls drm_sched_resubmit_jobs() which invokes the run_job callbacks. If vc4_fence_create() is called in this path and implicitly uses GFP_KERNEL (via kzalloc_obj), memory reclaim might block waiting for fences from the already hung GPU, preventing the reset from ever completing. > diff --git a/drivers/gpu/drm/vc4/vc4_sched.c b/drivers/gpu/drm/vc4/vc4_sched.c > --- /dev/null > +++ b/drivers/gpu/drm/vc4/vc4_sched.c [ ... ] > +static void > +vc4_sched_job_free(struct drm_sched_job *sched_job) > +{ > + struct vc4_job *job = to_vc4_job(sched_job); > + > + vc4_job_cleanup(job); > +} [Severity: Low] Will this compile successfully? The function vc4_job_cleanup() is called here but it does not appear to be defined anywhere in this patch or the surrounding code. [ ... ] > +static struct dma_fence *vc4_bin_job_run(struct drm_sched_job *sched_job) > +{ [ ... ] > + /* Release the previous fence if the scheduler is resubmitting this > + * job after a GPU reset. > + */ > + if (job->base.irq_fence) > + dma_fence_put(job->base.irq_fence); > + job->base.irq_fence = dma_fence_get(fence); [Severity: High] Does this code leak the extra dma_fence reference? We acquire an extra reference here with dma_fence_get() in both the bin and render job paths, but I don't see a corresponding dma_fence_put() in the job teardown paths. Doesn't this mean every executed job permanently leaks a fence? [ ... ] > +static void > +vc4_reset(struct drm_device *dev) > +{ > + struct vc4_dev *vc4 = to_vc4_dev(dev); > + > + drm_err(dev, "Resetting GPU.\n"); > + > + mutex_lock(&vc4->power_lock); > + if (vc4->power_refcount) { > + /* Power the device off and back on the by dropping the > + * reference on runtime PM. > + */ > + pm_runtime_put_sync_suspend(&vc4->v3d->pdev->dev); > + pm_runtime_get_sync(&vc4->v3d->pdev->dev); [Severity: High] This is a pre-existing issue, but shouldn't we check the return value of pm_runtime_get_sync() here? If the device fails to resume, it remains powered off. The function then proceeds to call vc4_irq_reset() which performs MMIO writes to V3D_INTCTL and V3D_INTENA. Writing to unclocked hardware typically triggers bus errors and a kernel crash. > + } > + mutex_unlock(&vc4->power_lock); > + > + vc4_irq_reset(dev); > +} > + > +static enum drm_gpu_sched_stat > +vc4_gpu_reset_for_timeout(struct vc4_dev *vc4, struct drm_sched_job > *sched_job) > +{ > + struct vc4_job *job = to_vc4_job(sched_job); > + enum vc4_queue q; > + > + mutex_lock(&vc4->reset_lock); [Severity: High] Can this mutex_lock() race with a concurrent timeout handler? VC4 has separate schedulers for the BIN and RENDER queues. If jobs on both queues time out simultaneously, Thread A might acquire the lock, reset the GPU, restart both schedulers, and unlock. Thread B would block here. Once Thread A releases the lock, Thread B wakes up and proceeds to use sched_job. However, since Thread A already restarted the schedulers, the RENDER scheduler could immediately execute and complete Thread B's job, freeing it. Wouldn't this cause a use-after-free when Thread B finally acquires the lock and dereferences sched_job below? > + > + /* block scheduler */ > + for (q = 0; q < VC4_MAX_QUEUES; q++) > + drm_sched_stop(&vc4->queue[q].sched, sched_job); > + > + if (sched_job) { > + drm_sched_increase_karma(sched_job); > + > + /* If the guilty job is a BIN job, also increase the karma > + * of its paired render job. Otherwise, the RENDER job would > + * be submitted to the GPU without binner output. > + */ > + if (sched_job->sched == &vc4->queue[VC4_BIN].sched) { > + struct vc4_bin_job *bin = to_bin_job(sched_job); > + > + drm_sched_increase_karma(&bin->render->base.base); [Severity: High] Can bin->render be NULL here? If userspace submits a job that only has a BIN component, or if the RENDER job allocation failed prior to submission, won't dereferencing bin->render->base.base result in a kernel panic? [ ... ] > +static enum drm_gpu_sched_stat > +vc4_cl_job_timedout(struct drm_sched_job *sched_job, enum vc4_queue q) > +{ > + struct vc4_job *job = to_vc4_job(sched_job); > + struct vc4_dev *vc4 = job->vc4; > + u32 ctca = V3D_READ(V3D_CTNCA(q)); > + u32 ctra = V3D_READ(V3D_CTNRA0(q)); > + > + /* If the current address or return address have changed, then the GPU > + * has probably made progress and we should delay the reset. This could > + * fail if the GPU got in an infinite loop in the CL, but that is pretty > + * unlikely outside of an i-g-t testcase. > + */ > + if (job->timedout_ctca != ctca || job->timedout_ctra != ctra) { [Severity: High] This is a pre-existing issue, but does this logic safely handle infinite loops in command lists? It assumes any change in the execution address means forward progress. If an unprivileged user submits a command list with a backward branch, the execution address will constantly change. Won't this cause the timeout handler to infinitely defer the reset, resulting in a permanent GPU lockup and a denial of service? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
