[PATCH v2 0/3] drm/msm: Fix VM_BIND job, queue and entity lifetimes

[PATCH v2 0/3] drm/msm: Fix VM_BIND job, queue and entity lifetimes

[Jiwoo Kim]

These fix what happens to VM_BIND jobs, queues and entities when the
file or a VM_BIND queue is closed while jobs are still pending.

Patch 1 makes a VM_BIND job that is freed without having run apply its
queued unmaps before it drops the references that keep the unmapped
pages alive, release its BO pins, and mark the VM unusable, as the VM
no longer matches its pgtables. Patch 2 destroys the VM_BIND entity
when its queue is closed, instead of on the final put, which can come
after the VM's scheduler has been torn down. Patch 3 puts the queue
last in msm_vma_job_free(), as the queue may hold the last reference to
the VM.

Changes in v2:
- New patch 1, for an issue that Sashiko reported on v1 patch 1, which
  that patch makes easier to hit.
- Patch 2: changelog refers to patch 1.

v1: https://lore.kernel.org/r/20261114062541.2001-1-arpe1618@gmail.com

Jiwoo Kim (3):
  drm/msm: Release the pins and ops of a VM_BIND job that never ran
  drm/msm: Destroy the VM_BIND entity when its queue is closed
  drm/msm: Put the VM_BIND queue last in msm_vma_job_free()

 drivers/gpu/drm/msm/msm_gem_vma.c     | 77 +++++++++++++++++++++------
 drivers/gpu/drm/msm/msm_gpu.h         |  6 +++
 drivers/gpu/drm/msm/msm_submitqueue.c | 50 ++++++++++++-----
 3 files changed, 102 insertions(+), 31 deletions(-)


base-commit: 1e59855738454b14021b64115f0090eafc7526ae
-- 
2.43.0

[PATCH v2 1/3] drm/msm: Release the pins and ops of a VM_BIND job that never ran

[Jiwoo Kim]

msm_vma_job_run() applies the pgtable updates that vm_bind_job_prepare()
queued for a VM_BIND job, drops the GEM object references that
vm_op_enqueue() took for them, and unpins the BOs of the job. A job can
also be freed without having run: when its entity is killed, e.g. on
SIGKILL while the process exits, and when msm_ioctl_vm_bind() fails
part-way through vm_bind_job_prepare() or after it. msm_vma_job_free()
then frees the queued ops without applying them and without dropping
their references, and a killed job also keeps its BOs pinned. Besides
the leaks, the VM no longer matches its pgtables: for example, the
pages of a VMA that was unmapped from the VM are still mapped.

In msm_vma_job_free(), apply the queued updates the way
msm_vma_job_run() does on error: skip the maps, but apply the unmaps,
so that their references can be dropped without freeing pages that are
still mapped, and mark the VM unusable. Move that part of
msm_vma_job_run() into a helper for this. Also clear bos_pinned once
msm_vma_job_run() has unpinned the BOs, as msm_job_run() does for
submits, and unpin the BOs of a job that still has them pinned. The
job's fence does not keep them busy anymore at that point, so take the
object lock to unpin them.

This also marks the VM unusable when msm_ioctl_vm_bind() fails after
vm_bind_job_prepare() has queued updates, i.e. when the first op fails
part-way or when sync_file_create() fails. The VM is then left
partially updated, as when a later op fails.

The patch and its changelog were drafted with an AI coding assistant.
The issue has not been reproduced, and the change has only been
compile-tested.

Fixes: 2e6a8a1fe2b2 ("drm/msm: Add VM_BIND ioctl")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/r/sashiko-outbox-214234@kernel.org
Signed-off-by: Jiwoo Kim <arpe1618@gmail.com>
---
At commit 2e6a8a1fe2b2, msm_vma_job_run() still unpinned the BOs under
the object lock, and msm_gem_vm_unusable() did not exist yet, so older
stable trees need an adapted backport.

 drivers/gpu/drm/msm/msm_gem_vma.c | 65 +++++++++++++++++++++++--------
 1 file changed, 49 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
index 8b702422030a..a6f106d0cdef 100644
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -700,16 +700,16 @@ static const struct drm_gpuvm_ops msm_gpuvm_ops = {
 	.sm_step_unmap = msm_gem_vm_sm_step_unmap,
 };
 
