Re: [PATCH v11 1/2] gpu/buddy: replace dual-tree/force_merge with decoupled dirty tracker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Information leak due to extent_alloc() failure causing dirty VRAM to be incorrectly returned as clear memory.
- [High] Deadlock during direct memory reclaim because extent_alloc() uses GFP_KERNEL while holding the allocator lock.
- [Medium] State corruption and integer underflow in total_dirty due to extent split failures in gpu_dirty_tracker_remove_range().
--

commit 1a4e9ba2186b718711f70ef5e634d528dec6b22b
Author: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>

gpu/buddy: replace dual-tree/force_merge with decoupled dirty tracker

The current buddy allocator maintains separate clear_tree[] and
dirty_tree[] rbtrees per order, preventing coalescing between cleared
and dirty buddies. Under mixed workloads, this creates a merge barrier:
adjacent buddies frequently end up split across trees, forcing reliance
on __force_merge() during allocation.

> diff --git a/drivers/gpu/buddy.c b/drivers/gpu/buddy.c
> index 4d5ac375a538c..bfa6ef85f1186 100644
> --- a/drivers/gpu/buddy.c
> +++ b/drivers/gpu/buddy.c

[ ... ]

> +static struct gpu_dirty_extent *extent_alloc(struct gpu_dirty_tracker *dirty_tracker)
> +{
> +	return kmem_cache_alloc(slab_extents, GFP_KERNEL);
> +}

[Severity: High]
Does this code trigger a deadlock during direct memory reclaim?

This allocation is called from paths like gpu_buddy_free_list() where the
allocator's lock is explicitly held:

void gpu_buddy_free_list(struct gpu_buddy *mm, ...)
{
	...
	gpu_buddy_driver_lock_held(mm);
	__gpu_buddy_free_list(mm, objects, mark_clear, !mark_clear);
}

Since GFP_KERNEL allows blocking and invokes direct memory reclaim, the
shrinker (such as TTM's global shrinker) may attempt to swap out VRAM and
call back into the allocator to free blocks. This would attempt to acquire
the exact same driver allocator lock that is already held, leading to a
self-deadlock.

Should this use a non-reclaim flag like GFP_NOWAIT or be moved to a mempool
as hinted in the changelog (v6) to guarantee nodes without __GFP_NOFAIL?

[ ... ]

> +static void gpu_dirty_tracker_mark_dirty(struct gpu_dirty_tracker *dirty_tracker,
> +					 u64 start, u64 size)
> +{
> +	struct gpu_dirty_extent *left, *right, *dirty_extent;
> +	u64 end = start + size;
> +
> +	gpu_buddy_assert(size);

[ ... ]

> +	} else {
> +		/* Standalone extent. */
> +		dirty_extent = extent_alloc(dirty_tracker);
> +		if (!dirty_extent) {
> +			pr_warn_once("dirty extent allocation failed, skipping tracker update\n");
> +			return;
> +		}
> +		dirty_extent->start = start;
> +		dirty_extent->end   = end;
> +		insert_extent(dirty_tracker, dirty_extent);
> +	}
> +
> +	dirty_tracker->total_dirty += size;
> +}

[Severity: High]
Can this early return on allocation failure lead to an information leak by
improperly classifying dirty memory as clear?

If extent_alloc() fails in gpu_dirty_tracker_mark_dirty(), the function
logs a warning and returns early without recording the dirty extent. When
this block is later queried in gpu_dirty_range_state(), the function will
find no overlapping dirty extent and default to GPU_BLOCK_CLEAR:

static enum gpu_block_state
gpu_dirty_range_state(struct gpu_dirty_tracker *dirty_tracker, ...)
{
	...
	return GPU_BLOCK_CLEAR;
}

Consequently, in mark_free(), the block is erroneously marked with
GPU_BUDDY_HEADER_CLEAR:

static void mark_free(struct gpu_buddy *mm,
		      struct gpu_buddy_block *block)
{
	enum gpu_block_state block_state;

	block_state = gpu_dirty_range_state(&mm->dirty,
					    gpu_buddy_block_offset(block),
					    gpu_buddy_block_size(mm, block));
	__mark_free(mm, block, block_state);
}

A subsequent allocation requesting GPU_BUDDY_CLEAR_ALLOCATION would be
handed this dirty block. The caller trusts the clear flag, skips the
hardware zeroing pass, and potentially exposes previous VRAM contents to a
new context. Is there a safe fallback when dirty tracking cannot be updated?

[ ... ]

> +static void gpu_dirty_tracker_remove_range(struct gpu_dirty_tracker *dirty_tracker,
> +					   u64 start, u64 size)
> +{

[ ... ]

> +		if (extent_start < start && extent_end > end) {
> +			/*
> +			 * Removed range lies strictly inside this dirty extent:
> +			 * split it into the dirty left and right halves.
> +			 */
> +			struct gpu_dirty_extent *right = extent_alloc(dirty_tracker);
> +
> +			if (!right) {
> +				pr_warn_once("dirty extent allocation failed, skipping tracker update\n");
> +				dirty_extent = next;
> +				continue;
> +			}

[Severity: Medium]
Will an allocation failure here corrupt the rbtree state and cause
total_dirty to underflow?

If extent_alloc() fails while trying to split an extent in
gpu_dirty_tracker_remove_range(), it logs a warning and skips removal.
This leaves the old extent intact, encompassing the now-allocated block
range. 

When the block is later freed, gpu_dirty_tracker_mark_dirty() searches for
contiguous neighbors. Because the old extent perfectly overlaps the freed
range rather than just adjoining it, it falls through to the standalone
extent creation branch and inserts a duplicate overlapping extent.

Furthermore, gpu_dirty_tracker_mark_dirty() then unconditionally adds to the
counter:

	dirty_tracker->total_dirty += size;

This double-counting will eventually cause total_dirty to exceed mm->avail.
Since gpu_buddy_clear_avail() returns mm->avail - mm->dirty.total_dirty,
this wraps around to a huge U64 value, completely breaking the allocator's
clear/dirty steering logic.

Should failed splits mark the entire overlapping extent as dirty instead of
skipping, or should the allocator gracefully degrade?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825172432.80355-1-Arunpravin.PaneerSelvam@amd.com?part=1