Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/xe: missing kernel unmap when unplug eviction purges BO via ttm_bo_pipeline_gutting() - [High] drm/xe: xe_bo_shrink() unmaps BO before waiting on reservation fences or early-exit checks --
commit 5f1d487383f2c8c07f8f671e579bb556db6a0d58 Author: Arvind Yadav <[email protected]> drm/xe: Tear down kernel mappings in release_notify This commit moves the final kernel BO unmap from xe_gem_object_free() to xe_ttm_bo_release_notify() so that TTM eviction cannot race with __xe_bo_vunmap(). It also calls xe_bo_vunmap() in xe_bo_shrink() under the BO reservation lock before shrinking. > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c > index 42a6aa1a2e4a0..0793ed59a90a5 100644 > --- a/drivers/gpu/drm/xe/xe_bo.c > +++ b/drivers/gpu/drm/xe/xe_bo.c > @@ -1454,6 +1454,10 @@ long xe_bo_shrink(struct ttm_operation_ctx *ctx, > struct ttm_buffer_object *bo, > if (!xe_bo_eviction_valuable(bo, &place)) > return -EBUSY; > > + /* Shrinking can free pages without calling xe_bo_move_notify(). */ > + if (xe_bo_is_xe_bo(bo)) > + xe_bo_vunmap(xe_bo); [Severity: High] Does calling xe_bo_vunmap() here tear down bo->vmap before waiting on the BO's reservation fences and before the early-exit checks in xe_bo_shrink()? During xe_pt_update_ops_run(), if a VM_BIND batch has an operation that stages a page-table update on a page-table BO (for example clearing PTEs in op 0) and a subsequent operation in the same batch frees that page table (op 1), xe_pt_commit() -> xe_pt_destroy() unpins pt->bo and drops its GEM refcount to zero via xe_bo_put_deferred() while deferring the final put on pt_job_ops->deferred until the asynchronous CPU bind job finishes: drivers/gpu/drm/xe/xe_pt.c:xe_pt_destroy() { ... xe_bo_unpin(pt->bo); xe_bo_put_deferred(pt->bo, deferred, &added); ... } While that asynchronous job is still waiting to run, kswapd can run the second pass of xe_shrinker_walk() with ctx->no_wait_gpu set to false. In that pass, ttm_bo_shrink_suitable() allows BOs with unsignaled fences into xe_bo_shrink(), relying on ttm_bo_wait_ctx() inside xe_bo_shrink_purge() or ttm_bo_shrink() to wait for the reservation fences before reclaiming the BO. Because xe_bo_vunmap() now runs before xe_bo_shrink_purge() calls ttm_bo_wait_ctx(), xe_bo_shrink() clears pt_bo->vmap while the CPU bind job is still in flight and then blocks in ttm_bo_wait_ctx() waiting for that job. When run_pt_job() executes xe_cpu_bind_update_pgtables_execute(), update->pt_bo->vmap is already cleared, hitting the assertion and dereferencing a NULL mapping pointer in ops->clear(): drivers/gpu/drm/xe/xe_cpu_bind.c:xe_cpu_bind_update_pgtables_execute() { ... xe_assert(vm->xe, !iosys_map_is_null(&update->pt_bo->vmap)); if (pt_op->bind && !force_clear) ops->populate(tile, &update->pt_bo->vmap, update); else ops->clear(vm, tile, &update->pt_bo->vmap, update); ... } In addition, for live unpinned BOs where xe_bo_get_unless_zero() succeeds, calling xe_bo_vunmap() at the top of xe_bo_shrink() unmaps bo->vmap even when xe_bo_shrink() does not shrink the BO (such as when xe_pm_runtime_get_if_active() fails and jumps to out_unref, or when ttm_bo_shrink() returns -EBUSY during the non-writeback pass). > + > if (!xe_bo_is_xe_bo(bo) || !xe_bo_get_unless_zero(xe_bo)) > return xe_bo_shrink_purge(ctx, bo, scanned); > > @@ -1866,6 +1870,12 @@ static void xe_ttm_bo_release_notify(struct > ttm_buffer_object *ttm_bo) > bo = ttm_to_xe_bo(ttm_bo); > xe_assert(xe_bo_device(bo), !(bo->created && > kref_read(&ttm_bo->base.refcount))); > > + /* > + * The TTM BO refcount is zero, so eviction and swapout cannot > + * take a reference. Unmap before TTM frees the backing store. > + */ > + __xe_bo_vunmap(bo); [Severity: High] Can an unplug eviction purge the BO's backing pages and resource before xe_ttm_bo_release_notify() runs __xe_bo_vunmap()? When a kernel-mapped BO is unpinned during teardown (for example via xe_bo_unpin_map_no_vm() or xe_pt_destroy()) and the device is unplugged (such as in xe_device_remove() -> drm_dev_unplug() -> xe_bo_pci_dev_remove_all() -> ttm_resource_manager_evict_all()), TTM can evict the BO while holding a ttm_buffer_object kref before the final TTM BO reference is dropped. During ttm_bo_evict(), xe_evict_flags() selects purge_placement when the device is unplugged: drivers/gpu/drm/xe/xe_bo.c:xe_evict_flags() { ... if (device_unplugged && !tbo->base.dma_buf) { *placement = purge_placement; return; } ... } Because purge_placement has num_placement == 0, ttm_bo_evict() bypasses ttm_bo_handle_move_mem(), xe_bo_move(), and xe_bo_move_notify(), and instead calls ttm_bo_pipeline_gutting() directly: drivers/gpu/drm/ttm/ttm_bo.c:ttm_bo_evict() { ... if (!placement.num_placement) { ret = ttm_bo_wait_ctx(bo, ctx); if (ret) return ret; /* * Since we've already synced, this frees backing store * immediately. */ return ttm_bo_pipeline_gutting(bo); } ... } Since ttm_bo_wait_ctx() has just completed, ttm_bo_pipeline_gutting() immediately calls ttm_tt_unpopulate() and ttm_resource_free() while bo->vmap and bo->kmap are still active. When the last TTM BO reference is then dropped and xe_ttm_bo_release_notify() calls __xe_bo_vunmap() -> ttm_bo_kunmap(), a single-page cached BO (ttm_bo_map_kmap) will call kunmap(map->page) on a struct page already freed by ttm_tt_unpopulate(), and multi-page vmap/iomap BOs retain a dangling mapping to freed pages or IO memory until release_notify runs. Should xe_evict_flags() also call xe_bo_vunmap() when selecting purge_placement for an Xe BO? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
