Re: [PATCH v2] mmc: mmc_test: Fix counter tracking in mmc_test_alloc_mem()

[Lad, Prabhakar]

Hi Gustavo,

On Wed, May 20, 2026 at 8:41 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Gustavo,
>
> On Tue, 19 May 2026 at 20:44, Gustavo A. R. Silva
> <gustavo@xxxxxxxxxxxxxx> wrote:
> > On 5/19/26 07:44, Lad, Prabhakar wrote:
> > > On Tue, May 19, 2026 at 2:34 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > >> On Tue, 19 May 2026 at 15:30, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > >>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > >>>
> > >>> 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 <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > >>> ---
> > >>> 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/20260513201315.3186621-1-prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx/


Cheers,
Prabhakar