[PATCH v5 00/10] drm/msm: A couple lazy-vm fixes

[PATCH v5 00/10] drm/msm: A couple lazy-vm fixes

[Rob Clark]

The VM is created lazily to give userspace a chance to opt-in to
VM_BIND.  But Sashiko noticed a couple paths that were not handling
this properly.

Rob Clark (10):
  drm/msm: Fix barriers accessing ctx vm
  drm/msm: Rework queuelock
  drm/msm: Synchronize VM creation on ctxlock
  drm/msm/a6xx: Access VM directly in submit path
  drm/msm: Add helper to check for per-process pgtables VM
  drm/msm: Allow lazy VM creation to fail
  drm/msm: Don't fallback to shared VM for VM_BIND
  drm/msm: Validate lazy VM in GEM_NEW
  drm/msm: Fix per-process-pgtables check
  drm/msm: Remove invalid overflow check

 drivers/gpu/drm/msm/adreno/a6xx_gpu.c   |  2 +-
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 17 ++++++++++++----
 drivers/gpu/drm/msm/msm_drv.c           | 26 ++++++++++++++++---------
 drivers/gpu/drm/msm/msm_gem.c           | 11 ++++++++++-
 drivers/gpu/drm/msm/msm_gem_submit.c    | 18 ++++++++---------
 drivers/gpu/drm/msm/msm_gem_vma.c       | 12 ++++++++----
 drivers/gpu/drm/msm/msm_gpu.c           |  2 +-
 drivers/gpu/drm/msm/msm_gpu.h           |  4 ++--
 drivers/gpu/drm/msm/msm_submitqueue.c   | 22 ++++++++++-----------
 9 files changed, 71 insertions(+), 43 deletions(-)

-- 
2.55.0

[PATCH v5 01/10] drm/msm: Fix barriers accessing ctx vm

[Rob Clark]

