I left a comment on the commit in GitLab (https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/3db7d7d583419f7b1f2e141e36418802dbb25cf8#note_3658149) but it occurred to me that it may be closer to convention to reply here. In short, AFAIU the commit description describes fixing this by checking `ttm_tt_swapout()` inside `ttm_bo_swapout_cb()` but the actual diff is modifying `ttm_resource_try_charge()` checking inside `ttm_bo_alloc_at_place()`.
Perhaps I'm missing something? Sam On Thu, Sep 10, 2026 at 3:46 AM Christian König <[email protected]> wrote: > > On 9/10/26 09:14, Thomas Hellström wrote: > > On Wed, 2026-09-09 at 23:50 +0300, Vadim Nikitushkin wrote: > >> ttm_tt_swapout() returns the number of pages swapped out on success > >> and > >> a negative error code on failure; for a populated ttm it never > >> returns > >> zero. Commit b2ed01e7ad3d ("drm/ttm: Fix ttm_bo_swapout() infinite > >> LRU > >> walk on swapout failure") moved the bulk_move bookkeeping in > >> ttm_bo_swapout_cb() under "if (!ret)", so the > >> ttm_resource_del_bulk_move_unevictable() / > >> ttm_resource_move_to_lru_tail() > >> pair is now skipped on every successful swapout. The equivalent > >> change > >> for the shrinker in commit 1d59f36e95f7 ("drm/ttm: Fix > >> ttm_bo_shrink() > >> infinite LRU walk on backup failure") tests "lret > 0", which is what > >> was intended here as well. > >> > >> Before b2ed01e7ad3d the resource was taken off the bulk_move before > >> the > >> swapout; since then a swapped-out resource stays inside its BO's > >> bulk_move range (and on the manager LRU) although it is unevictable. > >> When it is later freed or the BO leaves the bulk_move > >> (ttm_resource_free(), ttm_bo_set_bulk_move() via amdgpu_vm_bo_del()), > >> ttm_resource_del_bulk_move() skips it because of its > >> !ttm_resource_unevictable() guard, so a range endpoint in pos->first > >> / > >> pos->last is left pointing at freed memory. The next > >> ttm_lru_bulk_move_tail() or ttm_resource_add_bulk_move() on that > >> cursor > >> is a use-after-free, seen as the resv WARN in > >> ttm_lru_bulk_move_add(), > >> "list_del corruption" in ttm_resource_move_to_lru_tail() or a NULL > >> dereference in ttm_resource_manager_next() -- minutes to hours after > >> a > >> hibernation, or at process exit / reboot following one. Samuel > >> Ainsworth's analysis of drm/amd issue 5387 (see Link) identified the > >> dangling cursor; the missing removal at swapout time is the reason it > >> dangles. > >> > >> Testing the condition for success restores the removal. On an AMD > >> Phoenix APU (ASUS UM3406GA, gfx1103) running suspend-then-hibernate > >> on > >> a 7.0.y stable kernel carrying the backport (Ubuntu 7.0.0-31) the bug > >> crashed 5 of 18 hibernation cycles; a function profile of one > >> hibernation showed 336 ttm_tt_swapout() calls and zero > >> ttm_resource_del_bulk_move_unevictable() calls. With this change the > >> removal happens for every swapped-out resource and 12 further cycles > >> were clean. > >> > >> Fixes: b2ed01e7ad3d ("drm/ttm: Fix ttm_bo_swapout() infinite LRU walk > >> on swapout failure") > >> Cc: [email protected] # v7.1+ > >> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/5387 > >> Link: > >> https://lore.kernel.org/dri-devel/cahyinpa6avacjoloje-qz1gyyx-9p0tn4nup8d_esl+ujee...@mail.gmail.com/ > >> Signed-off-by: Vadim Nikitushkin <[email protected]> > > > > Nice catch. > > Agreed, that is a really good one. We had tons of people staring at the code > without seeing that. > > > > > This also explains why https://patchwork.freedesktop.org/series/170311/ > > appeared to fix the issue. But that series actually kept the resource > > on the bulk sublist until someone bumped the LRU or removed it. > > > > Reviewed-by: Thomas Hellström <[email protected]> > > Reviewed-by: Christian König <[email protected]> > > If nobody comes up with some last second objections I'm going to push that to > drm-misc-fixes ASAP. > > Thanks, > Christian. > > > > >> --- > >> drivers/gpu/drm/ttm/ttm_bo.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c > >> b/drivers/gpu/drm/ttm/ttm_bo.c > >> index ef56c18..9b85b5f 100644 > >> --- a/drivers/gpu/drm/ttm/ttm_bo.c > >> +++ b/drivers/gpu/drm/ttm/ttm_bo.c > >> @@ -1434,7 +1434,7 @@ ttm_bo_swapout_cb(struct ttm_lru_walk *walk, > >> struct ttm_buffer_object *bo) > >> > >> if (ttm_tt_is_populated(tt)) { > >> ret = ttm_tt_swapout(bdev, tt, swapout_walk- > >>> gfp_flags); > >> - if (!ret) { > >> + if (ret > 0) { > >> spin_lock(&bdev->lru_lock); > >> ttm_resource_del_bulk_move_unevictable(bo- > >>> resource, bo); > >> ttm_resource_move_to_lru_tail(bo->resource); >
