Re: [PATCH v2] drm/imagination: acquire vm_ctx->lock before mapping memory to GPU VM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling `drm_gpuvm_bo_put()` without holding the GEM object's dma_resv lock in `pvr_vm_bind_op_fini()` causes lockdep splats and GEM gpuva list corruption on error paths.
- [High] Infinite loop in `pvr_vm_unmap_all()` when `pvr_vm_unmap_obj_locked()` fails, leading to a kernel thread hang (Denial of Service).
--

--- Patch [1]: [PATCH v2] drm/imagination: acquire vm_ctx->lock before mapping memory to GPU VM ---
commit 1da21cbc09436cbb38e021164bab86864e2bacca
Author: Icenowy Zheng <zhengxingda@iscas.ac.cn>

drm/imagination: acquire vm_ctx->lock before mapping memory to GPU VM

The drm gpuvm code doesn't protect find operation against map operation,
and the driver needs to ensure a map operation shouldn't happen when a
find operation is in progress.

> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index 396d349fb6ce4..ceb78694cd987 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> @@ -747,6 +747,7 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct pvr_gem_object *pvr_obj,
>  
>  	pvr_gem_object_get(pvr_obj);
>  
> +	mutex_lock(&vm_ctx->lock);
>  	err = drm_gpuvm_exec_lock(&vm_exec);
>  	if (err)
>  		goto err_cleanup;
> @@ -756,6 +757,7 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct pvr_gem_object *pvr_obj,
>  	drm_gpuvm_exec_unlock(&vm_exec);
>  
>  err_cleanup:
> +	mutex_unlock(&vm_ctx->lock);
>  	pvr_vm_bind_op_fini(&bind_op);

[Severity: High]
This is a pre-existing issue, but does calling pvr_vm_bind_op_fini() in the
error path here drop the drm_gpuvm_bo reference without holding the GEM's
dma_resv lock?

Looking at pvr_vm_bind_op_fini(), it calls drm_gpuvm_bo_put() unconditionally:

drivers/gpu/drm/imagination/pvr_vm.c:pvr_vm_bind_op_fini() {
    drm_gpuvm_bo_put(bind_op->gpuvm_bo);
    ...
}

When drm_gpuvm_bo_put() drops the reference count to zero, it destroys the BO
and removes it from the GEM's gpuva.list. This API requires the caller to hold
the dma_resv lock.

In error paths like when drm_gpuvm_exec_lock() fails, this cleanup path runs
without the dma_resv lock held.

Could this lead to list corruption and use-after-free conditions on the GEM's
gpuva lists if multiple threads operate concurrently?

>  
>  	return err;
>  }

[ ... ]

[Severity: High]
This is a pre-existing issue, but can pvr_vm_unmap_all() enter an infinite
loop if pvr_vm_unmap_obj_locked() fails?

Looking at pvr_vm_unmap_all():

drivers/gpu/drm/imagination/pvr_vm.c:pvr_vm_unmap_all() {
    for (;;) {
        ...
        va = drm_gpuva_find_first(&vm_ctx->gpuvm_mgr, ...);
        if (!va)
            break;

        pvr_obj = gem_to_pvr_gem(va->gem.obj);

        WARN_ON(pvr_vm_unmap_obj_locked(vm_ctx, pvr_obj,
                                        va->va.addr, va->va.range));
    }
}

The return value of pvr_vm_unmap_obj_locked() is tested inside a WARN_ON but
otherwise ignored. If it fails, the mapping is not removed from the tree, and
the next iteration will find the exact same mapping via drm_gpuva_find_first().

Since pvr_vm_unmap_obj_locked() uses DRM_EXEC_INTERRUPTIBLE_WAIT,
if a process is being killed (e.g. SIGKILL during teardown), this lock will
repeatedly fail with -ERESTARTSYS.

Does this cause an unrecoverable infinite loop in the kernel under
vm_ctx->lock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714073641.1935075-1-zhengxingda@iscas.ac.cn?part=1