Re: [PATCH v3 3/8] drm/vc4: Add DRM GPU scheduler infrastructure and job structures

[sashiko-bot@kernel.org]

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 <mcanal@igalia.com>

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/20260712-vc4-drm-scheduler-v3-0-ed6dc4defa53@igalia.com?part=3