Re: [PATCH] drm/ttm: fix swapped-out resources never leaving their bulk_move range

[Samuel Ainsworth]

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
<christian.koenig@xxxxxxx> 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: stable@xxxxxxxxxxxxxxx # v7.1+
> >> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/5387
> >> Link:
> >> https://lore.kernel.org/dri-devel/CAHYiNPa6aVacJoLOje-qZ1GyYx-9p0tN4NuP8D_eSL+UJeevXw@xxxxxxxxxxxxxx/
> >> Signed-off-by: Vadim Nikitushkin <bub4z0r@xxxxxxxxx>
> >
> > 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 <thomas.hellstrom@xxxxxxxxxxxxxxx>
>
> Reviewed-by: Christian König <christian.koenig@xxxxxxx>
>
> 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);
>