On Thu, 17 Oct 2024 12:02:06 +0200 Artur Paszkiewicz <[email protected]> wrote:
> Allocation would fail with ASan enabled if the size and alignment was > equal to half of the page size, e.g.: > > size_t pg_sz = 2 * (1 << 20); > rte_malloc(NULL, pg_sz / 2, pg_sz / 2); > > In such case, try_expand_heap_primary() only allocated one page but it > is not enough to fit this allocation with such alignment and > MALLOC_ELEM_TRAILER_LEN > 0, as correctly checked by > malloc_elem_can_hold(). > > Signed-off-by: Artur Paszkiewicz <[email protected]> > --- Looks like this patch got stuck in limbo because of the sporadic CI test failures. AI code review likes it bu has some suggestions: ## DPDK Patch Review: malloc: fix allocation for a specific case with ASan ### Summary This is a well-crafted bug fix patch that addresses an allocation failure when ASan is enabled and the allocation size/alignment equals half the page size. The patch is concise, well-explained, and correctly structured. --- ### Errors (Must Fix) **None identified.** The patch passes all mandatory checks. --- ### Warnings (Should Fix) **1. Consider Adding `Cc: [email protected]`** This fixes a real allocation failure that affects ASan users. If this bug exists in stable branches, it would be a good candidate for backporting. Per guidelines: "For stable release backport candidates: `Cc: [email protected]`" **2. Consider Adding a `Fixes:` Tag** If this is a regression introduced by a specific commit (for example, when ASan support or `MALLOC_ELEM_TRAILER_LEN` was added), a `Fixes:` tag would help identify which stable branches need the fix: ``` Fixes: <12-char-sha> ("original commit that introduced the bug") Cc: [email protected] ``` If this is a latent bug that existed since the original implementation, the `Fixes:` tag may not be necessary, but `Cc: [email protected]` would still be appropriate. --- ### Info (Consider) **1. Test Coverage** Consider whether a unit test could be added to `app/test` to verify this specific allocation pattern works correctly with ASan enabled. This would prevent regression. Example test case based on the commit message: ```c /* Test allocation where size == align == pg_sz/2 */ size_t pg_sz = 2 * (1 << 20); /* 2MB */ void *ptr = rte_malloc(NULL, pg_sz / 2, pg_sz / 2); TEST_ASSERT_NOT_NULL(ptr, "Allocation failed for size=align=pg_sz/2"); rte_free(ptr); ``` **2. Code Comment** The fix changes the allocation size calculation logic. A brief inline comment explaining the worst-case alignment scenario could help future maintainers: ```c /* Account for worst-case alignment: header may need to be padded up to 'align', * plus the element size, plus trailer (non-zero when ASan is enabled). */ alloc_sz = RTE_ALIGN_CEIL(RTE_MAX(MALLOC_ELEM_HEADER_LEN, align) + elt_size + MALLOC_ELEM_TRAILER_LEN, pg_sz); ``` --- ### Checklist Summary | Category | Status | |----------|--------| | Subject line ≤60 chars | ✓ (50 chars) | | Correct component prefix (`malloc:`) | ✓ | | Lowercase after colon | ✓ | | Imperative mood, no trailing period | ✓ | | Body wrapped at 75 chars | ✓ | | Body does not start with "It" | ✓ | | Signed-off-by present | ✓ | | `Fixes:` tag (if applicable) | ? (see warning) | | `Cc: [email protected]` (if applicable) | Missing | | Code changes are minimal and focused | ✓ | | Changes are consistent across both files | ✓ | --- ### Technical Assessment The fix looks correct. The original calculation: ```c RTE_ALIGN_CEIL(elt_size, align) + MALLOC_ELEM_OVERHEAD ``` ...doesn't properly account for the case where alignment padding before the user data could push the trailer beyond the allocated page boundary. The new calculation: ```c RTE_MAX(MALLOC_ELEM_HEADER_LEN, align) + elt_size + MALLOC_ELEM_TRAILER_LEN ``` ...correctly computes the worst-case space needed: the header (or alignment, whichever is larger) plus the actual element size plus the trailer. --- ### Verdict **Ready to merge** with minor suggestions. This is a clean, focused bug fix. The only recommendations are: 1. Add `Cc: [email protected]` if this should be backported 2. Optionally add a `Fixes:` tag if the originating commit can be identified The patch could be accepted as-is, but a v2 with `Cc: [email protected]` would be ideal for ensuring the fix reaches stable branches.
