Re: [PATCH] drm/v3d: fix to avoid cleaning up uninitialized CPU jobs

[Maíra Canal]

Hi Jeongjun,

On 25/05/26 11:04, Jeongjun Park wrote:
> The CPU submit ioctl checks cpu_job->job_type before the CPU job has been
> initialized with v3d_job_init(). When no CPU job extension is supplied,
> the check fails and the ioctl goes to the common error path.
>
> That path calls v3d_job_cleanup(), which expects a fully initialized
> v3d_job. However at this point the CPU job has only been allocated,
> so the embedded DRM scheduler job state has not been initialized yet.
>
> Initialize the CPU job after parsing extensions, but before validating
> the CPU job type and BO count. Keep pre-initialization failures on a

Considering that this is a minimal fix for stable, could you explain me
why do you believe moving v3d_job_init() is the best solution?

Best regards,
- Maíra

> separate path that only deallocates the job and releases multisync
> post-dependencies. Also use the CPU-job release callback so initialized
> CPU jobs are released through the normal cleanup path.
>
> Cc: stable@xxxxxxxxxx
> Fixes: aafc1a2bea67 ("drm/v3d: Add a CPU job submission")
> Signed-off-by: Jeongjun Park <aha310510@xxxxxxxxx>
> ---
>   drivers/gpu/drm/v3d/v3d_submit.c | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index 3ddd53b6f437..f5a1b388eaa3 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
> @@ -1300,10 +1300,18 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
>   		ret = v3d_get_extensions(file_priv, args->extensions, &se, cpu_job);
>   		if (ret) {
>   			drm_dbg(dev, "Failed to get extensions.\n");
> -			goto fail;
> +			v3d_job_deallocate((void *)&cpu_job);
> +			goto fail_init;
>   		}
>   	}
>   
> +	ret = v3d_job_init(v3d, file_priv, &cpu_job->base,
> +			   v3d_cpu_job_free, 0, &se, V3D_CPU);
> +	if (ret) {
> +		v3d_job_deallocate((void *)&cpu_job);
> +		goto fail_init;
> +	}
> +
>   	/* 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");
> @@ -1319,13 +1327,6 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
>   
>   	trace_v3d_submit_cpu_ioctl(&v3d->drm, cpu_job->job_type);
>   
> -	ret = v3d_job_init(v3d, file_priv, &cpu_job->base,
> -			   v3d_cpu_job_free, 0, &se, V3D_CPU);
> -	if (ret) {
> -		v3d_job_deallocate((void *)&cpu_job);
> -		goto fail;
> -	}
> -
>   	clean_job = cpu_job->indirect_csd.clean_job;
>   	csd_job = cpu_job->indirect_csd.job;
>   
> @@ -1402,6 +1403,8 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
>   	v3d_job_cleanup((void *)cpu_job);
>   	v3d_job_cleanup((void *)csd_job);
>   	v3d_job_cleanup(clean_job);
> +
> +fail_init:
>   	v3d_put_multisync_post_deps(&se);
>   
>   	return ret;