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

[Matthew Auld <matthew.auld@intel.com>]

On 02/10/2026 10:52, Thomas Hellström wrote:
> 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.
> 

r-b's 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))