Re: [PATCH] mmc: mmc_test: Fix __counted_by handling after kzalloc_flex() conversion

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Mon, May 18, 2026 at 12:08 PM Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Wed, 13 May 2026 at 22:13, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Fix logic issues introduced by the kzalloc_flex() conversion in
> > mmc_test_alloc_mem() due to interaction with the __counted_by
> > annotation on the flexible array.
> >
> > Bounds-checking sanitizers rely on the counter field reflecting the
> > allocated array size before any array access occurs. However, use
> > mem->cnt both as the allocation size and as the runtime insertion
> > index, causing incorrect indexing and potentially invalid bounds
> > tracking.
> >
> > Initialize mem->cnt to the maximum allocated number of segments
> > immediately after kzalloc_flex(), then use a separate local index
> > variable to track successfully allocated entries. Update mem->cnt to
> > the actual number of initialized elements before returning or entering
> > the cleanup path.
> >
> > Also rewrite mmc_test_free_mem() to use a forward for-loop, improving
> > readability and ensuring only initialized entries are freed.
> >
> > Fixes: c3126dccfd7b ("mmc: mmc_test: use kzalloc_flex")
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Thanks for your patch!
>
> > --- a/drivers/mmc/core/mmc_test.c
> > +++ b/drivers/mmc/core/mmc_test.c
> > @@ -316,11 +316,13 @@ static int mmc_test_buffer_transfer(struct mmc_test_card *test,
> >
> >  static void mmc_test_free_mem(struct mmc_test_mem *mem)
> >  {
> > +       unsigned int idx;
> > +
> >         if (!mem)
> >                 return;
> > -       while (mem->cnt--)
> > -               __free_pages(mem->arr[mem->cnt].page,
> > -                            mem->arr[mem->cnt].order);
> > +       for (idx = 0; idx < mem->cnt; idx++)
>
> for (unsigned int i; ...)?
>
Ok.

> > +               __free_pages(mem->arr[idx].page,
> > +                            mem->arr[idx].order);
> >         kfree(mem);
> >  }
> >
> > @@ -341,6 +343,7 @@ static struct mmc_test_mem *mmc_test_alloc_mem(unsigned long min_sz,
> >         unsigned long page_cnt = 0;
> >         unsigned long limit = nr_free_buffer_pages() >> 4;
> >         struct mmc_test_mem *mem;
> > +       unsigned int idx = 0;
> >
> >         if (max_page_cnt > limit)
> >                 max_page_cnt = limit;
> > @@ -356,6 +359,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 = max_segs;
> >
> >         while (max_page_cnt) {
> >                 struct page *page;
> > @@ -375,23 +379,26 @@ static struct mmc_test_mem *mmc_test_alloc_mem(unsigned long min_sz,
> >                                 goto out_free;
> >                         break;
> >                 }
> > -               mem->arr[mem->cnt].page = page;
> > -               mem->arr[mem->cnt].order = order;
> > -               mem->cnt += 1;
> > +               mem->arr[idx].page = page;
> > +               mem->arr[idx].order = order;
> > +               idx += 1;
>
> While looking rather ugly, I think starting with mem->cnt at zero,
> and updating it in each step like
>
>     mem->cnt++;
>     mem->arr[mem->cnt - 1].page = page;
>     mem->arr[mem->cnt - 1].order = order;
>
> would still be better, as it makes the dependency between mem->cnt and
> the size of mem->arr[] clearer (located closer to each other), and ...
>
>
Ok, I will start with mem->cnt at zero; with this I can drop changes
in mmc_test_free_mem().

Cheers,
Prabhakar

> >                 if (max_page_cnt <= (1UL << order))
> >                         break;
> >                 max_page_cnt -= 1UL << order;
> >                 page_cnt += 1UL << order;
> > -               if (mem->cnt >= max_segs) {
> > +               if (idx >= mem->cnt) {
> >                         if (page_cnt < min_page_cnt)
> >                                 goto out_free;
> >                         break;
> >                 }
> >         }
> >
> > +       mem->cnt = idx;
> > +
> >         return mem;
> >
> >  out_free:
> > +       mem->cnt = idx;
>
> ... as having to set mem->cnt twice looks rather fragile to me.
>
> >         mmc_test_free_mem(mem);
> >         return NULL;
> >  }
>
> Regardless, as the patch looks correct to me:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> 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