Re: [PATCH] drm/ttm: Represent LRU bulk moves as nested sublists

On Fri, 2026-08-14 at 13:46 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> - [High] Missing cursor adjustment in `ttm_lru_bulk_move_add` when
> re-seeding an empty bulk move anchor causes recently-used resources
> to be prematurely evicted.
> - [Medium] Computing `&man->lru[j]` when `man` is NULL in
> `ttm_bulk_move_drop_cursors` invokes Undefined Behavior and will
> trigger UBSAN panics.

Both issues verified as real and reachable. Will update for v3.

Thanks,
Thomas


> --
> 
> --- Patch [1]: [PATCH] drm/ttm: Represent LRU bulk moves as nested
> sublists ---
> commit b97657bee6dc1cddf7841bc260d33b4b040368e4
> Author: Thomas Hellström <[email protected]>
> Subject: drm/ttm: Represent LRU bulk moves as nested sublists
> 
> This patch changes the LRU bulk move mechanism to track each group as
> a
> real nested sublist instead of ranges. Each group gets a persistent
> anchor
> node linked on the manager LRU list along with a sublist holding its
> member
> resources, simplifying cursor tracking and eliminating range-
> corruption bugs.
> 
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/5387
> 
> > diff --git a/drivers/gpu/drm/ttm/ttm_resource.c
> > b/drivers/gpu/drm/ttm/ttm_resource.c
> > index 4e6d3d658d858..195f960b2d738 100644
> > --- a/drivers/gpu/drm/ttm/ttm_resource.c
> > +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> 
> [ ... ]
> 
> > +static void ttm_bulk_move_drop_cursors(struct ttm_device *bdev,
> > +					struct ttm_lru_bulk_move
> > *bulk)
> > +{
> > +	unsigned int i, j;
> > +
> > +	for (i = 0; i < TTM_NUM_MEM_TYPES; ++i) {
> > +		struct ttm_resource_manager *man =
> > ttm_manager_type(bdev, i);
> > +
> > +		for (j = 0; j < TTM_MAX_BO_PRIORITY; ++j) {
> > +			struct ttm_lru_bulk_move_pos *pos = &bulk-
> > >pos[i][j];
> > +			struct ttm_lru_item *lru, *next;
> > +
> > +			list_for_each_entry_safe(lru, next, &pos-
> > >sublist, link) {
> > +				struct ttm_resource_cursor
> > *cursor;
> > +
> > +				if (ttm_lru_item_is_res(lru)) {
> > +					WARN_ON_ONCE(1);
> > +					continue;
> > +				}
> > +				/*
> > +				 * This cursor descended here; its
> > main hitch
> > +				 * already sits on the manager
> > list, so just
> > +				 * detach it from this sublist.
> > +				 */
> > +				cursor = container_of(lru,
> > typeof(*cursor),
> > +						     
> > sublist_hitch);
> > +				cursor->cur_list = &man->lru[j];
> > +				list_del_init(&lru->link);
> > +			}
> > +			list_splice_tail_init(&pos->sublist, &man-
> > >lru[j]);
> 
> [Severity: Medium]
> When ttm_manager_type() returns NULL for unused memory types, doesn't
> the
> pointer arithmetic &man->lru[j] result in undefined behavior?
> 
> If the kernel is compiled with UBSAN, this can trigger a panic even
> if the
> list operations don't actually dereference the pointer. Should there
> be a
> check like if (!man) continue; before the inner loop?
> 
> > +			list_del_init(&pos->marker.link);
> > +		}
> > +	}
> > +}
> 
> [ ... ]
> 
> > +/* Add the resource to a bulk_move sublist */
> >  static void ttm_lru_bulk_move_add(struct ttm_lru_bulk_move *bulk,
> >  				  struct ttm_resource *res)
> >  {
> >  	struct ttm_lru_bulk_move_pos *pos =
> > ttm_lru_bulk_move_pos(bulk, res);
> > +	struct ttm_resource *first =
> > ttm_lru_first_res_or_null(&pos->sublist);
> > +	struct ttm_buffer_object *bo = res->bo;
> > +	struct ttm_resource_manager *man =
> > +		ttm_manager_type(bo->bdev, res->mem_type);
> >  
> > -	if (!pos->first) {
> > -		pos->first = res;
> > -		pos->last = res;
> > +	if (first) {
> > -		WARN_ON(pos->first->bo->base.resv != res->bo-
> > >base.resv);
> > -		ttm_lru_bulk_move_pos_tail(pos, res);
> > +		WARN_ON(first->bo->base.resv != res->bo-
> > >base.resv);
> >  	} else {
> > +		/*
> > +		 * Group empty (first activation, or all members
> > were pinned
> > +		 * or swapped out); re-seed the anchor at the tail
> > so it
> > +		 * counts as recently used.
> > +		 */
> > +		list_move_tail(&pos->marker.link, &man->lru[bo-
> > >priority]);
> >  	}
> 
> [Severity: High]
> If an active LRU traversal cursor has descended into this sublist and
> all
> resources are subsequently removed, the sublist becomes empty except
> for the
> cursor's hitch.
> 
> When a new resource is added and 'first' is NULL, we re-seed the
> anchor to
> the tail of the manager's LRU list. However, could this leave the
> descended
> cursor parked in the sublist without adjusting it?
> 
> If ttm_lru_bulk_move_pos_adjust_cursors() is not called to detach it,
> the
> resumed traversal might immediately process and evict this newly
> added
> resource, violating the LRU guarantee.
> 
> > +	/*
> > +	 * The resource may still be on another list (manager LRU
> > or
> > +	 * bdev->unevictable); move it unconditionally to keep
> > group
> > +	 * membership consistent.
> > +	 */
> > +	list_move_tail(&res->lru.link, &pos->sublist);
> >  }