[PATCH 0/3] drm/v3d: Miscellaneous fixes

[PATCH 0/3] drm/v3d: Miscellaneous fixes

[Maíra Canal]

Being brief, this small series collects a correctness fix (#1) reported by
Sashiko, a error-path fix (#2) reported by Sashiko, and a cleanup (#3)
found while reading the code.

The first patch is the only notable one: a v3d submission expands into a
chain of jobs (e.g. BIN + RENDER + CACHE CLEAN), but the BO list was only
ever attached to the last job. Since implicit synchronization is added
per-job and gated on job->bo_count, every job before the last one silently
skipped implicit dependencies. In practice the binning job could read a
buffer object while another context was still writing it. It's fixed by
looking up the BOs onto every job that references them.

Best regards,
- Maíra

---
Maíra Canal (3):
      drm/v3d: Associate BOs with every job that accesses them
      drm/v3d: Reject invalid indirect BO handle in indirect CSD setup
      drm/v3d: Use write_to_buffer() helper in performance query copy

 drivers/gpu/drm/v3d/v3d_sched.c  | 11 ++--------
 drivers/gpu/drm/v3d/v3d_submit.c | 43 ++++++++++++++++++++++------------------
 2 files changed, 26 insertions(+), 28 deletions(-)
---
base-commit: 2df5efb45425b48fbf5a3c1f36a686af26871009
change-id: 20260609-v3d-cpu-job-fixes-fbe2051bee3a

[PATCH 1/3] drm/v3d: Associate BOs with every job that accesses them

[Maíra Canal]

A submission can expand into a chain of jobs (e.g. bin + render + cache
clean), but v3d_lookup_bos() only looked up the user's BO list onto the
*last* job of the submission. Every earlier job was left with
bo_count == 0 and an empty bo[] array.

As a consequence, when implicit synchronization happens in
v3d_submit_lock_reservations(), earlier jobs get no implicit
dependencies at all, as the loop is gated on job->bo_count and earlier
jobs don't have any BO attached to them. With that, the BIN job reads the
same buffers as the RENDER job, yet nothing made it wait for a prior
writer to finish. The BIN job could therefore be dispatched to the
hardware and read a BO while another context was still writing it,
leading to data corruption that was only avoided as the userspace adds
explicit syncobjs.

Fix this by calling v3d_lookup_bos() for each job that references the
submission's BOs, so every job carries its own bo[]/bo_count and picks
up the correct implicit dependencies during reservation locking.

Fixes: dffa9b7a78c4 ("drm/v3d: Add missing implicit synchronization.")
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_submit.c | 41 +++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index ee2ac2540ed5..3d6582dfb1bf 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -68,7 +68,6 @@ v3d_submit_unlock_reservations(struct v3d_submit *submit)
 /**
  * v3d_lookup_bos() - Sets up job->bo[] with the GEM objects
  * referenced by the job.
- * @dev: DRM device
  * @file_priv: DRM file for this fd
  * @job: V3D job being set up
  * @bo_handles: GEM handles
@@ -82,23 +81,19 @@ v3d_submit_unlock_reservations(struct v3d_submit *submit)
  * failure, because that will happen at `v3d_job_free()`.
  */
 static int
-v3d_lookup_bos(struct v3d_submit *submit, u64 bo_handles, u32 bo_count)
+v3d_lookup_bos(struct drm_file *file_priv, struct v3d_job *job,
+	       u64 bo_handles, u32 bo_count)
 {
-	struct v3d_job *last_job = submit->jobs[submit->job_count - 1];
-
-	last_job->bo_count = bo_count;
-
-	if (!last_job->bo_count) {
-		/* See comment on bo_index for why we have to check
-		 * this.
-		 */
-		drm_warn(&submit->v3d->drm, "Rendering requires BOs\n");
+	if (!bo_count) {
+		drm_warn(&job->v3d->drm, "Rendering requires BOs\n");
 		return -EINVAL;
 	}
 
-	return drm_gem_objects_lookup(submit->file_priv,
+	job->bo_count = bo_count;
+
+	return drm_gem_objects_lookup(file_priv,
 				      (void __user *)(uintptr_t)bo_handles,
-				      last_job->bo_count, &last_job->bo);
+				      job->bo_count, &job->bo);
 }
 
 static void
@@ -446,7 +441,8 @@ v3d_setup_csd_jobs_and_bos(struct v3d_submit *submit,
 	if (IS_ERR(clean_job))
 		return PTR_ERR(clean_job);
 
-	return v3d_lookup_bos(submit, args->bo_handles, args->bo_handle_count);
+	return v3d_lookup_bos(submit->file_priv, &job->base,
+			      args->bo_handles, args->bo_handle_count);
 }
 
 static void
@@ -1066,6 +1062,11 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 					   &se);
 		if (ret)
 			goto fail;
+
+		ret = v3d_lookup_bos(submit.file_priv, &bin->base,
+				     args->bo_handles, args->bo_handle_count);
+		if (ret)
+			goto fail;
 	}
 
 	render = (struct v3d_render_job *)v3d_submit_add_job(&submit, V3D_RENDER);
@@ -1085,6 +1086,11 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto fail;
 
+	ret = v3d_lookup_bos(submit.file_priv, &render->base,
+			     args->bo_handles, args->bo_handle_count);
+	if (ret)
+		goto fail;
+
 	if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) {
 		clean_job = v3d_submit_add_job(&submit, V3D_CACHE_CLEAN);
 		if (IS_ERR(clean_job)) {
@@ -1097,10 +1103,6 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto fail;
 
-	ret = v3d_lookup_bos(&submit, args->bo_handles, args->bo_handle_count);
-	if (ret)
-		goto fail;
-
 	ret = v3d_submit_lock_reservations(&submit);
 	if (ret)
 		goto fail;
@@ -1359,7 +1361,8 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 	 * 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);
+		ret = v3d_lookup_bos(submit.file_priv, &cpu_job->base,
+				     args->bo_handles, args->bo_handle_count);
 		if (ret)
 			goto fail;
 	}