-static struct dma_fence *
-msm_vma_job_run(struct drm_sched_job *_job)
+/*
+ * Apply the pgtable updates queued for a job, and drop the references that
+ * the ops hold.  If ret is an error, or one occurs, the remaining maps are
+ * skipped, but the unmaps are still applied, as the pages they unmap must
+ * not be freed while they are still mapped.
+ */
+static int
+vm_bind_job_apply_ops(struct msm_vm_bind_job *job, int ret)
 {
-	struct msm_vm_bind_job *job = to_msm_vm_bind_job(_job);
-	struct drm_device *dev = job->vm->drm;
 	struct msm_gem_vm *vm = to_msm_vm(job->vm);
-	struct drm_gem_object *obj;
-	int ret = vm->unusable ? -EINVAL : 0;
-
-	vm_dbg("");
 
 	mutex_lock(&vm->mmu_lock);
 	vm->mmu->prealloc = &job->prealloc;
@@ -740,6 +740,22 @@ msm_vma_job_run(struct drm_sched_job *_job)
 	vm->mmu->prealloc = NULL;
 	mutex_unlock(&vm->mmu_lock);
 
+	return ret;
+}
+
+static struct dma_fence *
+msm_vma_job_run(struct drm_sched_job *_job)
+{
+	struct msm_vm_bind_job *job = to_msm_vm_bind_job(_job);
+	struct drm_device *dev = job->vm->drm;
+	struct msm_gem_vm *vm = to_msm_vm(job->vm);
+	struct drm_gem_object *obj;
+	int ret;
+
+	vm_dbg("");
+
+	ret = vm_bind_job_apply_ops(job, vm->unusable ? -EINVAL : 0);
+
 	/*
 	 * We failed to perform at least _some_ of the pgtable updates, so
 	 * now the VM is in an undefined state.  Game over!
@@ -753,6 +769,8 @@ msm_vma_job_run(struct drm_sched_job *_job)
 		msm_gem_unpin_active(obj);
 	}
 
+	job->bos_pinned = false;
+
 	mutex_unlock(&dev->gem_lru_mutex);
 
 	/* VM_BIND ops are synchronous, so no fence to wait on: */
@@ -766,26 +784,41 @@ msm_vma_job_free(struct drm_sched_job *_job)
 	struct msm_gem_vm *vm = to_msm_vm(job->vm);
 	struct drm_gem_object *obj;
 
+	/*
+	 * In error paths, and if the job was killed before it ran, we could
+	 * have unexecuted ops.  Their changes were already made to the VM, so
+	 * apply the unmaps, and give up on the VM.
+	 */
+	if (!list_empty(&job->vm_ops)) {
+		vm_bind_job_apply_ops(job, -ECANCELED);
+		msm_gem_vm_unusable(job->vm);
+	}
+
 	vm->mmu->funcs->prealloc_cleanup(vm->mmu, &job->prealloc);
 
 	atomic_sub(job->prealloc.count, &vm->prealloc_throttle.in_flight);
 
 	drm_sched_job_cleanup(_job);
 
+	/*
+	 * A job that was killed before it ran still has its BOs pinned.  The
+	 * job's fence does not keep them busy anymore, so take the object lock
+	 * to unpin them.
+	 */
+	if (job->bos_pinned) {
+		job_foreach_bo(obj, job) {
+			msm_gem_lock(obj);
+			msm_gem_unpin_locked(obj);
+			msm_gem_unlock(obj);
+		}
+	}
+
 	job_foreach_bo (obj, job)
 		drm_gem_object_put(obj);
 
 	msm_submitqueue_put(job->queue);
 	dma_fence_put(job->fence);
 
-	/* In error paths, we could have unexecuted ops: */
-	while (!list_empty(&job->vm_ops)) {
-		struct msm_vm_op *op =
-			list_first_entry(&job->vm_ops, struct msm_vm_op, node);
-		list_del(&op->node);
-		kfree(op);
-	}
-
 	wake_up(&vm->prealloc_throttle.wait);
 
 	kfree(job);
-- 
2.43.0

[PATCH v2 2/3] drm/msm: Destroy the VM_BIND entity when its queue is closed

[Jiwoo Kim]

The entity of a VM_BIND submitqueue is attached to the per-VM scheduler,
and msm_gem_vm_close() calls drm_sched_fini() on that scheduler when the
file is closed, which frees its run queues. The submitqueue itself is
only destroyed on its final put, though, and every VM_BIND job holds a
reference to it. msm_submitqueue_close() only flushes the entity, and a
queue that was closed earlier with MSM_SUBMITQUEUE_CLOSE is not flushed
at all.

The flush gives up after a timeout when the process exits, or on a
fatal signal, and it only kills the queued jobs when the process exits
because of SIGKILL. In these cases, and after MSM_SUBMITQUEUE_CLOSE,
the entity can still have a callback armed on the in-fence of a
VM_BIND job when drm_sched_fini() runs. When that fence signals,
drm_sched_entity_wakeup() dereferences entity->rq->sched, and so reads
a run queue that drm_sched_fini() has freed. If the jobs were killed,
the final put of the queue then calls drm_sched_entity_destroy(), which
reads the same freed run queue in drm_sched_entity_flush(). If they
were not, they never run, and keep the queue, its context and the VM
alive.

Destroy the VM_BIND entity when userspace closes the queue, on file
close or with MSM_SUBMITQUEUE_CLOSE, instead of on the final put, and
refuse new VM_BIND jobs on a closed queue. In the MSM_SUBMITQUEUE_CLOSE
path, do it after dropping ctxlock, as destroying the entity waits for
the jobs queued on it.

As a result, MSM_SUBMITQUEUE_CLOSE on a VM_BIND queue now waits
(killably) for the jobs already queued on it, and a fatal signal during
that wait cancels the remaining ones, where they used to still run. On
file close, the jobs still queued when the wait gives up are now always
cancelled, as they already were when the process was killed while
exiting, instead of being left on the entity. With the previous patch,
such jobs apply their unmaps and release their pins and references.

This was found with an AI coding assistant, which also drafted this
patch and its changelog. It has not been reproduced, and the change
has only been compile-tested.

Fixes: 92395af63a99 ("drm/msm: Add VM_BIND submitqueue")
Cc: stable@vger.kernel.org
Signed-off-by: Jiwoo Kim <arpe1618@gmail.com>
---
The scoped_guard() in msm_submitqueue_remove() relies on commit
ea69d489d3a6 ("drm/msm: Rework queuelock"), so trees older than v7.3
need a manual backport.

If MSM_SUBMITQUEUE_CLOSE should rather not block, the destroy of closed
VM_BIND queues could instead be deferred to file close, e.g. with a
per-context list of them.

 drivers/gpu/drm/msm/msm_gem_vma.c     |  5 +++
 drivers/gpu/drm/msm/msm_gpu.h         |  6 ++++
 drivers/gpu/drm/msm/msm_submitqueue.c | 50 +++++++++++++++++++--------
 3 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
index a6f106d0cdef..b1fa1d749c2f 100644
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -1532,6 +1532,11 @@ msm_ioctl_vm_bind(struct drm_device *dev, void *data, struct drm_file *file)
 	if (ret)
 		goto out_post_unlock;
 
+	if (queue->vm_bind_closed) {
+		ret = -ENOENT;
+		goto out_unlock;
+	}
+
 	if (args->flags & MSM_VM_BIND_FENCE_FD_IN) {
 		struct dma_fence *in_fence;
 
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index bc5b31eb1eca..561f2d998e99 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -596,6 +596,12 @@ struct msm_gpu_submitqueue {
 	struct kref ref;
 	struct drm_sched_entity *entity;
 
+	/**
+	 * @vm_bind_closed: the queue was closed by userspace, and its VM_BIND
+	 * entity is being or has been destroyed.  Protected by @lock.
+	 */
+	bool vm_bind_closed;
+
 	/** @_vm_bind_entity: used for @entity pointer for VM_BIND queues */
 	struct drm_sched_entity _vm_bind_entity[0];
 };
diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
index 8d550d281704..2dbe42551bd8 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -79,9 +79,6 @@ void msm_submitqueue_destroy(struct kref *kref)
 
 	idr_destroy(&queue->fence_idr);
 
-	if (queue->entity == &queue->_vm_bind_entity[0])
-		drm_sched_entity_destroy(queue->entity);
-
 	msm_context_put(queue->ctx);
 
 	kfree(queue);
@@ -107,6 +104,25 @@ struct msm_gpu_submitqueue *msm_submitqueue_get(struct msm_context *ctx,
 	return NULL;
 }
 
+/*
+ * The VM_BIND scheduler is torn down by msm_gem_vm_close() when the file is
+ * closed, but a VM_BIND queue can outlive that, as long as a job still holds
+ * a reference to it.  So destroy the VM_BIND entity here, when userspace
+ * closes the queue, rather than on the final put.  Both ways a queue is taken
+ * off ctx->submitqueues come through here.
+ */
+static void msm_submitqueue_close_vm_bind(struct msm_gpu_submitqueue *queue)
+{
+	if (queue->entity != &queue->_vm_bind_entity[0])
+		return;
+
+	mutex_lock(&queue->lock);
+	queue->vm_bind_closed = true;
+	mutex_unlock(&queue->lock);
+
+	drm_sched_entity_destroy(queue->entity);
+}
+
 void msm_submitqueue_close(struct msm_context *ctx)
 {
 	struct msm_gpu_submitqueue *queue, *tmp;
@@ -119,8 +135,7 @@ void msm_submitqueue_close(struct msm_context *ctx)
 	 * be any more user ioctls coming our way
 	 */
 	list_for_each_entry_safe(queue, tmp, &ctx->submitqueues, node) {
-		if (queue->entity == &queue->_vm_bind_entity[0])
-			drm_sched_entity_flush(queue->entity, MAX_WAIT_SCHED_ENTITY_Q_EMPTY);
+		msm_submitqueue_close_vm_bind(queue);
 		list_del(&queue->node);
 		msm_submitqueue_put(queue);
 	}
@@ -326,7 +341,7 @@ int msm_submitqueue_query(struct drm_device *drm, struct msm_context *ctx,
 
 int msm_submitqueue_remove(struct msm_context *ctx, u32 id)
 {
-	struct msm_gpu_submitqueue *entry;
+	struct msm_gpu_submitqueue *entry, *queue = NULL;
 
 	if (!ctx)
 		return 0;
@@ -338,16 +353,23 @@ int msm_submitqueue_remove(struct msm_context *ctx, u32 id)
 	if (!id)
 		return -ENOENT;
 
-	guard(rwsem_write)(&ctx->ctxlock);
-
-	list_for_each_entry(entry, &ctx->submitqueues, node) {
-		if (entry->id == id) {
-			list_del(&entry->node);
-			msm_submitqueue_put(entry);
-			return 0;
+	scoped_guard(rwsem_write, &ctx->ctxlock) {
+		list_for_each_entry(entry, &ctx->submitqueues, node) {
+			if (entry->id == id) {
+				list_del(&entry->node);
+				queue = entry;
+				break;
+			}
 		}
 	}
 
-	return -ENOENT;
+	if (!queue)
+		return -ENOENT;
+
+	/* Not under ctxlock: destroying the entity can wait for its jobs */
+	msm_submitqueue_close_vm_bind(queue);
+	msm_submitqueue_put(queue);
+
+	return 0;
 }
 
-- 
2.43.0

[PATCH v2 3/3] drm/msm: Put the VM_BIND queue last in msm_vma_job_free()

[Jiwoo Kim]

msm_vma_job_free() puts the job's submitqueue and then wakes up
vm->prealloc_throttle.wait. The job does not hold a reference to the
VM of its own: the VM is kept alive by the context, which the queue
holds a reference to. When the job holds the last reference to the
queue, as it can once the file has been closed, for example when a
cancelled job is freed, the put can drop the last reference to the VM,
and the wakeup then uses a VM that msm_gem_vm_free() has released.

Since commit 01c8d1f385f7 ("drm/msm: RCU-free the scheduler-containing
ring and VM objects"), msm_gem_vm_free() frees the VM with kfree_rcu(),
but msm_vma_job_free() is not in an RCU read-side critical section, so
a grace period can still end before the wakeup, e.g. if the task is
preempted in between. Before that commit, the VM was freed right away.

Put the queue only after the last use of the VM.

This was found with an AI coding assistant, which also drafted this
patch and its changelog. It has not been reproduced, and the change
has only been compile-tested.

Fixes: b74fae5492d1 ("drm/msm: Add VM_BIND throttling")
Cc: stable@vger.kernel.org
Signed-off-by: Jiwoo Kim <arpe1618@gmail.com>
---
 drivers/gpu/drm/msm/msm_gem_vma.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
index b1fa1d749c2f..1bffade78623 100644
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -816,11 +816,16 @@ msm_vma_job_free(struct drm_sched_job *_job)
 	job_foreach_bo (obj, job)
 		drm_gem_object_put(obj);
 
-	msm_submitqueue_put(job->queue);
 	dma_fence_put(job->fence);
 
 	wake_up(&vm->prealloc_throttle.wait);
 
+	/*
+	 * The queue may hold the last reference to the context, and so to the
+	 * VM, so put it only once we are done with the VM.
+	 */
+	msm_submitqueue_put(job->queue);
+
 	kfree(job);
 }
 
-- 
2.43.0

Re: [PATCH v2 0/3] drm/msm: Fix VM_BIND job, queue and entity lifetimes

[Rob Clark]

On Sat, Nov 14, 2026 at 12:19 AM Jiwoo Kim <arpe1618@gmail.com> wrote:
>
> These fix what happens to VM_BIND jobs, queues and entities when the
> file or a VM_BIND queue is closed while jobs are still pending.
>
> Patch 1 makes a VM_BIND job that is freed without having run apply its
> queued unmaps before it drops the references that keep the unmapped
> pages alive, release its BO pins, and mark the VM unusable, as the VM
> no longer matches its pgtables. Patch 2 destroys the VM_BIND entity
> when its queue is closed, instead of on the final put, which can come
> after the VM's scheduler has been torn down. Patch 3 puts the queue
> last in msm_vma_job_free(), as the queue may hold the last reference to
> the VM.

msm_gem_vm_close() waits on vm->last_fence which will be signaled
after msm_vma_job_run() returns.

My expectation is that if thread A is in midst of a VM_BIND ioctl,
perhaps not having created/updated last_fence yet, while thread B is
racing it closing the device fd, that file_operations::release will be
called after thread A's ioctl call returns.  Is this not the case?

BR,
-R

> Changes in v2:
> - New patch 1, for an issue that Sashiko reported on v1 patch 1, which
>   that patch makes easier to hit.
> - Patch 2: changelog refers to patch 1.
>
> v1: https://lore.kernel.org/r/20261114062541.2001-1-arpe1618@gmail.com
>
> Jiwoo Kim (3):
>   drm/msm: Release the pins and ops of a VM_BIND job that never ran
>   drm/msm: Destroy the VM_BIND entity when its queue is closed
>   drm/msm: Put the VM_BIND queue last in msm_vma_job_free()
>
>  drivers/gpu/drm/msm/msm_gem_vma.c     | 77 +++++++++++++++++++++------
>  drivers/gpu/drm/msm/msm_gpu.h         |  6 +++
>  drivers/gpu/drm/msm/msm_submitqueue.c | 50 ++++++++++++-----
>  3 files changed, 102 insertions(+), 31 deletions(-)
>
>
> base-commit: 1e59855738454b14021b64115f0090eafc7526ae
> --
> 2.43.0
>

Re: [PATCH v2 0/3] drm/msm: Fix VM_BIND job, queue and entity lifetimes

[Jiwoo Kim]

On Sat, Nov 14, 2026 at 7:03 AM Rob Clark <rob.clark@oss.qualcomm.com> wrote:
> msm_gem_vm_close() waits on vm->last_fence which will be signaled
> after msm_vma_job_run() returns.

vm->last_fence only collects the fences of GEM_SUBMIT jobs, in
submit_attach_object_fences(). VM_BIND jobs don't add theirs, so
msm_gem_vm_close() waits for a VM_BIND job only if a submit depends
on it.

> My expectation is that if thread A is in midst of a VM_BIND ioctl,
> perhaps not having created/updated last_fence yet, while thread B is
> racing it closing the device fd, that file_operations::release will be
> called after thread A's ioctl call returns.  Is this not the case?

It is the case. (The vm_bind_closed check in patch 2 is only for
MSM_SUBMITQUEUE_CLOSE, which can race with a VM_BIND ioctl on the same
queue in another thread.)

The problem is a job that the ioctl has already pushed and that is
still waiting for its in-fence, e.g. one from another device or
drm_file, when the file is released:

- If the queue was closed with MSM_SUBMITQUEUE_CLOSE first,
  msm_submitqueue_close() no longer sees it, and nothing waits for the
  job. Otherwise drm_sched_entity_flush() waits at most
  MAX_WAIT_SCHED_ENTITY_Q_EMPTY for an exiting task, and kills the
  remaining jobs only on SIGKILL. Either way, the job's reference
  keeps the queue, and so the entity in it, alive.
- msm_gem_vm_close() then calls drm_sched_fini() on vm->sched, which
  frees the run queue that the entity still points to.
- When the in-fence signals, drm_sched_entity_wakeup(), which the
  scheduler registered on it, uses entity->rq->sched;
  drm_sched_entity_kill() leaves that callback in place. A killed job
  is freed then too, and the final put of the queue calls
  drm_sched_entity_destroy(), whose flush reads entity->rq->sched
  again.

Patch 2 destroys a VM_BIND queue's entity when the queue is closed,
by MSM_SUBMITQUEUE_CLOSE or on file close, so always before
msm_gem_vm_close() tears the scheduler down.

This comes from code review (see the note in the patches), and I have
not reproduced it.

v3 is at
https://lore.kernel.org/r/20261114141851.134290-1-arpe1618@gmail.com
and its patch 2 is unchanged from v2.

Separately, Sashiko's review of v3 found an ordering problem that
predates this series: the VM_BIND queues of a VM update the VM's VMAs
at ioctl time, but their pgtable updates are not ordered across
queues, so an unmap can reach the pgtables before the map it undoes.
That does not need a queue to be closed, but v3 patch 1 adds another
way to get there: the unmaps it applies when freeing a killed job are
not ordered against other queues either. (Its other point, that a
killed job's fence signals before those unmaps are applied, I'll
address in v4 as well.)

For v4 I would make a job that unmaps a VMA wait for the job that maps
it. The other way round, a map that overtakes an unmap of the same
range fails or gets undone, which breaks that VM but doesn't leave
freed pages mapped. Ordering all VM_BIND jobs of a VM would be
simpler, but turnip uses one VM_BIND queue per device plus one per
Vulkan queue. Would that be the right direction?

Thanks,
Jiwoo Kim

Re: [PATCH v2 0/3] drm/msm: Fix VM_BIND job, queue and entity lifetimes

[Rob Clark]

On Sat, Nov 14, 2026 at 7:36 AM Jiwoo Kim <arpe1618@gmail.com> wrote:
>
> On Sat, Nov 14, 2026 at 7:03 AM Rob Clark <rob.clark@oss.qualcomm.com> wrote:
> > msm_gem_vm_close() waits on vm->last_fence which will be signaled
> > after msm_vma_job_run() returns.
>
> vm->last_fence only collects the fences of GEM_SUBMIT jobs, in
> submit_attach_object_fences(). VM_BIND jobs don't add theirs, so
> msm_gem_vm_close() waits for a VM_BIND job only if a submit depends
> on it.

Oh, right.  We should probably just update last_fence for VM_BIND in
the same way.  (Ie. using dma_fence_unwrap_merge() to deal with the
case that the current last_fence is on a different timeline.)  I
overlooked that.

> > My expectation is that if thread A is in midst of a VM_BIND ioctl,
> > perhaps not having created/updated last_fence yet, while thread B is
> > racing it closing the device fd, that file_operations::release will be
> > called after thread A's ioctl call returns.  Is this not the case?
>
> It is the case. (The vm_bind_closed check in patch 2 is only for
> MSM_SUBMITQUEUE_CLOSE, which can race with a VM_BIND ioctl on the same
> queue in another thread.)
>
> The problem is a job that the ioctl has already pushed and that is
> still waiting for its in-fence, e.g. one from another device or
> drm_file, when the file is released:
>
> - If the queue was closed with MSM_SUBMITQUEUE_CLOSE first,
>   msm_submitqueue_close() no longer sees it, and nothing waits for the
>   job. Otherwise drm_sched_entity_flush() waits at most
>   MAX_WAIT_SCHED_ENTITY_Q_EMPTY for an exiting task, and kills the
>   remaining jobs only on SIGKILL. Either way, the job's reference
>   keeps the queue, and so the entity in it, alive.
> - msm_gem_vm_close() then calls drm_sched_fini() on vm->sched, which
>   frees the run queue that the entity still points to.
> - When the in-fence signals, drm_sched_entity_wakeup(), which the
>   scheduler registered on it, uses entity->rq->sched;
>   drm_sched_entity_kill() leaves that callback in place. A killed job
>   is freed then too, and the final put of the queue calls
>   drm_sched_entity_destroy(), whose flush reads entity->rq->sched
>   again.

I think if last_fence is also accounting for VM_BIND jobs, this should
ensure that all VM_BIND jobs are also finished.

> Patch 2 destroys a VM_BIND queue's entity when the queue is closed,
> by MSM_SUBMITQUEUE_CLOSE or on file close, so always before
> msm_gem_vm_close() tears the scheduler down.
>
> This comes from code review (see the note in the patches), and I have
> not reproduced it.
>
> v3 is at
> https://lore.kernel.org/r/20261114141851.134290-1-arpe1618@gmail.com
> and its patch 2 is unchanged from v2.
>
> Separately, Sashiko's review of v3 found an ordering problem that
> predates this series: the VM_BIND queues of a VM update the VM's VMAs
> at ioctl time, but their pgtable updates are not ordered across
> queues, so an unmap can reach the pgtables before the map it undoes.
> That does not need a queue to be closed, but v3 patch 1 adds another
> way to get there: the unmaps it applies when freeing a killed job are
> not ordered against other queues either. (Its other point, that a
> killed job's fence signals before those unmaps are applied, I'll
> address in v4 as well.)

Yes, userspace must use fences/syncobjs to synchronize VM_BINDs if it
is using multiple VM_BIND queues with overlapping updates.  An error
would mark the vm unuseable, and the kernel will stop accepting
SUBMITs and VM_BINDs.  But maybe we should also handle errors in the
UNMAP path as well, as that also indicates that userspace is doing
something wrong?

Userspace shouldn't be allowed to cause the kernel to do something bad
(use-after-free, freeing pages that are still mapped, etc).  But it is
perfectly fine to let userspace shoot its own foot, for example
triggering an SMMU fault due to pages unmapped while the GPU is still
accessing them.

BR,
-R

> For v4 I would make a job that unmaps a VMA wait for the job that maps
> it. The other way round, a map that overtakes an unmap of the same
> range fails or gets undone, which breaks that VM but doesn't leave
> freed pages mapped. Ordering all VM_BIND jobs of a VM would be
> simpler, but turnip uses one VM_BIND queue per device plus one per
> Vulkan queue. Would that be the right direction?
>
> Thanks,
> Jiwoo Kim

Re: [PATCH v2 0/3] drm/msm: Fix VM_BIND job, queue and entity lifetimes

[Jiwoo Kim]

On Sat, Nov 14, 2026 at 8:40 AM Rob Clark <rob.clark@oss.qualcomm.com> wrote:
> Oh, right.  We should probably just update last_fence for VM_BIND in
> the same way.  (Ie. using dma_fence_unwrap_merge() to deal with the
> case that the current last_fence is on a different timeline.)  I
> overlooked that.

Will do in v4.

> I think if last_fence is also accounting for VM_BIND jobs, this should
> ensure that all VM_BIND jobs are also finished.

It covers their fences, but not the free of a killed job:
drm_sched_entity_kill_jobs_work() signals the finished fence and only
then calls free_job(), which can drop the last reference to the queue
after msm_gem_vm_close() has torn the scheduler down.
msm_submitqueue_destroy() then calls drm_sched_entity_destroy(), whose
flush reads entity->rq->sched from the run queue that drm_sched_fini()
freed. Jobs that ran are freed before drm_sched_fini(), by the
scheduler or in drm_sched_stop(). So I'd keep patch 2 on top of the
last_fence change.

> Yes, userspace must use fences/syncobjs to synchronize VM_BINDs if it
> is using multiple VM_BIND queues with overlapping updates.  An error
> would mark the vm unuseable, and the kernel will stop accepting
> SUBMITs and VM_BINDs.  But maybe we should also handle errors in the
> UNMAP path as well, as that also indicates that userspace is doing
> something wrong?

Yes, I'll do that, but it only catches an unmap that finds part of its
range unmapped, not one that runs while the range still holds the PTEs
of an older mapping, e.g. one whose own unmap is still queued on a
third queue. The early unmap then succeeds. If the map runs next,
before the third queue's unmap, its PTEs belong to no VMA, so nothing
keeps the BO alive for them until that unmap removes them, and none of
the three ops fails.

So instead of making unmaps wait for maps, in v4 I'd also track
whether the map of a VMA has been applied, and mark the VM unusable
when its unmap runs first. The jobs of a VM run one at a time on
vm->sched, but v3 patch 1 applies the unmaps of a killed job from the
kill worker, and msm_vma_job_run() reads vm->unusable before taking
mmu_lock, so a map waiting for the lock would still be applied. The
check has to move under the lock, and the flag has to be set before
the unmap path drops the lock.

This is all from reading the code; I haven't reproduced it.

Thanks,
Jiwoo Kim