Re: [PATCH V17 10/12] drm/xe: Add sysfs interface for bad gpu vram pages

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Lockless access to `pos->blocks` in `serialize_bad_pages` under RCU races with non-RCU list mutations, leading to potential Use-After-Free and uninitialized reads.
- [Medium] Multi-tile VRAM support is broken in the device-level sysfs interface.
- [Medium] The `max_pages` field is never initialized or retrieved from the firmware, resulting in `max_pages: 0` in the sysfs output.
- [Medium] Stateless `bin_attribute` read handler causes corrupted output (data shifting/duplication) during multi-chunk reads.
- [Medium] Racy `allocation_size` calculation and incorrect loop termination condition cause mid-line truncation of sysfs output and dead code.
--

--- Patch [10]: [PATCH V17 10/12] drm/xe: Add sysfs interface for bad gpu vram pages ---
commit ec8b371c3f2a5abb7fd5a41d3967e178a7625d0a
Author: Tejas Upadhyay <[email protected]>

drm/xe: Add sysfs interface for bad gpu vram pages

This patch adds a sysfs interface at /sys/bus/pci/devices/<bdf>/vram_bad_pages
to expose bad VRAM pages resulting from hardware faults. It uses an RCU-protected 
list traversal to read the bad pages asynchronously without blocking normal VRAM 
allocation operations.

> diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> index 51b98460481d9..372bb25448f61 100644
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
[ ... ]
> @@ -874,3 +874,144 @@ 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 size_t serialize_bad_pages(struct xe_ttm_vram_mgr *mgr, char *buf, size_t max_len)
> +{
> +	struct xe_ttm_vram_offline_resource *pos;
> +	struct gpu_buddy_block *block;
> +	size_t s = 0;
> +	int printed;
> +	int count = 0;
> +
> +	rcu_read_lock();
> +
> +	printed = scnprintf(buf + s, max_len - s, "max_pages: %d\n", mgr->max_pages);

[Severity: Medium]
Is max_pages ever populated by the firmware? It appears to be added to
struct xe_ttm_vram_mgr and printed here, but never initialized, which would
result in it always printing 0.

> +	s += printed;
> +
> +	list_for_each_entry_rcu(pos, &mgr->offlined_pages, offlined_link) {
> +		if (count >= 10000 || s >= max_len)
> +			break;

[Severity: Medium]
Does this loop terminate correctly when the buffer is exhausted? When scnprintf
runs out of space, it returns max_len - 1. This leaves s at max_len - 1, meaning
the condition s >= max_len evaluates to false. 

Could this cause the loop to iterate up to 10000 times, repeatedly calling
scnprintf with 1 byte remaining?

> +
> +		block = list_first_entry_or_null(&pos->blocks, struct gpu_buddy_block, link);

[Severity: High]
Can this lockless access to pos->blocks race with list mutations? If a thread
encounters a VRAM fault, xe_ttm_vram_reserve_page_at_addr() publishes the entry
to the queued_pages list before populating its blocks:

list_add_rcu(&nentry->queued_link, &vram_mgr->queued_pages);
ret = xe_ttm_vram_buddy_alloc(..., &nentry->blocks, ...);

Since xe_ttm_vram_buddy_alloc() modifies nentry->blocks using non-RCU list
operations without memory barriers, could serialize_bad_pages() dereference a
partially written pointer or a block being freed on an allocation error path?

> +		if (!block)
> +			continue;
> +
> +		printed = scnprintf(buf + s, max_len - s, "0x%016llx : 0x%016llx : %c\n",
> +				    gpu_buddy_block_offset(block) >> PAGE_SHIFT,
> +				    gpu_buddy_block_size(&mgr->mm, block), 'R');
> +		s += printed;
> +		count++;
> +	}
[ ... ]
> +static ssize_t vram_bad_pages_bin_read(struct file *filp, struct kobject *kobj,
> +				       const struct bin_attribute *attr, char *buf,
> +				       loff_t off, size_t count)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct ttm_resource_manager *man;
> +	struct xe_ttm_vram_mgr *mgr;
> +	size_t allocation_size;
> +	struct xe_device *xe;
> +	size_t full_data_len;
> +	int active_entries;
> +	char *temp_buf;
> +
> +	xe = pdev_to_xe_device(pdev);
> +	man = ttm_manager_type(&xe->ttm, XE_PL_VRAM0);

[Severity: Medium]
Does this sysfs interface support multi-tile devices correctly? By explicitly
fetching XE_PL_VRAM0, this appears to ignore bad pages on any secondary VRAM
tiles. 

Furthermore, since the exported PFNs are relative to the VRAM region's dpa_base,
could the output be ambiguous on multi-tile architectures without an explicit
tile identifier?

> +	if (!man)
> +		return -ENODEV;
> +	mgr = to_xe_ttm_vram_mgr(man);
> +
> +	active_entries = READ_ONCE(mgr->n_offlined_pages) + READ_ONCE(mgr->n_queued_pages);
> +
> +	if (active_entries > 10000)
> +		active_entries = 10000;
> +
> +	allocation_size = 64 + (active_entries * 48);

[Severity: Medium]
Could this lockless allocation size calculation truncate the output? If the list
of offlined or queued pages grows between this calculation and the RCU list
traversal in serialize_bad_pages(), the buffer might be too small to hold the
full output.

> +
> +	temp_buf = kvmalloc(allocation_size, GFP_KERNEL);
> +	if (!temp_buf)
> +		return -ENOMEM;
> +
> +	/* serialize_bad_pages uses rcu_read_lock internally */
> +	full_data_len = serialize_bad_pages(mgr, temp_buf, allocation_size);
> +
> +	if (off >= full_data_len) {
> +		kvfree(temp_buf);
> +		return 0;
> +	}
> +
> +	if (off + count > full_data_len)
> +		count = full_data_len - off;
> +
> +	memcpy(buf, temp_buf + off, count);

[Severity: Medium]
Is it safe to regenerate the text dynamically on every read chunk? Since
bin_attribute read callbacks do not persist state across multiple reads, if the
VRAM page list changes between consecutive read syscalls on the same file
descriptor, the offsets will no longer align. 

Could this cause corrupted, duplicated, or truncated text output for userspace
tools parsing the file?

> +
> +	kvfree(temp_buf);
> +	return count;
> +}
[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr_types.h b/drivers/gpu/drm/xe/xe_ttm_vram_mgr_types.h
> index dc97b0ad0e510..efcf3e1d4e80c 100644
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr_types.h
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr_types.h
> @@ -37,6 +37,8 @@ struct xe_ttm_vram_mgr {
>  	struct mutex lock;
>  	/** @mem_type: The TTM memory type */
>  	u32 mem_type;
> +	/** @max_pages: max pages that can be in offline queue retrieved from FW */
> +	u16 max_pages;

[Severity: Medium]
As mentioned earlier, is this max_pages field actually initialized and retrieved
from the firmware anywhere in the codebase?

>  };
>  
>  /**

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10