Re: [PATCH 3/4] drm/imagination: Add async VM_BIND ioctl

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Taking `vm_ctx->lock` inside a dma-fence signalling critical section creates a deadlock risk because legacy ioctl paths hold this lock during `GFP_KERNEL` memory allocations.
- [High] The `DRM_IOCTL_PVR_VM_BIND` ioctl accepts an unbounded number of operations, allowing a local denial-of-service via unbounded kernel memory allocations.
--

--- Patch [3]: [PATCH 3/4] drm/imagination: Add async VM_BIND ioctl ---
commit d69b2e6682fdd9c9fc81f9944e1b72971b7b0c0f
Author: Gyeyoung Baek <[email protected]>
Subject: drm/imagination: Add async VM_BIND ioctl

This commit adds a DRM_IOCTL_PVR_VM_BIND ioctl to the PowerVR driver to 
support Vulkan sparse binding needs. It allows batching bind and sync 
operations into a single request. Requests can be run asynchronously 
as drm_sched jobs when the DRM_PVR_VM_BIND_ASYNC flag is provided.

> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> index 5c965ef0274ff..ec6ed610e29e1 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
[ ... ]
> @@ -1130,6 +1130,93 @@ pvr_ioctl_vm_unmap(struct drm_device *drm_dev, void *raw_args,
[ ... ]
> +static int
> +pvr_ioctl_vm_bind(struct drm_device *drm_dev, void *raw_args,
> +		  struct drm_file *file)
> +{
> +	struct drm_pvr_ioctl_vm_bind_args *args = raw_args;
[ ... ]
> +	vm_ctx = pvr_vm_context_lookup(pvr_file, args->vm_context_handle);
> +	if (!vm_ctx) {
> +		err = -EINVAL;
> +		goto err_drm_dev_exit;
> +	}
> +
[Severity: High]
Could args->ops.count or args->sync_ops.count be arbitrarily large here?
Without an upper bound, could an unprivileged user exhaust system memory by
passing an extremely large count, as these allocations bypass memory cgroup
limits?

> +	if (args->ops.count) {
> +		err = PVR_UOBJ_GET_ARRAY(uapi_ops, &args->ops);
> +		if (err)
> +			goto err_put_vm_context;
> +	}
> +
> +	if (args->sync_ops.count) {
> +		err = PVR_UOBJ_GET_ARRAY(sync_ops, &args->sync_ops);
> +		if (err)
> +			goto err_free_uapi_ops;
> +	}
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index 45df76e61f7cf..f92bfeacd7ac4 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
[ ... ]
> @@ -1174,3 +1242,439 @@ pvr_vm_get_fw_mem_context(struct pvr_vm_context *vm_ctx)
[ ... ]
> +static struct dma_fence *
> +pvr_vm_bind_run_job(struct drm_sched_job *sched_job)
> +{
> +	struct pvr_vm_bind_job *job = to_pvr_vm_bind_job(sched_job);
> +	struct pvr_vm_context *vm_ctx = job->vm_ctx;
> +	int err = 0;
> +	bool cookie;
> +
> +	if (pvr_vm_context_is_unusable(vm_ctx))
> +		return ERR_PTR(-ECANCELED);
> +
> +	cookie = dma_fence_begin_signalling();
> +
[Severity: High]
Does acquiring vm_ctx->lock inside the dma-fence signalling critical section
create a circular deadlock risk?

The legacy pvr_vm_unmap() path acquires vm_ctx->lock to look up the mapping,
and then calls pvr_vm_unmap_obj_locked() -> pvr_vm_bind_op_unmap_init().
That init function performs GFP_KERNEL allocations under this same lock:

	bind_op->prev_va = kzalloc_obj(*bind_op->prev_va);
	bind_op->next_va = kzalloc_obj(*bind_op->next_va);
	...
	bind_op->mmu_op_ctx =
		pvr_mmu_op_context_create(vm_ctx->mmu_ctx, NULL, 0, 0);

Direct memory reclaim during these allocations might wait on dma-fences to be
signalled, which could deadlock if this lock is held.

> +	mutex_lock(&vm_ctx->lock);
> +
> +	for (u32 i = 0; i < job->op_count; i++) {
> +		err = pvr_vm_bind_op_exec(&job->ops[i]);
> +		if (err)
> +			break;
> +	}
[ ... ]
> +static struct pvr_vm_bind_op *
> +pvr_vm_bind_ops_create_from_uapi(struct pvr_vm_context *vm_ctx,
> +				 struct pvr_file *pvr_file,
> +				 const struct drm_pvr_vm_bind_op *uapi_ops,
> +				 u32 op_count)
> +{
> +	struct pvr_vm_bind_op *ops;
> +	int err;
> +
[Severity: High]
Could op_count be unbounded here? Since this allocates using GFP_KERNEL
without __GFP_ACCOUNT, large allocations won't be charged to a memcg,
potentially leading to system memory exhaustion or soft lockups during the
subsequent loop.

> +	ops = kvzalloc_objs(*ops, op_count, GFP_KERNEL);
> +	if (!ops)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (u32 prepared = 0; prepared < op_count; prepared++) {
> +		err = pvr_vm_bind_op_init_from_uapi(&ops[prepared], vm_ctx,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3