Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] xe: use-after-free race in xe_bo_apply_to_pinned() due to 0-refcount BOs - [High] xe: shrinker accounting leak when pinned BO is destroyed - [High] xe: unconditional panic due to NULL bdev in ___xe_bo_create error path --
commit 2ba63aa4ec480f34e429a19652b5cfeb92d01204 Author: Kornel Dulęba <[email protected]> RFC: drm/xe: Fix pinned list UAF panic This commit attempts to fix a list corruption panic observed when a buffer object is destroyed while still pinned. It modifies xe_ttm_bo_destroy() to check and remove the buffer object from the pinned list before it is freed. > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c > index 6921b6967330d..98c7c17cc46cc 100644 > --- a/drivers/gpu/drm/xe/xe_bo.c > +++ b/drivers/gpu/drm/xe/xe_bo.c > @@ -1885,6 +1885,11 @@ static void xe_ttm_bo_destroy(struct ttm_buffer_object > *ttm_bo) > if (bo->parent_obj) > xe_bo_put(bo->parent_obj); > > + spin_lock(&xe->pinned.lock); > + if (WARN_ON_ONCE(!list_empty(&bo->pinned_link))) > + list_del_init(&bo->pinned_link); > + spin_unlock(&xe->pinned.lock); [Severity: High] Does this expose a use-after-free race with xe_bo_apply_to_pinned()? When ttm_bo_release() drops the object's kref to 0, the object remains on the pinned list until xe_ttm_bo_destroy() acquires the xe->pinned.lock and removes it here. Concurrently, xe_bo_apply_to_pinned() can iterate over the pinned list, find the object, and unconditionally call xe_bo_get(bo). Since the refcount is already 0, xe_bo_get() will call kref_get() on a 0-refcount object, which triggers a refcount underflow warning and leads to a use-after-free panic. Should xe_bo_apply_to_pinned() use xe_bo_get_unless_zero() to safely skip objects undergoing destruction? [Severity: High] Does removing the buffer object from the pinned list here bypass shrinker accounting? When a buffer object is pinned via xe_bo_pin_external(), it calls xe_ttm_tt_account_subtract() to remove its pages from the shrinker's shrinkable_pages pool. By handling the cleanup directly in xe_ttm_bo_destroy() instead of the normal xe_bo_unpin_external() path, xe_ttm_tt_account_add() is never called to return the pages to the shrinker. Furthermore, for native buffer objects, the teardown path in xe_ttm_tt_unpopulate() will still execute and call xe_ttm_tt_account_subtract() again. Because the addition was missed and a double-subtraction occurs, this creates a permanent negative delta for shrinkable_pages. Due to unsigned integer math, this underflow converts into a massive positive value, potentially forcing the kernel into an infinite memory reclaim loop. [Severity: High] Can this spin_lock() cause a kernel panic if reached from the ___xe_bo_create() error path? In ___xe_bo_create(), if __xe_bo_placement_for_flags() fails, the error path directly calls xe_ttm_bo_destroy(&bo->ttm). At this point, ttm_bo_init_reserved() has not yet been called, meaning bo->ttm.bdev is NULL. At the top of xe_ttm_bo_destroy(), it computes: struct xe_device *xe = ttm_to_xe_device(ttm_bo->bdev); This yields an invalid pointer (NULL - offsetof(struct xe_device, ttm)). When the new code unconditionally attempts to lock xe->pinned.lock, it will dereference this invalid xe pointer and trigger a kernel panic. Should the error path in ___xe_bo_create() free the buffer object directly or pass the xe device context explicitly, rather than relying on the uninitialized bdev? > + > mutex_lock(&xe->mem_access.vram_userfault.lock); > if (!list_empty(&bo->vram_userfault_link)) > list_del(&bo->vram_userfault_link); -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
