Re: [PATCH v3] drm/v3d: bound CPU-job query writes to their destination BO

[Maíra Canal]

Hi Michael,

Thanks for your patch!

On 17/06/26 09:58, Michael Bommarito wrote:
> +
> +/* Reject a query CPU job whose writes would land outside their BO. */
> +static int
> +v3d_cpu_job_bounds_check(struct v3d_cpu_job *job)
> +{
> +	struct drm_device *dev = &job->base.v3d->drm;
> +	struct v3d_timestamp_query_info *tquery = &job->timestamp_query;
> +	struct v3d_copy_query_results_info *copy = &job->copy;
> +	u32 elem = copy->do_64bit ? sizeof(u64) : sizeof(u32);
> +	struct v3d_bo *dst, *src;
> +	u64 slots, write_size;
> +	int i;

Sashiko pointed out that this should be a u32 at least.

> +
> +	switch (job->job_type) {
> +	case V3D_CPU_JOB_TYPE_TIMESTAMP_QUERY:
> +	case V3D_CPU_JOB_TYPE_RESET_TIMESTAMP_QUERY:
> +		/*
> +		 * Each query writes one u64 timestamp slot into bo[0]. The
> +		 * offset is cast to u64 so a 0xffffffff query offset cannot wrap
> +		 * the addition past the BO size.
> +		 */

"/* Each query writes one u64 timestamp slot into bo[0]. */" was enough
of a comment.

> +		dst = to_v3d_bo(job->base.bo[0]);
> +
> +		for (i = 0; i < tquery->count; i++) {
> +			if ((u64)tquery->queries[i].offset + sizeof(u64) >
> +			    dst->base.base.size)
> +				goto err_range;
> +		}
> +		return 0;
> +	case V3D_CPU_JOB_TYPE_COPY_TIMESTAMP_QUERY:
> +		/* Copies one u64 per query from bo[1] into bo[0]. */
> +		dst = to_v3d_bo(job->base.bo[0]);
> +		src = to_v3d_bo(job->base.bo[1]);
> +
> +		for (i = 0; i < tquery->count; i++) {
> +			if ((u64)tquery->queries[i].offset + sizeof(u64) >
> +			    src->base.base.size)
> +				goto err_range;
> +		}
> +
> +		write_size = (copy->availability_bit ? 2 : 1) * elem;
> +		return v3d_check_copy_extent(dev, dst->base.base.size,
> +					     copy->offset, copy->stride,
> +					     tquery->count, write_size);
> +	case V3D_CPU_JOB_TYPE_COPY_PERFORMANCE_QUERY:
> +		/*
> +		 * Each query writes nperfmons * DRM_V3D_MAX_PERF_COUNTERS
> +		 * counter slots into bo[0], plus an availability slot at index
> +		 * ncounters. nperfmons and ncounters are user values; the slot
> +		 * count and write size are computed in u64 (u32 * small
> +		 * constant) so they cannot wrap.
> +		 */

I preferred the previous comment (from v2):

/*
 * Each query writes nperfmons * DRM_V3D_MAX_PERF_COUNTERS
 * counter slots into bo[0], plus an availability slot at
 * index ncounters. nperfmons and ncounters are user values,
 * so the slot count is computed overflow-safe.
 */

> +		dst = to_v3d_bo(job->base.bo[0]);
> +
> +		slots = (u64)job->performance_query.nperfmons *
> +			DRM_V3D_MAX_PERF_COUNTERS;
> +		if (copy->availability_bit)
> +			slots = max(slots,
> +				    (u64)job->performance_query.ncounters + 1);
> +
> +		write_size = slots * elem;
> +		return v3d_check_copy_extent(dev, dst->base.base.size,
> +					     copy->offset, copy->stride,
> +					     job->performance_query.count,
> +					     write_size);
> +	case V3D_CPU_JOB_TYPE_INDIRECT_CSD: {
> +		struct v3d_indirect_csd_info *icsd = &job->indirect_csd;

I'd prefer to call it "indirect_csd".

> +
> +		/*
> +		 * The exec-time work-group rewrite reads three u32 counts from
> +		 * bo[0] at the user-supplied offset, then writes each count back
> +		 * into the indirect BO at a user-supplied u32 index. Reject a
> +		 * missing indirect BO (drm_gem_object_lookup() returned NULL for
> +		 * a bad handle, which the exec path would otherwise dereference)
> +		 * and bound both accesses.
> +		 */
> +		if (!icsd->indirect)
> +			return -ENOENT;

No need for that after [1].

[1] 
https://lore.kernel.org/dri-devel/20260703-v3d-cpu-job-fixes-v3-0-bc51b1f3eeb5@xxxxxxxxxx/T/#m6c2cb9021bc6b524fb713150f94bdfee3553cf72

> +
> +		dst = to_v3d_bo(job->base.bo[0]);

New line. Also, instead of the big comment on the beginning of this
case, I'd prefer a smaller comment here explaining that 3 is the three
dimensions of workgroup counts (x,y,z).

> +		if ((u64)icsd->offset + 3 * sizeof(u32) > dst->base.base.size)
> +			goto err_range;
> +
> +		src = to_v3d_bo(icsd->indirect);

I'm not sure src makes a lot of sense here, as we copy from BO to the
indirect BO. Maybe dst?

> +		for (i = 0; i < 3; i++) {
> +			u32 uidx = icsd->wg_uniform_offsets[i];
> +
> +			if (uidx != 0xffffffff &&

Add a comment explaining that, as 0xffffffff means "skip this rewrite",
the exec path doesn't write it, so there is no need to range-check it.

Finally, although it's a fix, I'd prefer to apply it to drm-misc-next as
(1) it's a reasonably big change and (2) it's not a critical bug. So, if
possible, rebase the patch on top of drm-misc-next.

Best regards,
- Maíra