Re: [PATCH v3 11/14] drm/v3d: Refactor CPU ioctl into unified submission chain

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing validation of `drm_gem_object_lookup()` return value leads to a deterministic kernel crash.
- [Critical] Calling `vmap()` on imported PRIME BOs panics the kernel due to a NULL pages array.
- [High] Calling `drm_sched_job_cleanup()` on jobs that have already been pushed to the DRM scheduler causes a use-after-free and corrupts the scheduler's internal state.
- [High] Implicit BO fences are only added to the last job in a submission, causing earlier GPU jobs to execute prematurely without waiting for synchronization.
--

commit 572410b6778693137514a8800e2af096467fd0aa
Author: Maíra Canal <mcanal@igalia.com>

drm/v3d: Refactor CPU ioctl into unified submission chain

Restructure the CPU ioctl so that all job types, including indirect CSD,
use a single struct v3d_submit chain and a single DRM exec context.

> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index 62c23feb8fbba..1df7ab528422a 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
> @@ -1317,90 +1312,51 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
[ ... ]
> +	/* Look up the CPU jobs' BOs before v3d_setup_csd_jobs_and_bos() appends
> +	 * the CSD and clean jobs in the case of indirect CSD job.
> +	 */
>  	if (args->bo_handle_count) {
>  		ret = v3d_lookup_bos(&submit, args->bo_handles, args->bo_handle_count);

[Severity: High]
This isn't a bug introduced by this patch, but it looks like BOs might be
attached exclusively to the last job in the submission.

v3d_lookup_bos() assigns the user-provided BO array and bo_count to
submit->jobs[submit->job_count - 1]. Later, v3d_submit_lock_reservations()
iterates over the jobs and calls drm_sched_job_add_implicit_dependencies()
only for jobs with bo_count > 0.

Consequently, earlier jobs in the submission, such as the BIN job in a CL
submission or the CSD job in an indirect CSD submission, receive no
implicit dependencies for the BOs they access. Could this allow them to be
scheduled and executed immediately, racing with other processes or queues
currently writing to those BOs on the GPU?

[Severity: Critical]
This is a pre-existing issue, but I am wondering if mapping these BOs properly
handles imported PRIME BOs.

v3d_get_bo_vaddr() maps GEM objects into kernel space by calling
vmap(obj->pages, ...). However, for PRIME BOs imported via
v3d_prime_import_sg_table(), the drm_gem_shmem backend does not populate the
obj->pages array.

By bypassing the drm_gem_shmem_vmap() helper and directly accessing obj->pages,
v3d_get_bo_vaddr() passes NULL as the pages array to vmap(). Can a local
user trigger a kernel panic by importing a DMA-buf, passing it as a buffer to
a CPU job, and causing v3d_get_bo_vaddr() to crash?

>  		if (ret)
>  			goto fail;
[ ... ]
> +	if (cpu_job->job_type == V3D_CPU_JOB_TYPE_INDIRECT_CSD) {
> +		ret = v3d_setup_csd_jobs_and_bos(&submit, &cpu_job->indirect_csd.args,
> +						 NULL);

[Severity: Critical]
This is a pre-existing issue, but shouldn't the return value of
drm_gem_object_lookup() be validated when processing the indirect CSD params?

In v3d_get_cpu_indirect_csd_params(), drm_gem_object_lookup() is called
with a user-provided handle and the return value is assigned directly to
info->indirect without checking for NULL.

If userspace passes an invalid handle, NULL is stored. Later, when the CPU job
executes, v3d_rewrite_csd_job_wg_counts_from_indirect() calls
v3d_get_bo_vaddr(to_v3d_bo(indirect_csd->indirect)). 

Would this pass a NULL pointer to v3d_get_bo_vaddr(), which unconditionally
accesses obj->pages, resulting in a deterministic kernel panic?

[ ... ]
> +	ret = v3d_submit_jobs(&submit, 0, &se);
> +	if (ret)
> +		goto fail_unreserve;
>  
>  	return 0;
>  
>  fail_unreserve:
>  	v3d_submit_unlock_reservations(&submit);
>  fail:
>  	v3d_submit_cleanup_jobs(&submit);

[Severity: High]
This isn't a bug introduced by this patch, but does calling
v3d_submit_cleanup_jobs() here clean up jobs that have already been pushed?

In v3d_submit_jobs(), jobs are pushed to the DRM scheduler via v3d_push_job().
If a subsequent call to drm_sched_job_add_dependency() fails for a later job
in the submission chain, the function returns an error without unwinding the
already-pushed jobs.

The caller then branches to this error path that invokes
v3d_submit_cleanup_jobs(), which iterates over all jobs and calls
v3d_job_cleanup() -> drm_sched_job_cleanup().

Calling drm_sched_job_cleanup() on a job that the scheduler already owns
violates the scheduler API contract. It sets job->s_fence = NULL and destroys
the dependencies XArray while the scheduler thread might be executing the job.
Can this lead to a NULL pointer dereference and memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-v3d-sched-misc-fixes-v3-0-d7114bba55a0@igalia.com?part=11