[PATCH 0/2] drm/msm: fix dma-buf sharing on targets without per-process pgtables

[PATCH 0/2] drm/msm: fix dma-buf sharing on targets without per-process pgtables

[Dmitry Baryshkov]

Targets without per-process pgtables -- everything pre-a6xx, plus a6xx
parts where split pagetables are unavailable -- hand every DRM file the
GPU's global VM.  The rest of the driver still treats a context VM as
private to that context, so closing a handle tears down mappings other
files are still using, and frees the iova for immediate reuse.

A dma-buf shared between two clients therefore keeps sampling an address
that the next allocation has taken over.  On a530 this is every
ext_image_dma_buf_import sampling test, reading back all zeros.

A VM shared between contexts needs the lazy, refcounted teardown the
driver already implements for kms->vm.  Rather than adding a second
mechanism, the first patch makes that machinery reusable and the second
one puts the shared GPU VM behind it.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
Dmitry Baryshkov (2):
      drm/msm: factor out a locking put_iova_spaces() wrapper
      drm/msm: don't tear down shared VM mappings on handle close

 drivers/gpu/drm/msm/msm_gem.c | 45 ++++++++++++++++++++++++++++---------------
 drivers/gpu/drm/msm/msm_gpu.c |  4 +++-
 drivers/gpu/drm/msm/msm_gpu.h |  3 +++
 3 files changed, 36 insertions(+), 16 deletions(-)
---
base-commit: 6b8c8af514d739d0335f5579b585e02babe8a727
change-id: 20260822-msm-fix-export-c4a4cd9239fc

Best regards,
--  
With best wishes
Dmitry

[PATCH 1/2] drm/msm: factor out a locking put_iova_spaces() wrapper

[Dmitry Baryshkov]

put_iova_spaces() asserts that the caller already holds the VM and object
locks, but is not named accordingly.  Both of the callers which do not
hold those locks yet -- msm_gem_close() and msm_gem_vma_put() -- open-code
the same lock, tear down, unlock sequence around it.

Rename it to put_iova_spaces_locked() and give the plain name to a wrapper
taking both locks, mirroring the get_and_pin_iova_range_locked() /
msm_gem_get_and_pin_iova_range() pair in the same file.  No functional
change.

Assisted-by: LLM
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index c4cff3d53d81..f90afffe3442 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -47,13 +47,23 @@ static int msm_gem_open(struct drm_gem_object *obj, struct drm_file *file)
 	return 0;
 }
 
+static void put_iova_spaces_locked(struct drm_gem_object *obj,
+				   struct drm_gpuvm *vm, bool close,
+				   const char *reason);
+
 static void put_iova_spaces(struct drm_gem_object *obj, struct drm_gpuvm *vm,
-			    bool close, const char *reason);
+			    bool close, const char *reason)
+{
+	struct drm_exec exec;
+
+	msm_gem_lock_vm_and_obj(&exec, obj, vm);
+	put_iova_spaces_locked(obj, vm, close, reason);
+	drm_exec_fini(&exec);     /* drop locks */
+}
 
 static void msm_gem_close(struct drm_gem_object *obj, struct drm_file *file)
 {
 	struct msm_context *ctx = file->driver_priv;
-	struct drm_exec exec;
 
 	update_ctx_mem(file, -obj->size);
 	msm_gem_vma_put(obj);
@@ -81,9 +91,7 @@ static void msm_gem_close(struct drm_gem_object *obj, struct drm_file *file)
 	dma_resv_wait_timeout(obj->resv, DMA_RESV_USAGE_BOOKKEEP, false,
 			      MAX_SCHEDULE_TIMEOUT);
 
-	msm_gem_lock_vm_and_obj(&exec, obj, ctx->vm);
 	put_iova_spaces(obj, ctx->vm, true, "close");
-	drm_exec_fini(&exec);     /* drop locks */
 }
 
 /*
@@ -106,11 +114,7 @@ void msm_gem_vma_put(struct drm_gem_object *obj)
 		return;
 
 #ifdef CONFIG_DRM_MSM_KMS
-	struct drm_exec exec;
-
-	msm_gem_lock_vm_and_obj(&exec, obj, priv->kms->vm);
 	put_iova_spaces(obj, priv->kms->vm, true, "vma_put");
-	drm_exec_fini(&exec);     /* drop locks */
 #endif
 }
 
