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