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

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 <[email protected]>

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/[email protected]?part=2