Re: [PATCH v3] drm/v3d: release CPU job resources on pre-init failures

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `v3d_job_deallocate()` on pre-init validation error paths causes a kernel panic in `drm_sched_job_cleanup()`.
- [High] Incomplete cleanup on `v3d_get_extensions()` failure causes resource leaks and deadlocks for chained extensions.
- [High] Early validation failures for INDIRECT_CSD CPU jobs bypass WW mutex unreservation and leak nested jobs.
--

--- Patch [1]: [PATCH v3] drm/v3d: release CPU job resources on pre-init failures ---
commit 0d9b615d1fa0aedf4a08be6bbd88de15b262217c
Author: Guangshuo Li <lgs201920130244@gmail.com>

drm/v3d: release CPU job resources on pre-init failures

This commit extracts CPU job resource cleanup into a new helper function
and calls it on pre-init failure paths in the drm/v3d driver. This helps
prevent leaking extension resources if validation fails before the job
destructor is installed.

> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index 7682b24f13ec5..77e94c25b2afd 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c

[ ... ]

> @@ -1320,12 +1326,14 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,

[Severity: High]
This is a pre-existing issue, but since the v3 patch notes mention removing
v3d_cpu_job_free_resources() from the v3d_get_extensions() error path,
can an error during extension parsing leak resources?

If v3d_get_extensions() loops through a chain of user extensions and a later
extension fails to parse, it returns an error immediately. Are the resources
and reservations acquired by previously parsed extensions in the chain unrolled
and freed?

For example, if an INDIRECT_CSD extension parses successfully but a
subsequent extension fails, it appears the BO reservations locked in
v3d_setup_csd_jobs_and_bos() are never unlocked, which could lead to a WW
mutex deadlock. Does v3d_get_extensions() handle chained extension failures
fully?

>  	/* Every CPU job must have a CPU job user extension */
>  	if (!cpu_job->job_type) {
>  		drm_dbg(dev, "CPU job must have a CPU job user extension.\n");
> +		v3d_cpu_job_free_resources(cpu_job);
>  		ret = -EINVAL;
>  		goto fail;
>  	}
>  
>  	if (args->bo_handle_count != cpu_job_bo_handle_count[cpu_job->job_type]) {
>  		drm_dbg(dev, "This CPU job was not submitted with the proper number of BOs.\n");
> +		v3d_cpu_job_free_resources(cpu_job);
>  		ret = -EINVAL;
>  		goto fail;
>  	}

[Severity: High]
This is a pre-existing issue, but does jumping to the fail label here leave
cpu_job allocated and crash during cleanup?

When jumping to the fail block, cpu_job is not set to NULL and
v3d_job_deallocate() is not called. The fail block executes:

fail:
        v3d_job_cleanup((void *)cpu_job);

This eventually calls drm_sched_job_cleanup(&job->base). Because
v3d_job_init() has not been called yet, job->s_fence is NULL. Can this
result in a NULL pointer dereference and kernel panic in
drm_sched_job_cleanup()?

[Severity: High]
This is also a pre-existing issue, but can these early validation paths leak
nested jobs and cause deadlocks for INDIRECT_CSD jobs?

When an INDIRECT_CSD extension is parsed earlier, v3d_setup_csd_jobs_and_bos()
allocates nested csd_job and clean_job structures and locks their GEM BO
reservations. If validation fails here, the code jumps to fail, bypassing the
fail_unreserve label.

While v3d_cpu_job_free_resources() drops the indirect GEM reference, it
does not free the nested jobs or unlock their context. At the fail block,
the local csd_job and clean_job variables are still NULL (they are assigned
later in the function), so v3d_job_cleanup() does nothing for them. Can
this permanently leak the jobs and leave the BOs locked?

> @@ -1335,6 +1343,7 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
>  	ret = v3d_job_init(v3d, file_priv, &cpu_job->base,
>  			   v3d_cpu_job_free, 0, &se, V3D_CPU);
>  	if (ret) {
> +		v3d_cpu_job_free_resources(cpu_job);
>  		v3d_job_deallocate((void *)&cpu_job);
>  		goto fail;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718071047.3037514-1-lgs201920130244@gmail.com?part=1