Re: [PATCH RFC 03/12] drm/shmem: Add a drm_gem_shmem_sync() helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] drm/shmem: use-after-free in drm_gem_shmem_sync() due to unlocked sgt walk
- [Critical] drm/shmem: drm_gem_shmem_sync() calls dma_sync_single_* on coalesced IOVA sgl
- [High] drm/shmem: drm_gem_shmem_sync() allocates pages on unbacked or purged BOs
--

commit d4e8dcdfcd9df9ada386f6de7bfd50169a8e2266
Author: Faith Ekstrand <faith.ekstrand@collabora.com>

drm/shmem: Add a drm_gem_shmem_sync() helper

This commit adds a drm_gem_shmem_sync() helper to synchronize CPU-mapped
shmem GEM objects between the CPU and GPU via dma_sync_single_for_device()
and dma_sync_single_for_cpu().

> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 13c4a68fdde0e..9bf727fc1c197 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -790,6 +790,99 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_shmem_mmap);
>  
> +/**
> + * drm_gem_shmem_sync - Sync CPU-mapped data
> + * @shmem: shmem GEM object
> + * @offset: Offset into the GEM object
> + * @size: Size of the area to sync
> + * @type: Type of synchronization
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_gem_shmem_sync(struct drm_gem_shmem_object *shmem, size_t offset,
> +		       size_t size, enum drm_gem_shmem_sync_type type)
> +{
> +	const struct drm_device *dev = shmem->base.dev;
> +	struct sg_table *sgt;
> +	struct scatterlist *sgl;
> +	unsigned int count;
> +
> +	/* Make sure the range is in bounds. */
> +	if (offset + size < offset || offset + size > shmem->base.size)
> +		return -EINVAL;
> +
> +	/* Disallow CPU-cache maintenance on imported buffers. */
> +	if (drm_gem_is_imported(&shmem->base))
> +		return -EINVAL;
> +
> +	switch (type) {
> +	case DRM_GEM_SHMEM_SYNC_CPU_CACHE_FLUSH:
> +	case DRM_GEM_SHMEM_SYNC_CPU_CACHE_FLUSH_AND_INVALIDATE:
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* Don't bother if it's WC-mapped */
> +	if (shmem->map_wc)
> +		return 0;
> +
> +	/* Nothing to do if the size is zero. */
> +	if (size == 0)
> +		return 0;
> +
> +	sgt = drm_gem_shmem_get_pages_sgt(shmem);
> +	if (IS_ERR(sgt))
> +		return PTR_ERR(sgt);

[Severity: High]
Won't calling drm_gem_shmem_get_pages_sgt() here allocate backing pages and
create a DMA mapping even when the object has no backing pages yet
(!shmem->pages) or has already been purged (shmem->madv < 0)?

In drivers/gpu/drm/drm_gem_shmem_helper.c, drm_gem_shmem_get_pages_sgt()
calls drm_gem_shmem_get_pages_sgt_locked():

	if (shmem->sgt)
		return shmem->sgt;

	drm_WARN_ON(obj->dev, drm_gem_is_imported(obj));

	ret = drm_gem_shmem_get_pages_locked(shmem);

If drm_gem_shmem_sync() is called on an unbacked object, this populates
shmem->pages and shmem->sgt even though no CPU-mapped pages exist to sync.

More importantly, if the object was already purged via
drm_gem_shmem_purge_locked(), shmem->madv is -1 and shmem->sgt is NULL.
Calling drm_gem_shmem_get_pages_sgt() allocates fresh backing pages (or
increments pages_use_count on truncated pages if a VMA is still open) and
creates a new shmem->sgt while shmem->madv remains -1. Because
drm_gem_shmem_is_purgeable() in include/drm/drm_gem_shmem_helper.h checks
for shmem->madv > 0:

	return (shmem->madv > 0) &&
		!refcount_read(&shmem->pages_pin_count) && shmem->sgt &&
		!shmem->base.dma_buf && !drm_gem_is_imported(&shmem->base);

and drm_gem_shmem_madvise_locked() ignores updates when shmem->madv < 0,
those newly allocated pages and sg_table cannot be faulted in by userspace
and can never be reclaimed by the shrinker until the GEM object is
destroyed.

Should drm_gem_shmem_sync() check under shmem->base.resv whether the object
is purged or has no pages before attempting to sync?

> +
> +	for_each_sgtable_dma_sg(sgt, sgl, count) {

[Severity: Critical]
Can traversing sgt here race with drm_gem_shmem_purge_locked() and cause a
use-after-free?

In drivers/gpu/drm/drm_gem_shmem_helper.c, drm_gem_shmem_get_pages_sgt()
unlocks shmem->base.resv before returning sgt, and only increments
shmem->pages_use_count while leaving shmem->pages_pin_count at zero:

	ret = dma_resv_lock_interruptible(shmem->base.resv, NULL);
	if (ret)
		return ERR_PTR(ret);
	sgt = drm_gem_shmem_get_pages_sgt_locked(shmem);
	dma_resv_unlock(shmem->base.resv);

	return sgt;

For non-imported shmem objects, shmem->sgt and shmem->pages are protected by
shmem->base.resv. If userspace marks the buffer purgeable (shmem->madv > 0)
while shmem->pages_pin_count is zero, a concurrent shrinker thread calling
drm_gem_shmem_purge_locked() (or a caller invoking
__drm_gem_shmem_free_sgt_locked()) can acquire shmem->base.resv right after
drm_gem_shmem_get_pages_sgt() unlocks it:

	dma_unmap_sgtable(dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0);
	sg_free_table(shmem->sgt);
	kfree(shmem->sgt);
	shmem->sgt = NULL;

	drm_gem_shmem_put_pages_locked(shmem);

That unmaps and frees shmem->sgt and releases the backing pages while
drm_gem_shmem_sync() is walking sgt and running DMA cache maintenance in
for_each_sgtable_dma_sg().

Should drm_gem_shmem_sync() hold shmem->base.resv (using
drm_gem_shmem_get_pages_sgt_locked()) across the sg_table walk?

> +		if (size == 0)
> +			break;
> +
> +		dma_addr_t paddr = sg_dma_address(sgl);
> +		size_t len = sg_dma_len(sgl);
> +
> +		if (len <= offset) {
> +			offset -= len;
> +			continue;
> +		}
> +
> +		paddr += offset;
> +		len -= offset;
> +		len = min_t(size_t, len, size);
> +		size -= len;
> +		offset = 0;
> +
> +		/* It's unclear whether dma_sync_xxx() is the right API to do CPU
> +		 * cache maintenance given an IOMMU can register their own
> +		 * implementation doing more than just CPU cache flushes/invalidation,
> +		 * and what we really care about here is CPU caches only, but that's
> +		 * the best we have that is both arch-agnostic and does at least the
> +		 * CPU cache maintenance on a <page,offset,size> tuple.
> +		 *
> +		 * Also, I wish we could do a single
> +		 *
> +		 *	dma_sync_single_for_device(BIDIR)
> +		 *
> +		 * and get a flush+invalidate, but that's not how it's implemented
> +		 * in practice (at least on arm64), so we have to make it
> +		 *
> +		 *	dma_sync_single_for_device(TO_DEVICE)
> +		 *	dma_sync_single_for_cpu(FROM_DEVICE)
> +		 *
> +		 * for the flush+invalidate case.
> +		 */
> +		dma_sync_single_for_device(dev->dev, paddr, len, DMA_TO_DEVICE);
> +		if (type == DRM_GEM_SHMEM_SYNC_CPU_CACHE_FLUSH_AND_INVALIDATE)
> +			dma_sync_single_for_cpu(dev->dev, paddr, len, DMA_FROM_DEVICE);

[Severity: Critical]
Is it safe to pass coalesced scatterlist DMA segments (sg_dma_address(sgl)
and sg_dma_len(sgl)) to dma_sync_single_for_device() and
dma_sync_single_for_cpu() when the device is behind an IOMMU?

In drm_gem_shmem_get_pages_sgt_locked(), shmem->sgt is mapped with
dma_map_sgtable(). When dev->dev uses an IOMMU DMA domain, iommu_dma_map_sg()
coalesces physically discontiguous scatterlist entries (sgt->orig_nents) into
virtually contiguous IOVA segments (sgt->nents), so a single sg_dma_len(sgl)
entry can span multiple discontiguous physical pages.

However, in drivers/iommu/dma-iommu.c, iommu_dma_sync_single_for_cpu() and
iommu_dma_sync_single_for_device() expect a physically contiguous buffer (as
mapped by dma_map_single() or dma_map_page()) and only translate the starting
IOVA:

	phys = iommu_iova_to_phys(iommu_get_dma_domain(dev), dma_handle);
	if (!dev_is_dma_coherent(dev)) {
		arch_sync_dma_for_cpu(phys, size, dir);
		arch_sync_dma_flush();
	}

On a non-coherent device with an IOMMU, calling dma_sync_single_for_device()
and dma_sync_single_for_cpu() across a coalesced sg_dma_len(sgl) range will
perform CPU cache maintenance on [phys, phys + len) starting from the first
page's physical address, leaving the subsequent pages of the GEM object
unsynced while flushing or invalidating unrelated physical memory adjacent
to the first page.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-v3d-cached-maps-v1-0-0a6a6b363382@igalia.com?part=3