|WARNING| [v4] net/cksum: compute raw cksum for several segments

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch fixes checksum computation bugs in `rte_raw_cksum_mbuf()` when processing multi-segment packets. The core issues are integer truncation in byte-order swapping and carry loss during accumulation.

---

## Errors

### 1. Missing test for `rte_pktmbuf_chain()` failure path resource leak
**File:** `app/test/test_cksum.c:288`

In `test_l4_cksum_multi_mbufs()`, when `rte_pktmbuf_chain()` fails in the loop, the already-allocated mbufs in `m[]` are leaked because the cleanup path `rte_pktmbuf_free_bulk(m, segs_len)` assumes all `segs_len` entries are valid, but entries after the failure point are still NULL.

**Current code:**
```c
if (m_hdr) {
    if (rte_pktmbuf_chain(m_hdr, m[i]))
        GOTO_FAIL("Cannot chain mbuf");
}
```

**Issue:** If chaining fails at iteration `i=2`, `m[0]` and `m[1]` are allocated but `m[2]` through `m[segs_len-1]` are NULL. `rte_pktmbuf_free_bulk(m, segs_len)` will attempt to free NULL pointers, which is safe per the API but does not free the chained mbufs already attached to `m_hdr`.

**Fix:** Free `m_hdr` (which includes all successfully chained mbufs) and track the last successfully allocated index:
```c
for (i = 0; i < segs_len; i++) {
    m[i] = rte_pktmbuf_alloc(pktmbuf_pool);
    if (m[i] == NULL) {
        if (m_hdr)
            rte_pktmbuf_free(m_hdr);
        return -1;  /* or GOTO_FAIL */
    }

    data = rte_pktmbuf_append(m[i], segs[i]);
    if (data == NULL) {
        rte_pktmbuf_free(m[i]);
        if (m_hdr)
            rte_pktmbuf_free(m_hdr);
        return -1;
    }

    memcpy(data, pktdata + off, segs[i]);
    off += segs[i];

    if (m_hdr) {
        if (rte_pktmbuf_chain(m_hdr, m[i])) {
            rte_pktmbuf_free(m[i]);  /* free the unchained mbuf */
            rte_pktmbuf_free(m_hdr); /* free the chain */
            return -1;
        }
    } else {
        m_hdr = m[i];
    }
}
```

### 2. `rte_pktmbuf_free_bulk()` used on chained mbufs
**File:** `app/test/test_cksum.c:312, 318`

After successfully chaining, all mbufs except `m[0]` (== `m_hdr`) are internal chain segments. Calling `rte_pktmbuf_free_bulk(m, segs_len)` attempts to free `m[1]`, `m[2]`, etc. as independent mbufs, but they are already part of `m_hdr`'s chain. This causes double-free corruption.

**Fix:** Only free the head mbuf:
```c
rte_pktmbuf_free(m_hdr);
return 0;

fail:
    if (m_hdr)
        rte_pktmbuf_free(m_hdr);
    return -1;
```

---

## Warnings

### 1. Missing release notes entry
**File:** (missing) `doc/guides/rel_notes/release_26_07.rst` or current release file

This patch fixes a correctness bug in the public checksum API (`rte_raw_cksum_mbuf()`). A release notes entry is required under "Fixed Issues" or "Fixes" to document that multi-segment checksum computation was producing incorrect results.

**Suggested addition:**
```rst
* **net: Fixed multi-segment checksum computation.**

  Fixed integer truncation and carry loss bugs in ``rte_raw_cksum_mbuf()``
  that caused incorrect checksums when packet data spanned multiple mbufs.
```

### 2. Test data array length could be `size_t` or `unsigned int`
**File:** `app/test/test_cksum.c:119-123`

The array `test_cksum_ipv4_tcp_multi_segs_len[]` is declared as `const uint8_t` but passed to a function expecting `size_t segs_len`. While `sizeof()` returns `size_t` which matches the parameter type, the array element type mismatch is confusing. Consider using `const unsigned int` or adding a comment explaining the choice of `uint8_t` (presumably to save space since segment counts are always small).

**Current:**
```c
static const uint8_t test_cksum_ipv4_tcp_multi_segs_len[] = {
    66, 61, 52,
};
```

**Clearer:**
```c
static const unsigned int test_cksum_ipv4_tcp_multi_segs_len[] = {
    66, 61, 52,
};
```
Or add a comment: `/* uint8_t sufficient: max 256 segments */`

---

## Info

### 1. Test packet comment could clarify "113" payload size
**File:** `app/test/test_cksum.c:88-90`

The Scapy command comment mentions `os.urandom(113)` but does not explain why 113 bytes was chosen. Adding a brief note (e.g., "chosen to produce 3 segments with the given lengths") would help maintainers understand the test design.

### 2. Consider defining `NB_MBUF` constant for array size
**File:** `app/test/test_cksum.c:267`

The array `struct rte_mbuf *m[NB_MBUF]` uses `NB_MBUF` which is defined elsewhere in the test file. If `NB_MBUF` is smaller than the maximum `segs_len` passed to this function, an out-of-bounds write could occur. Verify that `NB_MBUF >= max(segs_len)` or add a bounds check:
```c
if (segs_len > NB_MBUF)
    return -1;
```

---

## Correctness Review: The Core Bug Fix

The patch correctly identifies and fixes two bugs in `lib/net/rte_cksum.h`:

1. **Byte-order swap truncation:** Changing `rte_bswap16((uint16_t)tmp)` to `rte_bswap32(tmp)` preserves the upper 16 bits when swapping for odd-boundary alignment. The previous code cast `uint32_t` to `uint16_t` (truncating the carry bits from the prior segment) before swapping, then implicitly widened back to `uint32_t` for the `sum +=` operation, losing carry information.

2. **Carry loss on overflow:** Widening `sum` from `uint32_t` to `uint64_t` and using `__rte_raw_cksum_reduce_u64()` preserves carries across 32-bit overflow. The previous `uint32_t sum` would wrap silently when summing large segments.

Both fixes are necessary and correct. The new reduction function properly folds the 64-bit sum down to 16 bits by reducing each 32-bit half independently then combining.

---

## Final Recommendation

**Do not merge** until the resource leak and double-free issues in the test code are resolved. The checksum algorithm fix in `rte_cksum.h` is correct, but the test infrastructure has critical memory management bugs that must be addressed first.