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 <[email protected]> wrote:
Hi Prabhakar,
On Tue, 19 May 2026 at 15:30, Prabhakar <[email protected]> wrote:
From: Lad Prabhakar <[email protected]>
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 <[email protected]>
---
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.)
