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