@@ -409,8 +413,8 @@ static struct drm_gpuva *lookup_vma(struct drm_gem_object *obj,
  * mapping.
  */
 static void
-put_iova_spaces(struct drm_gem_object *obj, struct drm_gpuvm *vm,
-		bool close, const char *reason)
+put_iova_spaces_locked(struct drm_gem_object *obj, struct drm_gpuvm *vm,
+		       bool close, const char *reason)
 {
 	struct drm_gpuvm_bo *vm_bo, *tmp;
 
@@ -669,7 +673,7 @@ void msm_gem_unpin_iova(struct drm_gem_object *obj, struct drm_gpuvm *vm)
 		msm_gem_unpin_locked(obj);
 	}
 	if (!is_kms_vm(vm))
-		put_iova_spaces(obj, vm, true, "close");
+		put_iova_spaces_locked(obj, vm, true, "close");
 	drm_exec_fini(&exec);     /* drop locks */
 }
 
@@ -831,7 +835,7 @@ void msm_gem_purge(struct drm_gem_object *obj)
 	GEM_WARN_ON(!is_purgeable(msm_obj));
 
 	/* Get rid of any iommu mapping(s): */
-	put_iova_spaces(obj, NULL, false, "purge");
+	put_iova_spaces_locked(obj, NULL, false, "purge");
 
 	msm_gem_vunmap(obj);
 
@@ -869,7 +873,7 @@ void msm_gem_evict(struct drm_gem_object *obj)
 	GEM_WARN_ON(is_unevictable(msm_obj));
 
 	/* Get rid of any iommu mapping(s): */
-	put_iova_spaces(obj, NULL, false, "evict");
+	put_iova_spaces_locked(obj, NULL, false, "evict");
 
 	drm_vma_node_unmap(&obj->vma_node, dev->anon_inode->i_mapping);
 
@@ -1082,7 +1086,7 @@ static void msm_gem_free_object(struct drm_gem_object *obj)
 				drm_exec_retry_on_contention(&exec);
 			}
 		}
-		put_iova_spaces(obj, NULL, true, "free");
+		put_iova_spaces_locked(obj, NULL, true, "free");
 		drm_exec_fini(&exec);     /* drop locks */
 	}
 

-- 
2.47.3

[PATCH 2/2] drm/msm: don't tear down shared VM mappings on handle close

[Dmitry Baryshkov]

On targets (like A530 / MSM8996) without per-process pgtables
msm_gpu_create_private_vm() uses the global VM, so all DRM files share
one GPU address space.  msm_gem_close() unmaps the object from ctx->vm,
which on those targets pulls the buffer out from under every other file
that still has it open, and frees the iova for immediate reuse.

A dma-buf imported into a second file hits this as soon as the exporter
closes its handle: the importer's texture keeps sampling the old
address, which the next allocation has taken over.  On a530 this is
every ext_image_dma_buf_import sampling test, reading back all zeros.

The VMA teardown a shared VM needs is the one already used for kms->vm
-- defer it to the @vma_ref drop, when the last handle and dma_buf
reference are gone.  That restores the pre-drm_gpuvm lifetime without
reintroducing the reference loop, since a BO with a live vma_ref is held
by userspace anyway.

