From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/v3d: fix to avoid cleaning up uninitialized CPU jobs
Date: Tue, 26 May 2026 06:51:56 +1000
Message-ID: <review-patch1-20260525140422.1545327-1-aha310510@gmail.com>
In-Reply-To: <20260525140422.1545327-1-aha310510@gmail.com>
References: <20260525140422.1545327-1-aha310510@gmail.com> <20260525140422.1545327-1-aha310510@gmail.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Bug analysis is correct.** The existing code at lines 1300=E2=80=931305 (=
in the patch base) checks `cpu_job->job_type` before `v3d_job_init()` has b=
een called. When no CPU extension is supplied, `job_type` is 0 (from `kcall=
oc`), the check fails, and the code falls through to `fail:` which calls `v=
3d_job_cleanup()` =E2=86=92 `drm_sched_job_cleanup()` on a job whose `drm_s=
ched_job` base was never initialized. Good catch.

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

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

```c
ret =3D 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_submi=
t.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 compil=
e.

The original code correctly uses `v3d_job_free` here =E2=80=94 the kref-bas=
ed free callback. If the intent is to have the kref free callback also rele=
ase the query arrays, a new `v3d_cpu_job_kref_free` function with the corre=
ct `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_quer=
y.queries` / `cpu_job->performance_query.queries`, but then `v3d_job_init()=
` fails:

```c
ret =3D 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_jo=
b->timestamp_query.queries` and `cpu_job->performance_query.queries` are le=
aked. The `fail_init` label jumps past the cleanup that would free them.

The same issue exists in the extension failure path:

```c
ret =3D 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 alloc=
ations are not freed before `v3d_job_deallocate()` destroys the container s=
truct. (This particular leak also exists in the original code =E2=80=94 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 b=
ased on an older tree. Note that the existing `kvfree` lines on drm-next ar=
e themselves a use-after-free bug =E2=80=94 they access `cpu_job->timestamp=
_query.queries` after `v3d_job_cleanup()` may have already freed `cpu_job` =
via `kref_put` =E2=86=92 `v3d_job_free` =E2=86=92 `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 do=
es.
3. Free query arrays *before* calling `v3d_job_cleanup()` in the `fail` pat=
h (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