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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/msm: BO pin leak and VM state corruption in
  msm_vma_job_free()
--

commit 805b414859330e06db0b87fe27330012423f8dcc
Author: Jiwoo Kim <arpe1618@gmail.com>

drm/msm: Destroy the VM_BIND entity when its queue is closed

This commit destroys the drm_sched_entity of a VM_BIND submitqueue when
userspace closes the queue via MSM_SUBMITQUEUE_CLOSE or file close
instead of on the final submitqueue reference drop, and rejects new
VM_BIND jobs on closed queues.

> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> index 8d550d281704e..2dbe42551bd8f 100644
> --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
[ ... ]
> @@ -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);
> +}

[Severity: High]
Does cancelling queued VM_BIND jobs in drm_sched_entity_destroy() leak
pinned GEM objects and leave job->vm out of sync with the IOMMU page
tables?

Previously, msm_submitqueue_remove() did not touch the entity, and
msm_submitqueue_close() only called drm_sched_entity_flush(), which left
remaining jobs on the entity unless the process exited via SIGKILL.  Calling
drm_sched_entity_destroy() runs drm_sched_entity_fini() ->
drm_sched_entity_kill() whenever the flush times out on file close (for
example, while a job waits on an unsignaled in-fence) or is interrupted by
a fatal signal during MSM_SUBMITQUEUE_CLOSE.

Killed jobs bypass msm_vma_job_run() and go straight to
msm_vma_job_free(), which neither unpins job->bos_pinned BOs nor drops the
op->obj reference acquired by vm_op_enqueue():

drivers/gpu/drm/msm/msm_gem_vma.c:msm_vma_job_free() {
	...
	/* 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);
	}
	...
}

In addition, vm_bind_job_prepare() has already updated the VMA state in
job->vm (marking new VMAs as mapped and closing unmapped ones), so
discarding job->vm_ops without applying the page-table updates or calling
msm_gem_vm_unusable(job->vm) leaves the VM state inconsistent while the
context can still be used after MSM_SUBMITQUEUE_CLOSE.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114062541.2001-1-arpe1618@gmail.com?part=1