Fixes: 111fdd2198e6 ("drm/msm: drm_gpuvm conversion")
Assisted-by: LLM
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem.c | 13 ++++++++++++-
 drivers/gpu/drm/msm/msm_gpu.c |  4 +++-
 drivers/gpu/drm/msm/msm_gpu.h |  3 +++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index f90afffe3442..e73cf49c360e 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -63,6 +63,7 @@ static void put_iova_spaces(struct drm_gem_object *obj, struct drm_gpuvm *vm,
 
 static void msm_gem_close(struct drm_gem_object *obj, struct drm_file *file)
 {
+	struct msm_drm_private *priv = obj->dev->dev_private;
 	struct msm_context *ctx = file->driver_priv;
 
 	update_ctx_mem(file, -obj->size);
@@ -84,6 +85,10 @@ static void msm_gem_close(struct drm_gem_object *obj, struct drm_file *file)
 	if (msm_context_is_vmbind(ctx))
 		return;
 
+	/* A global VM's VMAs are torn down by the @vma_ref drop above */
+	if (priv->gpu && ctx->vm == priv->gpu->vm)
+		return;
+
 	/*
 	 * TODO we might need to kick this to a queue to avoid blocking
 	 * in CLOSE ioctl
@@ -95,7 +100,7 @@ static void msm_gem_close(struct drm_gem_object *obj, struct drm_file *file)
 }
 
 /*
- * Get/put for kms->vm VMA
+ * Get/put for VMAs in VMs shared between contexts: kms->vm, gpu->vm
  */
 
 void msm_gem_vma_get(struct drm_gem_object *obj)
@@ -110,6 +115,12 @@ void msm_gem_vma_put(struct drm_gem_object *obj)
 	if (atomic_dec_return(&to_msm_bo(obj)->vma_ref))
 		return;
 
+	if (priv->gpu && priv->gpu->vm_shared) {
+		dma_resv_wait_timeout(obj->resv, DMA_RESV_USAGE_BOOKKEEP, false,
+				      MAX_SCHEDULE_TIMEOUT);
+		put_iova_spaces(obj, priv->gpu->vm, true, "vma_put");
+	}
+
 	if (!priv->kms)
 		return;
 
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 0c2c35636251..31d84e2b123a 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -879,8 +879,10 @@ 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) && kernel_managed)
+	if (IS_ERR_OR_NULL(vm) && kernel_managed) {
 		vm = drm_gpuvm_get(gpu->vm);
+		gpu->vm_shared = true;
+	}
 
 	return vm;
 }
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index d27d54bdb7a7..7722776e9129 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -223,6 +223,9 @@ struct msm_gpu {
 
 	struct drm_gpuvm *vm;
 
+	/** @vm_shared: Has @vm been handed out as a context VM? */
+	bool vm_shared;
+
 	/* Power Control: */
 	struct regulator *gpu_reg, *gpu_cx;
 	struct clk_bulk_data *grp_clks;

-- 
2.47.3

Re: [PATCH 1/2] drm/msm: factor out a locking put_iova_spaces() wrapper

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The VMA reference count (`vma_ref`) is decremented and checked outside of the VMA/object lock, leading to a TOCTOU race where an active VMA can be unmapped and freed while in use by another thread.
--

--- Patch [1]: [PATCH 1/2] drm/msm: factor out a locking put_iova_spaces() wrapper ---
commit 3d4e26e168daef686a19b61570238be2722c0b4d
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Subject: drm/msm: factor out a locking put_iova_spaces() wrapper

This commit factors out a locking put_iova_spaces_locked() wrapper and
updates callers. It aims to clean up redundant lock, tear down, and unlock
sequences around the teardown path.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index de4e60e28a711..8b979d84bb958 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c

[ ... ]

> @@ -105,11 +113,7 @@ void msm_gem_vma_put(struct drm_gem_object *obj)
>  		return;
>  
>  #ifdef CONFIG_DRM_MSM_KMS
> -	struct drm_exec exec;
> -
> -	msm_gem_lock_vm_and_obj(&exec, obj, priv->kms->vm);
>  	put_iova_spaces(obj, priv->kms->vm, true, "vma_put");
> -	drm_exec_fini(&exec);     /* drop locks */
>  #endif
>  }

[ ... ]

