Re: [PATCH v2 1/2] drm/xe: Expose retired VRAM pages via drm-ras

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/ras: UAF in drm_ras_nl_get_retired_resources_dumpit() due to missing node lifetime management
- [Medium] drm/xe: lockless sequential list traversal in xe_ttm_vram_get_retired_page skips moving entries
- [Medium] xe: data race in xe_ttm_vram_get_retired_page due to lockless pos->blocks traversal
- [Low] netlink: use of u64 and pad attributes for new families instead of uint
--

commit fb04233297e64629b7560f39b814687dd56470e6
Author: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>

drm/xe: Expose retired VRAM pages via drm-ras

Add a proper userspace interface using the drm-ras generic netlink family
for offlined VRAM pages.

> diff --git a/Documentation/netlink/specs/drm_ras.yaml b/Documentation/netlink/specs/drm_ras.yaml
> index 4c2ac9a1ba3f5..42ab1cceaea73 100644
> --- a/Documentation/netlink/specs/drm_ras.yaml
> +++ b/Documentation/netlink/specs/drm_ras.yaml
[ ... ]
> @@ -100,6 +123,45 @@ attribute-sets:
>          name: error-value
>          type: u32
>          doc: Current value of the error counter.
> +  -
> +    name: retired-resource-attrs
> +    attributes:
[ ... ]
> +      -
> +        name: address
> +        type: u64
> +        doc: Device address of the retired VRAM page (e.g. DPA).
> +      -
> +        name: size
> +        type: u64
> +        doc: Size of the retired VRAM page in bytes.
> +      -
> +        name: max-pages
> +        type: u32
> +        doc: >-
> +             Maximum VRAM pages that can be retired. Present only in the
> +             leading summary entry of the dump (no address/size/status).
> +      -
> +        name: status
> +        type: u32
> +        doc: Retirement status of the retired VRAM page.
> +        enum: retired-resource-status
> +      -
> +        name: pad
> +        type: pad

[Severity: Low]
Does this new family attribute set need manual padding and fixed-width u64
types?

The netlink guidelines recommend using the dynamically sized uint type instead
of u64 and manual padding for new attributes.

