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

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

[Icenowy Zheng]

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.

In some cases a find operation will be in progress when doing map/unmap
operations, and the find operation will do a NULL pointer deference.

An example of the stack trace of such NULL deference is shown below:

```
Unable to handle kernel access to user memory without uaccess routines at
virtual address 0000000000000010

[<ffffffff01e989d4>] drm_gpuva_find+0x28/0x6c [drm_gpuvm]
[<ffffffff01ed3a40>] pvr_vm_unmap+0x34/0x68 [powervr]
[<ffffffff01ec69da>] pvr_ioctl_vm_unmap+0x2e/0x50 [powervr]
[<ffffffff8080ce0a>] drm_ioctl_kernel+0x8e/0xdc
[<ffffffff8080d016>] drm_ioctl+0x1be/0x3e0
[<ffffffff802bec3e>] __riscv_sys_ioctl+0xba/0xc4
[<ffffffff80d858b2>] do_trap_ecall_u+0x23e/0x3f4
[<ffffffff80d92288>] handle_exception+0x168/0x174
```

As all occurences of drm_gpuva_find*() are already guarded by
vm_ctx->lock, make pvr_vm_map() to acquire this lock to prevent
disturbing any find operation. This fixes the NULL deference problem in
drm_gpuva_find*().

Cc: stable@vger.kernel.org
Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
Fixes: 4bc736f890ce ("drm/imagination: vm: make use of GPUVM's drm_exec helper")
Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
---
Changes in v2:
- Dropped wrongly duplicated mutex_unlock() call and reordered it to
  before pvr_vm_bind_op_fini() (the lock is acquired after
  pvr_vm_bind_op_map_init() call). (Thanks to Brajesh)
- Added a extra Fixes pointing to the original broken code that was
  refactored by the original Fixes (but still broken). (Thanks to
  Alessio)
- Fixed some typos in commit message. (Thanks to Alessio)
- Added a stacktrace of the oops that occured on my board. (As suggested
  by Alessio)

 drivers/gpu/drm/imagination/pvr_vm.c | 2 ++
 1 file changed, 2 insertions(+)

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);
 
 	return err;
-- 
2.52.0

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

[sashiko-bot]

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

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

[Icenowy Zheng]

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.

As all occurences of drm_gpuva_find*() is already guarded by
vm_ctx->lock, make pvr_vm_map() to acquire this lock to prevent
disturbing any find operation.

This fixes occasional NULL deference in drm_gpuva_find*().

Cc: stable@vger.kernel.org
Fixes: 4bc736f890ce ("drm/imagination: vm: make use of GPUVM's drm_exec helper")
Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
---
Changes in v2:
- Fixed wrong commit prefix.

 drivers/gpu/drm/imagination/pvr_vm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
index e1ec60f34b6e6..eea88e7ad03c1 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;
@@ -754,9 +755,11 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct pvr_gem_object *pvr_obj,
 	err = pvr_vm_bind_op_exec(&bind_op);
 
 	drm_gpuvm_exec_unlock(&vm_exec);
+	mutex_unlock(&vm_ctx->lock);
 
 err_cleanup:
 	pvr_vm_bind_op_fini(&bind_op);
+	mutex_unlock(&vm_ctx->lock);
 
 	return err;
 }
-- 
2.52.0

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

[Brajesh Gupta]

On Wed, 2026-04-22 at 01:57 +0800, Icenowy Zheng wrote:
Hi Icenowy,
> 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.
> 
> As all occurences of drm_gpuva_find*() is already guarded by
> vm_ctx->lock, make pvr_vm_map() to acquire this lock to prevent
> disturbing any find operation.
> 
> This fixes occasional NULL deference in drm_gpuva_find*().
> 
> Cc: stable@vger.kernel.org
> Fixes: 4bc736f890ce ("drm/imagination: vm: make use of GPUVM's drm_exec helper")
> Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
> ---
> Changes in v2:
> - Fixed wrong commit prefix.
> 
>  drivers/gpu/drm/imagination/pvr_vm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index e1ec60f34b6e6..eea88e7ad03c1 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;
> @@ -754,9 +755,11 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct pvr_gem_object *pvr_obj,
>  	err = pvr_vm_bind_op_exec(&bind_op);
>  
>  	drm_gpuvm_exec_unlock(&vm_exec);
> +	mutex_unlock(&vm_ctx->lock);
Can you drop extra mutex_unlock() from here?
>  
>  err_cleanup:
>  	pvr_vm_bind_op_fini(&bind_op);
> +	mutex_unlock(&vm_ctx->lock);
And move this unlock call before pvr_vm_bind_op_fini() call.

Thanks,
Brajesh
>  
>  	return err;
>  }

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

[Alessio Belle]

Hi Icenowy,

On Wed, 2026-04-22 at 01:57 +0800, Icenowy Zheng wrote:
> 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.
> 
> As all occurences of drm_gpuva_find*() is already guarded by

nit: is -> are

> vm_ctx->lock, make pvr_vm_map() to acquire this lock to prevent
> disturbing any find operation.
> 
> This fixes occasional NULL deference in drm_gpuva_find*().

nit: NULL deference -> NULL [pointer] dereference

Do you still have kernel logs following this bug?

> 
> Cc: stable@vger.kernel.org
> Fixes: 4bc736f890ce ("drm/imagination: vm: make use of GPUVM's drm_exec helper")

As far as I can see, that commit swapped one way of locking resources with
another, but the problem of VA find/map/unmap operations not being protected by
the same lock already existed in commit ff5f643de0bf ("drm/imagination: Add GEM
and VM related code").

Thanks,
Alessio

> Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
> ---
> Changes in v2:
> - Fixed wrong commit prefix.
> 
>  drivers/gpu/drm/imagination/pvr_vm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index e1ec60f34b6e6..eea88e7ad03c1 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;
> @@ -754,9 +755,11 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct pvr_gem_object *pvr_obj,
>  	err = pvr_vm_bind_op_exec(&bind_op);
>  
>  	drm_gpuvm_exec_unlock(&vm_exec);
> +	mutex_unlock(&vm_ctx->lock);
>  
>  err_cleanup:
>  	pvr_vm_bind_op_fini(&bind_op);
> +	mutex_unlock(&vm_ctx->lock);
>  
>  	return err;
>  }