> @@ -408,8 +412,8 @@ static struct drm_gpuva *lookup_vma(struct drm_gem_object *obj,
>   * mapping.
>   */
>  static void
> -put_iova_spaces(struct drm_gem_object *obj, struct drm_gpuvm *vm,
> -		bool close, const char *reason)
> +put_iova_spaces_locked(struct drm_gem_object *obj, struct drm_gpuvm *vm,
> +		       bool close, const char *reason)
>  {
>  	struct drm_gpuvm_bo *vm_bo, *tmp;

[Severity: Critical]
This is a pre-existing issue, but can the lockless decrement of vma_ref in
msm_gem_vma_put() cause a race condition?

If CPU1 calls msm_gem_close() -> msm_gem_vma_put() and decrements vma_ref to
0, but gets preempted before taking the lock and calling put_iova_spaces():

CPU2 could concurrently open the GEM object (e.g., via flink), calling
msm_gem_open() -> msm_gem_vma_get(), incrementing vma_ref back to 1,
mapping the object, and submitting a job.

When CPU1 resumes, it continues into put_iova_spaces() and eventually
put_iova_spaces_locked():

drivers/gpu/drm/msm/msm_gem.c:put_iova_spaces_locked() {
        ...
        drm_gpuvm_bo_for_each_va_safe (vma, vmatmp, vm_bo) {
                msm_gem_vma_unmap(vma, reason);
                if (close)
                        msm_gem_vma_close(vma);
        }
        ...
}

Since vma_ref isn't re-checked inside the lock, the IOVA space is torn down
unconditionally. Could this result in tearing down an active IOVA mapping
while the GPU is executing a job, leading to an IOMMU fault or memory
corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260822-msm-fix-export-v1-0-917b3cbedd43@oss.qualcomm.com?part=1

Re: [PATCH 2/2] drm/msm: don't tear down shared VM mappings on handle close

[sashiko-bot]

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

Pre-existing issues:
- [High] Sleeping in atomic context
- [High] Deferred teardown TOCTOU race via flink name re-opening causing IOVA Use-After-Free
--

--- Patch [2]: [PATCH 2/2] drm/msm: don't tear down shared VM mappings on handle close ---
commit c4a550033efcf938dc9587175385266d18df3979
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm: don't tear down shared VM mappings on handle close

This patch defers the teardown of shared GPU VM mappings until the vma_ref
count drops to zero. This aims to prevent pulling the buffer out from under
other files that still have it open when sharing a global VM address space
on targets without per-process pgtables.

Fixes: 111fdd2198e6 ("drm/msm: drm_gpuvm conversion")

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 8b979d84bb958..f17ecd97b72a3 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
[ ... ]
> @@ -109,6 +114,12 @@ void msm_gem_vma_put(struct drm_gem_object *obj)
>  	if (atomic_dec_return(&to_msm_bo(obj)->vma_ref))
>  		return;
>  
> +	if (priv->gpu && priv->gpu->vm_shared) {
> +		dma_resv_wait_timeout(obj->resv, DMA_RESV_USAGE_BOOKKEEP, false,
> +				      MAX_SCHEDULE_TIMEOUT);
> +		put_iova_spaces(obj, priv->gpu->vm, true, "vma_put");
> +	}
> +

[Severity: High]
This is a pre-existing issue, but can this lockless vma_ref check lead to an
IOVA use-after-free if a GEM handle is concurrently closed and re-opened via
flink?

If Thread A closes the last handle, atomic_dec_return() drops vma_ref to 0
and it proceeds to unmap the buffer. However, the flink name remains valid
until drm_gem_object_handle_put_unlocked() runs. If Thread B calls
DRM_IOCTL_GEM_OPEN concurrently, msm_gem_open() can increment vma_ref back to
1 and submit a job using the IOVA. Thread A then acquires locks in
put_iova_spaces() and unconditionally unmaps the buffer via
put_iova_spaces_locked() without re-verifying vma_ref:

drivers/gpu/drm/msm/msm_gem.c:put_iova_spaces_locked() {
    ...
    drm_gpuvm_bo_for_each_va_safe (vma, vmatmp, vm_bo) {
        msm_gem_vma_unmap(vma, reason);
        if (close)
            msm_gem_vma_close(vma);
    }
}

Will this destroy Thread B's mapping while its job is running, freeing the
IOVA for immediate reuse?

[Severity: High]
This isn't a bug introduced by this patch, but does adding these blocking
calls cause a scheduling while atomic panic when tearing down a writeback
connector?

The drm core function drm_writeback_connector_cleanup() holds a spinlock with
IRQs disabled when tearing down pending jobs:

drivers/gpu/drm/drm_writeback.c:drm_writeback_connector_cleanup() {
    spin_lock_irqsave(&wb_connector->job_lock, flags);
    list_for_each_entry_safe(pos, n, &wb_connector->job_queue, list_entry) {
        list_del(&pos->list_entry);
        drm_writeback_cleanup_job(pos);
    }
}

This calls into msm_gem_vma_put() through the teardown path
(dpu_wb_conn_cleanup_job -> dpu_encoder_cleanup_wb_job ->
dpu_encoder_phys_wb_cleanup_wb_job -> msm_framebuffer_cleanup ->
msm_gem_vma_put).

Since dma_resv_wait_timeout() can wait indefinitely and put_iova_spaces()
acquires ww_mutexes via drm_exec, could this result in sleeping while holding
a spinlock with IRQs disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260822-msm-fix-export-v1-0-917b3cbedd43@oss.qualcomm.com?part=2