> diff --git a/drivers/gpu/drm/drm_ras.c b/drivers/gpu/drm/drm_ras.c
> index b099eb67836e7..46ee897a13c94 100644
> --- a/drivers/gpu/drm/drm_ras.c
> +++ b/drivers/gpu/drm/drm_ras.c
[ ... ]
> @@ -564,6 +577,148 @@ int drm_ras_nl_clear_error_counter_doit(struct sk_buff *skb,
[ ... ]
> +int drm_ras_nl_get_retired_resources_dumpit(struct sk_buff *skb,
> +					    struct netlink_callback *cb)
> +{
> +	const struct genl_info *info = genl_info_dump(cb);
> +	struct drm_ras_ctx *ctx = (void *)cb->ctx;
> +	struct drm_ras_retired_resource res;
> +	struct drm_ras_node *node;
> +	struct nlattr *hdr;
> +	u32 node_id;
> +	u64 cursor;
> +	int ret = 0;
> +
> +	if (!info->attrs ||
> +	    GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_RETIRED_RESOURCE_ATTRS_NODE_ID))
> +		return -EINVAL;
> +
> +	node_id = nla_get_u32(info->attrs[DRM_RAS_A_RETIRED_RESOURCE_ATTRS_NODE_ID]);
> +
> +	node = xa_load(&drm_ras_xa, node_id);
> +	if (!node || node->type != DRM_RAS_NODE_TYPE_RETIRED_RESOURCES ||
> +	    !node->query_retired_resource)
> +		return -ENOENT;
> +
> +	/* Leading type-specific capacity summary (e.g. max pages), emitted once. */
> +	if (!ctx->summary_sent && node->query_retired_summary) {

[Severity: High]
Can the locklessly loaded node pointer be freed concurrently?

The generic netlink interface runs without genl_mutex. xa_load() drops the
RCU read lock before returning, and the node lacks any reference counting.

If a device teardown concurrently removes the node via cleanup_node() and
frees its memory, could accessing node->query_retired_summary() cause a
use-after-free?

> +		memset(&res, 0, sizeof(res));
> +		ret = node->query_retired_summary(node, &res);
> +		if (ret == 0) {
[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> index 9a514d983e90b..b56b543a56a24 100644
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
[ ... ]
> @@ -624,6 +626,123 @@ u64 xe_ttm_vram_get_avail(struct ttm_resource_manager *man)
[ ... ]
> +int xe_ttm_vram_get_retired_page(struct xe_device *xe, u64 min_addr,
> +				 u64 *addr, u64 *size, u32 *status)
> +{
> +	struct xe_ttm_vram_offline_resource *pos;
> +	struct ttm_resource_manager *man;
> +	struct gpu_buddy_block *block;
> +	struct xe_ttm_vram_mgr *mgr;
> +	u64 best_addr = 0, best_size = 0;
> +	struct xe_tile *tile;
> +	u32 best_status = 0;
> +	bool found = false;
> +	u8 id;
> +
> +	for_each_tile(tile, xe, id) {
> +		struct xe_vram_region *vr = tile->mem.vram;
> +		u64 a, s;
> +
> +		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) {

[Severity: Medium]
Will this lockless sequential traversal skip entries if they are moving
from queued_pages to offlined_pages?

If xe_ttm_vram_retry_queued_pages() concurrently moves an element from
queued_pages to offlined_pages after this loop has already scanned
offlined_pages, the element would also be missing when the second loop
scans queued_pages. This would cause the element to be permanently skipped
during the dump.

> +			block = list_first_entry_or_null(&pos->blocks,
> +							 struct gpu_buddy_block, link);
> +			if (block) {
> +				a = gpu_buddy_block_offset(block) + vr->dpa_base;
> +				s = gpu_buddy_block_size(&mgr->mm, block);

[Severity: Medium]
Is it safe to read pos->blocks and the block header locklessly here?

If a page reservation is concurrently retried via
xe_ttm_vram_retry_queued_pages(), xe_ttm_vram_buddy_alloc() dynamically
allocates and appends a new block to pos->blocks using a plain list append.

Because this traversal relies on RCU, could it observe an uninitialized
gpu_buddy_block header, resulting in garbage address and size values being
returned to userspace?

> +			} else {
> +				a = pos->addr + vr->dpa_base;
> +				s = SZ_4K;
> +			}
> +
> +			if (a >= min_addr && (!found || a < best_addr)) {
> +				best_addr = a;
> +				best_size = s;
> +				best_status = DRM_RAS_RETIRED_RESOURCE_STATUS_RETIRED;
> +				found = true;
> +			}
> +		}
> +
> +		list_for_each_entry_rcu(pos, &mgr->queued_pages, queued_link) {
> +			block = list_first_entry_or_null(&pos->blocks,
> +							 struct gpu_buddy_block, link);
> +			if (block) {
> +				a = gpu_buddy_block_offset(block) + vr->dpa_base;
> +				s = gpu_buddy_block_size(&mgr->mm, block);
> +			} else {
> +				a = pos->addr + vr->dpa_base;
> +				s = SZ_4K;
> +			}
> +
> +			if (a >= min_addr && (!found || a < best_addr)) {
> +				best_addr = a;
> +				best_size = s;
> +				best_status = pos->status == XE_PAGE_RESERVE_FAIL ?
> +					      DRM_RAS_RETIRED_RESOURCE_STATUS_FAILED :
> +					      DRM_RAS_RETIRED_RESOURCE_STATUS_PENDING;
> +				found = true;
> +			}
> +		}
> +
> +		rcu_read_unlock();
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007080354.3072751-1-aravind.iddamsetty@linux.intel.com?part=1