Don't rely on store ordering to protect us from caller seeing a
partially initialized vm.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: feb8ef4636a4 ("drm/msm: Add opt-in for VM_BIND")
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_drv.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 32d5ebea2596..ec88155e0ed7 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -224,18 +224,19 @@ struct drm_gpuvm *msm_context_vm(struct drm_device *dev, struct msm_context *ctx
 {
 	static DEFINE_MUTEX(init_lock);
 	struct msm_drm_private *priv = dev->dev_private;
+	struct drm_gpuvm *vm = smp_load_acquire(&ctx->vm);
 
 	/* Once ctx->vm is created it is valid for the lifetime of the context: */
-	if (ctx->vm)
-		return ctx->vm;
+	if (vm)
+		return vm;
+
+	guard(mutex)(&init_lock);
 
-	mutex_lock(&init_lock);
 	if (!ctx->vm) {
-		ctx->vm = msm_gpu_create_private_vm(
+		vm = msm_gpu_create_private_vm(
 			priv->gpu, current, !ctx->userspace_managed_vm);
-
+		smp_store_release(&ctx->vm, vm);
 	}
-	mutex_unlock(&init_lock);
 
 	return ctx->vm;
 }
-- 
2.55.0

Re: [PATCH v5 01/10] drm/msm: Fix barriers accessing ctx vm

[sashiko-bot]

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

Pre-existing issues:
- [High] MSM VM Lazy Initialization Violation / NULL Pointer Dereference in `msm_ioctl_gem_submit` and `msm_gem_new_handle`
- [High] Incomplete barrier fix: `msm_gem_close()` performs a lockless plain read of `ctx->vm` leading to a data race
--

--- Patch [1]: [PATCH v5 01/10] drm/msm: Fix barriers accessing ctx vm ---
commit fdc4f5db0f02ec8c94395cb949b516d777a3923b
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Fix barriers accessing ctx vm

This patch fixes a race condition by using smp_load_acquire and
smp_store_release when accessing ctx->vm. This prevents callers from
seeing a partially initialized vm.

> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 79f78e7e16e9d..3c8b5083a1fbe 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -225,18 +225,19 @@ struct drm_gpuvm *msm_context_vm(struct drm_device *dev, struct msm_context *ctx
>  {
>  	static DEFINE_MUTEX(init_lock);
>  	struct msm_drm_private *priv = dev->dev_private;
> +	struct drm_gpuvm *vm = smp_load_acquire(&ctx->vm);

[Severity: High]
This isn't a bug introduced by this patch, but since we are fixing the
barriers for ctx->vm here, do we also need to update msm_gem_close()?

It appears to perform a lockless plain read of ctx->vm:

drivers/gpu/drm/msm/msm_gem.c:msm_gem_close() {
	...
	if (!ctx->vm)
		return;

	/* ... */
	msm_gem_lock_vm_and_obj(&exec, obj, ctx->vm);
	...
}

Could this race with the smp_store_release() during VM creation and
observe a partially initialized VM object?

[Severity: High]
This is a pre-existing issue, but I noticed there are still direct accesses
to ctx->vm instead of using msm_context_vm() for lazy initialization.

For example, in msm_ioctl_gem_submit():

drivers/gpu/drm/msm/msm_gem_submit.c:msm_ioctl_gem_submit() {
	...
	if (to_msm_vm(ctx->vm)->unusable)
	...
}

And also in msm_gem_new_handle():

drivers/gpu/drm/msm/msm_gem.c:msm_gem_new_handle() {
	...
	struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(ctx->vm);
	...
}

If ctx->vm is NULL before initialization, could these direct accesses
result in a NULL pointer dereference?

>  
>  	/* Once ctx->vm is created it is valid for the lifetime of the context: */
> -	if (ctx->vm)
> -		return ctx->vm;
> +	if (vm)
> +		return vm;
> +
> +	guard(mutex)(&init_lock);
>  
> -	mutex_lock(&init_lock);
>  	if (!ctx->vm) {
> -		ctx->vm = msm_gpu_create_private_vm(
> +		vm = msm_gpu_create_private_vm(
>  			priv->gpu, current, !ctx->userspace_managed_vm);
> -
> +		smp_store_release(&ctx->vm, vm);
>  	}
> -	mutex_unlock(&init_lock);
>  
>  	return ctx->vm;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716201538.365824-1-robin.clark@oss.qualcomm.com?part=1

[PATCH v5 02/10] drm/msm: Rework queuelock

[Rob Clark]

Rename to ctxlock, and use cleanup guards to manage releasing the lock.
This will let us re-use it for other per-context read/write serial-
ization, such as VM creation.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_drv.c         |  2 +-
 drivers/gpu/drm/msm/msm_gpu.h         |  4 ++--
 drivers/gpu/drm/msm/msm_submitqueue.c | 14 +++-----------
 3 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index ec88155e0ed7..384f32f4d0c2 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -251,7 +251,7 @@ static int context_init(struct drm_device *dev, struct drm_file *file)
 		return -ENOMEM;
 
 	INIT_LIST_HEAD(&ctx->submitqueues);
-	rwlock_init(&ctx->queuelock);
+	init_rwsem(&ctx->ctxlock);
 
 	kref_init(&ctx->ref);
 	msm_submitqueue_init(dev, ctx);
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 6c83b8cbbb90..d27d54bdb7a7 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -392,8 +392,8 @@ msm_gpu_sysprof_no_ifpc(struct msm_gpu *gpu)
  * struct msm_context - per-drm_file context
  */
 struct msm_context {
-	/** @queuelock: synchronizes access to submitqueues list */
-	rwlock_t queuelock;
+	/** @ctxlock: synchronizes access to submitqueues list, etc */
+	struct rw_semaphore ctxlock;
 	/** @submitqueues: list of &msm_gpu_submitqueue created by userspace */
 	struct list_head submitqueues;
 	/**
diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
index 1a5a77b28016..8b5980d5a6a7 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -93,18 +93,15 @@ struct msm_gpu_submitqueue *msm_submitqueue_get(struct msm_context *ctx,
 	if (!ctx)
 		return NULL;
 
-	read_lock(&ctx->queuelock);
+	guard(rwsem_read)(&ctx->ctxlock);
 
 	list_for_each_entry(entry, &ctx->submitqueues, node) {
 		if (entry->id == id) {
 			kref_get(&entry->ref);
-			read_unlock(&ctx->queuelock);
-
 			return entry;
 		}
 	}
 
-	read_unlock(&ctx->queuelock);
 	return NULL;
 }
 
@@ -237,7 +234,7 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_context *ctx,
 		return ret;
 	}
 
-	write_lock(&ctx->queuelock);
+	guard(rwsem_write)(&ctx->ctxlock);
 
 	queue->ctx = msm_context_get(ctx);
 	queue->id = ctx->queueid++;
@@ -251,8 +248,6 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_context *ctx,
 
 	list_add_tail(&queue->node, &ctx->submitqueues);
 
-	write_unlock(&ctx->queuelock);
-
 	return 0;
 }
 
@@ -335,19 +330,16 @@ int msm_submitqueue_remove(struct msm_context *ctx, u32 id)
 	if (!id)
 		return -ENOENT;
 
-	write_lock(&ctx->queuelock);
+	guard(rwsem_write)(&ctx->ctxlock);
 
 	list_for_each_entry(entry, &ctx->submitqueues, node) {
 		if (entry->id == id) {
 			list_del(&entry->node);
-			write_unlock(&ctx->queuelock);
-
 			msm_submitqueue_put(entry);
 			return 0;
 		}
 	}
 
-	write_unlock(&ctx->queuelock);
 	return -ENOENT;
 }
 
-- 
2.55.0

Re: [PATCH v5 02/10] drm/msm: Rework queuelock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The conversion to `guard(rwsem_write)` in `msm_submitqueue_remove()` extends the lock's scope to cover `msm_submitqueue_put()`, leading to lock contention and potential deadlocks.
--

--- Patch [2]: [PATCH v5 02/10] drm/msm: Rework queuelock ---
commit 578ea2d7dbb1b26cbd22beab7c73963f3cc195e7
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Rework queuelock

This commit renames queuelock to ctxlock and changes it from a rwlock
to a rw_semaphore. It also introduces cleanup guards to manage releasing
the lock.

> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> index 1a5a77b280163..8b5980d5a6a79 100644
> --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
[ ... ]
> @@ -335,19 +330,16 @@ int msm_submitqueue_remove(struct msm_context *ctx, u32 id)
>  	if (!id)
>  		return -ENOENT;
>  
> -	write_lock(&ctx->queuelock);
> +	guard(rwsem_write)(&ctx->ctxlock);
>  
>  	list_for_each_entry(entry, &ctx->submitqueues, node) {
>  		if (entry->id == id) {
>  			list_del(&entry->node);
> -			write_unlock(&ctx->queuelock);
> -
>  			msm_submitqueue_put(entry);

[Severity: High]
Does extending the lock's scope over msm_submitqueue_put() create a deadlock
risk?

By using guard(rwsem_write), the ctxlock remains held until the function
returns. Previously, the lock was explicitly released before calling
msm_submitqueue_put().

For VM_BIND queues, dropping the last reference can call
drm_sched_entity_destroy(), which blocks in drm_sched_entity_flush() waiting
for pending jobs to finish. If userspace submits a job with an in-fence
dependency and then removes the queue, the kernel will wait inside
msm_submitqueue_put() while still holding the write lock. 

If another thread needs the ctxlock to signal the dependency or perform other
IOCTLs on the same context, could this result in a permanent deadlock?

>  			return 0;
>  		}
>  	}
>  
> -	write_unlock(&ctx->queuelock);
>  	return -ENOENT;
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716201538.365824-1-robin.clark@oss.qualcomm.com?part=2

[PATCH v5 03/10] drm/msm: Synchronize VM creation on ctxlock

[Rob Clark]

And serialize setting EN_VM_BIND against VM creation.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 5 ++++-
 drivers/gpu/drm/msm/msm_drv.c           | 3 +--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index c62c45bb0ddb..0d54141ff089 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -503,7 +503,9 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_context *ctx,
 		if (!perfmon_capable())
 			return UERR(EPERM, drm, "invalid permissions");
 		return msm_context_set_sysprof(ctx, gpu, value);
-	case MSM_PARAM_EN_VM_BIND:
+	case MSM_PARAM_EN_VM_BIND: {
+		guard(rwsem_read)(&ctx->ctxlock);
+
 		/* We can only support VM_BIND with per-process pgtables: */
 		if (ctx->vm == gpu->vm)
 			return UERR(EINVAL, drm, "requires per-process pgtables");
@@ -518,6 +520,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_context *ctx,
 		ctx->userspace_managed_vm = value;
 
 		return 0;
+	}
 	default:
 		return UERR(EINVAL, drm, "%s: invalid param: %u", gpu->name, param);
 	}
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 384f32f4d0c2..2fd1b717d8e6 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -222,7 +222,6 @@ static void load_gpu(struct drm_device *dev)
  */
 struct drm_gpuvm *msm_context_vm(struct drm_device *dev, struct msm_context *ctx)
 {
-	static DEFINE_MUTEX(init_lock);
 	struct msm_drm_private *priv = dev->dev_private;
 	struct drm_gpuvm *vm = smp_load_acquire(&ctx->vm);
 
@@ -230,7 +229,7 @@ struct drm_gpuvm *msm_context_vm(struct drm_device *dev, struct msm_context *ctx
 	if (vm)
 		return vm;
 
-	guard(mutex)(&init_lock);
+	guard(rwsem_write)(&ctx->ctxlock);
 
 	if (!ctx->vm) {
 		vm = msm_gpu_create_private_vm(
-- 
2.55.0

Re: [PATCH v5 03/10] drm/msm: Synchronize VM creation on ctxlock

[sashiko-bot]

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

Pre-existing issues:
- [High] Uninitialized pointer dereference in msm_ioctl_gem_submit bypassing lazy VM initialization
- [High] Privilege escalation via bypassed VM_BIND hardware capability check
--

--- Patch [3]: [PATCH v5 03/10] drm/msm: Synchronize VM creation on ctxlock ---
commit 7a10b1ce7cb7f72d046e9af19750da0d0ced94cb
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Synchronize VM creation on ctxlock

This commit synchronizes virtual memory creation using ctxlock rather
than init_lock. It also adds a read lock when setting EN_VM_BIND to
serialize it against VM creation.

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
[ ... ]
> @@ -503,7 +503,9 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_context *ctx,
>  		if (!perfmon_capable())
>  			return UERR(EPERM, drm, "invalid permissions");
>  		return msm_context_set_sysprof(ctx, gpu, value);
> -	case MSM_PARAM_EN_VM_BIND:
> +	case MSM_PARAM_EN_VM_BIND: {
> +		guard(rwsem_read)(&ctx->ctxlock);
> +
>  		/* We can only support VM_BIND with per-process pgtables: */
>  		if (ctx->vm == gpu->vm)
>  			return UERR(EINVAL, drm, "requires per-process pgtables");

[Severity: High]
This is a pre-existing issue, but does checking ctx->vm == gpu->vm
properly enforce the per-process pgtable requirement? 

Since the VM is lazily initialized, wouldn't ctx->vm be NULL here?
(This appears to be expected by the subsequent if (ctx->vm) check).

Because ctx->vm is NULL, it will never equal gpu->vm, which seems to
completely bypass this hardware capability check. If a legacy device lacks
create_private_vm, couldn't an unprivileged user bypass this check and gain
userspace-managed control over the global GPU page tables when the VM is
later created?


While reviewing the lazy VM initialization semantics in this subsystem, I
noticed another pre-existing issue in a related file not touched by this
patch:

> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -571,4 +571,5 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>  
>  	if (args->pad)
>  		return -EINVAL;
>  
>  	if (to_msm_vm(ctx->vm)->unusable)
>  		return UERR(EPIPE, dev, "context is unusable");

[Severity: High]
This is a pre-existing issue, but does msm_ioctl_gem_submit() safely access
the virtual memory context here?

If a user opens the DRM device to create a new context and immediately issues
a DRM_IOCTL_MSM_GEM_SUBMIT ioctl, ctx->vm will still be NULL because it has
not yet been lazily initialized via msm_context_vm().

Could this direct dereference of ctx->vm cause a reliable kernel panic when
evaluated?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716201538.365824-1-robin.clark@oss.qualcomm.com?part=3

[PATCH v5 04/10] drm/msm/a6xx: Access VM directly in submit path

[Rob Clark]

The GEM_SUBMIT ioctl has already ensured that the VM is created, so we
aren't expecting to lazily create the VM this deep into the ioctl.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 8b3bb2fd433b..7f1673163f67 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -229,7 +229,7 @@ static void a6xx_set_pagetable(struct a6xx_gpu *a6xx_gpu,
 {
 	bool sysprof = msm_gpu_sysprof_no_perfcntr_zap(&a6xx_gpu->base.base);
 	struct msm_context *ctx = submit->queue->ctx;
-	struct drm_gpuvm *vm = msm_context_vm(submit->dev, ctx);
+	struct drm_gpuvm *vm = ctx->vm;
 	struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
 	phys_addr_t ttbr;
 	u32 asid;
-- 
2.55.0

Re: [PATCH v5 04/10] drm/msm/a6xx: Access VM directly in submit path

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Direct access to uninitialized `ctx->vm` during early ioctl validation causes a NULL pointer dereference.
--

--- Patch [4]: [PATCH v5 04/10] drm/msm/a6xx: Access VM directly in submit path ---
commit 006ce0b84643f1999004dcef9a0a6a167c16efae
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/a6xx: Access VM directly in submit path

This commit changes the a6xx_set_pagetable() function to access the
context VM directly (ctx->vm) instead of calling msm_context_vm(). It
relies on the assumption that the GEM_SUBMIT ioctl has already ensured
the VM is created by the time this code is reached.

> drm/msm/a6xx: Access VM directly in submit path
> 
> The GEM_SUBMIT ioctl has already ensured that the VM is created, so we
> aren't expecting to lazily create the VM this deep into the ioctl.

[Severity: Critical]
This is a pre-existing issue, but does the GEM_SUBMIT ioctl actually ensure
the VM is created?

Looking at msm_ioctl_gem_submit() in drivers/gpu/drm/msm/msm_gem_submit.c,
the early validation path accesses the VM directly without calling
msm_context_vm() to ensure it has been created:

drivers/gpu/drm/msm/msm_gem_submit.c:msm_ioctl_gem_submit() {
    ...
	if (to_msm_vm(ctx->vm)->unusable)
		return UERR(EPIPE, dev, "context is unusable");
    ...
}

Can unprivileged userspace trigger a NULL pointer dereference by calling the
MSM_GEM_SUBMIT ioctl on a fresh DRM device descriptor where the context VM
has not been lazily initialized yet?

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 8b3bb2fd433ba..7f1673163f671 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -229,7 +229,7 @@ static void a6xx_set_pagetable(struct a6xx_gpu *a6xx_gpu,
>  {
>  	bool sysprof = msm_gpu_sysprof_no_perfcntr_zap(&a6xx_gpu->base.base);
>  	struct msm_context *ctx = submit->queue->ctx;
> -	struct drm_gpuvm *vm = msm_context_vm(submit->dev, ctx);
> +	struct drm_gpuvm *vm = ctx->vm;
>  	struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
>  	phys_addr_t ttbr;
>  	u32 asid;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716201538.365824-1-robin.clark@oss.qualcomm.com?part=4

[PATCH v5 05/10] drm/msm: Add helper to check for per-process pgtables VM

[Rob Clark]

This will simplify a following commit to allow lazy VM creation to fail.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 0d54141ff089..5738eac9aebd 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -356,6 +356,12 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned long iova, int flags,
 	return 0;
 }
 
+static bool
+valid_per_process_vm(struct msm_gpu *gpu, struct drm_gpuvm *vm)
+{
+	return (vm != gpu->vm);
+}
+
 int adreno_get_param(struct msm_gpu *gpu, struct msm_context *ctx,
 		     uint32_t param, uint64_t *value, uint32_t *len)
 {
@@ -414,12 +420,12 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_context *ctx,
 		*value = gpu->suspend_count;
 		return 0;
 	case MSM_PARAM_VA_START:
-		if (vm == gpu->vm)
+		if (!valid_per_process_vm(gpu, vm))
 			return UERR(EINVAL, drm, "requires per-process pgtables");
 		*value = vm->mm_start;
 		return 0;
 	case MSM_PARAM_VA_SIZE:
-		if (vm == gpu->vm)
+		if (!valid_per_process_vm(gpu, vm))
 			return UERR(EINVAL, drm, "requires per-process pgtables");
 		*value = vm->mm_range;
 		return 0;
-- 
2.55.0

[PATCH v5 06/10] drm/msm: Allow lazy VM creation to fail

[Rob Clark]

In the next commit, we'll stop falling back to shared VM if private VM
creation fails.

This isn't expected to happen in practice, it would either require small
memory allocations to fail, or missing support in arm-smmu-qcom for
setting up per-process pgtable support (ie. missing patch during
bringup).

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c |  2 +-
 drivers/gpu/drm/msm/msm_drv.c           |  9 ++++++++-
 drivers/gpu/drm/msm/msm_gem_submit.c    | 12 ++++++++----
 drivers/gpu/drm/msm/msm_gem_vma.c       | 12 ++++++++----
 drivers/gpu/drm/msm/msm_submitqueue.c   |  8 +++++++-
 5 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 5738eac9aebd..84101e534ebe 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -359,7 +359,7 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned long iova, int flags,
 static bool
 valid_per_process_vm(struct msm_gpu *gpu, struct drm_gpuvm *vm)
 {
-	return (vm != gpu->vm);
+	return vm && (vm != gpu->vm);
 }
 
 int adreno_get_param(struct msm_gpu *gpu, struct msm_context *ctx,
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 2fd1b717d8e6..52ff72d8e17a 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -422,10 +422,14 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev,
 {
 	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_context *ctx = file->driver_priv;
+	struct drm_gpuvm *vm = msm_context_vm(dev, ctx);
 
 	if (!priv->gpu)
 		return -EINVAL;
 
+	if (!vm)
+		return UERR(ENOMEM, dev, "no VM");
+
 	if (msm_context_is_vmbind(ctx))
 		return UERR(EINVAL, dev, "VM_BIND is enabled");
 
@@ -436,7 +440,7 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev,
 	 * Don't pin the memory here - just get an address so that userspace can
 	 * be productive
 	 */
-	return msm_gem_get_iova(obj, msm_context_vm(dev, ctx), iova);
+	return msm_gem_get_iova(obj, vm, iova);
 }
 
 static int msm_ioctl_gem_info_set_iova(struct drm_device *dev,
@@ -450,6 +454,9 @@ static int msm_ioctl_gem_info_set_iova(struct drm_device *dev,
 	if (!priv->gpu)
 		return -EINVAL;
 
+	if (!vm)
+		return UERR(ENOMEM, dev, "no VM");
+
 	if (msm_context_is_vmbind(ctx))
 		return UERR(EINVAL, dev, "VM_BIND is enabled");
 
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 3c6bc90c3d48..ab33d499a0e8 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -30,7 +30,7 @@
  */
 
 static struct msm_gem_submit *submit_create(struct drm_device *dev,
-		struct msm_gpu *gpu,
+		struct msm_gpu *gpu, struct drm_gpuvm *vm,
 		struct msm_gpu_submitqueue *queue, uint32_t nr_bos,
 		uint32_t nr_cmds, u64 drm_client_id)
 {
@@ -66,7 +66,7 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
 
 	kref_init(&submit->ref);
 	submit->dev = dev;
-	submit->vm = msm_context_vm(dev, queue->ctx);
+	submit->vm = vm;
 	submit->gpu = gpu;
 	submit->cmd = (void *)&submit->bos[nr_bos];
 	submit->queue = queue;
@@ -554,6 +554,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
 	struct msm_drm_private *priv = dev->dev_private;
 	struct drm_msm_gem_submit *args = data;
 	struct msm_context *ctx = file->driver_priv;
+	struct drm_gpuvm *vm = msm_context_vm(dev, ctx);
 	struct msm_gem_submit *submit = NULL;
 	struct msm_gpu *gpu = priv->gpu;
 	struct msm_gpu_submitqueue *queue;
@@ -569,10 +570,13 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
 	if (!gpu)
 		return -ENXIO;
 
+	if (!vm)
+		return UERR(ENOMEM, dev, "no VM");
+
 	if (args->pad)
 		return -EINVAL;
 
-	if (to_msm_vm(ctx->vm)->unusable)
+	if (to_msm_vm(vm)->unusable)
 		return UERR(EPIPE, dev, "context is unusable");
 
 	/* for now, we just have 3d pipe.. eventually this would need to
@@ -609,7 +613,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
 		}
 	}
 
-	submit = submit_create(dev, gpu, queue, args->nr_bos, args->nr_cmds,
+	submit = submit_create(dev, gpu, vm, queue, args->nr_bos, args->nr_cmds,
 			       file->client_id);
 	if (IS_ERR(submit)) {
 		ret = PTR_ERR(submit);
diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
index 3ed05ab0eeef..1fd4789f2be3 100644
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -954,7 +954,7 @@ msm_gem_vm_close(struct drm_gpuvm *gpuvm)
 
 
 static struct msm_vm_bind_job *
-vm_bind_job_create(struct drm_device *dev, struct drm_file *file,
+vm_bind_job_create(struct drm_device *dev, struct drm_file *file, struct drm_gpuvm *vm,
 		   struct msm_gpu_submitqueue *queue, uint32_t nr_ops)
 {
 	struct msm_vm_bind_job *job;
@@ -971,7 +971,7 @@ vm_bind_job_create(struct drm_device *dev, struct drm_file *file,
 		return ERR_PTR(ret);
 	}
 
-	job->vm = msm_context_vm(dev, queue->ctx);
+	job->vm = vm;
 	job->queue = queue;
 	INIT_LIST_HEAD(&job->vm_ops);
 
@@ -1430,6 +1430,7 @@ msm_ioctl_vm_bind(struct drm_device *dev, void *data, struct drm_file *file)
 	struct msm_drm_private *priv = dev->dev_private;
 	struct drm_msm_vm_bind *args = data;
 	struct msm_context *ctx = file->driver_priv;
+	struct drm_gpuvm *vm = msm_context_vm(dev, ctx);
 	struct msm_vm_bind_job *job = NULL;
 	struct msm_gpu *gpu = priv->gpu;
 	struct msm_gpu_submitqueue *queue;
@@ -1444,11 +1445,14 @@ msm_ioctl_vm_bind(struct drm_device *dev, void *data, struct drm_file *file)
 	if (!gpu)
 		return -ENXIO;
 
+	if (!vm)
+		return UERR(ENOMEM, dev, "no VM");
+
 	/*
 	 * Maybe we could allow just UNMAP ops?  OTOH userspace should just
 	 * immediately close the device file and all will be torn down.
 	 */
-	if (to_msm_vm(msm_context_vm(dev, ctx))->unusable)
+	if (to_msm_vm(vm)->unusable)
 		return UERR(EPIPE, dev, "context is unusable");
 
 	/*
@@ -1479,7 +1483,7 @@ msm_ioctl_vm_bind(struct drm_device *dev, void *data, struct drm_file *file)
 		}
 	}
 
-	job = vm_bind_job_create(dev, file, queue, args->nr_ops);
+	job = vm_bind_job_create(dev, file, vm, queue, args->nr_ops);
 	if (IS_ERR(job)) {
 		ret = PTR_ERR(job);
 		goto out_post_unlock;
diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
index 8b5980d5a6a7..9a4c4e6425bf 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -172,6 +172,7 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_context *ctx,
 	struct msm_drm_private *priv = drm->dev_private;
 	struct msm_gpu_submitqueue *queue;
 	enum drm_sched_priority sched_prio;
+	struct drm_gpuvm *vm = NULL;
 	unsigned ring_nr;
 	int ret;
 
@@ -184,6 +185,11 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_context *ctx,
 	if (flags & MSM_SUBMITQUEUE_VM_BIND) {
 		unsigned sz;
 
+		vm = msm_context_vm(drm, ctx);
+
+		if (!vm)
+			return UERR(ENOMEM, drm, "no VM");
+
 		/* Not allowed for kernel managed VMs (ie. kernel allocs VA) */
 		if (!msm_context_is_vmbind(ctx))
 			return -EINVAL;
@@ -215,7 +221,7 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_context *ctx,
 	queue->flags = flags;
 
 	if (flags & MSM_SUBMITQUEUE_VM_BIND) {
-		struct drm_gpu_scheduler *sched = &to_msm_vm(msm_context_vm(drm, ctx))->sched;
+		struct drm_gpu_scheduler *sched = &to_msm_vm(vm)->sched;
 
 		queue->entity = &queue->_vm_bind_entity[0];
 
-- 
2.55.0

Re: [PATCH v5 06/10] drm/msm: Allow lazy VM creation to fail

[sashiko-bot]

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

Pre-existing issues:
- [High] Direct access to `ctx->vm` via `drm_gpuvm_resv_obj()` in `msm_gem_new_handle()` causes a NULL pointer dereference when called before the VM is lazily initialized.
--

--- Patch [6]: [PATCH v5 06/10] drm/msm: Allow lazy VM creation to fail ---
commit b8fda3fb0d624d38526d9746f2057050a7931a85
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Allow lazy VM creation to fail

In the next commit, we'll stop falling back to shared VM if private VM
creation fails. This isn't expected to happen in practice, it would either
require small memory allocations to fail, or missing support in arm-smmu-qcom.

> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 26ea8a28be474..06b142d82e4d0 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
[ ... ]
> @@ -554,6 +554,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>  	struct msm_drm_private *priv = dev->dev_private;
>  	struct drm_msm_gem_submit *args = data;
>  	struct msm_context *ctx = file->driver_priv;
> +	struct drm_gpuvm *vm = msm_context_vm(dev, ctx);

[Severity: High]
This is a pre-existing issue, but seeing msm_context_vm() added here made
me look at other places where ctx->vm is accessed, and I noticed a potential
issue in msm_gem_new_handle().

If a user opens the DRM device and directly issues the DRM_IOCTL_MSM_GEM_NEW
ioctl with the MSM_BO_NO_SHARE flag before the VM is lazily initialized
(e.g., before creating a submitqueue), could this result in a NULL pointer
dereference?

> drivers/gpu/drm/msm/msm_gem.c:msm_gem_new_handle() {
> 	...
> 	if (flags & MSM_BO_NO_SHARE) {
> 		struct msm_context *ctx = file->driver_priv;
> 		struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(ctx->vm);

Because drm_gpuvm_resv_obj(ctx->vm) directly dereferences the pointer
without calling msm_context_vm() first to ensure it is created, it seems
this could crash the kernel if ctx->vm is still NULL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716201538.365824-1-robin.clark@oss.qualcomm.com?part=6

[PATCH v5 07/10] drm/msm: Don't fallback to shared VM for VM_BIND

[Rob Clark]

If the user wants a userspace managed VM (EN_VM_BIND) don't silently
fall back to shared VM.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_drv.c | 3 ++-
 drivers/gpu/drm/msm/msm_gpu.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 52ff72d8e17a..73d99bde26f1 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -234,7 +234,8 @@ struct drm_gpuvm *msm_context_vm(struct drm_device *dev, struct msm_context *ctx
 	if (!ctx->vm) {
 		vm = msm_gpu_create_private_vm(
 			priv->gpu, current, !ctx->userspace_managed_vm);
-		smp_store_release(&ctx->vm, vm);
+		if (!IS_ERR_OR_NULL(vm))
+			smp_store_release(&ctx->vm, vm);
 	}
 
 	return ctx->vm;
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 18ed00e5f143..d02c37380414 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -875,7 +875,7 @@ msm_gpu_create_private_vm(struct msm_gpu *gpu, struct task_struct *task,
 			to_msm_vm(vm)->pid = get_pid(task_pid(task));
 	}
 
-	if (IS_ERR_OR_NULL(vm))
+	if (IS_ERR_OR_NULL(vm) && !kernel_managed)
 		vm = drm_gpuvm_get(gpu->vm);
 
 	return vm;
-- 
2.55.0

Re: [PATCH v5 07/10] drm/msm: Don't fallback to shared VM for VM_BIND

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The fallback condition for `vm` in `msm_gpu_create_private_vm` incorrectly uses `!kernel_managed` instead of `kernel_managed`, breaking legacy contexts on older hardware and failing to prevent fallback for VM_BIND contexts.
--

--- Patch [7]: [PATCH v5 07/10] drm/msm: Don't fallback to shared VM for VM_BIND ---
commit fff36b547a5d97354f5a0baebf86eafdd34bd4f8
Author: Rob Clark <robin.clark@oss.qualcomm.com>
Date:   Thu Jul 16 20:15:30 2026 +0000

drm/msm: Don't fallback to shared VM for VM_BIND

This commit modifies the fallback logic for VM creation to prevent userspace 
managed VMs (VM_BIND) from falling back to the shared kernel-managed VM if 
private VM creation fails.

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 18ed00e5f143b..d02c37380414b 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -875,7 +875,7 @@ msm_gpu_create_private_vm(struct msm_gpu *gpu, struct task_struct *task,
>  			to_msm_vm(vm)->pid = get_pid(task_pid(task));
>  	}
>  
> -	if (IS_ERR_OR_NULL(vm))
> +	if (IS_ERR_OR_NULL(vm) && !kernel_managed)

[Severity: High]
Does this boolean logic operate backwards from the stated intent of the patch?

If a legacy application runs on older hardware (A5xx or prior) that lacks
private VM support, kernel_managed is true. The !kernel_managed check
evaluates to false, skipping the fallback and returning NULL or an error.
Would this break command submission on these platforms?

Conversely, if a VM_BIND context (kernel_managed == false) fails to create
a private VM due to memory pressure, the !kernel_managed check evaluates to
true. This causes it to fall back to the kernel-managed shared VM, allowing
the very behavior the patch was trying to prevent.

If that VM_BIND fallback occurs, could it trigger a kernel panic later?
Userspace could issue a VM_BIND ioctl with a non-zero offset against the
erroneously assigned kernel-managed VM, hitting this BUG_ON:

drivers/gpu/drm/msm/msm_gem_vma.c:msm_gem_vma_new() {
    ...
    if (vm->managed) {
        BUG_ON(offset != 0);
        BUG_ON(!obj);
    ...
}

>  		vm = drm_gpuvm_get(gpu->vm);
>  
>  	return vm;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716201538.365824-1-robin.clark@oss.qualcomm.com?part=7

[PATCH v5 08/10] drm/msm: Validate lazy VM in GEM_NEW

[Rob Clark]

Otherwise creating a _NO_SHARE BO before any BOs are mapped could cause
a NPE.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: b58e12a66e47 ("drm/msm: Add _NO_SHARE flag")
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index efd3d3c9a449..afe0373f693a 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1146,8 +1146,16 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 		msm_gem_object_set_name(obj, "%s", name);
 
 	if (flags & MSM_BO_NO_SHARE) {
+		struct msm_drm_private *priv = dev->dev_private;
 		struct msm_context *ctx = file->driver_priv;
-		struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(ctx->vm);
+		struct drm_gpuvm *vm = msm_context_vm(dev, ctx);
+
+		if (!priv->gpu || !vm) {
+			ret = UERR(EINVAL, dev, "not supported with shared VM");
+			goto out_put;
+		}
+
+		struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(vm);
 
 		drm_gem_object_get(r_obj);
 
@@ -1157,6 +1165,7 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 	ret = drm_gem_handle_create(file, obj, handle);
 
 	/* drop reference from allocate - handle holds it now */
+out_put:
 	drm_gem_object_put(obj);
 
 	return ret;
-- 
2.55.0

Re: [PATCH v5 08/10] drm/msm: Validate lazy VM in GEM_NEW

[sashiko-bot]

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

New issues:
- [High] The fallback logic for shared VMs in `msm_gpu_create_private_vm()` is inverted, causing VM_BIND contexts to incorrectly fall back to the shared VM while legacy contexts fail to do so.

Pre-existing issues:
- [High] Concurrent modification of `obj->resv` after publishing the GEM object to `priv->objects` causes a mismatched lock/unlock sequence if accessed via debugfs.
--

--- Patch [8]: [PATCH v5 08/10] drm/msm: Validate lazy VM in GEM_NEW ---
commit b2a817e93a22417a9189cd5ad2d48b886f18f14d
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Validate lazy VM in GEM_NEW

Otherwise creating a _NO_SHARE BO before any BOs are mapped could cause
a NPE.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 2cb3ab04f1250..9bdbcf1279b00 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -1147,8 +1147,16 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>  		msm_gem_object_set_name(obj, "%s", name);
>  
>  	if (flags & MSM_BO_NO_SHARE) {
> +		struct msm_drm_private *priv = dev->dev_private;
>  		struct msm_context *ctx = file->driver_priv;
> -		struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(ctx->vm);
> +		struct drm_gpuvm *vm = msm_context_vm(dev, ctx);

[Severity: High]
Does this call to msm_context_vm() trigger inverted fallback logic for shared
VMs in drivers/gpu/drm/msm/msm_gpu.c?

When msm_context_vm() evaluates !ctx->userspace_managed_vm, it passes this as
the kernel_managed parameter to msm_gpu_create_private_vm().

In msm_gpu_create_private_vm(), the fallback condition is:

    if (IS_ERR_OR_NULL(vm) && !kernel_managed)
        vm = drm_gpuvm_get(gpu->vm);

This causes VM_BIND contexts (where kernel_managed is false) to silently fall
back to the shared VM, directly violating the intent described in the commit
'drm/msm: Don't fallback to shared VM for VM_BIND'.

Conversely, for legacy contexts (where kernel_managed is true), the fallback
is skipped. This leaves vm as NULL on legacy targets that lack private VM
support, which would break context initialization completely.

> +
> +		if (!priv->gpu || !vm) {
> +			ret = UERR(EINVAL, dev, "not supported with shared VM");
> +			goto out_put;
> +		}
> +
> +		struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(vm);
>  
>  		drm_gem_object_get(r_obj);
>  
>  		obj->resv = r_obj->resv;

[Severity: High]
This is a pre-existing issue, but can this concurrent modification of
obj->resv cause a mismatched lock/unlock sequence if accessed via debugfs?

In drivers/gpu/drm/msm/msm_gem.c:msm_gem_new(), the BO is allocated,
obj->resv is initialized to &obj->_resv, and it is added to the global
priv->objects list under priv->obj_lock. This makes the object globally
visible.

If debugfs iterates over priv->objects concurrently via msm_gem_show(), it
calls msm_gem_describe(obj) which evaluates msm_gem_trylock(obj) and locks
the original &obj->_resv.

If the ioctl thread executing msm_gem_new_handle() then overwrites obj->resv
here without holding any locks, the subsequent msm_gem_unlock(obj) in debugfs
will unlock the new r_obj->resv (which it never locked).

This mismatched sequence would cause a kernel BUG in ww_mutex_unlock or a
lockdep splat.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716201538.365824-1-robin.clark@oss.qualcomm.com?part=8

[PATCH v5 09/10] drm/msm: Fix per-process-pgtables check

[Rob Clark]

ctx->vm should not be inialized yet (or if it has, an error is returned
immediately following this check), so this isn't a valid way to check
for per-process-pgtable support.

Instead just check if create_private_vm() is supported.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: feb8ef4636a4 ("drm/msm: Add opt-in for VM_BIND")
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 84101e534ebe..3370cd44382f 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -513,7 +513,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_context *ctx,
 		guard(rwsem_read)(&ctx->ctxlock);
 
 		/* We can only support VM_BIND with per-process pgtables: */
-		if (ctx->vm == gpu->vm)
+		if (!gpu->funcs->create_private_vm)
 			return UERR(EINVAL, drm, "requires per-process pgtables");
 
 		/*
-- 
2.55.0

Re: [PATCH v5 09/10] drm/msm: Fix per-process-pgtables check

[sashiko-bot]

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

New issues:
- [Critical] Logic inversion in fallback condition allows VM_BIND contexts to hijack the global shared GPU VM, while breaking legacy contexts on older hardware.

Pre-existing issues:
- [High] Missing lock protection when reading and updating `ctx->sysprof`, leading to refcount and pm_runtime leaks.
--

--- Patch [9]: [PATCH v5 09/10] drm/msm: Fix per-process-pgtables check ---
commit 439295368270634dcd942a5c715dc37574234eb9
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Fix per-process-pgtables check

ctx->vm should not be inialized yet (or if it has, an error is returned
immediately following this check), so this isn't a valid way to check
for per-process-pgtable support. Instead just check if create_private_vm()
is supported.

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 84101e534ebef..3370cd44382fe 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -513,7 +513,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_context *ctx,

[Severity: High]
This is a pre-existing issue, but does the MSM_PARAM_SYSPROF ioctl handler
in this function need to hold any context-specific locks before calling
msm_context_set_sysprof()?

drivers/gpu/drm/msm/msm_submitqueue.c:msm_context_set_sysprof() {
...
	/* unwind old value: */
	switch (ctx->sysprof) {
	case 2:
		pm_runtime_put_autosuspend(&gpu->pdev->dev);
		fallthrough;
	case 1:
		refcount_dec(&gpu->sysprof_active);
		fallthrough;
	case 0:
		break;
	}

	/* Some gpu families require additional setup for sysprof */
	if (gpu->funcs->sysprof_setup)
		gpu->funcs->sysprof_setup(gpu, false);

	ctx->sysprof = sysprof;
...
}

If two threads execute the MSM_PARAM_SYSPROF ioctl concurrently on the same
context, they both enter msm_context_set_sysprof() without any locks.
Both could increment the global refcounts based on the new value, but read
the same old ctx->sysprof value (e.g., 0) during the unwind phase.

Neither thread decrements the old refcounts, but both update ctx->sysprof.
When the context is closed, the refcounts are only decremented once,
leaving a permanent leak that prevents the GPU from powering down.

>  		guard(rwsem_read)(&ctx->ctxlock);
>  
>  		/* We can only support VM_BIND with per-process pgtables: */
> -		if (ctx->vm == gpu->vm)
> +		if (!gpu->funcs->create_private_vm)
>  			return UERR(EINVAL, drm, "requires per-process pgtables");

[Severity: Critical]
While looking at the VM_BIND changes, does the fallback logic in
msm_gpu_create_private_vm() accidentally invert its condition?

drivers/gpu/drm/msm/msm_gpu.c:msm_gpu_create_private_vm() {
...
	if (IS_ERR_OR_NULL(vm) && !kernel_managed)
		vm = drm_gpuvm_get(gpu->vm);
...
}

If private VM creation fails or is unsupported for a VM_BIND context
(where kernel_managed is false), this condition evaluates to true,
and the context falls back to the global shared GPU VM.

Could this allow userspace to hijack the global shared VM and modify
its pagetables?

Additionally, this logic inversion prevents legacy contexts (where
kernel_managed is true) from falling back to the global VM on older hardware,
causing initialization failures.

>  		/*

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716201538.365824-1-robin.clark@oss.qualcomm.com?part=9

[PATCH v5 10/10] drm/msm: Remove invalid overflow check

[Rob Clark]

On overflow struct_size() would return SIZE_MAX.  But kzalloc() (and
friends) check this already, so we can just remove the check.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem_submit.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index ab33d499a0e8..ddd52142c1cf 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -36,15 +36,11 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
 {
 	static atomic_t ident = ATOMIC_INIT(0);
 	struct msm_gem_submit *submit;
-	uint64_t sz;
+	size_t sz;
 	int ret;
 
 	sz = struct_size(submit, bos, nr_bos) +
 			((u64)nr_cmds * sizeof(submit->cmd[0]));
-
-	if (sz > SIZE_MAX)
-		return ERR_PTR(-ENOMEM);
-
 	submit = kzalloc(sz, GFP_KERNEL | __GFP_NOWARN);
 	if (!submit)
 		return ERR_PTR(-ENOMEM);
-- 
2.55.0

Re: [PATCH v5 10/10] drm/msm: Remove invalid overflow check

[sashiko-bot]

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

New issues:
- [Critical] Changing the allocation size variable `sz` from `uint64_t` to `size_t` and removing the `sz > SIZE_MAX` check introduces a critical integer truncation vulnerability on 32-bit systems, leading to a massive heap buffer overflow.

Pre-existing issues:
- [High] Relocations allocated in `submit_lookup_cmds` are permanently leaked if an error occurs before `submit->nr_cmds` is initialized later in the ioctl.
--

--- Patch [10]: [PATCH v5 10/10] drm/msm: Remove invalid overflow check ---
commit eb92124b933ee21b339fcbad2bb72fb24329057d
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Remove invalid overflow check

This commit removes the overflow check for the allocation size in
submit_create(), noting that struct_size() saturates at SIZE_MAX and
kzalloc() already handles it. It also changes the type of the sz
variable from uint64_t to size_t.

> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 06b142d82e4d0..d914602fd443a 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -36,15 +36,11 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
>  {
>  	static atomic_t ident = ATOMIC_INIT(0);
>  	struct msm_gem_submit *submit;
> -	uint64_t sz;
> +	size_t sz;
>  	int ret;
>  
>  	sz = struct_size(submit, bos, nr_bos) +
>  			((u64)nr_cmds * sizeof(submit->cmd[0]));
> -
> -	if (sz > SIZE_MAX)
> -		return ERR_PTR(-ENOMEM);
> -
>  	submit = kzalloc(sz, GFP_KERNEL | __GFP_NOWARN);

[Severity: Critical]
Does this change introduce an integer truncation on 32-bit systems?

Since sz is now a size_t, the 64-bit sum of struct_size() and the
nr_cmds multiplication can be silently truncated to 32 bits before being
passed to kzalloc().

For example, if a large nr_cmds like 0x8000000 is provided, the added size
could be 0x100000000, which truncates to 0 in a 32-bit size_t.

Could this cause kzalloc() to succeed with a smaller size than required,
resulting in an out-of-bounds write when setting submit->cmd[i] later?

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
msm_ioctl_gem_submit() leak the relocations?

Looking at submit_lookup_cmds(), memory is allocated for relocs:

    submit->cmd[i].relocs = kmalloc(sz, GFP_KERNEL | __GFP_NOWARN);

If an error occurs here or later in the ioctl, the code jumps to the error
path before submit->nr_cmds is initialized at the end of
msm_ioctl_gem_submit():

    submit->nr_cmds = args->nr_cmds;

Then __msm_gem_submit_destroy() will use the uninitialized submit->nr_cmds,
which is 0, for its cleanup loop:

    for (i = 0; i < submit->nr_cmds; i++)
        kfree(submit->cmd[i].relocs);

Could this result in permanently leaking all successfully allocated relocs
arrays if this error path is repeatedly triggered?

>  	if (!submit)
>  		return ERR_PTR(-ENOMEM);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716201538.365824-1-robin.clark@oss.qualcomm.com?part=10