[PATCH v6 00/16] drm/msm: A couple lazy-vm fixes

[PATCH v6 00/16] 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.  Or at least that is how this series started out.

Rob Clark (16):
  drm/msm: Fix barriers accessing ctx vm
  drm/msm: Rework queuelock
  drm/msm: Synchronize VM creation on ctxlock
  drm/msm: Synchronize set_sysprof on ctxlock
  drm/msm: Move nr_cmds initialization
  drm/msm: Remove redundant SIZE_MAX check
  drm/msm/a6xx: Access VM directly in submit path
  drm/msm: Add helper to check for per-process pgtables VM
  drm/msm/gem: Remove useless locking in GEM import
  drm/msm/gem: Extract bookkeeping init helper
  drm/msm/gem: Set resv before exposing obj
  drm/msm/gem: Validate lazy VM in GEM_NEW
  drm/msm: Allow lazy VM creation to fail
  drm/msm: Don't fallback to shared VM for VM_BIND
  drm/msm: Fix per-process-pgtables check
  drm/msm: Fixup invalid overflow check

 drivers/gpu/drm/msm/adreno/a6xx_gmu.c   |  2 +-
 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_fb.c            |  4 +-
 drivers/gpu/drm/msm/msm_gem.c           | 73 +++++++++++++------------
 drivers/gpu/drm/msm/msm_gem.h           |  2 +-
 drivers/gpu/drm/msm/msm_gem_submit.c    | 31 +++++------
 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   | 24 ++++----
 12 files changed, 109 insertions(+), 90 deletions(-)

-- 
2.55.0

[PATCH v6 01/16] 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>
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

[PATCH v6 02/16] 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

[PATCH v6 03/16] 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

[PATCH v6 04/16] drm/msm: Synchronize set_sysprof on ctxlock

[Rob Clark]

A user that was perfmon_capable() could try to race setting SYSPROF
param on multiple threads to trigger a reference leak.

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

diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
index 8b5980d5a6a7..233c4f35ab12 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -9,6 +9,8 @@
 
 int msm_context_set_sysprof(struct msm_context *ctx, struct msm_gpu *gpu, int sysprof)
 {
+	guard(rwsem_write)(&ctx->ctxlock);
+
 	/*
 	 * Since pm_runtime and sysprof_active are both refcounts, we
 	 * call apply the new value first, and then unwind the previous
-- 
2.55.0

[PATCH v6 05/16] drm/msm: Move nr_cmds initialization

[Rob Clark]

Previously if we entered an error path between these two points, we
could leak the relocs tables due to submit->nr_cmds still being zero.

In practice, relocs are disallowed on a6xx+, and non-ancient userspace
will not use relocs on earlier gens unless running on an ancient kernel.
But userspace could use this to trigger a memory leak.

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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 3c6bc90c3d48..aa48ea4e7f58 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -260,6 +260,9 @@ static int submit_lookup_cmds(struct msm_gem_submit *submit,
 			ret = -ENOMEM;
 			goto out;
 		}
+
+		submit->nr_cmds = i + 1;
+
 		ret = copy_from_user(submit->cmd[i].relocs, userptr, sz);
 		if (ret) {
 			ret = -EFAULT;
@@ -719,8 +722,6 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
 			goto out;
 	}
 
-	submit->nr_cmds = args->nr_cmds;
-
 	idr_preload(GFP_KERNEL);
 
 	spin_lock(&queue->idr_lock);
-- 
2.55.0

[PATCH v6 06/16] drm/msm: Remove redundant SIZE_MAX check

[Rob Clark]

kmalloc() will already fail and return NULL if passed SIZE_MAX.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem_submit.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index aa48ea4e7f58..040354ee16d1 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -250,11 +250,6 @@ static int submit_lookup_cmds(struct msm_gem_submit *submit,
 
 		sz = array_size(submit_cmd.nr_relocs,
 				sizeof(struct drm_msm_gem_submit_reloc));
-		/* check for overflow: */
-		if (sz == SIZE_MAX) {
-			ret = -ENOMEM;
-			goto out;
-		}
 		submit->cmd[i].relocs = kmalloc(sz, GFP_KERNEL | __GFP_NOWARN);
 		if (!submit->cmd[i].relocs) {
 			ret = -ENOMEM;
-- 
2.55.0

[PATCH v6 07/16] 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

[PATCH v6 08/16] 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 v6 09/16] drm/msm/gem: Remove useless locking in GEM import

[Rob Clark]

The locking has changed a few times over the years, and this extra
locking was the mistake of evolution.  Harmless but useless.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index efd3d3c9a449..1aa844ce4f2c 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1301,23 +1301,18 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
 	npages = size / PAGE_SIZE;
 
 	msm_obj = to_msm_bo(obj);
-	msm_gem_lock(obj);
 	msm_obj->sgt = sgt;
 	msm_obj->pages = kvmalloc_objs(struct page *, npages);
 	if (!msm_obj->pages) {
-		msm_gem_unlock(obj);
 		ret = -ENOMEM;
 		goto fail;
 	}
 
 	ret = drm_prime_sg_to_page_array(sgt, msm_obj->pages, npages);
 	if (ret) {
-		msm_gem_unlock(obj);
 		goto fail;
 	}
 
-	msm_gem_unlock(obj);
-
 	drm_gem_lru_move_tail(&priv->lru.pinned, obj);
 
 	mutex_lock(&priv->obj_lock);
-- 
2.55.0

[PATCH v6 10/16] drm/msm/gem: Extract bookkeeping init helper

[Rob Clark]

Clean up duplicated logic between import and new paths.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 1aa844ce4f2c..59143be07428 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1232,10 +1232,21 @@ static int msm_gem_new_impl(struct drm_device *dev, uint32_t flags,
 	return 0;
 }
 
+static int msm_gem_init_bookkeeping(struct drm_gem_object *obj)
+{
+	struct msm_drm_private *priv = obj->dev->dev_private;
+
+	drm_gem_lru_move_tail(&priv->lru.unbacked, obj);
+
+	mutex_lock(&priv->obj_lock);
+	list_add_tail(&to_msm_bo(obj)->node, &priv->objects);
+	mutex_unlock(&priv->obj_lock);
+
+	return drm_gem_create_mmap_offset(obj);
+}
+
 struct drm_gem_object *msm_gem_new(struct drm_device *dev, size_t size, uint32_t flags)
 {
-	struct msm_drm_private *priv = dev->dev_private;
-	struct msm_gem_object *msm_obj;
 	struct drm_gem_object *obj = NULL;
 	int ret;
 
@@ -1251,8 +1262,6 @@ struct drm_gem_object *msm_gem_new(struct drm_device *dev, size_t size, uint32_t
 	if (ret)
 		return ERR_PTR(ret);
 
-	msm_obj = to_msm_bo(obj);
-
 	ret = drm_gem_object_init(dev, obj, size);
 	if (ret)
 		goto fail;
@@ -1264,13 +1273,7 @@ struct drm_gem_object *msm_gem_new(struct drm_device *dev, size_t size, uint32_t
 	 */
 	mapping_set_gfp_mask(obj->filp->f_mapping, GFP_HIGHUSER);
 
-	drm_gem_lru_move_tail(&priv->lru.unbacked, obj);
-
-	mutex_lock(&priv->obj_lock);
-	list_add_tail(&msm_obj->node, &priv->objects);
-	mutex_unlock(&priv->obj_lock);
-
-	ret = drm_gem_create_mmap_offset(obj);
+	ret = msm_gem_init_bookkeeping(obj);
 	if (ret)
 		goto fail;
 
@@ -1284,7 +1287,6 @@ struct drm_gem_object *msm_gem_new(struct drm_device *dev, size_t size, uint32_t
 struct drm_gem_object *msm_gem_import(struct drm_device *dev,
 		struct dma_buf *dmabuf, struct sg_table *sgt)
 {
-	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_gem_object *msm_obj;
 	struct drm_gem_object *obj;
 	size_t size, npages;
@@ -1313,13 +1315,7 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
 		goto fail;
 	}
 
-	drm_gem_lru_move_tail(&priv->lru.pinned, obj);
-
-	mutex_lock(&priv->obj_lock);
-	list_add_tail(&msm_obj->node, &priv->objects);
-	mutex_unlock(&priv->obj_lock);
-
-	ret = drm_gem_create_mmap_offset(obj);
+	ret = msm_gem_init_bookkeeping(obj);
 	if (ret)
 		goto fail;
 
-- 
2.55.0

[PATCH v6 11/16] drm/msm/gem: Set resv before exposing obj

[Rob Clark]

Don't swap the resv object _after_ exposing the newly created obj in LRU
or global objects list, as that creates a race condition where another
thread could lock the object using the original (per-obj) resv, but then
unlock after the resv is replaced.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c |  2 +-
 drivers/gpu/drm/msm/msm_fb.c          |  4 ++--
 drivers/gpu/drm/msm/msm_gem.c         | 32 ++++++++++++++++-----------
 drivers/gpu/drm/msm/msm_gem.h         |  2 +-
 4 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 2e5d7b53a0c3..9aed142faea1 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -1546,7 +1546,7 @@ static int a6xx_gmu_memory_alloc(struct a6xx_gmu *gmu, struct a6xx_gmu_bo *bo,
 		flags |= MSM_BO_MAP_PRIV;
 	}
 
-	bo->obj = msm_gem_new(dev, size, flags);
+	bo->obj = msm_gem_new(dev, size, flags, NULL);
 	if (IS_ERR(bo->obj))
 		return PTR_ERR(bo->obj);
 
diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
index 9b681e144c07..ee17a47f31c4 100644
--- a/drivers/gpu/drm/msm/msm_fb.c
+++ b/drivers/gpu/drm/msm/msm_fb.c
@@ -267,11 +267,11 @@ msm_alloc_stolen_fb(struct drm_device *dev, int w, int h, int p, uint32_t format
 	/* allocate backing bo */
 	size = mode_cmd.pitches[0] * mode_cmd.height;
 	DBG("allocating %d bytes for fb %d", size, dev->primary->index);
-	bo = msm_gem_new(dev, size, MSM_BO_SCANOUT | MSM_BO_WC | MSM_BO_STOLEN);
+	bo = msm_gem_new(dev, size, MSM_BO_SCANOUT | MSM_BO_WC | MSM_BO_STOLEN, NULL);
 	if (IS_ERR(bo)) {
 		dev_warn(dev->dev, "could not allocate stolen bo\n");
 		/* try regular bo: */
-		bo = msm_gem_new(dev, size, MSM_BO_SCANOUT | MSM_BO_WC);
+		bo = msm_gem_new(dev, size, MSM_BO_SCANOUT | MSM_BO_WC, NULL);
 	}
 	if (IS_ERR(bo)) {
 		DRM_DEV_ERROR(dev->dev, "failed to allocate buffer object\n");
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 59143be07428..cb00bdf21ef6 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1134,10 +1134,17 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 		size_t size, uint32_t flags, uint32_t *handle,
 		char *name)
 {
-	struct drm_gem_object *obj;
+	struct drm_gem_object *obj, *r_obj = NULL;
 	int ret;
 
-	obj = msm_gem_new(dev, size, flags);
+	if (flags & MSM_BO_NO_SHARE) {
+		struct msm_context *ctx = file->driver_priv;
+		struct drm_gpuvm *vm = msm_context_vm(dev, ctx);
+
+		r_obj = drm_gpuvm_resv_obj(vm);
+	}
+
+	obj = msm_gem_new(dev, size, flags, r_obj);
 
 	if (IS_ERR(obj))
 		return PTR_ERR(obj);
@@ -1145,15 +1152,6 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 	if (name)
 		msm_gem_object_set_name(obj, "%s", name);
 
-	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);
-
-		drm_gem_object_get(r_obj);
-
-		obj->resv = r_obj->resv;
-	}
-
 	ret = drm_gem_handle_create(file, obj, handle);
 
 	/* drop reference from allocate - handle holds it now */
@@ -1245,7 +1243,9 @@ static int msm_gem_init_bookkeeping(struct drm_gem_object *obj)
 	return drm_gem_create_mmap_offset(obj);
 }
 
-struct drm_gem_object *msm_gem_new(struct drm_device *dev, size_t size, uint32_t flags)
+struct drm_gem_object *
+msm_gem_new(struct drm_device *dev, size_t size, uint32_t flags,
+	    struct drm_gem_object *r_obj)
 {
 	struct drm_gem_object *obj = NULL;
 	int ret;
@@ -1262,6 +1262,11 @@ struct drm_gem_object *msm_gem_new(struct drm_device *dev, size_t size, uint32_t
 	if (ret)
 		return ERR_PTR(ret);
 
+	if (flags & MSM_BO_NO_SHARE) {
+		drm_gem_object_get(r_obj);
+		obj->resv = r_obj->resv;
+	}
+
 	ret = drm_gem_object_init(dev, obj, size);
 	if (ret)
 		goto fail;
@@ -1298,6 +1303,7 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
 	if (ret)
 		return ERR_PTR(ret);
 
+	obj->resv = dmabuf->resv;
 	drm_gem_private_object_init(dev, obj, size);
 
 	npages = size / PAGE_SIZE;
@@ -1331,7 +1337,7 @@ void *msm_gem_kernel_new(struct drm_device *dev, size_t size, uint32_t flags,
 			 uint64_t *iova)
 {
 	void *vaddr;
-	struct drm_gem_object *obj = msm_gem_new(dev, size, flags);
+	struct drm_gem_object *obj = msm_gem_new(dev, size, flags, NULL);
 	int ret;
 
 	if (IS_ERR(obj))
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 762e546d25ef..47ff2bbc1915 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -296,7 +296,7 @@ int msm_gem_cpu_fini(struct drm_gem_object *obj);
 int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 		size_t size, uint32_t flags, uint32_t *handle, char *name);
 struct drm_gem_object *msm_gem_new(struct drm_device *dev,
-		size_t size, uint32_t flags);
+		size_t size, uint32_t flags, struct drm_gem_object *r_obj);
 void *msm_gem_kernel_new(struct drm_device *dev, size_t size, uint32_t flags,
 			 struct drm_gpuvm *vm, struct drm_gem_object **bo,
 			 uint64_t *iova);
-- 
2.55.0

[PATCH v6 12/16] drm/msm/gem: 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>
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index cb00bdf21ef6..153a4e4a916c 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1138,9 +1138,13 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 	int ret;
 
 	if (flags & MSM_BO_NO_SHARE) {
+		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 || !vm)
+			return UERR(EINVAL, dev, "not supported with shared VM");
+
 		r_obj = drm_gpuvm_resv_obj(vm);
 	}
 
-- 
2.55.0

[PATCH v6 13/16] 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 040354ee16d1..6b0bee6c39bc 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;
@@ -552,6 +552,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;
@@ -567,10 +568,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
@@ -607,7 +611,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 233c4f35ab12..d307cdd17d90 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -174,6 +174,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;
 
@@ -186,6 +187,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;
@@ -217,7 +223,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

[PATCH v6 14/16] 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..a1c4adcac128 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

[PATCH v6 15/16] 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>
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

[PATCH v6 16/16] drm/msm: Fixup 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.

On the other hand, we should be using the overflow helpers to calculate
the cmd array size.

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 | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 6b0bee6c39bc..5862db05297a 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -36,14 +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);
+	sz = size_add(struct_size(submit, bos, nr_bos),
+		      array_size(sizeof(submit->cmd[0]), nr_cmds));
 
 	submit = kzalloc(sz, GFP_KERNEL | __GFP_NOWARN);
 	if (!submit)
-- 
2.55.0

Re: [PATCH v6 06/16] drm/msm: Remove redundant SIZE_MAX check

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Direct access to ctx->vm before VM lazy initialization in ioctl entry point.
--

--- Patch [6]: [PATCH v6 06/16] drm/msm: Remove redundant SIZE_MAX check ---
commit f2972a9fd99ad8ac26829ed01da6279a307ca6e7
Author: Rob Clark <robin.clark@oss.qualcomm.com>

    drm/msm: Remove redundant SIZE_MAX check

    kmalloc() will already fail and return NULL if passed SIZE_MAX.

> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index d55897c358371..62acee148bf2b 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -250,11 +250,6 @@ static int submit_lookup_cmds(struct msm_gem_submit *submit,
>  
>  		sz = array_size(submit_cmd.nr_relocs,
>  				sizeof(struct drm_msm_gem_submit_reloc));
> -		/* check for overflow: */
> -		if (sz == SIZE_MAX) {
> -			ret = -ENOMEM;
> -			goto out;
> -		}
>  		submit->cmd[i].relocs = kmalloc(sz, GFP_KERNEL | __GFP_NOWARN);
>  		if (!submit->cmd[i].relocs) {
>  			ret = -ENOMEM;

[Severity: Critical]
This is a pre-existing issue, but looking at the same file in
msm_ioctl_gem_submit(), there appears to be a direct access to ctx->vm
before it is lazily initialized:

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");
	...
}

If a userspace process opens the DRM device and immediately calls the
DRM_IOCTL_MSM_GEM_SUBMIT ioctl before calling any other ioctls that would
initialize the VM (using the msm_context_vm() accessor), won't ctx->vm be
NULL?

Since to_msm_vm() is a container_of macro wrapping the first struct member,
passing a NULL pointer yields a NULL pointer. Could dereferencing ->unusable
here allow an unprivileged local user to trigger a kernel panic by calling
this ioctl before the VM is initialized?

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

Re: [PATCH v6 02/16] drm/msm: Rework queuelock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Deadlock in `msm_submitqueue_remove()` due to holding `ctxlock` during queue destruction.
--

--- Patch [2]: [PATCH v6 02/16] drm/msm: Rework queuelock ---
commit 308ab05b33546bb1bb25510a8f2b68c4b798cefc
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Rework queuelock

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.

> 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);
>  			return 0;
>  		}
>  	}

[Severity: High]
Does using guard() here extend the lock lifetime and create a deadlock?

In the original code, the lock was explicitly dropped before calling
msm_submitqueue_put(entry). By using guard(), the write lock is now held
during the msm_submitqueue_put() call since the scope of the guard is the
entire function.

If a user destroys a queue while jobs are pending, msm_submitqueue_put() can
drop the last reference. This calls drm_sched_entity_destroy(), which blocks
waiting for those pending jobs to complete.

If another job needs to be submitted to signal a fence that the pending job is
waiting on, the submission path will call msm_submitqueue_get() and block
trying to acquire ctxlock for reading. This creates a circular deadlock where
queue destruction waits for the job to complete, but the job waits for a
submission that is blocked waiting for ctxlock.

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

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

Re: [PATCH v6 09/16] drm/msm/gem: Remove useless locking in GEM import

[sashiko-bot]

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

Pre-existing issues:
- [High] Pre-existing error path vulnerability in `msm_gem_import` causes a double-free of the exporter's `sgt`, NULL pointer dereference, and wild pointer dereferences during cleanup.
--

--- Patch [9]: [PATCH v6 09/16] drm/msm/gem: Remove useless locking in GEM import ---
commit 19246e79feb40c4e0066b56b3385fea83fcc3d75
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/gem: Remove useless locking in GEM import
    
This patch removes unnecessary locking during the GEM object import process
in msm_gem_import().

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 2cb3ab04f1250..a358fb5ca6d73 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -1302,23 +1302,18 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
>  	npages = size / PAGE_SIZE;
>  
>  	msm_obj = to_msm_bo(obj);
> -	msm_gem_lock(obj);
>  	msm_obj->sgt = sgt;
>  	msm_obj->pages = kvmalloc_objs(struct page *, npages);
>  	if (!msm_obj->pages) {
> -		msm_gem_unlock(obj);
>  		ret = -ENOMEM;
>  		goto fail;
>  	}
>  
>  	ret = drm_prime_sg_to_page_array(sgt, msm_obj->pages, npages);
>  	if (ret) {
> -		msm_gem_unlock(obj);
>  		goto fail;

[Severity: High]
This is a pre-existing issue, but does this error path lead to a NULL
pointer dereference and a double-free?

When msm_gem_import() jumps to fail, it calls drm_gem_object_put(obj).
At this point, obj->import_attach has not yet been set (it is assigned
later by the caller drm_gem_prime_import_dev() on success).

During cleanup, msm_gem_free_object() checks drm_gem_is_imported(obj).
Since import_attach is NULL, this evaluates to false, causing the cleanup
to fall through to the native path:

msm_gem_free_object() ->
    put_pages(obj)

In put_pages(), the code incorrectly frees the exporter-owned sgt (leading
to a double-free) and then calls drm_gem_put_pages().

Finally, drm_gem_put_pages() unconditionally accesses
file_inode(obj->filp)->i_mapping. Because obj->filp is NULL for imported
objects, this causes a NULL pointer dereference.

>  	}
>  
> -	msm_gem_unlock(obj);
> -
>  	drm_gem_lru_move_tail(&priv->lru.pinned, obj);
>  
>  	mutex_lock(&priv->obj_lock);

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

Re: [PATCH v6 07/16] drm/msm/a6xx: Access VM directly in submit path

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The size calculation for the `msm_gem_submit` structure wraps around on 64-bit architectures, bypassing the `sz > SIZE_MAX` check and leading to a heap buffer overflow.
- [High] Direct access to `ctx->vm` in `msm_ioctl_gem_submit` before the VM is guaranteed to be created causes a NULL pointer dereference.
--

--- Patch [7]: [PATCH v6 07/16] drm/msm/a6xx: Access VM directly in submit path ---
commit e716d49994df650aa89eca4941226567c71dbab5
Author: Rob Clark <robin.clark@oss.qualcomm.com>

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

This patch changes the submit path to access the VM directly from the context
rather than calling msm_context_vm() to lazily create it. It relies on the
assumption that the ioctl has already ensured the VM is created prior to this
point in the execution flow.

> 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: High]
This isn't a bug introduced by this patch, but does this assumption hold true?

