drivers/mmc/core/mmc_test.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
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@bp.renesas.com>
---
drivers/mmc/core/mmc_test.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/core/mmc_test.c b/drivers/mmc/core/mmc_test.c
index ab38e4c45a8d..e0e1b5df76dc 100644
--- 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++)
+ __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;
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;
mmc_test_free_mem(mem);
return NULL;
}
--
2.54.0Hi Prabhakar,
On Wed, 13 May 2026 at 22:13, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> 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@bp.renesas.com>
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; ...)?
> + __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 ...
> 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@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
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
Hi Geert,
Thank you for the review.
On Mon, May 18, 2026 at 12:08 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Wed, 13 May 2026 at 22:13, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > 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@bp.renesas.com>
>
> 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@glider.be>
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> 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
Hi Prabhakar,
On Tue, 19 May 2026 at 12:05, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> On Mon, May 18, 2026 at 12:08 PM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Wed, 13 May 2026 at 22:13, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > 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@bp.renesas.com>
> > > --- 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().
I don't think you can drop these changes, as mmc_test_free_mem()
does mem->cnt-- _before_ accessing mem->arr[mem->cnt].
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
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
Hi Geert,
On Tue, May 19, 2026 at 11:09 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, 19 May 2026 at 12:05, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > On Mon, May 18, 2026 at 12:08 PM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > On Wed, 13 May 2026 at 22:13, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > 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@bp.renesas.com>
>
> > > > --- 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().
>
> I don't think you can drop these changes, as mmc_test_free_mem()
> does mem->cnt-- _before_ accessing mem->arr[mem->cnt].
>
Ack.
Cheers,
Prabhakar
© 2016 - 2026 Red Hat, Inc.