Re: [v3] net/cksum: compute raw cksum for several segments

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon,  4 Aug 2025 11:54:30 +0800
Su Sai <spiderdetective.ss@gmail.com> wrote:

> The rte_raw_cksum_mbuf function is used to compute
> the raw checksum of a packet.
> If the packet payload stored in multi mbuf, the function
> will goto the hard case. In hard case,
> the variable 'tmp' is a type of uint32_t,
> so rte_bswap16 will drop high 16 bit.
> Meanwhile, the variable 'sum' is a type of uint32_t,
> so 'sum += tmp' will drop the carry when overflow.
> Both drop will make cksum incorrect.
> This commit fixes the above bug.
> 
> Signed-off-by: Su Sai <spiderdetective.ss@gmail.com>
> ---

AI review found some issues.

Summary of Findings
Severity	Issue
Error	Double-free of chained mbuf segments in test_l4_cksum_multi_mbufs() — success path frees m[0] (the whole chain) then also frees m[1], m[2]
Error	Missing Fixes: tag referencing the commit that introduced rte_raw_cksum_mbuf()
Warning	Missing Cc: stable@dpdk.org for backport
Warning	Double-free in the fail: error path if some segments were already chained


---

## Deep Dive Review: `[v3] net/cksum: compute raw cksum for several segments`

### Summary

This patch fixes a real bug in `rte_raw_cksum_mbuf()` where multi-segment checksum computation loses data due to two issues: (1) `rte_bswap16()` on a `uint32_t` drops the upper 16 bits, and (2) a `uint32_t` accumulator `sum` can overflow and lose carries. The fix widens `sum` to `uint64_t`, uses `rte_bswap32()` instead of `rte_bswap16()`, and adds a new `__rte_raw_cksum_reduce_u64()` reduction function. A test case is also added.

---

### Correctness Analysis

**The core fix is correct.** The original code had two bugs in the "hard case" (multi-segment path):

1. **`rte_bswap16((uint16_t)tmp)`** — `tmp` is the result of `__rte_raw_cksum()` which returns a value in the range [0, 0x1FFFE] (a folded-once 32-bit sum). Casting to `uint16_t` before byte-swapping silently drops the carry bit. The fix to use `rte_bswap32(tmp)` preserves all bits.

2. **`uint32_t sum` overflow** — After the `bswap32`, `tmp` can be up to 0xFFFF0000. Accumulating multiple such values in a `uint32_t` can overflow and lose carries. Widening `sum` to `uint64_t` prevents this.

**`__rte_raw_cksum_reduce_u64()` correctness check:** The function folds a 64-bit sum into 16 bits by first reducing the low 32 bits, then reducing the high 32 bits, adding them, and reducing again. This is correct — it handles the full range of a 64-bit accumulator. The intermediate `tmp` is `uint32_t`, and the sum of two `uint16_t` values fits in `uint32_t`, so the final `__rte_raw_cksum_reduce(tmp)` correctly folds any carry.

**No correctness bugs found in the fix itself.**

---

### Potential Issues

**Error: Missing `Fixes:` tag and `Cc: stable@dpdk.org`**

This patch fixes a real bug in existing code (`rte_raw_cksum_mbuf` producing incorrect checksums for multi-segment mbufs). Per DPDK process, bug fixes should include a `Fixes:` tag referencing the commit that introduced the bug, and `Cc: stable@dpdk.org` for backport consideration. Both are missing.

**Warning: Commit message body starts with "The"**

While the guidelines specifically forbid starting with "It", the commit message could be improved. More importantly, the body could more clearly describe the *impact* — that multi-segment packets get incorrect checksums, potentially causing packet drops or silent data corruption. The current description is somewhat mechanical ("drop high 16 bit") without stating the user-visible consequence.

**Warning: Double-free risk in `test_l4_cksum_multi_mbufs()` error path**

Lines 228-229 (the success path) and lines 234-237 (the `fail:` path) both iterate and free `m[i]`. However, after a successful `rte_pktmbuf_chain(m_hdr, m[i])` call, `m[i]` becomes a segment of the chained mbuf `m_hdr` (which is `m[0]`). Freeing `m[0]` via `rte_pktmbuf_free()` will walk the chain and free all chained segments. Then freeing `m[1]`, `m[2]`, etc. individually would be a **double-free**.

On the **success path** (lines 228-229): `rte_pktmbuf_free(m[0])` frees the entire chain including `m[1]` and `m[2]`. Then `rte_pktmbuf_free(m[1])` and `rte_pktmbuf_free(m[2])` double-free those segments. This is a bug.

On the **error path** (lines 234-237): The same double-free occurs if `rte_pktmbuf_chain` succeeded for some segments before a later failure.

**Suggested fix for the success path:**
```c
rte_pktmbuf_free(m_hdr);  /* frees entire chain */
return 0;
```

**Suggested fix for the error path:**
```c
fail:
	if (m_hdr)
		rte_pktmbuf_free(m_hdr);  /* frees chained segments */
	/* Free any remaining unchained segments */
	for (i = (m_hdr ? 1 : 0); i < segs_len; i++) {
		/* Only free if not yet chained */
		if (m[i] && m[i] != m_hdr && m[i]->next == NULL)
			rte_pktmbuf_free(m[i]);
	}
```

Or more simply, since chain failures are unlikely, set `m[i] = NULL` after a successful chain:
```c
	if (rte_pktmbuf_chain(m_hdr, m[i]) == 0)
		m[i] = NULL;  /* now owned by chain */
	else
		GOTO_FAIL("Cannot chain mbuf");
```

Then both the success and error cleanup paths can safely iterate and free all non-NULL `m[i]`.

**Warning: `off` variable type mismatch**

`off` is declared as `size_t` on line 186 but is later reused on line 218 as `off = hdr_lens.l2_len + hdr_lens.l3_len` for a different purpose (L4 offset passed to the cksum verify functions). This reuse is not a bug per se, but a different variable name (e.g., `l4_off`) would improve clarity and avoid confusion. Minor style point.

**Info: Test coverage is good**

The test data uses three segments with an odd-length middle segment (61 bytes), which exercises the byte-swap alignment correction. This is exactly the scenario that triggers the bug. The test validates via `rte_ipv4_udptcp_cksum_mbuf_verify()`, confirming the checksum matches end-to-end.

---

### Summary of Findings

| Severity | Issue |
|----------|-------|
| **Error** | Double-free of chained mbuf segments in `test_l4_cksum_multi_mbufs()` — success path frees `m[0]` (the whole chain) then also frees `m[1]`, `m[2]` |
| **Error** | Missing `Fixes:` tag referencing the commit that introduced `rte_raw_cksum_mbuf()` |
| **Warning** | Missing `Cc: stable@dpdk.org` for backport |
| **Warning** | Double-free in the `fail:` error path if some segments were already chained |