|WARNING| [v2] gro: fix returned packet count after UDP fragment merge

dpdklab at iol.unh.edu dpdklab at iol.unh.edu
Fri Nov 6 09:46:32 CET 2026


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

_AI Code Review_

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

# DPDK Patch Review

## ERRORS

None.

## WARNINGS

**Missing release notes**

The fix corrects a severe double-free bug that corrupts mempools and produces malformed packets. This should be documented in the current release notes (`doc/guides/rel_notes/release_*.rst`) in the "Fixed Issues" section.

Add an entry describing the fix:
```rst
* **gro: Fixed packet count after UDP fragment merge.**

  Fixed a double-free bug in ``rte_gro_reassemble_burst()`` where merges
  performed during table flush were not reflected in the returned packet
  count. This caused the function to return stale mbuf pointers that had
  already been chained into earlier packets, leading to mempool corruption
  and malformed packets being returned to the caller.
```

**Missing functional test coverage**

This bug occurs when:
1. UDP fragments arrive out of order such that an intermediate fragment enables a merge between two previously non-adjacent fragments
2. The merge happens during the timeout flush at the end of the burst

A test case in `app/test/test_gro.c` should reproduce this scenario:
- Send a burst with fragments arriving out of order (e.g., frag 0, frag 2, then frag 1)
- Verify the returned packet count matches the actual number of packets in `pkts[]`
- Verify no mbuf pointers appear multiple times in the returned array
- Verify merged packets have correct payload and no stripped headers as standalone packets

Without such a test, this class of bug could regress undetected.

## INFO

None.

---

**Correctness analysis:**

The fix is correct. Before the change:
- `nb_after_gro` counts packets after insertion-time merges
- Flush-time merges in `gro_udp4_merge_items()` reduce the array but only update the local loop counter `i`
- Returning `nb_after_gro` produces a count larger than the actual number of packets written

After the change:
- When the flush block executes, `i` is the actual post-flush packet count
- When the flush block is skipped, `i == nb_pkts == nb_after_gro` (no change to the non-merging case)

The code path for the non-merging case:
```c
for (i = 0; i < nb_pkts; i++) {
    /* ... */
    if (IS_IPV4_TCP_PKT(...)) {
        /* ... inserts, may decrement nb_after_gro ... */
    }
    /* ... other protocol checks ... */
    if (unprocess_num > 0)
        pkts[nb_after_gro++] = pkts[i];  /* nb_after_gro stays <= i+1 */
}
/* Flush block skipped when do_flush is 0 */
/* Here: i == nb_pkts, and the returned value i is correct */
```

In the merging case (flush executed):
```c
/* After loop: nb_after_gro reflects insertion merges only */
if (do_flush) {
    i = 0;
    /* Flush calls gro_*_merge_items(), writes to pkts[i++] */
    /* i is now the true output count, less than nb_after_gro if merge occurred */
}
return i;  /* Correct count */
```

The fix addresses the root cause: `nb_after_gro` is stale after flush, `i` is always current.

Review-Result: WARNING


More information about the test-report mailing list