Looking at the ioctl entry point in msm_ioctl_gem_submit(), if userspace
triggers this ioctl before any operation that creates the VM, the VM pointer
is initially null:

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 this direct access to ctx->vm cause a null pointer dereference, since a
preceding call to msm_context_vm() hasn't guaranteed its existence yet?

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index e293b4ca808a4..9240c4af687c9 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;

While reviewing the submit path related to this change, I noticed another
pre-existing issue in the submission creation logic:

drivers/gpu/drm/msm/msm_gem_submit.c:submit_create() {
    ...
	sz = struct_size(submit, bos, nr_bos) +
			((u64)nr_cmds * sizeof(submit->cmd[0]));

	if (sz > SIZE_MAX)
		return ERR_PTR(-ENOMEM);
    ...
}

[Severity: Critical]
This is a pre-existing issue, but does this check protect against integer
overflows effectively?

Since sz is a 64-bit unsigned integer, the check sz > SIZE_MAX is
mathematically impossible and acts as dead code.

If a user provides a sufficiently large nr_bos, struct_size() saturates to
SIZE_MAX. Adding a non-zero value for nr_cmds then causes sz to wrap around
to a small value.

Could this lead to an undersized buffer allocation from kzalloc() and a
subsequent out-of-bounds heap write during initialization?

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

