Hi Gustavo, On Tue, 19 May 2026 at 20:44, Gustavo A. R. Silva <[email protected]> wrote: > On 5/19/26 07:44, Lad, Prabhakar 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. > > An auxiliary variable could be used to avoid having to update the > counter too early[1][2].
Thank you! In light of this, Prabhakar's v1[3] is the better solution. But perhaps that the addition of "mem->cnt = max_segs;" after the kzalloc_flex() should be dropped, as it is done automatically by compilers that support __counted_by, but is irrelevant for compilers that do not support it (the correct value is set at the end anyway). Gustavo, what do you think? > I think it'll eventually become best practice to defer updating the > counter until after the flexible array has been fully initialized, or > after every major update that requires changing its boundaries. In case the update is a _decrease_. If the update is an _increase_, it must be done first? > [1] https://git.kernel.org/linus/ea9e148c803b24eb > [2] > https://embeddedor.com/blog/2024/06/18/how-to-use-the-new-counted_by-attribute-in-c-and-linux/ > (I'm in the process of updating this blogpost with some *alloc_flex() > examples and more.) [3] "[PATCH] mmc: mmc_test: Fix __counted_by handling after kzalloc_flex() conversion" https://lore.kernel.org/[email protected] 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
