[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;
-- 

Claude review: drm/v3d: fix to avoid cleaning up uninitialized CPU jobs

[Claude Code Review Bot]

Overall Series Review

Subject: drm/v3d: fix to avoid cleaning up uninitialized CPU jobs
Author: Jeongjun Park <aha310510@gmail.com>
Patches: 1
Reviewed: 2026-05-26T06:51:56.642302

---

This is a single-patch fix for a real bug: `v3d_submit_cpu_ioctl` calls `v3d_job_cleanup()` on a CPU job that was never initialized with `v3d_job_init()`, causing undefined behavior via `drm_sched_job_cleanup()` on uninitialized scheduler state. The bug diagnosis is correct and the general approach — move `v3d_job_init()` earlier and add a separate error path for pre-init failures — is the right direction.

However, the patch has two compile-blocking problems and a memory leak, and appears to be based on an older tree (it does not include the `kvfree` query cleanup lines present on drm-next). As submitted, this patch will not build.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: fix to avoid cleaning up uninitialized CPU jobs

[Claude Code Review Bot]

Patch Review

**Bug analysis is correct.** The existing code at lines 1300–1305 (in the patch base) checks `cpu_job->job_type` before `v3d_job_init()` has been called. When no CPU extension is supplied, `job_type` is 0 (from `kcalloc`), the check fails, and the code falls through to `fail:` which calls `v3d_job_cleanup()` → `drm_sched_job_cleanup()` on a job whose `drm_sched_job` base was never initialized. Good catch.

**Issue 1 (build failure): `v3d_cpu_job_free` has the wrong signature and is inaccessible.**

The patch changes the free callback passed to `v3d_job_init`:

```c
ret = v3d_job_init(v3d, file_priv, &cpu_job->base,
		   v3d_cpu_job_free, 0, &se, V3D_CPU);
```

But `v3d_job_init` expects `void (*free)(struct kref *ref)` (see `v3d_submit.c:167`), while the only `v3d_cpu_job_free` in the tree is in `v3d_sched.c:129` with signature `void v3d_cpu_job_free(struct drm_sched_job *sched_job)`. These are incompatible types. Additionally, that function is declared `static`, so it is not visible from `v3d_submit.c`. This will fail to compile.

The original code correctly uses `v3d_job_free` here — the kref-based free callback. If the intent is to have the kref free callback also release the query arrays, a new `v3d_cpu_job_kref_free` function with the correct `struct kref *ref` signature would need to be added in `v3d_submit.c`.

**Issue 2 (memory leak): `fail_init` path leaks query arrays.**

When `v3d_get_extensions()` succeeds and populates `cpu_job->timestamp_query.queries` / `cpu_job->performance_query.queries`, but then `v3d_job_init()` fails:

```c
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;
}
```

`v3d_job_deallocate()` only does `kfree(*container)`, freeing the `cpu_job` struct itself. The separately allocated query arrays pointed to by `cpu_job->timestamp_query.queries` and `cpu_job->performance_query.queries` are leaked. The `fail_init` label jumps past the cleanup that would free them.

The same issue exists in the extension failure path:

```c
ret = v3d_get_extensions(file_priv, args->extensions, &se, cpu_job);
if (ret) {
	v3d_job_deallocate((void *)&cpu_job);
	goto fail_init;
}
```

`v3d_get_extensions()` can succeed at allocating queries for one extension, then fail on a subsequent extension, returning an error. The partial allocations are not freed before `v3d_job_deallocate()` destroys the container struct. (This particular leak also exists in the original code — it's pre-existing.)

**Issue 3 (wrong tree base): patch conflicts with drm-next.**

The current drm-next `fail:` label includes:

```c
fail:
	v3d_job_cleanup((void *)cpu_job);
	v3d_job_cleanup((void *)csd_job);
	v3d_job_cleanup(clean_job);
	v3d_put_multisync_post_deps(&se);
	kvfree(cpu_job->timestamp_query.queries);
	kvfree(cpu_job->performance_query.queries);
```

The patch's context hunk doesn't show the `kvfree` lines, confirming it's based on an older tree. Note that the existing `kvfree` lines on drm-next are themselves a use-after-free bug — they access `cpu_job->timestamp_query.queries` after `v3d_job_cleanup()` may have already freed `cpu_job` via `kref_put` → `v3d_job_free` → `kfree(job)`.

**Suggested approach:**

1. Keep `v3d_job_free` as the kref callback (not `v3d_cpu_job_free`).
2. Move `v3d_job_init()` before the `job_type` validation, as this patch does.
3. Free query arrays *before* calling `v3d_job_cleanup()` in the `fail` path (fixing the existing use-after-free).
4. In the `fail_init` path, also free the query arrays before calling `v3d_job_deallocate()`.

---
Generated by Claude Code Patch Reviewer