Re: [PATCH] drm/xe: Fix shrinker accounting double-subtraction on nested external pins

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Critical] Direct manipulation of `pin_count` by display framebuffer pins breaks the nested pin accounting, leading to a permanent shrinker leak and Use-After-Free.

Pre-existing issues:
- [High] madvise(DONTNEED) on imported dma-bufs underflows the shrinkable page count.
--

commit 0965d36457ef31061b507cea1086d7d661b82782
Author: Thomas Hellström <thomas.hellstrom@linux.intel.com>

drm/xe: Fix shrinker accounting double-subtraction on nested external pins

This commit guards shrinker accounting calls with pin-count transition
checks in the external pin paths. It ensures accounting is only updated
on the outermost pin and the final unpin to prevent underflows caused
by nested pins on imported buffers.

> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index dde309821237b..f8e473b14cfcb 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
[ ... ]
> @@ -3235,18 +3236,19 @@ int xe_bo_pin(struct xe_bo *bo, struct drm_exec *exec)
>  void xe_bo_unpin_external(struct xe_bo *bo)
>  {
>  	struct xe_device *xe = xe_bo_device(bo);
> +	bool last_unpin = bo->ttm.pin_count == 1;

[Severity: Critical]
Does this strict pin count equality break nested pin accounting when a
display framebuffer pin is involved?

If a user creates a BO, exports it via dma-buf, and sets it as a scanout
framebuffer, __xe_pin_fb_vma() will intentionally bypass xe_bo_pin_external()
and directly increment bo->ttm.pin_count to 2.

When the external pin is released, xe_bo_unpin_external() observes a pin
count of 2 rather than 1. With the new logic here, it skips removing the BO
from xe->pinned.late.external and skips adding pages back to the shrinker.

Finally, when the FB is unpinned, __xe_unpin_fb_vma() directly drops the pin
count to 0 without executing the required list and shrinker cleanups. This
leads to a permanent shrinker page accounting leak and a use-after-free
if the BO is freed while still linked into the shrinker list.

>  
>  	xe_assert(xe, !bo->vm);
>  	xe_assert(xe, xe_bo_is_pinned(bo));
>  	xe_assert(xe, xe_bo_is_user(bo));
>  
>  	spin_lock(&xe->pinned.lock);
> -	if (bo->ttm.pin_count == 1 && !list_empty(&bo->pinned_link))
> +	if (last_unpin && !list_empty(&bo->pinned_link))
>  		list_del_init(&bo->pinned_link);
>  	spin_unlock(&xe->pinned.lock);
>  
>  	ttm_bo_unpin(&bo->ttm);
> -	if (bo->ttm.ttm && ttm_tt_is_populated(bo->ttm.ttm))
> +	if (last_unpin && bo->ttm.ttm && ttm_tt_is_populated(bo->ttm.ttm))
>  		xe_ttm_tt_account_add(xe, bo->ttm.ttm);

[Severity: High]
This is a pre-existing issue, but does the shrinker accounting underflow
for imported dma-bufs when userspace marks them as DONTNEED?

In xe_ttm_tt_populate(), external BOs return early without calling
xe_ttm_tt_account_add(), meaning their pages are never added to the
shrinkable page count. However, TTM unconditionally sets the populated flag.

If an unprivileged user application imports a dma-buf and calls the MADVISE
ioctl to mark it as DONTNEED, xe_bo_set_purgeable_shrinker() sees the
populated flag and subtracts the pages from shrinkable pages. This causes an
integer underflow, leading to excessive shrinker invocations and system
instability.

> 
>  	/*
>  	 * FIXME: If we always use the reserve / unreserve functions for locking

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914134540.385186-1-thomas.hellstrom@linux.intel.com?part=1