|WARNING| [6/6] app/test: add test for IP reassembly

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/165536

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-16

# DPDK Patch Review

## PATCH 1/6: ip_frag: tolerate duplicate fragments

### Errors

None.

### Warnings

None.

### Info

The duplicate detection logic is correct. The loop scans existing fragments, compares offset and length for exact matches, and drops only the duplicate mbuf via `IP_FRAG_MBUF2DR()` without modifying the entry's accounting. This matches the RFC 791 requirement to tolerate duplicates.

---

## PATCH 2/6: ip_frag: discard datagrams with overlapping fragments

### Errors

1. **Resource leak on overlap detection**: When an overlap is detected, the code calls `ip_frag_free(fp, dr)` to free all collected fragments, then `ip_frag_key_invalidate(&fp->key)` to invalidate the entry, then `IP_FRAG_MBUF2DR(dr, mb)` to drop the incoming fragment. However, the entry remains in the table (occupies a slot) until it is reaped by timeout or collision. The `ip_frag_key_invalidate()` call zeroes the key but does not decrement `tbl->use_entries`, so the slot is leaked until table destroy or explicit reap. The overlap path should either call a cleanup function that decrements `use_entries`, or the invalidate function should be fixed to do so. As written, repeated overlaps can exhaust the table.

### Warnings

None.

### Info

The overlap detection geometry is correct: `ofs < existing.ofs + existing.len && existing.ofs < ofs + len` detects any byte-range overlap. The decision to discard the entire datagram on overlap matches Linux and RFC 8200/5722.

---

## PATCH 3/6: ip_frag: include protocol in IPv4 reassembly key

### Errors

None.

### Warnings

None.

### Info

The fix is correct. Packing `next_proto_id` into the upper 16 bits of the 32-bit `key.id` field (which previously held only the 16-bit `packet_id`) makes the key comply with RFC 791 section 3.2 without changing the key layout or hash. The shift-and-OR preserves the low 16 bits for the packet ID.

---

## PATCH 4/6: ip_frag: drop IPv6 fragments with unexpected headers

### Errors

None.

### Warnings

None.

### Info

The check is appropriate. The code only handles a fragment header immediately following the IPv6 header; anything else (Hop-by-Hop, Routing, Destination Options, etc.) in the unfragmentable part causes `ipv6_frag_reassemble()` to patch the wrong offset and corrupt the result. Dropping such fragments is permitted by RFC 8200 and recommended by RFC 9099.

---

## PATCH 5/6: ip_frag: reject oversized reassembled datagrams

### Errors

None.

### Warnings

None.

### Info

The length checks are correct. For IPv4, `ip_ofs + ip_len + mb->l3_len > UINT16_MAX` ensures the reassembled datagram (including the IPv4 header in `l3_len`) does not exceed 65535 bytes, preventing silent truncation of `total_length`. For IPv6, `ip_ofs + ip_len > UINT16_MAX` checks the payload alone (the reassembled datagram has no fragment header, so `payload_len` is capped at 65535).

---

## PATCH 6/6: app/test: add test for IP reassembly

### Errors

1. **Test uses `NOHUGE_SKIP` but should use `NOHUGE_OK`**: The test calls `rte_pktmbuf_pool_create()` with `SOCKET_ID_ANY`, which allocates from hugepages if available but can fall back to standard memory. However, `REGISTER_FAST_TEST` is called with `NOHUGE_SKIP`, meaning the test is skipped when hugepages are unavailable. The comment at the top of the file says `l2_len == 0` (no special alignment), and the test does not require hugepage-backed memory (it's not doing DMA). Change `NOHUGE_SKIP` to `NOHUGE_OK` so the test runs in CI environments without hugepages.

### Warnings

1. **Missing release notes entry**: This patch adds new functional tests to `app/test`, which are user-visible (developers run `meson test` to validate their changes). While test-only changes generally do not require release notes, a new test suite that validates previously-untested library behavior (IP reassembly correctness) should be mentioned in the release notes under "New Tests" or similar. The patch should update `doc/guides/rel_notes/release_25_03.rst` (or the appropriate current release file).

### Info

The test coverage is comprehensive: sweep of sizes/orders, minimum fragments, MAX_FRAG boundary, timeout, zero-length, duplicate, overlap, extension-header rejection, and oversize rejection. The use of `rte_mbuf_check()` to validate segment chains is good practice. The `ut_setup()` mempool full check will pinpoint leaks to the failing case.

---

## Summary

**Critical issue (PATCH 2)**: The overlap-discard path leaks table entries (does not decrement `use_entries` after invalidating the key). This will exhaust the table under repeated overlaps.

**Test issue (PATCH 6)**: `REGISTER_FAST_TEST` should use `NOHUGE_OK` instead of `NOHUGE_SKIP` to allow the test to run without hugepages.

**Missing documentation (PATCH 6)**: New test suite should be noted in the release notes.

All other patches are correct.