-- 
2.54.0

Re: [PATCH 1/3] drm/v3d: Associate BOs with every job that accesses them

[Iago Toral]

Hi Maíra,

I have a couple of thoughts here:

1. The original code seems to very specifica about targeting this only
for the last job, which makes it look like a very intentional decision,
so I wonder if there is something we are missing here. I am adding Emma
to the CC in case she has any thoughts about it.

2. I am not aware of any issues despite not having implicit sync for
all the jobs in the chanin. Like you mention, this is because user-
space is already trying to handle job depedencies, so I wonder if we
should instead work on the opposite direction and try to drop implicit
sync in the kernel entirely. Would that make sense? What are other
drivers doing in this regard?

As for the patch itself, I have a comment below:

El mié, 10-06-2026 a las 19:50 -0300, Maíra Canal escribió:
> A submission can expand into a chain of jobs (e.g. bin + render +
> cache
> clean), but v3d_lookup_bos() only looked up the user's BO list onto
> the
> *last* job of the submission. Every earlier job was left with
> bo_count == 0 and an empty bo[] array.
> 
> As a consequence, when implicit synchronization happens in
> v3d_submit_lock_reservations(), earlier jobs get no implicit
> dependencies at all, as the loop is gated on job->bo_count and
> earlier
> jobs don't have any BO attached to them. With that, the BIN job reads
> the
> same buffers as the RENDER job, yet nothing made it wait for a prior
> writer to finish. The BIN job could therefore be dispatched to the
> hardware and read a BO while another context was still writing it,
> leading to data corruption that was only avoided as the userspace
> adds
> explicit syncobjs.
> 
> Fix this by calling v3d_lookup_bos() for each job that references the
> submission's BOs, so every job carries its own bo[]/bo_count and
> picks
> up the correct implicit dependencies during reservation locking.
> 
> Fixes: dffa9b7a78c4 ("drm/v3d: Add missing implicit
> synchronization.")
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/v3d/v3d_submit.c | 41 +++++++++++++++++++++---------
> ----------
>  1 file changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c
> b/drivers/gpu/drm/v3d/v3d_submit.c
> index ee2ac2540ed5..3d6582dfb1bf 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
> @@ -68,7 +68,6 @@ v3d_submit_unlock_reservations(struct v3d_submit
> *submit)
>  /**
>   * v3d_lookup_bos() - Sets up job->bo[] with the GEM objects
>   * referenced by the job.
> - * @dev: DRM device
>   * @file_priv: DRM file for this fd
>   * @job: V3D job being set up
>   * @bo_handles: GEM handles
> @@ -82,23 +81,19 @@ v3d_submit_unlock_reservations(struct v3d_submit
> *submit)
>   * failure, because that will happen at `v3d_job_free()`.
>   */
>  static int
> -v3d_lookup_bos(struct v3d_submit *submit, u64 bo_handles, u32
> bo_count)
> +v3d_lookup_bos(struct drm_file *file_priv, struct v3d_job *job,
> +	       u64 bo_handles, u32 bo_count)
>  {

Wouldn't it make more sense to have this function take the submit like
it did originally and loop through all the jobs in a chain instead of
calling this multiple times for each job in a chain?

> -	struct v3d_job *last_job = submit->jobs[submit->job_count -
> 1];
> -
> -	last_job->bo_count = bo_count;
> -
> -	if (!last_job->bo_count) {
> -		/* See comment on bo_index for why we have to check
> -		 * this.
> -		 */
> -		drm_warn(&submit->v3d->drm, "Rendering requires
> BOs\n");
> +	if (!bo_count) {
> +		drm_warn(&job->v3d->drm, "Rendering requires
> BOs\n");
>  		return -EINVAL;
>  	}
>  
> -	return drm_gem_objects_lookup(submit->file_priv,
> +	job->bo_count = bo_count;
> +
> +	return drm_gem_objects_lookup(file_priv,
>  				      (void __user
> *)(uintptr_t)bo_handles,
> -				      last_job->bo_count, &last_job-
> >bo);
> +				      job->bo_count, &job->bo);
>  }
>  
>  static void
> @@ -446,7 +441,8 @@ v3d_setup_csd_jobs_and_bos(struct v3d_submit
> *submit,
>  	if (IS_ERR(clean_job))
>  		return PTR_ERR(clean_job);
>  
> -	return v3d_lookup_bos(submit, args->bo_handles, args-
> >bo_handle_count);
> +	return v3d_lookup_bos(submit->file_priv, &job->base,
> +			      args->bo_handles, args-
> >bo_handle_count);
>  }
>  
>  static void
> @@ -1066,6 +1062,11 @@ v3d_submit_cl_ioctl(struct drm_device *dev,
> void *data,
>  					   &se);
>  		if (ret)
>  			goto fail;
> +
> +		ret = v3d_lookup_bos(submit.file_priv, &bin->base,
> +				     args->bo_handles, args-
> >bo_handle_count);
> +		if (ret)
> +			goto fail;
>  	}
>  
>  	render = (struct v3d_render_job
> *)v3d_submit_add_job(&submit, V3D_RENDER);
> @@ -1085,6 +1086,11 @@ v3d_submit_cl_ioctl(struct drm_device *dev,
> void *data,
>  	if (ret)
>  		goto fail;
>  
> +	ret = v3d_lookup_bos(submit.file_priv, &render->base,
> +			     args->bo_handles, args-
> >bo_handle_count);
> +	if (ret)
> +		goto fail;
> +
>  	if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) {
>  		clean_job = v3d_submit_add_job(&submit,
> V3D_CACHE_CLEAN);
>  		if (IS_ERR(clean_job)) {
> @@ -1097,10 +1103,6 @@ v3d_submit_cl_ioctl(struct drm_device *dev,
> void *data,
>  	if (ret)
>  		goto fail;
>  
> -	ret = v3d_lookup_bos(&submit, args->bo_handles, args-
> >bo_handle_count);
> -	if (ret)
> -		goto fail;
> -
>  	ret = v3d_submit_lock_reservations(&submit);
>  	if (ret)
>  		goto fail;
> @@ -1359,7 +1361,8 @@ v3d_submit_cpu_ioctl(struct drm_device *dev,
> void *data,
>  	 * 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);
> +		ret = v3d_lookup_bos(submit.file_priv, &cpu_job-
> >base,
> +				     args->bo_handles, args-
> >bo_handle_count);
>  		if (ret)
>  			goto fail;
>  	}
> 

Re: [PATCH 1/3] drm/v3d: Associate BOs with every job that accesses them

[Maíra Canal]

Hi Iago,

On 11/06/26 03:17, Iago Toral wrote:
> Hi Maíra,
> 
> I have a couple of thoughts here:
> 
> 1. The original code seems to very specifica about targeting this only
> for the last job, which makes it look like a very intentional decision,
> so I wonder if there is something we are missing here. I am adding Emma
> to the CC in case she has any thoughts about it.
> 

I have the impression that this is a consequence of several design
changes that the driver passed through the years. Initially, the v3d
driver only had a CL submission and attaching BOs to the last job meant
attaching to the render job. This was fine, only considering the caveat
mentioned in commit dffa9b7a78c4 "drm/v3d: Add missing implicit
synchronization": "we currently only implicitly sync for the rendering
pass, not binning -- if you texture-from-pixmap in the binning vertex
shader you'll miss out on synchronization."

However, with time, we added the CACHE_CLEAN flag to the CL submission
(so the last job could be a CACHE_CLEAN job) and we added CSD and TFU
submissions. Then, attaching the BOs to the last job wasn't the correct
approach anymore.

> 2. I am not aware of any issues despite not having implicit sync for
> all the jobs in the chanin. Like you mention, this is because user-
> space is already trying to handle job depedencies, so I wonder if we
> should instead work on the opposite direction and try to drop implicit
> sync in the kernel entirely. Would that make sense? What are other
> drivers doing in this regard?
> 

Maybe some one who has been around for longer could bring a historical
overview about this topic. From my understand, I don't think we can drop
implicit sync because it's part of the dma-buf synchronization contract.
If I'm not mistaken, v3d_submit_attach_object_fences() publishes our
fence so a consumer (e.g. compositor, KMS) can wait on us, and
drm_sched_job_add_implicit_dependencies() makes us wait on other actors'
fences already on the buffer. Mesa only syncs within its own context,
but it doesn't sync shared buffers cross-processes.

We can think about creating a EXPLICIT_SYNC flag for the BOs in the
future. We can talk more about this idea privately.

> As for the patch itself, I have a comment below:
> 
> El mié, 10-06-2026 a las 19:50 -0300, Maíra Canal escribió:
>> A submission can expand into a chain of jobs (e.g. bin + render +
>> cache
>> clean), but v3d_lookup_bos() only looked up the user's BO list onto
>> the
>> *last* job of the submission. Every earlier job was left with
>> bo_count == 0 and an empty bo[] array.
>>
>> As a consequence, when implicit synchronization happens in
>> v3d_submit_lock_reservations(), earlier jobs get no implicit
>> dependencies at all, as the loop is gated on job->bo_count and
>> earlier
>> jobs don't have any BO attached to them. With that, the BIN job reads
>> the
>> same buffers as the RENDER job, yet nothing made it wait for a prior
>> writer to finish. The BIN job could therefore be dispatched to the
>> hardware and read a BO while another context was still writing it,
>> leading to data corruption that was only avoided as the userspace
>> adds
>> explicit syncobjs.
>>
>> Fix this by calling v3d_lookup_bos() for each job that references the
>> submission's BOs, so every job carries its own bo[]/bo_count and
>> picks
>> up the correct implicit dependencies during reservation locking.
>>
>> Fixes: dffa9b7a78c4 ("drm/v3d: Add missing implicit
>> synchronization.")
>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>> ---
>>   drivers/gpu/drm/v3d/v3d_submit.c | 41 +++++++++++++++++++++---------
>> ----------
>>   1 file changed, 22 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c
>> b/drivers/gpu/drm/v3d/v3d_submit.c
>> index ee2ac2540ed5..3d6582dfb1bf 100644
>> --- a/drivers/gpu/drm/v3d/v3d_submit.c
>> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
>> @@ -68,7 +68,6 @@ v3d_submit_unlock_reservations(struct v3d_submit
>> *submit)
>>   /**
>>    * v3d_lookup_bos() - Sets up job->bo[] with the GEM objects
>>    * referenced by the job.
>> - * @dev: DRM device
>>    * @file_priv: DRM file for this fd
>>    * @job: V3D job being set up
>>    * @bo_handles: GEM handles
>> @@ -82,23 +81,19 @@ v3d_submit_unlock_reservations(struct v3d_submit
>> *submit)
>>    * failure, because that will happen at `v3d_job_free()`.
>>    */
>>   static int
>> -v3d_lookup_bos(struct v3d_submit *submit, u64 bo_handles, u32
>> bo_count)
>> +v3d_lookup_bos(struct drm_file *file_priv, struct v3d_job *job,
>> +	       u64 bo_handles, u32 bo_count)
>>   {
> 
> Wouldn't it make more sense to have this function take the submit like
> it did originally and loop through all the jobs in a chain instead of
> calling this multiple times for each job in a chain?

We don't want to attach the BOs to all the jobs in the chain. For
example, I don't believe we would want to attach BOs to a CLEAN_CACHE
job.

Best regards,
- Maíra

Re: [PATCH 1/3] drm/v3d: Associate BOs with every job that accesses them

[Emma Anholt]

On 2026-06-10 23:17, Iago Toral wrote:
> Hi Maíra,
> 
> I have a couple of thoughts here:
> 
> 1. The original code seems to very specifica about targeting this only
> for the last job, which makes it look like a very intentional decision,
> so I wonder if there is something we are missing here. I am adding Emma
> to the CC in case she has any thoughts about it.
> 
> 2. I am not aware of any issues despite not having implicit sync for
> all the jobs in the chanin. Like you mention, this is because user-
> space is already trying to handle job depedencies, so I wonder if we
> should instead work on the opposite direction and try to drop implicit
> sync in the kernel entirely. Would that make sense? What are other
> drivers doing in this regard?

Yeah, you should be working toward dropping implicit sync entirely. 
tu's VM_BIND is really nice and where you want to be eventually, but
even if not, then you at least want something like
MSM_SUBMIT_NO_IMPLICIT.

I probably just didn't think about this implicit sync case in the
initial implementation, because a bin job dependency on results from
another context is just not a thing that's going to happen in practice. 
The implicit sync was mostly about render targets / texturing to be
ordered correctly with X11, and about lifetime management of the BO
(which only needed the last use).

> As for the patch itself, I have a comment below:
> 
> El mié, 10-06-2026 a las 19:50 -0300, Maíra Canal escribió:
>> A submission can expand into a chain of jobs (e.g. bin + render +
>> cache
>> clean), but v3d_lookup_bos() only looked up the user's BO list onto
>> the
>> *last* job of the submission. Every earlier job was left with
>> bo_count == 0 and an empty bo[] array.
>> 
>> As a consequence, when implicit synchronization happens in
>> v3d_submit_lock_reservations(), earlier jobs get no implicit
>> dependencies at all, as the loop is gated on job->bo_count and
>> earlier
>> jobs don't have any BO attached to them. With that, the BIN job reads
>> the
>> same buffers as the RENDER job, yet nothing made it wait for a prior
>> writer to finish. The BIN job could therefore be dispatched to the
>> hardware and read a BO while another context was still writing it,
>> leading to data corruption that was only avoided as the userspace
>> adds
>> explicit syncobjs.
>> 
>> Fix this by calling v3d_lookup_bos() for each job that references the
>> submission's BOs, so every job carries its own bo[]/bo_count and
>> picks
>> up the correct implicit dependencies during reservation locking.
>> 
>> Fixes: dffa9b7a78c4 ("drm/v3d: Add missing implicit
>> synchronization.")
>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>> ---
>>  drivers/gpu/drm/v3d/v3d_submit.c | 41 +++++++++++++++++++++---------
>> ----------
>>  1 file changed, 22 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c
>> b/drivers/gpu/drm/v3d/v3d_submit.c
>> index ee2ac2540ed5..3d6582dfb1bf 100644
>> --- a/drivers/gpu/drm/v3d/v3d_submit.c
>> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
>> @@ -68,7 +68,6 @@ v3d_submit_unlock_reservations(struct v3d_submit
>> *submit)
>>  /**
>>   * v3d_lookup_bos() - Sets up job->bo[] with the GEM objects
>>   * referenced by the job.
>> - * @dev: DRM device
>>   * @file_priv: DRM file for this fd
>>   * @job: V3D job being set up
>>   * @bo_handles: GEM handles
>> @@ -82,23 +81,19 @@ v3d_submit_unlock_reservations(struct v3d_submit
>> *submit)
>>   * failure, because that will happen at `v3d_job_free()`.
>>   */
>>  static int
>> -v3d_lookup_bos(struct v3d_submit *submit, u64 bo_handles, u32
>> bo_count)
>> +v3d_lookup_bos(struct drm_file *file_priv, struct v3d_job *job,
>> +	       u64 bo_handles, u32 bo_count)
>>  {
> 
> Wouldn't it make more sense to have this function take the submit like
> it did originally and loop through all the jobs in a chain instead of
> calling this multiple times for each job in a chain?
> 
>> -	struct v3d_job *last_job = submit->jobs[submit->job_count -
>> 1];
>> -
>> -	last_job->bo_count = bo_count;
>> -
>> -	if (!last_job->bo_count) {
>> -		/* See comment on bo_index for why we have to check
>> -		 * this.
>> -		 */
>> -		drm_warn(&submit->v3d->drm, "Rendering requires
>> BOs\n");
>> +	if (!bo_count) {
>> +		drm_warn(&job->v3d->drm, "Rendering requires
>> BOs\n");
>>  		return -EINVAL;
>>  	}
>>  
>> -	return drm_gem_objects_lookup(submit->file_priv,
>> +	job->bo_count = bo_count;
>> +
>> +	return drm_gem_objects_lookup(file_priv,
>>  				      (void __user
>> *)(uintptr_t)bo_handles,
>> -				      last_job->bo_count, &last_job-
>> >bo);
>> +				      job->bo_count, &job->bo);
>>  }
>>  
>>  static void
>> @@ -446,7 +441,8 @@ v3d_setup_csd_jobs_and_bos(struct v3d_submit
>> *submit,
>>  	if (IS_ERR(clean_job))
>>  		return PTR_ERR(clean_job);
>>  
>> -	return v3d_lookup_bos(submit, args->bo_handles, args-
>> >bo_handle_count);
>> +	return v3d_lookup_bos(submit->file_priv, &job->base,
>> +			      args->bo_handles, args-
>> >bo_handle_count);
>>  }
>>  
>>  static void
>> @@ -1066,6 +1062,11 @@ v3d_submit_cl_ioctl(struct drm_device *dev,
>> void *data,
>>  					   &se);
>>  		if (ret)
>>  			goto fail;
>> +
>> +		ret = v3d_lookup_bos(submit.file_priv, &bin->base,
>> +				     args->bo_handles, args-
>> >bo_handle_count);
>> +		if (ret)
>> +			goto fail;
>>  	}
>>  
>>  	render = (struct v3d_render_job
>> *)v3d_submit_add_job(&submit, V3D_RENDER);
>> @@ -1085,6 +1086,11 @@ v3d_submit_cl_ioctl(struct drm_device *dev,
>> void *data,
>>  	if (ret)
>>  		goto fail;
>>  
>> +	ret = v3d_lookup_bos(submit.file_priv, &render->base,
>> +			     args->bo_handles, args-
>> >bo_handle_count);
>> +	if (ret)
>> +		goto fail;
>> +
>>  	if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) {
>>  		clean_job = v3d_submit_add_job(&submit,
>> V3D_CACHE_CLEAN);
>>  		if (IS_ERR(clean_job)) {
>> @@ -1097,10 +1103,6 @@ v3d_submit_cl_ioctl(struct drm_device *dev,
>> void *data,
>>  	if (ret)
>>  		goto fail;
>>  
>> -	ret = v3d_lookup_bos(&submit, args->bo_handles, args-
>> >bo_handle_count);
>> -	if (ret)
>> -		goto fail;
>> -
>>  	ret = v3d_submit_lock_reservations(&submit);
>>  	if (ret)
>>  		goto fail;
>> @@ -1359,7 +1361,8 @@ v3d_submit_cpu_ioctl(struct drm_device *dev,
>> void *data,
>>  	 * 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);
>> +		ret = v3d_lookup_bos(submit.file_priv, &cpu_job-
>> >base,
>> +				     args->bo_handles, args-
>> >bo_handle_count);
>>  		if (ret)
>>  			goto fail;
>>  	}
>>

Re: [PATCH 1/3] drm/v3d: Associate BOs with every job that accesses them

[Maíra Canal]

Hi Emma,

On 12/06/26 14:44, Emma Anholt wrote:
> On 2026-06-10 23:17, Iago Toral wrote:
>> Hi Maíra,
>>
>> I have a couple of thoughts here:
>>
>> 1. The original code seems to very specifica about targeting this only
>> for the last job, which makes it look like a very intentional decision,
>> so I wonder if there is something we are missing here. I am adding Emma
>> to the CC in case she has any thoughts about it.
>>
>> 2. I am not aware of any issues despite not having implicit sync for
>> all the jobs in the chanin. Like you mention, this is because user-
>> space is already trying to handle job depedencies, so I wonder if we
>> should instead work on the opposite direction and try to drop implicit
>> sync in the kernel entirely. Would that make sense? What are other
>> drivers doing in this regard?
> 
> Yeah, you should be working toward dropping implicit sync entirely.
> tu's VM_BIND is really nice and where you want to be eventually, but
> even if not, then you at least want something like
> MSM_SUBMIT_NO_IMPLICIT.
> 

I believe the main issue we would have to implement something like
VM_BIND is the fact that V3D only has one global page table (that we
could switch on runtime, but it would come with the cost that all the HW
queues would only be able to operate on BOs from one client).

NO_IMPLICIT would be a more reasonable option.

> I probably just didn't think about this implicit sync case in the
> initial implementation, because a bin job dependency on results from
> another context is just not a thing that's going to happen in practice.
> The implicit sync was mostly about render targets / texturing to be
> ordered correctly with X11, and about lifetime management of the BO
> (which only needed the last use).
> 

But does it make sense to attach the implicit dependencies to the
CACHE_CLEAN job (last job) and not to the RENDER job or the CSD job?

Best regards,
- Maíra

Re: [PATCH 1/3] drm/v3d: Associate BOs with every job that accesses them

[Emma Anholt]

On 2026-06-13 16:22, Maíra Canal wrote:
> Hi Emma,
> 
> On 12/06/26 14:44, Emma Anholt wrote:
>> On 2026-06-10 23:17, Iago Toral wrote:
>>> Hi Maíra,
>>>
>>> I have a couple of thoughts here:
>>>
>>> 1. The original code seems to very specifica about targeting this only
>>> for the last job, which makes it look like a very intentional decision,
>>> so I wonder if there is something we are missing here. I am adding Emma
>>> to the CC in case she has any thoughts about it.
>>>
>>> 2. I am not aware of any issues despite not having implicit sync for
>>> all the jobs in the chanin. Like you mention, this is because user-
>>> space is already trying to handle job depedencies, so I wonder if we
>>> should instead work on the opposite direction and try to drop implicit
>>> sync in the kernel entirely. Would that make sense? What are other
>>> drivers doing in this regard?
>> 
>> Yeah, you should be working toward dropping implicit sync entirely.
>> tu's VM_BIND is really nice and where you want to be eventually, but
>> even if not, then you at least want something like
>> MSM_SUBMIT_NO_IMPLICIT.
>> 
> 
> I believe the main issue we would have to implement something like
> VM_BIND is the fact that V3D only has one global page table (that we
> could switch on runtime, but it would come with the cost that all the HW
> queues would only be able to operate on BOs from one client).
> 
> NO_IMPLICIT would be a more reasonable option.

I had forgotten how bad v3d's mmu is, with the contiguous mem
requirement.  I don't think VM_BIND is going to be reasonable -- we
can't guarantee new page tables for other clients, so switching isn't
reasonable.

HW queues only being able to operate on BOs the client has imported is
the right behavior, though!  It's not a requirement from DRM, but it's a
very strong expectation from the software using it.  V3D should really
be using the GMP to mask the clients from each other.
> 
>> I probably just didn't think about this implicit sync case in the
>> initial implementation, because a bin job dependency on results from
>> another context is just not a thing that's going to happen in practice.
>> The implicit sync was mostly about render targets / texturing to be
>> ordered correctly with X11, and about lifetime management of the BO
>> (which only needed the last use).
>> 
> 
> But does it make sense to attach the implicit dependencies to the
> CACHE_CLEAN job (last job) and not to the RENDER job or the CSD job?
> 

You need to make sure that the BO list also gets added to at least the
first job (assuming they're in a dependency chain), because it should
correctly wait to implicit sync against queued writes to those BOs from
other clients.  And, if you ever do shrinker, then each job would need
to know its BOs.

[PATCH 2/3] drm/v3d: Reject invalid indirect BO handle in indirect CSD setup

[Maíra Canal]

v3d_get_cpu_indirect_csd_params() looks up the indirect buffer object
from a userspace-supplied handle but never checks the result. A bogus
or stale handle makes drm_gem_object_lookup() return NULL, which is
then stored in info->indirect and only dereferenced later when the
indirect CSD job runs, turning a userspace mistake into a NULL pointer
dereference in the kernel.

Bail out with -ENOENT as soon as the lookup fails, so the bad handle is
rejected at submission time.

Fixes: 18b8413b25b7 ("drm/v3d: Create a CPU job extension for a indirect CSD job")
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_submit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index 3d6582dfb1bf..122b93eaa8bf 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -601,6 +601,8 @@ v3d_get_cpu_indirect_csd_params(struct drm_file *file_priv,
 	       sizeof(indirect_csd.wg_uniform_offsets));
 
 	info->indirect = drm_gem_object_lookup(file_priv, indirect_csd.indirect);
+	if (!info->indirect)
+		return -ENOENT;
 
 	return 0;
 }

-- 
2.54.0

Re: [PATCH 2/3] drm/v3d: Reject invalid indirect BO handle in indirect CSD setup

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Pre-existing double-free vulnerability in CPU job extension error handling.
- [Critical] Missing bounds checks on userspace-provided offsets in v3d CPU jobs allows arbitrary kernel memory access.
- [High] Missing check for vmap failure when mapping GEM objects for CPU jobs.
--

commit 8d6290edb4b042b28b895df1c1037e680a314bfc
Author: Maíra Canal <mcanal@igalia.com>

drm/v3d: Reject invalid indirect BO handle in indirect CSD setup

This commit fixes a NULL pointer dereference in the v3d driver by rejecting
invalid indirect buffer object handles during indirect CSD job setup. It
returns -ENOENT if drm_gem_object_lookup() fails to find the object.

> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index 3d6582dfb1bfc..122b93eaa8bf2 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
> @@ -601,6 +601,8 @@ v3d_get_cpu_indirect_csd_params(struct drm_file *file_priv,
>  	       sizeof(indirect_csd.wg_uniform_offsets));
>  
>  	info->indirect = drm_gem_object_lookup(file_priv, indirect_csd.indirect);

[Severity: Critical]
This is a pre-existing issue, but does this code lack bounds checking on
the userspace-provided offsets before using them to access kernel memory?

In v3d_get_cpu_indirect_csd_params(), offsets like indirect_csd.offset and
indirect_csd.wg_uniform_offsets are copied from the userspace extension
struct without any validation against the bounds of the GEM object.

When the job executes in v3d_rewrite_csd_job_wg_counts_from_indirect():

	wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset);
	...
	u32 uniform_idx = indirect_csd->wg_uniform_offsets[i];
	((uint32_t *)indirect->vaddr)[uniform_idx] = wg_counts[i];

Can a malicious userspace application supply arbitrarily large offsets to
read from or write to arbitrary kernel memory addresses relative to the
mapped BO? Similar issues seem to exist in v3d_timestamp_query() as well:

	value_addr = ((u8 *)bo->vaddr) + timestamp_query->queries[i].offset;
	*((u64 *)value_addr) = i == 0 ? ktime_get_ns() : 0ull;

[Severity: High]
This isn't a bug introduced by this patch, but does the driver fail to
handle vmap() returning NULL when mapping these GEM objects?

In v3d_get_bo_vaddr():

	bo->vaddr = vmap(obj->pages, obj->base.size >> PAGE_SHIFT, VM_MAP,
			 pgprot_writecombine(PAGE_KERNEL));

If vmap() fails, bo->vaddr is NULL. CPU job execution functions like
v3d_rewrite_csd_job_wg_counts_from_indirect() unconditionally assume
bo->vaddr is valid:

	v3d_get_bo_vaddr(bo);
	v3d_get_bo_vaddr(indirect);
	wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset);

Can this lead to a NULL pointer dereference if the system is under memory
pressure or if a very large BO is submitted?

[Severity: Critical]
This is also a pre-existing issue, but is there a double-free
vulnerability in the CPU job extension error handling?

When parsing CPU job parameters in v3d_get_cpu_timestamp_query_params(),
if a failure occurs, the error path calls:

	error:
		v3d_timestamp_query_info_free(&job->timestamp_query, i);
		return err;

The helper v3d_timestamp_query_info_free() frees the queries array:

	kvfree(query_info->queries);

but it does not set the pointer to NULL.

If the error propagates to v3d_submit_cpu_ioctl(), it jumps to its error
path and calls v3d_submit_cleanup_jobs() -> v3d_cpu_job_free(). This
function unconditionally calls the free helper again:

	v3d_timestamp_query_info_free(&job->timestamp_query,
				      job->timestamp_query.count);

Can this allow a malicious userspace application to trigger a double free
of kernel memory?

> +	if (!info->indirect)
> +		return -ENOENT;
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-v3d-cpu-job-fixes-v1-0-0d9c88989edc@igalia.com?part=2

[PATCH 3/3] drm/v3d: Use write_to_buffer() helper in performance query copy

[Maíra Canal]

The copy of performance query results to the output buffer open-codes
the 32-bit/64-bit selection with two nearly identical loops. As the
write_to_buffer() helper already encapsulates the do_64bit decision,
use it instead of open-coding it.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_sched.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 53deae1a2eda..17751c8d7f16 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -594,15 +594,8 @@ v3d_write_performance_query_result(struct v3d_cpu_job *job, void *data,
 
 		v3d_perfmon_stop(v3d, perfmon, true);
 
-		if (job->copy.do_64bit) {
-			for (j = 0; j < perfmon->ncounters; j++)
-				write_to_buffer_64(data, offset + j,
-						   perfmon->values[j]);
-		} else {
-			for (j = 0; j < perfmon->ncounters; j++)
-				write_to_buffer_32(data, offset + j,
-						   perfmon->values[j]);
-		}
+		for (j = 0; j < perfmon->ncounters; j++)
+			write_to_buffer(data, offset + j, job->copy.do_64bit, perfmon->values[j]);
 
 		v3d_perfmon_put(perfmon);
 	}

-- 
2.54.0

Re: [PATCH 3/3] drm/v3d: Use write_to_buffer() helper in performance query copy

[sashiko-bot]

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

Pre-existing issues:
- [Critical] User-controlled offsets, strides, and counts are used to access Buffer Object (BO) memory without any bounds checking, leading to arbitrary out-of-bounds kernel memory reads and writes.
- [Critical] The function `v3d_get_bo_vaddr()` does not check if `vmap()` returns NULL, which leads to a subsequent NULL pointer dereference.
--

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

drm/v3d: Use write_to_buffer() helper in performance query copy

The copy of performance query results to the output buffer is refactored
to use the write_to_buffer() helper instead of open-coding the
32-bit/64-bit selection. This replaces two nearly identical loops.

> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
> index 53deae1a2eda9..17751c8d7f163 100644
> --- a/drivers/gpu/drm/v3d/v3d_sched.c
> +++ b/drivers/gpu/drm/v3d/v3d_sched.c
> @@ -594,15 +594,8 @@ v3d_write_performance_query_result(struct v3d_cpu_job *job, void *data,
>  
>  		v3d_perfmon_stop(v3d, perfmon, true);
>  
> -		if (job->copy.do_64bit) {
> -			for (j = 0; j < perfmon->ncounters; j++)
> -				write_to_buffer_64(data, offset + j,
> -						   perfmon->values[j]);
> -		} else {
> -			for (j = 0; j < perfmon->ncounters; j++)
> -				write_to_buffer_32(data, offset + j,
> -						   perfmon->values[j]);
> -		}
> +		for (j = 0; j < perfmon->ncounters; j++)
> +			write_to_buffer(data, offset + j, job->copy.do_64bit, perfmon->values[j]);
                        ^^^^^^^^^^^^^^^
[Severity: Critical]
This is a pre-existing issue, but does this write out of bounds?

Userspace submits a CPU job (e.g., V3D_CPU_JOB_TYPE_COPY_PERFORMANCE_QUERY)
providing offset and stride via DRM ioctls. These values are parsed without
being validated against the actual allocated size of the target BO.

In v3d_submit.c:v3d_get_cpu_copy_performance_query_params():
	job->copy.offset = copy.offset;
	job->copy.stride = copy.stride;

The unvalidated offset is used to compute the base write pointer in
v3d_sched.c:v3d_copy_performance_query():
	data = ((u8 *)bo->vaddr) + copy->offset;

This loop then blindly writes to data while incrementing the pointer by
copy->stride without checking boundaries, which can lead to arbitrary
out-of-bounds kernel memory reads and writes. Any unprivileged userspace
process that can open the DRM device can trigger this via
DRM_IOCTL_V3D_SUBMIT_CPU.

A similar unvalidated out-of-bounds access is present in other CPU jobs like
indirect CSD:

v3d_sched.c:v3d_rewrite_csd_job_wg_counts_from_indirect():
	wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset);

>  
>  		v3d_perfmon_put(perfmon);
>  	}

[Severity: Critical]
This is a pre-existing issue, but does v3d_get_bo_vaddr() dereference a NULL
pointer when vmap() fails?

When the system is under memory pressure, vmap() can fail and return NULL.
However, v3d_get_bo_vaddr() assigns this NULL pointer directly to bo->vaddr
without checking:

v3d_bo.c:v3d_get_bo_vaddr():
	bo->vaddr = vmap(obj->pages, obj->base.size >> PAGE_SHIFT, VM_MAP,
			 pgprot_writecombine(PAGE_KERNEL));

Callers compute offsets from bo->vaddr and proceed to read or write to it
without any NULL checks. For example:

v3d_sched.c:v3d_copy_performance_query():
	v3d_get_bo_vaddr(bo);

	data = ((u8 *)bo->vaddr) + copy->offset;

This is reachable by an untrusted userspace program triggering CPU jobs via
DRM ioctls while the system is under memory pressure.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-v3d-cpu-job-fixes-v1-0-0d9c88989edc@igalia.com?part=3

Re: [PATCH 0/3] drm/v3d: Miscellaneous fixes

[Iago Toral]

I had some questions about patch 1, but patches 2 and 3 are:

Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>

El mié, 10-06-2026 a las 19:50 -0300, Maíra Canal escribió:
> Being brief, this small series collects a correctness fix (#1)
> reported by
> Sashiko, a error-path fix (#2) reported by Sashiko, and a cleanup
> (#3)
> found while reading the code.
> 
> The first patch is the only notable one: a v3d submission expands
> into a
> chain of jobs (e.g. BIN + RENDER + CACHE CLEAN), but the BO list was
> only
> ever attached to the last job. Since implicit synchronization is
> added
> per-job and gated on job->bo_count, every job before the last one
> silently
> skipped implicit dependencies. In practice the binning job could read
> a
> buffer object while another context was still writing it. It's fixed
> by
> looking up the BOs onto every job that references them.
> 
> Best regards,
> - Maíra
> 
> ---
> Maíra Canal (3):
>       drm/v3d: Associate BOs with every job that accesses them
>       drm/v3d: Reject invalid indirect BO handle in indirect CSD
> setup
>       drm/v3d: Use write_to_buffer() helper in performance query copy
> 
>  drivers/gpu/drm/v3d/v3d_sched.c  | 11 ++--------
>  drivers/gpu/drm/v3d/v3d_submit.c | 43 ++++++++++++++++++++++--------
> ----------
>  2 files changed, 26 insertions(+), 28 deletions(-)
> ---
> base-commit: 2df5efb45425b48fbf5a3c1f36a686af26871009
> change-id: 20260609-v3d-cpu-job-fixes-fbe2051bee3a
> 
>