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 <[email protected]> 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/[email protected]?part=1