Re: [PATCH v6 03/16] drm/msm: Synchronize VM creation on ctxlock

[sashiko-bot]

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

Pre-existing issues:
- [Critical] `msm_context_vm()` propagates errors as `ERR_PTR` or `NULL` without callers checking, leading to wild pointer dereferences.
- [Critical] Direct access to uninitialized `ctx->vm` in `msm_ioctl_gem_submit()` causes a guaranteed NULL pointer dereference.
- [High] Hardware capability check in `adreno_set_param()` fails due to lazy VM initialization, bypassing restrictions.
--

--- Patch [3]: [PATCH v6 03/16] drm/msm: Synchronize VM creation on ctxlock ---
commit 78d3ed784b530d2c180edb9b49939bea521dccd3
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Synchronize VM creation on ctxlock
    
This commit serializes the setting of EN_VM_BIND against VM creation
by synchronizing it on ctxlock.

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index c62c45bb0ddbb..0d54141ff089c 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)

[Severity: High]
This isn't a bug introduced by this patch, but does this hardware capability
check properly account for lazy VM initialization?

Since VMs are lazily initialized, ctx->vm will be NULL at this stage. This
means the check evaluates NULL == gpu->vm, which is always false. Does this
allow all hardware, even those without per-process pagetables, to bypass this 
restriction?

