Re: [PATCH RESEND] malloc: fix allocation for a specific case with ASan

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 17 Oct 2024 12:02:06 +0200
Artur Paszkiewicz <artur.paszkiewicz@intel.com> 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 <artur.paszkiewicz@intel.com>
> ---

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: stable@dpdk.org`**

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: stable@dpdk.org`"

**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: stable@dpdk.org
```

If this is a latent bug that existed since the original implementation, the `Fixes:` tag may not be necessary, but `Cc: stable@dpdk.org` 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: stable@dpdk.org` (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: stable@dpdk.org` 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: stable@dpdk.org` would be ideal for ensuring the fix reaches stable branches.