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

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

[Jeongjun Park]

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
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@kernel.org
Fixes: aafc1a2bea67 ("drm/v3d: Add a CPU job submission")
Signed-off-by: Jeongjun Park <aha310510@gmail.com>
---
 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;
-- 

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@kernel.org
> Fixes: aafc1a2bea67 ("drm/v3d: Add a CPU job submission")
> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> ---
>   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;

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

[Jeongjun Park]

Hi Maíra,

Maíra Canal <mcanal@igalia.com> wrote:
>
> 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?
>

Because according to the implementation of v3d_submit_cpu_ioctl(),
v3d_job_cleanup() should never be called when v3d_job_init() fails to
execute successfully, but it is currently being called.

Therefore, I called the init function before the job_type check and added
the missing v3d_job_deallocate(), which should be called when
v3d_get_extensions() fails. However, upon re-analysis, it appears that
adding the fail_init label is unnecessary now, and I have discovered
another serious issue.

If v3d_job_init() fails after v3d_get_extensions() succeeds, a memory leak
may occur in the memory allocated by v3d_get_extensions(). In such cases,
v3d_job_cleanup() cannot be called, and consequently, v3d_cpu_job_free()
cannot be used either. Therefore, it seems necessary to add a separate
helper function to handle this case.

I will quickly write a v2 patch reflecting these changes.

> Best regards,
> - Maíra
>

Regards,
Jeongjun Park

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

[Maíra Canal]

Hi Jeongjun,

On 02/06/26 12:41, Jeongjun Park wrote:
> Hi Maíra,
> 
> Maíra Canal <mcanal@igalia.com> wrote:
>>
>> 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?
>>
> 
> Because according to the implementation of v3d_submit_cpu_ioctl(),
> v3d_job_cleanup() should never be called when v3d_job_init() fails to
> execute successfully, but it is currently being called.
> 
> Therefore, I called the init function before the job_type check and added
> the missing v3d_job_deallocate(), which should be called when
> v3d_get_extensions() fails. However, upon re-analysis, it appears that
> adding the fail_init label is unnecessary now, and I have discovered
> another serious issue.
> 
> If v3d_job_init() fails after v3d_get_extensions() succeeds, a memory leak
> may occur in the memory allocated by v3d_get_extensions(). In such cases,
> v3d_job_cleanup() cannot be called, and consequently, v3d_cpu_job_free()
> cannot be used either. Therefore, it seems necessary to add a separate
> helper function to handle this case.
> 
> I will quickly write a v2 patch reflecting these changes.

I wouldn't worry a lot about this particular issue, as I already
addressed it in [1].

[1] 
https://lore.kernel.org/dri-devel/20260510-v3d-sched-misc-fixes-v2-0-ca4aba343ef6@igalia.com/T/

Best regards,
- Maíra

> 
>> Best regards,
>> - Maíra
>>
> 
> Regards,
> Jeongjun Park