Re: [PATCH v2 2/2] drm/xe: Fix stale pinned_link entry when fb-pin performs the final unpin

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Thu, 2026-10-01 at 18:26 +0100, Matthew Auld wrote:
> On 01/10/2026 14:40, Thomas Hellström wrote:
> > xe_bo_pin_external() and xe_bo_unpin_external() maintain the bo's
> > pinned_link list membership in xe->pinned.late.external, based on
> > whether the current call is the outermost pin or the final unpin.
> > However, the same external bo's pin_count can also be raised and
> > lowered directly by __xe_pin_fb_vma()/__xe_unpin_fb_vma(), which
> > pin
> > the bo as a display scanout buffer without going through
> > xe_bo_pin_external()/xe_bo_unpin_external() at all, and have no
> > notion of, or ownership over, pinned_link.
> > 
> > If a bo is pinned both externally (e.g. dma-buf export) and as an
> > fb,
> > and the external unpin happens first, xe_bo_unpin_external()
> > correctly
> > observes pin_count > 1 and leaves the bo on pinned_link. When the
> > fb
> > unpin later performs the true last unpin (pin_count 1 -> 0), it
> > never
> > touches pinned_link, leaving the bo linked on xe-
> > >pinned.late.external
> > indefinitely. Once the bo is subsequently freed, this stale list
> > entry
> > points into freed memory, corrupting the list and risking a
> > use-after-free the next time the list is walked or spliced.
> > 
> > The backup object pin/unpin sites in
> > xe_bo_notifier_prepare_pinned()/
> > xe_bo_notifier_unprepare_pinned() have the same bypass
> > characteristic,
> > though they never add their bo to a pinned list, so are not
> > affected
> > by this particular list-corruption issue.
> > 
> > Move the pinned_link removal into xe_bo_account_unpin(), which
> > already
> > runs on every unpin path (kernel, external, framebuffer, backup
> > object) right before the true 1 -> 0 pin_count transition. Since
> > list_del_init() only operates on the node itself, this removal is
> > list-agnostic and safe to perform regardless of which list
> > (external
> > or kernel_bo_present) the bo happens to be linked on, or which code
> > path is performing the final unpin. Drop the now-redundant explicit
> > list_del_init() calls in xe_bo_unpin_external() and xe_bo_unpin().
> 
> I think you can also use this idea to bypass the p2p checking? Create
> a 
> VRAM + TT buffer, turn it into an fb and use it for scanout. While fb
> pinned you dma-buf export/import + map it (dynamic). AFAICT you can 
> trick xe_dma_buf_map() since this buffer looks like you can migrate
> it 
> (dual placement) but since already fb pinned this will make it skip
> the 
> actual migrate/validate, so it falls through to mapping the VRAM
> using 
> the pci address. However, this could be on a system that lacks p2p 
> support. Don't see what prevents that?

Thanks, for reviewing, Matt. Yup that concern indeed seems valid. I'll
craft a separate patch for that. 

Meanwhile, Sashiko flagged a couple of issues with the current series,
so I'll send out a v3. Please let me know if your R-Bs still hold.

Thanks,
Thomas



> 
> > 
> > Fixes: 44e694958b95 ("drm/xe/display: Implement display support")
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: intel-xe@lists.freedesktop.org
> > Cc: <stable@vger.kernel.org> # v6.8+
> > Assisted-by: LLM
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > 
> > v2:
> > - New patch
> > ---
> >   drivers/gpu/drm/xe/xe_bo.c | 25 ++++++++++++++++---------
> >   1 file changed, 16 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_bo.c
> > b/drivers/gpu/drm/xe/xe_bo.c
> > index 2fbbba7cf4b0..581bfded21db 100644
> > --- a/drivers/gpu/drm/xe/xe_bo.c
> > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > @@ -487,12 +487,27 @@ static void xe_bo_account_pin(struct xe_bo
> > *bo)
> >    * held, before &ttm_buffer_object.pin_count is decremented by
> >    * ttm_bo_unpin(), so that the check against the true 1->0
> > transition sees
> >    * the pin count that is about to be released.
> > + *
> > + * On the true last unpin, also removes @bo from whichever pinned-
> > bo list
> > + * (external or kernel_bo_present) it may currently be linked on,
> > since a
> > + * bo's final unpin can happen through a pin path (e.g.
> > framebuffer,
> > + * backup object) that has no notion of, or ownership over, that
> > list.
> > + * This is safe and list-agnostic: list_del_init() only needs the
> > node
> > + * itself, not knowledge of which list it is threaded through, and
> > is a
> > + * no-op if @bo is not linked.
> >    */
> >   static void xe_bo_account_unpin(struct xe_bo *bo)
> >   {
> >   	struct xe_device *xe = xe_bo_device(bo);
> > +	bool last_unpin = bo->ttm.pin_count == 1;
> >   
> > -	if (bo->ttm.pin_count == 1 && bo->ttm.ttm &&
> > ttm_tt_is_populated(bo->ttm.ttm))
> > +	if (last_unpin && !list_empty(&bo->pinned_link)) {
> > +		spin_lock(&xe->pinned.lock);
> > +		list_del_init(&bo->pinned_link);
> > +		spin_unlock(&xe->pinned.lock);
> > +	}
> > +
> > +	if (last_unpin && bo->ttm.ttm && ttm_tt_is_populated(bo-
> > >ttm.ttm))
> >   		xe_ttm_tt_account_add(xe, bo->ttm.ttm);
> >   }
> >   
> > @@ -3311,11 +3326,6 @@ void xe_bo_unpin_external(struct xe_bo *bo)
> >   	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))
> > -		list_del_init(&bo->pinned_link);
> > -	spin_unlock(&xe->pinned.lock);
> > -
> >   	xe_bo_unpin_account(bo);
> >   
> >   	/*
> > @@ -3337,10 +3347,7 @@ void xe_bo_unpin(struct xe_bo *bo)
> >   	xe_assert(xe, xe_bo_is_pinned(bo));
> >   
> >   	if (mem_type_is_vram(place->mem_type) || bo->flags &
> > XE_BO_FLAG_GGTT) {
> > -		spin_lock(&xe->pinned.lock);
> >   		xe_assert(xe, !list_empty(&bo->pinned_link));
> > -		list_del_init(&bo->pinned_link);
> > -		spin_unlock(&xe->pinned.lock);
> >   
> >   		if (bo->backup_obj) {
> >   			if (xe_bo_is_pinned(bo->backup_obj))