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

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Tue, May 19, 2026 at 2:34 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> 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.

Cheers,
Prabhakar