* [PATCH v2 0/2] drm/v3d: Fix indirect CSD jobs with zeroed workgroups
@ 2026-06-01 19:13 Maíra Canal
2026-06-01 19:13 ` [PATCH v2 1/2] drm/v3d: Fix vaddr leak when indirect CSD has " Maíra Canal
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Maíra Canal @ 2026-06-01 19:13 UTC (permalink / raw)
To: Melissa Wen, Iago Toral Quiroga, Jose Maria Casanova Crespo
Cc: kernel-dev, dri-devel, stable, Maíra Canal
Hi,
Indirect CSD lets userspace defer the workgroup counts to a GPU buffer
that is only filled at runtime, so the counts are unknown at submission
time and can legitimately turn out to be zero.
However, exercing this case exposed two issues in the CSD path.
1. Virtual address leaks when the indirect CSD has zeroed workgroups.
2. CSD jobs with zeroed workgroups shouldn't be submitted to hardware.
This series intends to address both issues.
Best regards,
- Maíra
---
v1 -> v2: https://lore.kernel.org/r/20260530-v3d-fix-indirect-csd-v1-0-15533948663f@igalia.com
- [2/2] Don't check the whole cfg[0-2], check only the number of workgroups (Iago Toral)
- [2/2] Add a comment about how the HW interprets 0 (Iago Toral)
---
Maíra Canal (2):
drm/v3d: Fix vaddr leak when indirect CSD has zeroed workgroups
drm/v3d: Skip CSD when it has zeroed workgroups
drivers/gpu/drm/v3d/v3d_sched.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
---
base-commit: 5ab62dd3687bcc2cc542b99385aabac5c996db6f
change-id: 20260516-v3d-fix-indirect-csd-6806cf20a57a
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v2 1/2] drm/v3d: Fix vaddr leak when indirect CSD has zeroed workgroups
2026-06-01 19:13 [PATCH v2 0/2] drm/v3d: Fix indirect CSD jobs with zeroed workgroups Maíra Canal
@ 2026-06-01 19:13 ` Maíra Canal
2026-06-04 3:44 ` Claude review: " Claude Code Review Bot
2026-06-01 19:13 ` [PATCH v2 2/2] drm/v3d: Skip CSD when it " Maíra Canal
2026-06-04 3:44 ` Claude review: drm/v3d: Fix indirect CSD jobs with " Claude Code Review Bot
2 siblings, 1 reply; 10+ messages in thread
From: Maíra Canal @ 2026-06-01 19:13 UTC (permalink / raw)
To: Melissa Wen, Iago Toral Quiroga, Jose Maria Casanova Crespo
Cc: kernel-dev, dri-devel, stable, Maíra Canal
v3d_rewrite_csd_job_wg_counts_from_indirect() maps both the indirect
buffer and the workgroup buffer and is expected to release them before
returning. When any of the workgroup counts read from the buffer is zero,
the function bailed out early and skipped the cleanup, leaking the vaddr
mappings of both BOs.
Jump to the cleanup path instead of returning directly, so the mappings
are always dropped.
Cc: stable@vger.kernel.org
Fixes: 18b8413b25b7 ("drm/v3d: Create a CPU job extension for a indirect CSD job")
Suggested-by: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
drivers/gpu/drm/v3d/v3d_sched.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 94bf628dc91c..47f83936cd73 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -403,7 +403,7 @@ v3d_rewrite_csd_job_wg_counts_from_indirect(struct v3d_cpu_job *job)
wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset);
if (wg_counts[0] == 0 || wg_counts[1] == 0 || wg_counts[2] == 0)
- return;
+ goto unmap_bo;
args->cfg[0] = wg_counts[0] << V3D_CSD_CFG012_WG_COUNT_SHIFT;
args->cfg[1] = wg_counts[1] << V3D_CSD_CFG012_WG_COUNT_SHIFT;
@@ -428,6 +428,7 @@ v3d_rewrite_csd_job_wg_counts_from_indirect(struct v3d_cpu_job *job)
}
}
+unmap_bo:
v3d_put_bo_vaddr(indirect);
v3d_put_bo_vaddr(bo);
}
--
2.54.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Claude review: drm/v3d: Fix vaddr leak when indirect CSD has zeroed workgroups
2026-06-01 19:13 ` [PATCH v2 1/2] drm/v3d: Fix vaddr leak when indirect CSD has " Maíra Canal
@ 2026-06-04 3:44 ` Claude Code Review Bot
0 siblings, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 3:44 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Status: Good**
This patch fixes a straightforward resource leak. In `v3d_rewrite_csd_job_wg_counts_from_indirect()`, two BOs are mapped at the top of the function:
```c
v3d_get_bo_vaddr(bo);
v3d_get_bo_vaddr(indirect);
```
When any workgroup count is zero, the early `return` at line 406 skips the cleanup:
```c
if (wg_counts[0] == 0 || wg_counts[1] == 0 || wg_counts[2] == 0)
return; // leaked both vaddr mappings
```
The fix correctly changes `return` to `goto unmap_bo`, jumping to the existing cleanup that calls `v3d_put_bo_vaddr()` on both BOs. The label placement is correct — it's right before both `v3d_put_bo_vaddr` calls at the end of the function.
One minor observation: the `goto` label name `unmap_bo` references only one BO, but both `indirect` and `bo` are unmapped there. A name like `unmap` would be slightly more precise, but this is a very minor style nit and not worth a respin given it follows common kernel convention where the label names the first cleanup action.
No issues with this patch.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] drm/v3d: Skip CSD when it has zeroed workgroups
2026-06-01 19:13 [PATCH v2 0/2] drm/v3d: Fix indirect CSD jobs with zeroed workgroups Maíra Canal
2026-06-01 19:13 ` [PATCH v2 1/2] drm/v3d: Fix vaddr leak when indirect CSD has " Maíra Canal
@ 2026-06-01 19:13 ` Maíra Canal
2026-06-02 7:10 ` Iago Toral
2026-06-04 3:44 ` Claude review: " Claude Code Review Bot
2026-06-04 3:44 ` Claude review: drm/v3d: Fix indirect CSD jobs with " Claude Code Review Bot
2 siblings, 2 replies; 10+ messages in thread
From: Maíra Canal @ 2026-06-01 19:13 UTC (permalink / raw)
To: Melissa Wen, Iago Toral Quiroga, Jose Maria Casanova Crespo
Cc: kernel-dev, dri-devel, stable, Maíra Canal
A compute shader dispatch encodes its workgroup counts in the CFG0..CFG2
registers. Kicking off a dispatch with a zero count in any of the three
dimensions is invalid. First, the hardware will process 0 as 65536,
causing an illegitimate submission. But over that, a submission with a
zeroed workgroup dimension should be a no-op.
These zeroed counts can reach the dispatch path through an indirect CSD
job, whose workgroup counts are only known once the indirect buffer is
read and may legitimately be zero, but such scenario should only result in
a no-op.
Don't submit the job to the hardware when any of the workgroup counts is
zero, so the job completes immediately instead of running the shader.
Cc: stable@vger.kernel.org
Fixes: d223f98f0209 ("drm/v3d: Add support for compute shader dispatch.")
Suggested-by: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
drivers/gpu/drm/v3d/v3d_sched.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 47f83936cd73..681d10af4c8e 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -352,6 +352,15 @@ v3d_csd_job_run(struct drm_sched_job *sched_job)
return NULL;
}
+ /* For dispatch dimensions, HW interprets 0 as 65536, causing
+ * illegitimate submissions that must be rejected. Note that
+ * 65535 (2^16 - 1) is the maximum number of workgroups per dimension.
+ */
+ if (!V3D_GET_FIELD(job->args.cfg[0], V3D_CSD_QUEUED_CFG0_NUM_WGS_X) ||
+ !V3D_GET_FIELD(job->args.cfg[1], V3D_CSD_QUEUED_CFG1_NUM_WGS_Y) ||
+ !V3D_GET_FIELD(job->args.cfg[2], V3D_CSD_QUEUED_CFG2_NUM_WGS_Z))
+ return NULL;
+
v3d->queue[V3D_CSD].active_job = &job->base;
v3d_invalidate_caches(v3d);
--
2.54.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 2/2] drm/v3d: Skip CSD when it has zeroed workgroups
2026-06-01 19:13 ` [PATCH v2 2/2] drm/v3d: Skip CSD when it " Maíra Canal
@ 2026-06-02 7:10 ` Iago Toral
2026-06-04 3:44 ` Claude review: " Claude Code Review Bot
1 sibling, 0 replies; 10+ messages in thread
From: Iago Toral @ 2026-06-02 7:10 UTC (permalink / raw)
To: Maíra Canal, Melissa Wen, Jose Maria Casanova Crespo
Cc: kernel-dev, dri-devel, stable
El lun, 01-06-2026 a las 16:13 -0300, Maíra Canal escribió:
> A compute shader dispatch encodes its workgroup counts in the
> CFG0..CFG2
> registers. Kicking off a dispatch with a zero count in any of the
> three
> dimensions is invalid. First, the hardware will process 0 as 65536,
> causing an illegitimate submission. But over that, a submission with
> a
> zeroed workgroup dimension should be a no-op.
>
> These zeroed counts can reach the dispatch path through an indirect
> CSD
> job, whose workgroup counts are only known once the indirect buffer
> is
> read and may legitimately be zero, but such scenario should only
> result in
> a no-op.
>
> Don't submit the job to the hardware when any of the workgroup counts
> is
> zero, so the job completes immediately instead of running the shader.
>
> Cc: stable@vger.kernel.org
> Fixes: d223f98f0209 ("drm/v3d: Add support for compute shader
> dispatch.")
> Suggested-by: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
> drivers/gpu/drm/v3d/v3d_sched.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c
> b/drivers/gpu/drm/v3d/v3d_sched.c
> index 47f83936cd73..681d10af4c8e 100644
> --- a/drivers/gpu/drm/v3d/v3d_sched.c
> +++ b/drivers/gpu/drm/v3d/v3d_sched.c
> @@ -352,6 +352,15 @@ v3d_csd_job_run(struct drm_sched_job *sched_job)
> return NULL;
> }
>
> + /* For dispatch dimensions, HW interprets 0 as 65536,
> causing
> + * illegitimate submissions that must be rejected. Note that
> + * 65535 (2^16 - 1) is the maximum number of workgroups per
> dimension.
> + */
I am not sure this description is accurate. I think passing in 0
doesn't cause illegitimate submissions. I think the issue is that if we
allow 0 to be interpreted as 65536 then can't tell if an indirect job
actually configured 0 as the dimension. So I'd rephrase this as:
/* The hw interprets a workgroup size of 0 as 65536, however,
* the user-space driver exposes a maximum of 65535. So a 0 on any
* dimension means that we have no workgroups and the compute shader
* should not be dispatched.
*/
With that change, both patches are:
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
> + if (!V3D_GET_FIELD(job->args.cfg[0],
> V3D_CSD_QUEUED_CFG0_NUM_WGS_X) ||
> + !V3D_GET_FIELD(job->args.cfg[1],
> V3D_CSD_QUEUED_CFG1_NUM_WGS_Y) ||
> + !V3D_GET_FIELD(job->args.cfg[2],
> V3D_CSD_QUEUED_CFG2_NUM_WGS_Z))
> + return NULL;
> +
> v3d->queue[V3D_CSD].active_job = &job->base;
>
> v3d_invalidate_caches(v3d);
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Claude review: drm/v3d: Skip CSD when it has zeroed workgroups
2026-06-01 19:13 ` [PATCH v2 2/2] drm/v3d: Skip CSD when it " Maíra Canal
2026-06-02 7:10 ` Iago Toral
@ 2026-06-04 3:44 ` Claude Code Review Bot
1 sibling, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 3:44 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Status: Good, one minor observation**
This patch prevents the hardware from seeing a dispatch with zero workgroup counts. As the comment explains, the hardware interprets 0 in the NUM_WGS fields as 65536 (since the field is 16-bit and 0 wraps), which would cause an illegitimate submission.
The check uses `V3D_GET_FIELD` to extract only the workgroup count fields from `cfg[0..2]`:
```c
if (!V3D_GET_FIELD(job->args.cfg[0], V3D_CSD_QUEUED_CFG0_NUM_WGS_X) ||
!V3D_GET_FIELD(job->args.cfg[1], V3D_CSD_QUEUED_CFG1_NUM_WGS_Y) ||
!V3D_GET_FIELD(job->args.cfg[2], V3D_CSD_QUEUED_CFG2_NUM_WGS_Z))
return NULL;
```
This is correct. The `V3D_GET_FIELD` macro extracts bits [31:16] from each register (the workgroup count), ignoring the offset in bits [15:0]. This was improved in v2 per reviewer feedback — checking the full register word would have falsely triggered if an offset field happened to be non-zero while the count was zero, or failed to trigger if the whole word was non-zero but the count field was zero.
The placement of the check is correct — it's after the `s_fence->finished.error` check but before `active_job` assignment, `v3d_job_start_stats`, `v3d_switch_perfmon`, and register writes. This means none of those side effects occur when the job is skipped, so there's no resource or state imbalance.
Returning `NULL` from `v3d_csd_job_run` follows the existing pattern (the error path just above does the same) and tells the scheduler the job completed immediately with no hardware fence to wait on.
**Minor observation**: The existing error path defensively clears `v3d->queue[V3D_CSD].active_job = NULL` before returning NULL. The new zero-count path does not. This is technically fine because `active_job` hasn't been assigned to this job yet at this point, but for consistency with the existing error path's defensive style, adding the same `active_job = NULL` assignment could be considered. This is not a bug — just a minor consistency point.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 10+ messages in thread
* Claude review: drm/v3d: Fix indirect CSD jobs with zeroed workgroups
2026-06-01 19:13 [PATCH v2 0/2] drm/v3d: Fix indirect CSD jobs with zeroed workgroups Maíra Canal
2026-06-01 19:13 ` [PATCH v2 1/2] drm/v3d: Fix vaddr leak when indirect CSD has " Maíra Canal
2026-06-01 19:13 ` [PATCH v2 2/2] drm/v3d: Skip CSD when it " Maíra Canal
@ 2026-06-04 3:44 ` Claude Code Review Bot
2 siblings, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 3:44 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: drm/v3d: Fix indirect CSD jobs with zeroed workgroups
Author: =?utf-8?q?Ma=C3=ADra_Canal?= <mcanal@igalia.com>
Patches: 4
Reviewed: 2026-06-04T13:44:21.425217
---
This is a clean, well-targeted 2-patch bugfix series for the V3D DRM driver's indirect CSD (Compute Shader Dispatch) path. Indirect CSD allows userspace to defer workgroup counts to a GPU buffer that is only resolved at runtime. When those counts legitimately turn out to be zero, two bugs are exposed: (1) a vaddr leak and (2) the hardware misinterpreting 0 as 65536 workgroups.
The series is correctly ordered — patch 1 fixes the resource leak in the CPU job rewrite function, and patch 2 prevents the invalid dispatch from reaching hardware. Both are tagged for stable, which is appropriate given these are correctness/resource-leak fixes with minimal risk. The v2 changes (checking only the workgroup count fields via `V3D_GET_FIELD` rather than the entire cfg register, plus an explanatory comment) address reviewer feedback well.
No significant issues found. The series looks good to merge.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 0/2] drm/v3d: Fix indirect CSD jobs with zeroed workgroups
@ 2026-06-02 17:50 Maíra Canal
2026-06-02 17:50 ` [PATCH v4 1/2] drm/v3d: Fix vaddr leak when indirect CSD has " Maíra Canal
0 siblings, 1 reply; 10+ messages in thread
From: Maíra Canal @ 2026-06-02 17:50 UTC (permalink / raw)
To: Melissa Wen, Iago Toral Quiroga, Jose Maria Casanova Crespo
Cc: kernel-dev, dri-devel, stable, Maíra Canal
Hi,
Indirect CSD lets userspace defer the workgroup counts to a GPU buffer
that is only filled at runtime, so the counts are unknown at submission
time and can legitimately turn out to be zero.
However, exercing this case exposed two issues in the CSD path.
1. Virtual address leaks when the indirect CSD has zeroed workgroups.
2. CSD jobs with zeroed workgroups shouldn't be submitted to hardware.
This series intends to address both issues.
Tested with the following CTS tests:
- dEQP-VK.compute.*.indirect_dispatch.upload_buffer.empty_command_x*
- dEQP-VK.compute.*.indirect_dispatch.upload_buffer.empty_command_y*
- dEQP-VK.compute.*.indirect_dispatch.upload_buffer.empty_command_z*
Best regards,
- Maíra
---
v1 -> v2: https://lore.kernel.org/r/20260530-v3d-fix-indirect-csd-v1-0-15533948663f@igalia.com
- [2/2] Don't check the whole cfg[0-2], check only the number of workgroups (Iago Toral)
- [2/2] Add a comment about how the HW interprets 0 (Iago Toral)
v2 -> v3: https://lore.kernel.org/r/20260601-v3d-fix-indirect-csd-v2-0-aaebf035b936@igalia.com
- [1/2, 2/2] Add Iago's R-b (Iago Toral)
- [2/2] Adjust the comment to make it more accurate (Iago Toral)
v3 -> v4: https://lore.kernel.org/r/20260602-v3d-fix-indirect-csd-v3-0-cc79e06e543c@igalia.com
- [2/2] Always rewrite CFG[0..2] from the indirect buffer to avoid
preserving stale contents from user space.
---
Maíra Canal (2):
drm/v3d: Fix vaddr leak when indirect CSD has zeroed workgroups
drm/v3d: Skip CSD when it has zeroed workgroups
drivers/gpu/drm/v3d/v3d_sched.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
---
base-commit: ae0383e5a9a4b12d68c76c4769857def4665deff
change-id: 20260516-v3d-fix-indirect-csd-6806cf20a57a
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v4 1/2] drm/v3d: Fix vaddr leak when indirect CSD has zeroed workgroups
2026-06-02 17:50 [PATCH v4 0/2] " Maíra Canal
@ 2026-06-02 17:50 ` Maíra Canal
2026-06-04 2:23 ` Claude review: " Claude Code Review Bot
0 siblings, 1 reply; 10+ messages in thread
From: Maíra Canal @ 2026-06-02 17:50 UTC (permalink / raw)
To: Melissa Wen, Iago Toral Quiroga, Jose Maria Casanova Crespo
Cc: kernel-dev, dri-devel, stable, Maíra Canal
v3d_rewrite_csd_job_wg_counts_from_indirect() maps both the indirect
buffer and the workgroup buffer and is expected to release them before
returning. When any of the workgroup counts read from the buffer is zero,
the function bailed out early and skipped the cleanup, leaking the vaddr
mappings of both BOs.
Jump to the cleanup path instead of returning directly, so the mappings
are always dropped.
Cc: stable@vger.kernel.org
Fixes: 18b8413b25b7 ("drm/v3d: Create a CPU job extension for a indirect CSD job")
Suggested-by: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
---
drivers/gpu/drm/v3d/v3d_sched.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 94bf628dc91c..47f83936cd73 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -403,7 +403,7 @@ v3d_rewrite_csd_job_wg_counts_from_indirect(struct v3d_cpu_job *job)
wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset);
if (wg_counts[0] == 0 || wg_counts[1] == 0 || wg_counts[2] == 0)
- return;
+ goto unmap_bo;
args->cfg[0] = wg_counts[0] << V3D_CSD_CFG012_WG_COUNT_SHIFT;
args->cfg[1] = wg_counts[1] << V3D_CSD_CFG012_WG_COUNT_SHIFT;
@@ -428,6 +428,7 @@ v3d_rewrite_csd_job_wg_counts_from_indirect(struct v3d_cpu_job *job)
}
}
+unmap_bo:
v3d_put_bo_vaddr(indirect);
v3d_put_bo_vaddr(bo);
}
--
2.54.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Claude review: drm/v3d: Fix vaddr leak when indirect CSD has zeroed workgroups
2026-06-02 17:50 ` [PATCH v4 1/2] drm/v3d: Fix vaddr leak when indirect CSD has " Maíra Canal
@ 2026-06-04 2:23 ` Claude Code Review Bot
0 siblings, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 2:23 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Status: Good — correct and minimal fix.**
The bug: `v3d_rewrite_csd_job_wg_counts_from_indirect()` maps two BOs via `v3d_get_bo_vaddr()` at the top, but when any workgroup count is zero, it returned early via `return;`, skipping the `v3d_put_bo_vaddr()` cleanup at the bottom. This leaks the vaddr mappings.
The fix replaces the bare `return` with `goto unmap_bo`, jumping to a label placed just before the existing cleanup:
```c
if (wg_counts[0] == 0 || wg_counts[1] == 0 || wg_counts[2] == 0)
- return;
+ goto unmap_bo;
```
```c
+unmap_bo:
v3d_put_bo_vaddr(indirect);
v3d_put_bo_vaddr(bo);
```
This is the obvious correct fix. The `goto` is the standard kernel pattern for cleanup-on-early-exit. The Fixes tag correctly references the commit that introduced the indirect CSD path. Has Iago's Reviewed-by.
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 0/2] drm/v3d: Fix indirect CSD jobs with zeroed workgroups
@ 2026-06-02 11:57 Maíra Canal
2026-06-02 11:57 ` [PATCH v3 1/2] drm/v3d: Fix vaddr leak when indirect CSD has " Maíra Canal
0 siblings, 1 reply; 10+ messages in thread
From: Maíra Canal @ 2026-06-02 11:57 UTC (permalink / raw)
To: Melissa Wen, Iago Toral Quiroga, Jose Maria Casanova Crespo
Cc: kernel-dev, dri-devel, stable, Maíra Canal
Hi,
Indirect CSD lets userspace defer the workgroup counts to a GPU buffer
that is only filled at runtime, so the counts are unknown at submission
time and can legitimately turn out to be zero.
However, exercing this case exposed two issues in the CSD path.
1. Virtual address leaks when the indirect CSD has zeroed workgroups.
2. CSD jobs with zeroed workgroups shouldn't be submitted to hardware.
This series intends to address both issues.
Best regards,
- Maíra
---
v1 -> v2: https://lore.kernel.org/r/20260530-v3d-fix-indirect-csd-v1-0-15533948663f@igalia.com
- [2/2] Don't check the whole cfg[0-2], check only the number of workgroups (Iago Toral)
- [2/2] Add a comment about how the HW interprets 0 (Iago Toral)
v2 -> v3: https://lore.kernel.org/r/20260601-v3d-fix-indirect-csd-v2-0-aaebf035b936@igalia.com
- [1/2, 2/2] Add Iago's R-b (Iago Toral)
- [2/2] Adjust the comment to make it more accurate (Iago Toral)
---
Maíra Canal (2):
drm/v3d: Fix vaddr leak when indirect CSD has zeroed workgroups
drm/v3d: Skip CSD when it has zeroed workgroups
drivers/gpu/drm/v3d/v3d_sched.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
---
base-commit: ae0383e5a9a4b12d68c76c4769857def4665deff
change-id: 20260516-v3d-fix-indirect-csd-6806cf20a57a
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 1/2] drm/v3d: Fix vaddr leak when indirect CSD has zeroed workgroups
2026-06-02 11:57 [PATCH v3 0/2] drm/v3d: Fix indirect CSD jobs with " Maíra Canal
@ 2026-06-02 11:57 ` Maíra Canal
2026-06-04 2:49 ` Claude review: " Claude Code Review Bot
0 siblings, 1 reply; 10+ messages in thread
From: Maíra Canal @ 2026-06-02 11:57 UTC (permalink / raw)
To: Melissa Wen, Iago Toral Quiroga, Jose Maria Casanova Crespo
Cc: kernel-dev, dri-devel, stable, Maíra Canal
v3d_rewrite_csd_job_wg_counts_from_indirect() maps both the indirect
buffer and the workgroup buffer and is expected to release them before
returning. When any of the workgroup counts read from the buffer is zero,
the function bailed out early and skipped the cleanup, leaking the vaddr
mappings of both BOs.
Jump to the cleanup path instead of returning directly, so the mappings
are always dropped.
Cc: stable@vger.kernel.org
Fixes: 18b8413b25b7 ("drm/v3d: Create a CPU job extension for a indirect CSD job")
Suggested-by: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
---
drivers/gpu/drm/v3d/v3d_sched.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 94bf628dc91c..47f83936cd73 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -403,7 +403,7 @@ v3d_rewrite_csd_job_wg_counts_from_indirect(struct v3d_cpu_job *job)
wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset);
if (wg_counts[0] == 0 || wg_counts[1] == 0 || wg_counts[2] == 0)
- return;
+ goto unmap_bo;
args->cfg[0] = wg_counts[0] << V3D_CSD_CFG012_WG_COUNT_SHIFT;
args->cfg[1] = wg_counts[1] << V3D_CSD_CFG012_WG_COUNT_SHIFT;
@@ -428,6 +428,7 @@ v3d_rewrite_csd_job_wg_counts_from_indirect(struct v3d_cpu_job *job)
}
}
+unmap_bo:
v3d_put_bo_vaddr(indirect);
v3d_put_bo_vaddr(bo);
}
--
2.54.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Claude review: drm/v3d: Fix vaddr leak when indirect CSD has zeroed workgroups
2026-06-02 11:57 ` [PATCH v3 1/2] drm/v3d: Fix vaddr leak when indirect CSD has " Maíra Canal
@ 2026-06-04 2:49 ` Claude Code Review Bot
0 siblings, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 2:49 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Status: Good**
This patch fixes a resource leak in `v3d_rewrite_csd_job_wg_counts_from_indirect()`. Looking at the existing code in the kernel tree (`v3d_sched.c:400-406`):
```c
v3d_get_bo_vaddr(bo);
v3d_get_bo_vaddr(indirect);
wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset);
if (wg_counts[0] == 0 || wg_counts[1] == 0 || wg_counts[2] == 0)
return;
```
The bare `return` at line 406 skips the cleanup at lines 431-432 (`v3d_put_bo_vaddr(indirect)` / `v3d_put_bo_vaddr(bo)`), leaking both mappings. The fix correctly replaces `return` with `goto unmap_bo`, which jumps to the cleanup path:
```c
if (wg_counts[0] == 0 || wg_counts[1] == 0 || wg_counts[2] == 0)
goto unmap_bo;
```
```c
unmap_bo:
v3d_put_bo_vaddr(indirect);
v3d_put_bo_vaddr(bo);
```
The label placement is correct — it sits right before both `v3d_put_bo_vaddr` calls, matching the acquisition order (bo, indirect acquired; indirect, bo released). The label name `unmap_bo` is slightly ambiguous since it unmaps both BOs, but this is a minor style nit that doesn't affect correctness.
One observation: when the zero-workgroup path is taken, `args->cfg[0..4]` are left untouched — they retain whatever userspace submitted. This is fine because patch 2 ensures the CSD job is never submitted to hardware when counts are zero.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 0/2] drm/v3d: Fix indirect CSD jobs with zeroed workgroups
@ 2026-05-30 19:51 Maíra Canal
2026-05-30 19:51 ` [PATCH 1/2] drm/v3d: Fix vaddr leak when indirect CSD has " Maíra Canal
0 siblings, 1 reply; 10+ messages in thread
From: Maíra Canal @ 2026-05-30 19:51 UTC (permalink / raw)
To: Melissa Wen, Iago Toral Quiroga, Jose Maria Casanova Crespo
Cc: kernel-dev, dri-devel, stable, Maíra Canal
Hi,
Indirect CSD lets userspace defer the workgroup counts to a GPU buffer
that is only filled at runtime, so the counts are unknown at submission
time and can legitimately turn out to be zero.
However, exercing this case exposed two issues in the CSD path.
1. Virtual address leaks when the indirect CSD has zeroed workgroups.
2. CSD jobs with zeroed workgroups shouldn't be submitted to hardware.
This series intends to address both issues.
Best regards,
- Maíra
---
Maíra Canal (2):
drm/v3d: Fix vaddr leak when indirect CSD has zeroed workgroups
drm/v3d: Skip CSD when it has zeroed workgroups
drivers/gpu/drm/v3d/v3d_sched.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
---
base-commit: 5ab62dd3687bcc2cc542b99385aabac5c996db6f
change-id: 20260516-v3d-fix-indirect-csd-6806cf20a57a
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/2] drm/v3d: Fix vaddr leak when indirect CSD has zeroed workgroups
2026-05-30 19:51 [PATCH 0/2] drm/v3d: Fix indirect CSD jobs with " Maíra Canal
@ 2026-05-30 19:51 ` Maíra Canal
2026-06-04 5:15 ` Claude review: " Claude Code Review Bot
0 siblings, 1 reply; 10+ messages in thread
From: Maíra Canal @ 2026-05-30 19:51 UTC (permalink / raw)
To: Melissa Wen, Iago Toral Quiroga, Jose Maria Casanova Crespo
Cc: kernel-dev, dri-devel, stable, Maíra Canal
v3d_rewrite_csd_job_wg_counts_from_indirect() maps both the indirect
buffer and the workgroup buffer and is expected to release them before
returning. When any of the workgroup counts read from the buffer is zero,
the function bailed out early and skipped the cleanup, leaking the vaddr
mappings of both BOs.
Jump to the cleanup path instead of returning directly, so the mappings
are always dropped.
Cc: stable@vger.kernel.org
Fixes: 18b8413b25b7 ("drm/v3d: Create a CPU job extension for a indirect CSD job")
Suggested-by: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
drivers/gpu/drm/v3d/v3d_sched.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 94bf628dc91c..47f83936cd73 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -403,7 +403,7 @@ v3d_rewrite_csd_job_wg_counts_from_indirect(struct v3d_cpu_job *job)
wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset);
if (wg_counts[0] == 0 || wg_counts[1] == 0 || wg_counts[2] == 0)
- return;
+ goto unmap_bo;
args->cfg[0] = wg_counts[0] << V3D_CSD_CFG012_WG_COUNT_SHIFT;
args->cfg[1] = wg_counts[1] << V3D_CSD_CFG012_WG_COUNT_SHIFT;
@@ -428,6 +428,7 @@ v3d_rewrite_csd_job_wg_counts_from_indirect(struct v3d_cpu_job *job)
}
}
+unmap_bo:
v3d_put_bo_vaddr(indirect);
v3d_put_bo_vaddr(bo);
}
--
2.54.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Claude review: drm/v3d: Fix vaddr leak when indirect CSD has zeroed workgroups
2026-05-30 19:51 ` [PATCH 1/2] drm/v3d: Fix vaddr leak when indirect CSD has " Maíra Canal
@ 2026-06-04 5:15 ` Claude Code Review Bot
0 siblings, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 5:15 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Assessment: Good. No issues.**
This is a straightforward and correct fix. The function `v3d_rewrite_csd_job_wg_counts_from_indirect()` maps two BOs via `v3d_get_bo_vaddr()` (which calls `vmap()` internally), then checks if any workgroup count is zero:
```c
if (wg_counts[0] == 0 || wg_counts[1] == 0 || wg_counts[2] == 0)
return;
```
The bare `return` skips the cleanup at the end of the function:
```c
v3d_put_bo_vaddr(indirect);
v3d_put_bo_vaddr(bo);
```
The fix correctly changes `return` to `goto unmap_bo` and adds the label before the cleanup calls. The unmap order (indirect first, then bo) matches the reverse of the mapping order, which is good practice. The label name `unmap_bo` is a reasonable label — arguably `unmap` would be more precise since it unmaps both `bo` and `indirect`, but this follows common kernel style where the label names the first cleanup action.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2026-06-04 5:15 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-01 19:13 [PATCH v2 0/2] drm/v3d: Fix indirect CSD jobs with zeroed workgroups Maíra Canal
2026-06-01 19:13 ` [PATCH v2 1/2] drm/v3d: Fix vaddr leak when indirect CSD has " Maíra Canal
2026-06-04 3:44 ` Claude review: " Claude Code Review Bot
2026-06-01 19:13 ` [PATCH v2 2/2] drm/v3d: Skip CSD when it " Maíra Canal
2026-06-02 7:10 ` Iago Toral
2026-06-04 3:44 ` Claude review: " Claude Code Review Bot
2026-06-04 3:44 ` Claude review: drm/v3d: Fix indirect CSD jobs with " Claude Code Review Bot
-- strict thread matches above, loose matches on Subject: below --
2026-06-02 17:50 [PATCH v4 0/2] " Maíra Canal
2026-06-02 17:50 ` [PATCH v4 1/2] drm/v3d: Fix vaddr leak when indirect CSD has " Maíra Canal
2026-06-04 2:23 ` Claude review: " Claude Code Review Bot
2026-06-02 11:57 [PATCH v3 0/2] drm/v3d: Fix indirect CSD jobs with " Maíra Canal
2026-06-02 11:57 ` [PATCH v3 1/2] drm/v3d: Fix vaddr leak when indirect CSD has " Maíra Canal
2026-06-04 2:49 ` Claude review: " Claude Code Review Bot
2026-05-30 19:51 [PATCH 0/2] drm/v3d: Fix indirect CSD jobs with " Maíra Canal
2026-05-30 19:51 ` [PATCH 1/2] drm/v3d: Fix vaddr leak when indirect CSD has " Maíra Canal
2026-06-04 5:15 ` Claude review: " Claude Code Review Bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox