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

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

On 5/19/26 07:44, Lad, Prabhakar wrote:
> Hi Geert,
> 
> Thank you for the review.
> 
> On Tue, May 19, 2026 at 2:34 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>
>> Hi Prabhakar,
>>
>> On Tue, 19 May 2026 at 15:30, Prabhakar <prabhakar.csengg@gmail.com> wrote:
>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>>
>>> 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@bp.renesas.com>
>>> ---
>>> 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].

I think it'll eventually become best practice to defer updating the
counter until after the flexible array has been fully initialized, or
after every major update that requires changing its boundaries.

-Gustavo

[1] https://git.kernel.org/linus/ea9e148c803b24eb
[2] https://embeddedor.com/blog/2024/06/18/how-to-use-the-new-counted_by-attribute-in-c-and-linux/
(I'm in the process of updating this blogpost with some *alloc_flex() examples and more.)