Re: [PATCH V18 12/14] drm/xe: Expose bad VRAM pages via debugfs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Use-After-Free and data race due to lockless traversal of pos->blocks.
- [Medium] TOCTOU race in xe_ttm_vram_reserve_page_at_addr leads to duplicate tracking entries and state inconsistency.
- [Low] The max_pages field is printed in debugfs but is never initialized or populated.
--

commit b60ddd8b2f9645ba022492a4803a15126cca2249
Author: Tejas Upadhyay <tejas.upadhyay@intel.com>
Subject: drm/xe: Expose bad VRAM pages via debugfs

This commit adds a debugfs file vram_bad_pages that shows offlined and
queued VRAM pages across all tiles. Each entry displays the page frame
number, GPU page size, and status flag.

> diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
[ ... ]
> @@ -874,3 +875,68 @@ int xe_ttm_vram_handle_addr_fault(struct xe_device *xe, u64 addr)
>  	return xe_ttm_vram_reserve_page_at_addr(xe, addr - vr->dpa_base, vram_mgr, mm);
>  }
>  EXPORT_SYMBOL(xe_ttm_vram_handle_addr_fault);
> +
> +static int vram_bad_pages_show(struct seq_file *m, void *unused)
> +{
> +	struct xe_device *xe = m->private;
> +	struct xe_ttm_vram_offline_resource *pos;
> +	struct ttm_resource_manager *man;
> +	struct gpu_buddy_block *block;
> +	struct xe_ttm_vram_mgr *mgr;
> +	struct xe_tile *tile;
> +	u8 id;
> +
> +	man = ttm_manager_type(&xe->ttm, XE_PL_VRAM0);
> +	if (man)
> +		seq_printf(m, "max_pages: %d\n",
> +			   to_xe_ttm_vram_mgr(man)->max_pages);

[Severity: Low]
Is there a regression where max_pages is printed but never initialized? 

It is defined in struct xe_ttm_vram_mgr and printed here, but it doesn't
appear to be assigned a value anywhere in the driver codebase.

Will this debugfs file always incorrectly output max_pages: 0?

> +
> +	for_each_tile(tile, xe, id) {
> +		struct xe_vram_region *vr = tile->mem.vram;
> +
> +		man = ttm_manager_type(&xe->ttm, XE_PL_VRAM0 + id);
> +		if (!man || !vr)
> +			continue;
> +		mgr = to_xe_ttm_vram_mgr(man);
> +
> +		rcu_read_lock();
> +
> +		list_for_each_entry_rcu(pos, &mgr->offlined_pages, offlined_link) {
> +			block = list_first_entry_or_null(&pos->blocks,
> +							 struct gpu_buddy_block, link);

[Severity: High]
In xe_ttm_vram_reserve_page_at_addr(), an entry is published to the
queued_pages list via list_add_rcu() before its blocks are populated:

xe_ttm_vram_reserve_page_at_addr() {
    ...
    ++vram_mgr->n_queued_pages;
    list_add_rcu(&nentry->queued_link, &vram_mgr->queued_pages);
    ...
}

If the subsequent buddy allocation fails halfway, it rolls back and frees
the blocks directly back to the slab allocator without respecting RCU grace
periods. 

Additionally, gpu_buddy_alloc_blocks() modifies the list using standard
list_add and list_del rather than RCU-safe variants, which could create a
data race on the list pointers themselves.

Could an RCU reader here obtain a pointer to a block just as it is being
freed, resulting in a Use-After-Free regression when accessing the block
later?

> +			if (!block)
> +				continue;
> +
> +			seq_printf(m, "0x%016llx : 0x%016llx : R\n",
> +				   (gpu_buddy_block_offset(block) + vr->dpa_base) >> PAGE_SHIFT,
> +				   gpu_buddy_block_size(&mgr->mm, block));
> +		}
> +
> +		list_for_each_entry_rcu(pos, &mgr->queued_pages, queued_link) {

[Severity: Medium]
When xe_ttm_vram_reserve_page_at_addr() is called for an unallocated
address, it allocates nentry under the lock but drops the lock before
adding it to queued_pages:

xe_ttm_vram_reserve_page_at_addr() {
    scoped_guard(mutex, &vram_mgr->lock) {
        if (xe_ttm_vram_page_already_processed(vram_mgr, addr))
            return -EEXIST;
        ... if block is NULL, nentry is NOT added here ...
    }
    
    ... lock is dropped ...
    
    if (block) {
        ...
    } else {
        scoped_guard(mutex, &vram_mgr->lock) {
            ++vram_mgr->n_queued_pages;
            list_add_rcu(&nentry->queued_link, &vram_mgr->queued_pages);
            ...
        }
    }
}

Multiple concurrent faults on the same address could both bypass the
xe_ttm_vram_page_already_processed() check because neither added their
entry to the list yet.

Both threads would then reacquire the lock and add their nentry to
queued_pages. One might succeed (moving to offlined_pages) while the other
fails (staying in queued_pages).

Could this iteration encounter a regression with duplicate tracking entries
for the exact same physical address, resulting in conflicting states being
exposed here?

> +			u64 pfn, blk_size;
> +
> +			block = list_first_entry_or_null(&pos->blocks,
> +							 struct gpu_buddy_block, link);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826135136.204044-16-tejas.upadhyay@intel.com?part=12