Re: [PATCH v15 5/6] drm/gem/shmem: Introduce __drm_gem_shmem_free_sgt_locked()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] `__drm_gem_shmem_free_sgt_locked` does not check if the GEM object is imported, corrupting memory by unmapping and freeing an alien scatterlist owned by the dma-buf exporter.
--

commit e016ae9bac4aed16d70a96e80b500f0d496fdf7d
Author: Lyude Paul <lyude@redhat.com>

drm/gem/shmem: Introduce __drm_gem_shmem_free_sgt_locked()

This commit extracts the scatterlist unmapping and freeing logic into a new
exported helper, __drm_gem_shmem_free_sgt_locked(). This is intended to allow
callers, such as the Rust shmem bindings, to manually unmap the sgt for a
GEM object as needed.

> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 545933c7f7121..c989459eb2159 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c

[ ... ]

> @@ -158,6 +158,30 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
>  
> +/**
> + * __drm_gem_shmem_release_sgt_locked - Unpin and DMA unmap pages, and release the
> + * cached scatter/gather table for an shmem GEM object.
> + * @shmem: shmem GEM object
> + *
> + * If the passed shmem object has an active scatter/gather table for driver
> + * usage, this function will unmap it and release the memory associated with it.
> + * It is the responsibility of the caller to ensure it holds the dma_resv_lock
> + * for this object.
> + *
> + * Drivers should not need to call this function themselves, it is mainly
> + * intended for usage in the Rust shmem bindings.
> + */
> +void __drm_gem_shmem_free_sgt_locked(struct drm_gem_shmem_object *shmem)
> +{
> +	dma_resv_assert_held(shmem->base.resv);
> +
> +	dma_unmap_sgtable(shmem->base.dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0);
> +	sg_free_table(shmem->sgt);
> +	kfree(shmem->sgt);
> +	shmem->sgt = NULL;
> +}
> +EXPORT_SYMBOL_GPL(__drm_gem_shmem_free_sgt_locked);

[Severity: Critical]
Does this helper need to check if the GEM object is imported?

If __drm_gem_shmem_free_sgt_locked() is called unconditionally on an imported
GEM object (such as from the Rust bindings dropping an SGTableMap), it seems
like it will unmap and free the scatterlist that is actually owned by the
dma-buf exporter. 

In the original code within drm_gem_shmem_release(), this memory was protected
by an explicit check:

	if (drm_gem_is_imported(obj)) {
		drm_prime_gem_destroy(obj, shmem->sgt);
	} else {
		...
		if (shmem->sgt)
			__drm_gem_shmem_free_sgt_locked(shmem);

Can bypassing the drm_gem_is_imported() check here lead to memory corruption
or double-free issues when the exporter also tries to clean up the table?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529183702.677677-1-lyude@redhat.com?part=5