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 use-after-free of CPU job query arrays on error path
Date: Sat, 16 May 2026 08:58:37 +1000
Message-ID: <review-patch1-20260515-v3d-cpu-job-leaks-v1-1-7f147cbbf935@igalia.com>
In-Reply-To: <20260515-v3d-cpu-job-leaks-v1-1-7f147cbbf935@igalia.com>
References: <20260515-v3d-cpu-job-leaks-v1-0-7f147cbbf935@igalia.com> <20260515-v3d-cpu-job-leaks-v1-1-7f147cbbf935@igalia.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

**Analysis of the bug:** The old code had `v3d_cpu_job_free()` as a `drm_sc=
hed_backend_ops.free_job` callback in `v3d_sched.c`, which called `v3d_job_=
cleanup()` at the end. On the ioctl error path (`fail:` label in `v3d_submi=
t_cpu_ioctl`), `v3d_job_cleanup()` was called first (which does `kref_put =
=E2=86=92 kfree`), then the code tried to access `cpu_job->timestamp_query.=
queries` and `cpu_job->performance_query.queries` =E2=80=94 a clear use-aft=
er-free. This analysis is correct.

**Fix approach:** The patch moves the cleanup logic from the scheduler's `.=
free_job` callback into a kref destructor (`v3d_cpu_job_free(struct kref *r=
ef)` in `v3d_submit.c`), registered via `v3d_job_init()`. The scheduler now=
 uses the generic `v3d_sched_job_free`, which calls `v3d_job_cleanup() =E2=
=86=92 v3d_job_put() =E2=86=92 kref_put(..., job->free)`, ultimately dispat=
ching to the new `v3d_cpu_job_free`. This mirrors exactly how `v3d_render_j=
ob_free` works:

```c
static void
v3d_render_job_free(struct kref *ref)
{
	struct v3d_render_job *job =3D container_of(ref, struct v3d_render_job,
						  base.refcount);
	...
	v3d_job_free(ref);
}
```

The new function follows the same pattern:

```c
static void
v3d_cpu_job_free(struct kref *ref)
{
	struct v3d_cpu_job *job =3D container_of(ref, struct v3d_cpu_job,
					       base.refcount);

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

	v3d_performance_query_info_free(&job->performance_query,
					job->performance_query.count);

	v3d_job_free(ref);
}
```

This is correct. The `v3d_timestamp_query_info_free` and `v3d_performance_q=
uery_info_free` functions both check for `query_info->queries` being non-NU=
LL before iterating, so they're safe to call when no queries were set up (e=
.g., early error paths or CPU job types that don't use queries).

The removal of the manual `kvfree()` calls from the `fail:` label is safe b=
ecause `v3d_job_cleanup()` will now trigger the full teardown via kref.

The `v3d_job_init()` call is correctly updated to pass `v3d_cpu_job_free` i=
nstead of `v3d_job_free`:

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

**No issues found.** This is a solid fix.

---
Generated by Claude Code Patch Reviewer