Re: [PATCH RFC 03/13] drm/gma500: Export GEM objects as dma-bufs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] drm/gma500: stale GTT PTEs on resume in
  psb_gem_mm_populate_resources()
- [Medium] drm/gma500: missing stolen check in psb_gem_prime_export()
- [Medium] drm/gma500: unchecked set_pages_array_wc() in
  psb_gem_get_pages_locked()
- [Low] drm/gma500: unused split of psb_gem_pin_locked() and
  psb_gem_unpin_locked()
--

commit 32b59ef191363e7eeb07c9ca33caea7c6e0ca7e5
Author: Joseph Telaak <joseph@telaak.dev>

drm/gma500: Export GEM objects as dma-bufs

This commit adds PRIME export, pin, unpin, get_sg_table, vmap and vunmap
hooks for psb_gem_object so companion GPUs can import gma500 scanout
buffers as dma-bufs.

> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
> index 4a4ecfb7b299d..9eff7a739c98a 100644
> --- a/drivers/gpu/drm/gma500/gem.c
> +++ b/drivers/gpu/drm/gma500/gem.c
> @@ -26,71 +29,104 @@
>   * PSB GEM object
>   */
>  
> -int psb_gem_pin(struct psb_gem_object *pobj)
> +/*
> + * The object's pages, write-combined, for a GART pin or a PRIME export: an
> + * exported object needs them, not a slot in the small GTT aperture.
> + */
> +static int psb_gem_get_pages_locked(struct psb_gem_object *pobj)
> +{
> +	struct drm_gem_object *obj = &pobj->base;
> +	struct page **pages;
> +
> +	dma_resv_assert_held(obj->resv);
> +
> +	if (pobj->pages_use++)
> +		return 0;
> +	pages = drm_gem_get_pages(obj);
> +	if (IS_ERR(pages)) {
> +		pobj->pages_use--;
> +		return PTR_ERR(pages);
> +	}
> +	set_pages_array_wc(pages, obj->size / PAGE_SIZE);
> +	pobj->pages = pages;
> +	return 0;
> +}

[Severity: Medium]
What happens if set_pages_array_wc() fails here?

On failure, _set_pages_array() rolls back its PAT reservations and leaves
the pages write-back in the direct map.  Ignoring the error allows
psb_gem_prime_vmap() to create a write-combined vmap() alias against those
write-back pages, and causes psb_gem_put_pages_locked() to later call
set_pages_array_wb() -> memtype_free() on ranges whose reservations were
already freed.

> +
> +static void psb_gem_put_pages_locked(struct psb_gem_object *pobj)
> +{
> +	struct drm_gem_object *obj = &pobj->base;
> +
> +	dma_resv_assert_held(obj->resv);
> +
> +	if (WARN_ON(!pobj->pages_use) || --pobj->pages_use)
> +		return;
> +	/* Reset caching flags */
> +	set_pages_array_wb(pobj->pages, obj->size / PAGE_SIZE);
> +	drm_gem_put_pages(obj, pobj->pages, true, false);
> +	pobj->pages = NULL;
> +}

[Severity: High]
Can this leave stale physical page mappings in the GTT after a resume?

psb_gem_get_pages_locked() sets pobj->pages for PRIME pins and vmaps while
pobj->in_gart remains 0.  On resume, psb_gem_mm_populate_resources()
checks pobj->pages rather than pobj->in_gart:

drivers/gpu/drm/gma500/gem.c:psb_gem_mm_populate_resources() {
	...
		pobj = container_of(r, struct psb_gem_object, resource);
		if (pobj->pages) {
			psb_gtt_insert_pages(pdev, &pobj->resource, pobj->pages);
			size += resource_size(&pobj->resource);
			++restored;
		}
	...
}

When the PRIME pin or vmap is later released, psb_gem_put_pages_locked()
frees pobj->pages back to the page allocator without calling
psb_gtt_remove_pages(), leaving the hardware GTT entries pointing at freed
system RAM instead of the scratch page.

> +
> +static int psb_gem_pin_locked(struct psb_gem_object *pobj)
>  {
>  	struct drm_gem_object *obj = &pobj->base;
>  	struct drm_device *dev = obj->dev;
>  	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>  	u32 gpu_base = dev_priv->gtt.gatt_start;
> -	struct page **pages;
>  	unsigned int npages;
>  	int ret;
>  
> -	ret = dma_resv_lock(obj->resv, NULL);
> -	if (drm_WARN_ONCE(dev, ret, "dma_resv_lock() failed, ret=%d\n", ret))
> -		return ret;
> +	dma_resv_assert_held(obj->resv);

[ ... ]

> -void psb_gem_unpin(struct psb_gem_object *pobj)
> +static void psb_gem_unpin_locked(struct psb_gem_object *pobj)
>  {
>  	struct drm_gem_object *obj = &pobj->base;
>  	struct drm_device *dev = obj->dev;
>  	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>  	u32 gpu_base = dev_priv->gtt.gatt_start;
>  	unsigned long npages;
> -	int ret;
>  
> -	ret = dma_resv_lock(obj->resv, NULL);
> -	if (drm_WARN_ONCE(dev, ret, "dma_resv_lock() failed, ret=%d\n", ret))
> -		return;
> +	dma_resv_assert_held(obj->resv);

[Severity: Low]
This isn't a bug, but the commit message says psb_gem_pin() and
psb_gem_unpin() are split into locked variants for the PRIME helpers.
psb_gem_prime_pin() and psb_gem_prime_unpin() call
psb_gem_get_pages_locked() and psb_gem_put_pages_locked() instead, leaving
psb_gem_pin_locked() and psb_gem_unpin_locked() with no callers other than
psb_gem_pin() and psb_gem_unpin().

[ ... ]

> +/*
> + * dma_buf_mmap() maps through the object's fake offset (drm_gem_prime_mmap()),
> + * which a dumb buffer only gets from MAP_DUMB: make sure it has one.
> + */
> +static struct dma_buf *psb_gem_prime_export(struct drm_gem_object *obj, int flags)
> +{
> +	int ret;
> +
> +	ret = drm_gem_create_mmap_offset(obj);
> +	if (ret)
> +		return ERR_PTR(ret);
> +	return drm_gem_prime_export(obj, flags);
> +}

[Severity: Medium]
Should psb_gem_prime_export() reject stolen-memory objects with
ERR_PTR(-EINVAL) before creating the mmap offset and exporting the
dma-buf?

A client that obtains a GEM handle to the stolen-memory fbdev framebuffer
via DRM_IOCTL_MODE_GETFB can otherwise export it through
DRM_IOCTL_PRIME_HANDLE_TO_FD, allocating a fake mmap offset and returning
a dma-buf whose attach and vmap callbacks fail with -EINVAL (triggering
WARN_ON_ONCE(ret) in dma_buf_vmap()) and whose dma_buf_mmap() faults
through the broken stolen-memory path in psb_gem_fault().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-powervr-sgx-v1-0-c69b84bac011@telaak.dev?part=3