>  			return UERR(EINVAL, drm, "requires per-process pgtables");

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 380b3e2bf734e..20c5a6cc82019 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -223,7 +223,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);
>  
> @@ -231,7 +230,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(

[Severity: Critical]
This is a pre-existing issue, but does msm_context_vm() propagate an
unhandled error pointer or NULL on failure?

If msm_gpu_create_private_vm() returns ERR_PTR or NULL (e.g., during an OOM
condition or on platforms without per-process pagetables), it appears
msm_context_vm() stores and returns this directly. 

Callers like msm_submitqueue_create() in msm_submitqueue.c blindly assume
success:

    struct drm_gpu_scheduler *sched = &to_msm_vm(msm_context_vm(drm, ctx))->sched;

If an error pointer or NULL is returned, to_msm_vm() yields an invalid
address. Could this lead to an immediate kernel crash when it is
dereferenced in drm_sched_entity_init()?


[Severity: Critical]
This is another pre-existing issue, but does direct access to ctx->vm cause a
NULL pointer dereference?

If a user issues a DRM_IOCTL_MSM_GEM_SUBMIT ioctl before executing any
commands that lazily initialize the VM, ctx->vm is still NULL.

In msm_ioctl_gem_submit():

    if (to_msm_vm(ctx->vm)->unusable)
        return UERR(EPIPE, dev, "context is unusable");

Because ctx->vm is NULL, the to_msm_vm() macro returns a negative offset
pointer. Does dereferencing ->unusable here immediately crash the kernel
instead of calling msm_context_vm() to lazily initialize it?

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

Re: [PATCH v6 12/16] drm/msm/gem: 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:
- [Medium] The check `!vm` fails to detect shared VMs, breaking the `MSM_BO_NO_SHARE` contract.

Pre-existing issues:
- [Critical] NULL Pointer Dereference when accessing uninitialized `ctx->vm` in `msm_ioctl_gem_submit`.
--

--- Patch [12]: [PATCH v6 12/16] drm/msm/gem: Validate lazy VM in GEM_NEW ---
commit 2c5a1210206f5856b0becc6857b23e80e4e3fe79
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/gem: Validate lazy VM in GEM_NEW

This commit adds a check in msm_gem_new_handle() to ensure the VM is
available before creating a BO with MSM_BO_NO_SHARE. It prevents a NULL
pointer dereference that could occur if a BO is created before any BOs have
been mapped.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 12d9b8a9c360a..b9da5b7ce8a31 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -1139,9 +1139,13 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>  	int ret;
>  
>  	if (flags & MSM_BO_NO_SHARE) {
> +		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 || !vm)
> +			return UERR(EINVAL, dev, "not supported with shared VM");
> +

[Severity: Medium]
Does the check for !vm correctly detect if a shared VM is in use?

If a platform does not support private VMs and falls back to the global
VM, msm_context_vm() will return the shared gpu->vm. In this case, !vm
will evaluate to false, allowing the allocation to proceed.

Could this allow the MSM_BO_NO_SHARE flag to succeed while silently sharing
the global reservation object, defeating the purpose of the flag and
creating hidden lock contention?

Would it be better to check if vm == priv->gpu->vm to ensure a private
VM is actually in use?

>  		r_obj = drm_gpuvm_resv_obj(vm);
>  	}
>  

