Hi Geert, Thank you for the review.
On Tue, May 19, 2026 at 2:34 PM Geert Uytterhoeven <[email protected]> wrote: > > Hi Prabhakar, > > On Tue, 19 May 2026 at 15:30, Prabhakar <[email protected]> wrote: > > From: Lad Prabhakar <[email protected]> > > > > Fix an counter tracking in mmc_test_alloc_mem() that causes a kernel panic > > during error unwinding. > > > > The `struct mmc_test_mem` uses the `__counted_by(cnt)` annotation on its > > flexible array member `arr`. While kzalloc_flex() initially sets the > > counter field (`cnt`) to `max_segs`, the allocation loop needs to track > > how many elements have actually been populated. > > > > Previously, leaving `mem->cnt` at `max_segs` meant that if the loop failed > > midway (e.g., "Failed to map sg list"), the error unwinding path in > > mmc_test_free_mem() would attempt to clean up uninitialized trailing > > array slots. This resulted in passing NULL pointers to __free_pages(), > > triggering a kernel panic: > > > > [ 66.172845] mmc0: Failed to map sg list > > [ 66.176722] Unable to handle kernel NULL pointer dereference at > > virtual address 0000000000000000 > > ... > > [ 66.432747] Call trace: > > [ 66.435191] ___free_pages+0x1c/0xc4 (P) > > [ 66.439119] __free_pages+0x14/0x20 > > [ 66.442608] mmc_test_area_cleanup+0x58/0x84 [mmc_test] > > > > Fix this by explicitly resetting `mem->cnt` to 0 immediately after > > allocation. Then, move the existing `mem->cnt` increment so that it occurs > > prior to populating each array slot, using `mem->cnt - 1` for the actual > > assignment index. This guarantees that the counter accurately tracks > > initialized entries for safe error cleanup, while dynamically expanding > > the `__counted_by` validation boundary ahead of each flexible array write. > > > > Additionally, rewrite the cleanup loop in mmc_test_free_mem() to use a > > standard forward for-loop. This addresses the unsafe post-decrement logic > > in the original `while (mem->cnt--)` loop which evaluated and decremented > > the counter field before indexing the array, and avoids a potential integer > > underflow/wrap-around of the counter field if the cleanup path is invoked > > when `mem->cnt` is 0. > > > > Fixes: c3126dccfd7b ("mmc: mmc_test: use kzalloc_flex") > > Signed-off-by: Lad Prabhakar <[email protected]> > > --- > > v1->v2: > > - Started with cnt = 0 and incremented before assignment to ensure > > accurate tracking of initialized entries in mmc_test_alloc_mem(). > > - In mmc_test_free_mem(), replaced the while loop with a forward for-loop to > > safely iterate over initialized entries without risking underflow. > > - Updated commit message to clarify the issue and the fix. > > Thanks for your patch! > > > --- a/drivers/mmc/core/mmc_test.c > > +++ b/drivers/mmc/core/mmc_test.c > > @@ -318,9 +318,8 @@ static void mmc_test_free_mem(struct mmc_test_mem *mem) > > { > > if (!mem) > > return; > > - while (mem->cnt--) > > - __free_pages(mem->arr[mem->cnt].page, > > - mem->arr[mem->cnt].order); > > + for (unsigned int i = 0; i < mem->cnt; i++) > > + __free_pages(mem->arr[i].page, mem->arr[i].order); > > kfree(mem); > > } > > > > @@ -356,6 +355,7 @@ static struct mmc_test_mem *mmc_test_alloc_mem(unsigned > > long min_sz, > > mem = kzalloc_flex(*mem, arr, max_segs); > > if (!mem) > > return NULL; > > + mem->cnt = 0; > > This is not needed, as it is set to zero by kzalloc_flex(). > Actually, kzalloc_flex() automatically sets mem->cnt to max_segs because cnt is annotated with __counted_by. Because of that implicit initialization, we need this explicit reset to get it back to zero. Cheers, Prabhakar
