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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Failing to allocate a dirty extent causes dirty memory to be falsely classified as clear, leaking uninitialized data to userspace.
- [High] Failing to allocate a dirty extent during `remove_range` corrupts the interval tree with overlapping extents and underflows `clear_avail`.
- [High] Using `GFP_KERNEL` allocation on the free path under the allocator lock creates a direct reclaim lock recursion deadlock risk.
--

--- Patch [1]: [PATCH v10 1/2] gpu/buddy: replace dual-tree/force_merge with decoupled dirty tracker ---
commit 81520a8dbad12cdd20c2cfc232d134998aef3e50
Author: Arunpravin Paneer Selvam <[email protected]>

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.
This replaces the dual-tree design with a single free_tree and a lightweight
out-of-band dirty tracker interval rbtree, removing the force_merge step and
allowing unconditional coalescing.

> diff --git a/drivers/gpu/buddy.c b/drivers/gpu/buddy.c
> --- a/drivers/gpu/buddy.c
> +++ b/drivers/gpu/buddy.c

[ ... ]

> @@ -118,6 +118,11 @@ RB_DECLARE_CALLBACKS_MAX(static, gpu_dirty_augment_cb,
> +static struct gpu_dirty_extent *extent_alloc(struct gpu_dirty_tracker *dirty_tracker)
> +{
> +	return kmem_cache_alloc(slab_extents, GFP_KERNEL);
> +}

[Severity: High]
Could the use of GFP_KERNEL here lead to a lock recursion deadlock?

When called from the free path in gpu_buddy_free_block() via
gpu_dirty_tracker_mark_dirty(), the driver's buddy allocator lock is
held. Since GFP_KERNEL permits direct memory reclaim, memory pressure
could cause the kernel to invoke DRM shrinkers. If a shrinker attempts
to free VRAM objects, it would try to acquire the exact same allocator
lock.

Would it be safer to use GFP_NOWAIT or a pre-allocated mempool for these
tracker allocations?

[ ... ]

> @@ -240,6 +240,49 @@ static void gpu_dirty_tracker_fini(struct gpu_dirty_tracker *dirty_tracker)
> +static void gpu_dirty_tracker_mark_dirty(struct gpu_dirty_tracker *dirty_tracker,
> +					 u64 start, u64 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;

[Severity: High]
Does skipping the tracker update on allocation failure risk leaking
uninitialized memory to userspace?

If extent_alloc() fails under memory pressure and we return early, the
newly dirty memory becomes completely untracked. When this block is
eventually freed or split, gpu_dirty_range_state() will incorrectly
return GPU_BLOCK_CLEAR because it cannot find the extent.

This causes __mark_free() to set the GPU_BUDDY_HEADER_CLEAR flag. If a
user process subsequently requests a cleared allocation, the driver will
trust this flag, skip the hardware zeroing pass, and hand out dirty
memory.

[ ... ]

> @@ -294,6 +294,76 @@ static void gpu_dirty_tracker_mark_dirty(struct gpu_dirty_tracker *dirty_tracker,
> +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;
> +			}
> +
> +			remove_extent(dirty_tracker, dirty_extent);

[Severity: High]
Can a failure to allocate the right-half extent here corrupt the
interval tree and underflow the available clear memory counter?

If extent_alloc() fails, the code skips splitting the extent and leaves
the entire encompassing old extent in the tree, bypassing the
total_dirty size subtraction.

Later, when the newly allocated block is freed,
gpu_dirty_tracker_mark_dirty() will attempt to add it back. Because the
block falls strictly inside the stale extent rather than abutting it, it
fails to merge and inserts an overlapping extent. This breaks the
non-overlapping invariant of the tree and double-counts the size in
total_dirty, which can eventually exceed the total available memory and
cause an unsigned underflow in gpu_buddy_clear_avail().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1