[Severity: Critical]
This is a pre-existing issue, but does msm_ioctl_gem_submit() have a
similar problem with lazy VM initialization?

Looking at msm_ioctl_gem_submit() in drivers/gpu/drm/msm/msm_gem_submit.c:

	if (!gpu)
		return -ENXIO;

	if (args->pad)
		return -EINVAL;

	if (to_msm_vm(ctx->vm)->unusable)
		return UERR(EPIPE, dev, "context is unusable");

If a userspace process calls the DRM_IOCTL_MSM_GEM_SUBMIT ioctl immediately
after opening the DRM device, ctx->vm will be NULL because no previous
operation has initialized it.

Since this code accesses ctx->vm directly instead of using msm_context_vm(),
will this cause a NULL pointer dereference when to_msm_vm() evaluates to NULL
and ->unusable is accessed?

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

Re: [PATCH v6 11/16] drm/msm/gem: Set resv before exposing obj

[sashiko-bot]

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

New issues:
- [Low] The patch modifies the signature of msm_gem_new() from 3 to 4 arguments but fails to update the caller in drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c. This results in a compiler error ('too few arguments to function').

Pre-existing issues:
- [Critical] Missing NULL check for the return value of msm_context_vm() leads to a kernel panic when dereferenced by drm_gpuvm_resv_obj(). This problem wasn't introduced by this patch, but remains a critical vulnerability.
--

--- Patch [11]: [PATCH v6 11/16] drm/msm/gem: Set resv before exposing obj ---
commit 96533966332980b8f99abb5ba08db109f97b565b
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/gem: Set resv before exposing obj

Don't swap the resv object _after_ exposing the newly created obj in LRU
or global objects list, as that creates a race condition where another
thread could lock the object using the original (per-obj) resv, but then
unlock after the resv is replaced.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 961e22d3c7292..12d9b8a9c360a 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c

[ ... ]

