Hi Geert, On Tue, May 19, 2026 at 2:55 PM Geert Uytterhoeven <[email protected]> wrote: > > Hi Prabhakar, > > On Tue, 19 May 2026 at 15:44, Lad, Prabhakar <[email protected]> > wrote: > > On Tue, May 19, 2026 at 2:34 PM Geert Uytterhoeven <[email protected]> > > wrote: > > > 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. > > Only when your compiler supports it[1]. > > OMG... > > When I commented on the LWN.net article[2], I considered only the case > where the compiler is too old, and the counter stays at zero when the > user forgets to initialize it explicitly. Now we have the opposite > case, where we need the counter to stay at zero :-( > Yeah, it definitely introduces some tricky, asymmetrical behavior depending on the toolchain.
Cheers, Prabhakar > [1] > https://elixir.bootlin.com/linux/v7.0.9/source/include/linux/compiler_types.h#L549 > [2] https://lwn.net/Articles/1063295/ > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > [email protected] > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like > that. > -- Linus Torvalds
