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

[Jeongjun Park]

Hi Maíra,

Maíra Canal <mcanal@xxxxxxxxxx> 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