> @@ -1135,10 +1135,17 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>  		size_t size, uint32_t flags, uint32_t *handle,
>  		char *name)
>  {
> -	struct drm_gem_object *obj;
> +	struct drm_gem_object *obj, *r_obj = NULL;
>  	int ret;
>  
> -	obj = msm_gem_new(dev, size, flags);
> +	if (flags & MSM_BO_NO_SHARE) {
> +		struct msm_context *ctx = file->driver_priv;
> +		struct drm_gpuvm *vm = msm_context_vm(dev, ctx);
> +
> +		r_obj = drm_gpuvm_resv_obj(vm);

[Severity: Critical]
This is a pre-existing issue, but can msm_context_vm() return NULL here?

If this runs on a KMS-only configuration where no GPU is bound, 
msm_context_vm() can return NULL. When passed to drm_gpuvm_resv_obj(vm),
would this cause a NULL pointer dereference since the macro expands to
((vm)->r_obj)?

Since unprivileged userspace can trigger this path via the MSM_GEM_NEW
ioctl with the MSM_BO_NO_SHARE flag, should a NULL check be added before
accessing vm?

> +	}
> +
> +	obj = msm_gem_new(dev, size, flags, r_obj);

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 762e546d25ef3..47ff2bbc19150 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -296,7 +296,7 @@ int msm_gem_cpu_fini(struct drm_gem_object *obj);
>  int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>  		size_t size, uint32_t flags, uint32_t *handle, char *name);
>  struct drm_gem_object *msm_gem_new(struct drm_device *dev,
> -		size_t size, uint32_t flags);
> +		size_t size, uint32_t flags, struct drm_gem_object *r_obj);

[Severity: Low]
Does this change miss updating some callers? Looking at mdp4_kms_init() in
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c, it still calls msm_gem_new() with
only 3 arguments:

mdp4_kms->blank_cursor_bo = msm_gem_new(dev, SZ_16K, MSM_BO_WC | MSM_BO_SCANOUT);

Could this cause a build failure when CONFIG_DRM_MSM_MDP4 is enabled due to
too few arguments?

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

Re: [PATCH v6 05/16] drm/msm: Move nr_cmds initialization

[sashiko-bot]

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

Pre-existing issues:
- [Critical] MSM VM Lazy Initialization NULL Pointer Dereference
--

--- Patch [5]: [PATCH v6 05/16] drm/msm: Move nr_cmds initialization ---
commit 7351feaad392cc3ab54b31c35b1352065b9a2c8c
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Move nr_cmds initialization

Previously if we entered an error path between these two points, we
could leak the relocs tables due to submit->nr_cmds still being zero.
Userspace could use this to trigger a memory leak.

> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 26ea8a28be474..d55897c358371 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c

[ ... ]

> @@ -719,8 +722,6 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>  			goto out;
>  	}
>  
> -	submit->nr_cmds = args->nr_cmds;
> -

[Severity: Critical]
This isn't a bug introduced by this patch, but can early validation in
msm_ioctl_gem_submit() trigger a NULL pointer dereference?

Because the MSM driver uses lazy initialization for virtual memory address
spaces, ctx->vm will be NULL if this is the first operation on a newly
created context. Unprivileged userspace applications could trigger this by
opening the DRM device and calling the MSM_GEM_SUBMIT ioctl before any
operation that initializes the VM.

If ctx->vm is not yet initialized, direct access will crash when checking
unusable status:

msm_ioctl_gem_submit() {
    ...
    if (to_msm_vm(ctx->vm)->unusable)
        return UERR(EPIPE, dev, "context is unusable");
    ...
}

Since base is at offset 0 in struct msm_gem_vm, to_msm_vm(NULL) returns
NULL, leading to a dereference of NULL->unusable.

Should this code path be updated to use msm_context_vm() to ensure the VM
is fully created before checking its status?

>  	idr_preload(GFP_KERNEL);
>  
>  	spin_lock(&queue->idr_lock);

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

Re: [PATCH v6 01/16] 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:
- [Critical] Direct access to ctx->vm before lazy initialization causes a NULL pointer dereference.
- [High] TOCTOU race in MSM_PARAM_EN_VM_BIND allows invalid VM configuration.
--

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

    drm/msm: Fix barriers accessing ctx vm
    
    Don't rely on store ordering to protect us from caller 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);
