Hi Gustavo, On Wed, May 20, 2026 at 8:41 AM Geert Uytterhoeven <[email protected]> wrote: > > 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? > Do you want me to send a v3 based on v1 [0] dropping "mem->cnt = max_segs;"?
[0] https://lore.kernel.org/all/[email protected]/ Cheers, Prabhakar
