Re: [PATCH] drm/v3d: release indirect CSD GEM on pre-init failures

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Calling `v3d_job_cleanup()` on an uninitialized CPU job crashes the kernel in early error paths.
- [High] Nested CPU job allocations and query arrays are leaked on error paths.
- [High] Missing NULL check for the `info->indirect` GEM object allows a delayed NULL pointer dereference during job execution.
- [High] Memory allocated for `se.out_syncs` is leaked in multiple ioctls if `v3d_get_extensions()` fails midway.
- [Medium] An uninitialized `ww_acquire_ctx` is passed to `ww_acquire_fini()`, leading to undefined behavior and lockdep crashes.
--

--- Patch [1]: [PATCH] drm/v3d: release indirect CSD GEM on pre-init failures ---
commit f91c6cff2fb0a592f891c8261f38ae45087334d1
Author: Guangshuo Li <lgs201920130244@gmail.com>

drm/v3d: release indirect CSD GEM on pre-init failures
    
The commit drops the indirect CSD GEM reference on pre-init error paths in
the CPU job submit ioctl to avoid leaking the object when v3d_job_init() or
extensions fail.

> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index 1db43c6a078d5..8ed95a2038102 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
> @@ -476,6 +476,7 @@ v3d_get_cpu_indirect_csd_params(struct drm_file *file_priv,
>  	struct v3d_dev *v3d = v3d_priv->v3d;
>  	struct drm_v3d_indirect_csd indirect_csd;
>  	struct v3d_indirect_csd_info *info = &job->indirect_csd;
> +	int ret;
>  
>  	if (!v3d_validate_cpu_job(file_priv, job))
>  		return -EINVAL;
> @@ -496,9 +497,15 @@ v3d_get_cpu_indirect_csd_params(struct drm_file *file_priv,
>  
>  	info->indirect = drm_gem_object_lookup(file_priv, indirect_csd.indirect);

[Severity: High]
This is a pre-existing issue, but what happens if the user passes an invalid
GEM handle?

If drm_gem_object_lookup() returns NULL in v3d_get_cpu_indirect_csd_params(),
the code proceeds to submit the job. During execution,
v3d_rewrite_csd_job_wg_counts_from_indirect() will attempt to use it:

v3d_rewrite_csd_job_wg_counts_from_indirect()
    v3d_get_bo_vaddr(to_v3d_bo(indirect_csd->indirect))
        
Can this lead to a NULL pointer dereference later in the pipeline?

> -	return v3d_setup_csd_jobs_and_bos(file_priv, v3d, &indirect_csd.submit,
> -					  &info->job, &info->clean_job,
> -					  NULL, &info->acquire_ctx);
> +	ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d, &indirect_csd.submit,
> +					 &info->job, &info->clean_job,
> +					 NULL, &info->acquire_ctx);
> +	if (ret && info->indirect) {
> +		drm_gem_object_put(info->indirect);
> +		info->indirect = NULL;
> +	}
> +
> +	return ret;
>  }

[ ... ]

> @@ -1311,6 +1318,10 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
>  	struct v3d_submit_ext *out_se = NULL;
>  	struct v3d_cpu_job *cpu_job = NULL;
>  	struct v3d_csd_job *csd_job = NULL;
>  	struct v3d_job *clean_job = NULL;
>  	struct ww_acquire_ctx acquire_ctx;

[Severity: Medium]
This isn't a bug introduced by this patch, but is acquire_ctx guaranteed to be
initialized when jobs don't require BOs?

If the ioctl args have bo_handle_count == 0, v3d_lock_bo_reservations() is
skipped, leaving acquire_ctx uninitialized on the stack.

The function later calls v3d_attach_fences_and_unlock_reservation(), which
unconditionally passes it to drm_gem_unlock_reservations() and
ww_acquire_fini(). 
    
Could this pass an uninitialized lock context to the ww_mutex subsystem?

>  	int ret;

[ ... ]

>  	if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
>  		ret = v3d_get_extensions(file_priv, args->extensions, &se, cpu_job);
>  		if (ret) {
>  			drm_dbg(dev, "Failed to get extensions.\n");
> +			if (cpu_job->indirect_csd.indirect) {
> +				drm_gem_object_put(cpu_job->indirect_csd.indirect);
> +				cpu_job->indirect_csd.indirect = NULL;
> +			}
>  			goto fail;

[Severity: High]
This is a pre-existing issue, but does v3d_get_extensions() handle cleanup
consistently across ioctls?

When v3d_get_extensions() parses a DRM_V3D_EXT_ID_MULTI_SYNC extension, it
allocates se.out_syncs. If a subsequent extension fails, the function
returns an error. 

While this function correctly jumps to the fail block to clean up, other
callers like v3d_submit_cl_ioctl() and v3d_submit_csd_ioctl() appear to
return directly:

v3d_submit_cl_ioctl()
    ret = v3d_get_extensions(...)
    if (ret)
        return ret;

Does this leave the out_syncs memory leaked by bypassing the cleanup at
v3d_put_multisync_post_deps(&se)?

>  		}
>  	}

[ ... ]

>  	ret = v3d_job_init(v3d, file_priv, &cpu_job->base,
>  			   v3d_cpu_job_free, 0, &se, V3D_CPU);
>  	if (ret) {
> +		if (cpu_job->indirect_csd.indirect) {
> +			drm_gem_object_put(cpu_job->indirect_csd.indirect);
> +			cpu_job->indirect_csd.indirect = NULL;
> +		}
>  		v3d_job_deallocate((void *)&cpu_job);

[Severity: High]
This isn't a bug introduced by this patch, but does shallowly deallocating the
job here leak the nested structures?

Earlier, v3d_setup_csd_jobs_and_bos() may allocate inner jobs:

v3d_setup_csd_jobs_and_bos()
    v3d_job_allocate(v3d, (void *)job, ...)
    v3d_job_allocate(v3d, (void *)clean_job, ...)

When v3d_job_deallocate() frees cpu_job, it zeroes the pointer. Since the
pointer is now NULL, the cleanup at the fail label will skip it,
leaving those nested job structs without a way to be freed.

>  		goto fail;

[Severity: Critical]
This is a pre-existing issue, but is it safe to pass the uninitialized
job to the cleanup path?

If execution jumps to the fail label before v3d_job_init() succeeds,
cpu_job will still be zero-initialized.

The cleanup path does:

fail:
    v3d_job_cleanup((void *)cpu_job);
        drm_sched_job_cleanup(&job->base);

Since base.s_fence is NULL, would drm_sched_job_cleanup() trigger a NULL
pointer dereference when it attempts to read the refcount:

if (kref_read(&job->s_fence->finished.refcount))

>  	}

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