>  
>  	/* 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;
>  }

[Severity: Critical]
This is a pre-existing issue, but I noticed that ctx->vm can be accessed 
directly before lazy initialization. If an unprivileged user opens the DRM 
device and immediately calls the MSM_GEM_SUBMIT IOCTL, it evaluates 
to_msm_vm(ctx->vm)->unusable without ensuring the VM has been initialized via 
msm_context_vm():

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");
    ...
}

Since ctx->vm is NULL, the to_msm_vm() macro computes an invalid pointer and 
dereferences its unusable field, causing a panic.

Similarly, calling the MSM_GEM_NEW IOCTL with the MSM_BO_NO_SHARE flag 
executes drm_gpuvm_resv_obj(ctx->vm) which also dereferences NULL:

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);
    ...
}

Can these paths be updated to ensure the VM is initialized via 
msm_context_vm() before accessing it?


[Severity: High]
This isn't a bug introduced by this patch, but I found a potential TOCTOU 
race in MSM_PARAM_EN_VM_BIND that could allow an invalid VM configuration. 

In adreno_gpu_set_param(), it locklessly checks if (ctx->vm):

drivers/gpu/drm/msm/adreno/adreno_gpu.c:adreno_gpu_set_param() {
    ...
	case MSM_PARAM_EN_VM_BIND:
        ...
		if (ctx->vm)
			return UERR(EBUSY, drm, "VM already created");

		ctx->userspace_managed_vm = value;
    ...
}

If Thread A calls the MSM_SET_PARAM IOCTL, checks ctx->vm and sees NULL, and 
concurrently Thread B calls an IOCTL like MSM_INFO_GET_IOVA which calls 
msm_context_vm(), Thread B will create the VM as kernel-managed. Thread A 
then resumes and sets ctx->userspace_managed_vm = 1. 

Does this result in an inconsistent state where the context claims userspace 
management but the underlying VM is kernel-managed?

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

Re: [PATCH v6 14/16] 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:

Pre-existing issues:
- [High] The patch exposes a tight-loop, unbounded memory leak of MMU pagetables by failing to cache error states during lazy VM initialization.
--

--- Patch [14]: [PATCH v6 14/16] drm/msm: Don't fallback to shared VM for VM_BIND ---
commit 4383cea99723fe11a1653c676500fab3a15f1fda
Author: Rob Clark <robin.clark@oss.qualcomm.com>

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

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

> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 3bd9a13837be3..50cc32184bb9d 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -235,7 +235,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);

[Severity: High]
This is a pre-existing issue, but does discarding the error pointer here
expose a tight loop memory leak of the mmu pagetables?

If msm_gpu_create_private_vm() returns an error pointer, ctx->vm stays NULL.
This allows userspace to repeatedly trigger vm creation on the same context
by calling the submit ioctl in a tight loop.

When msm_gpu_create_private_vm() fails (for example, if drm_sched_init()
fails inside msm_gem_vm_create()), the underlying mmu object allocated in
a6xx_create_private_vm() is leaked.

Previously, msm_gpu_create_private_vm() replaced this error pointer with a
fallback to gpu->vm. This bounded the leak to once per context. Because this
patch restricts the fallback to kernel managed vms, userspace managed vms
now return the error pointer directly.

Could we cache the error state in ctx->vm so we don't continually retry
failing vm creations, or fix the underlying leak by freeing the mmu object
on error?

>  	}
>  
>  	return ctx->vm;

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

Re: [PATCH v6 10/16] drm/msm/gem: Extract bookkeeping init helper

[sashiko-bot]

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

New issues:
- [Low] Imported dma-buf objects are incorrectly placed into the unbacked LRU list instead of the pinned LRU list.

Pre-existing issues:
- [Critical] Failure path in `msm_gem_import` incorrectly treats the imported object as a native object, erroneously freeing the exporter's `sg_table` and dropping page references, leading to a NULL pointer dereference.
--

--- Patch [10]: [PATCH v6 10/16] drm/msm/gem: Extract bookkeeping init helper ---
commit e9867494af3de09952793735c8d5be5dc88c19a0
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/gem: Extract bookkeeping init helper

Clean up duplicated logic between import and new paths.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index a358fb5ca6d73..961e22d3c7292 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -1233,10 +1233,21 @@ static int msm_gem_new_impl(struct drm_device *dev, uint32_t flags,
>  	return 0;
>  }
>  
> +static int msm_gem_init_bookkeeping(struct drm_gem_object *obj)
> +{
> +	struct msm_drm_private *priv = obj->dev->dev_private;
> +
> +	drm_gem_lru_move_tail(&priv->lru.unbacked, obj);

[Severity: Low]
Does this hardcode the unbacked LRU list for all callers?  The previous
code for imported objects placed them in the pinned LRU.  Will this now
route an imported, page-backed object into the unbacked LRU?

> +
> +	mutex_lock(&priv->obj_lock);
> +	list_add_tail(&to_msm_bo(obj)->node, &priv->objects);
> +	mutex_unlock(&priv->obj_lock);
> +
> +	return drm_gem_create_mmap_offset(obj);
> +}

[ ... ]

> @@ -1314,13 +1316,7 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
>  		goto fail;
>  	}
>  
> -	drm_gem_lru_move_tail(&priv->lru.pinned, obj);
> -
> -	mutex_lock(&priv->obj_lock);
> -	list_add_tail(&msm_obj->node, &priv->objects);
> -	mutex_unlock(&priv->obj_lock);
> -
> -	ret = drm_gem_create_mmap_offset(obj);
> +	ret = msm_gem_init_bookkeeping(obj);
>  	if (ret)
>  		goto fail;

[Severity: Critical]
This is a pre-existing issue, but can this failure path lead to a NULL
pointer dereference and double-free?

If msm_gem_init_bookkeeping() fails (e.g. from -ENOSPC) and we take this
goto fail path, drm_gem_object_put(obj) will be called.

Because import_attach is still NULL at this point, msm_gem_free_object()
will see drm_gem_is_imported(obj) as false and treat it as a native
object:

drivers/gpu/drm/msm/msm_gem.c:msm_gem_free_object() {
    if (drm_gem_is_imported(obj)) {
    ...
    } else {
        msm_gem_vunmap(obj);
        put_pages(obj);
    }
}

This would free the exporter's sgt using sg_free_table() and kfree().

It would also call drm_gem_put_pages() which unconditionally dereferences
obj->filp:

drivers/gpu/drm/drm_gem.c:drm_gem_put_pages() {
    mapping = file_inode(obj->filp)->i_mapping;
    ...
}

Since this object was initialized with drm_gem_private_object_init(),
obj->filp is NULL, causing a panic.

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