|SUCCESS| [v4, 10/10] net/gve: enforce DQO TSO header and length limits

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/171639

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-19

# DPDK Patch Review

Reviewing 10-patch series fixing correctness bugs in the GVE driver's DQO Tx path.

## PATCH 01/10: net/gve: fix DQO descriptor cleaning loop bound

**Errors:**

None.

**Analysis:**

The patch fixes an off-by-one error in `gve_tx_clean_descs_dqo()`:
```c
-	while (--nb_descs)
+	while (nb_descs--)
```
Pre-decrement `--nb_descs` tests the decremented value, so when called with `nb_descs=1`, the test evaluates `0` and the loop body never runs. Post-decrement `nb_descs--` tests the original value before decrementing, so `1` passes the test, the body runs once, then `nb_descs` becomes `0` and the loop exits. This correctly processes exactly `nb_descs` completions.

---

## PATCH 02/10: net/gve: fix DQO completion queue stall on unknown type

**Errors:**

None.

**Analysis:**

When an unknown completion type is encountered, the function returned without advancing `complq_tail`, causing every subsequent call to stop at the same entry and stall the queue permanently. The fix logs the unknown type and continues:
```c
default:
-	PMD_DRV_DP_LOG(ERR, "unknown completion type.");
-	return;
+	PMD_DRV_DP_LOG(ERR, "unknown completion type %u", compl_desc->type);
+	break;
```
The `break` allows the tail to advance (`next++` and the wrap/assign after the switch). This correctly skips malformed entries instead of wedging the queue.

---

## PATCH 03/10: net/gve: fix DQO Tx report event interval

**Errors:**

None.

**Warnings:**

None.

**Analysis:**

The patch changes completion request logic from packet-count-based to descriptor-count-based. Previously, a completion was requested every 32 packets via `txq->re_cnt`, but with multi-segment or TSO packets the ring could fill before 32 packets were posted, so no completion was ever requested. The fix requests a completion once `GVE_TX_MIN_RE_INTERVAL` descriptors have been posted since the last one, matching the Linux driver:
```c
-	txq->re_cnt += nb_tx;
-	if (txq->re_cnt >= GVE_TX_MIN_RE_INTERVAL) {
-		txd = &txr[(tx_id - 1) & mask];
-		txd->pkt.report_event = true;
-		txq->re_cnt = 0;
-	}
+	last_desc_idx = (tx_id - 1) & mask;
+	last_re_interval = (last_desc_idx - txq->last_re_idx) & mask;
+	if (unlikely(last_re_interval >= GVE_TX_MIN_RE_INTERVAL)) {
+		txd->pkt.report_event = true;
+		txq->last_re_idx = last_desc_idx;
+	}
```
The interval calculation `(last_desc_idx - txq->last_re_idx) & mask` correctly handles ring wrap. The report_event flag is now set inside the per-packet loop on the last descriptor of the packet that crosses the threshold, rather than after the burst on the last descriptor of the burst. Both are correct; the new placement is clearer. State is properly initialized in `gve_reset_tx_ring_state_dqo()`.

---

## PATCH 04/10: net/gve: fix stray TSO context descriptor on tag exhaustion

**Errors:**

None.

**Analysis:**

For TSO packets, the context descriptor was written and `tx_id` advanced before allocating a completion tag. If no tag was available, the burst stopped, but the tail written at the end of the burst still covered the orphaned context descriptor. The fix allocates the tag first:
```c
+	if (!gve_free_compl_tags_pop(txq, &compl_tag))
+		break;
+
 	if (tso) {
 		txd = &txr[tx_id];
 		gve_tx_fill_seg_desc_dqo(txd, tx_pkt);
 		tx_id = (tx_id + 1) & mask;
 	}
-
-	if (!gve_free_compl_tags_pop(txq, &compl_tag))
-		break;
```
Now if the tag pop fails, no descriptors have been written and the tail is not advanced. The context descriptor is only written after the tag is successfully allocated.

---

## PATCH 05/10: net/gve: add cacheline overlap margin to DQO Tx ring

**Errors:**

None.

**Analysis:**

The hardware prefetches descriptors in 64-byte cacheline units (4 descriptors of 16 bytes each). When producer tail and consumer head share a cacheline, CPU and DMA can collide. The fix reserves `GVE_TX_MIN_DESC_PREVENT_CACHE_OVERLAP` (4 descriptors) headroom by adding it to the ring space check:
```c
+	req_descs = nb_descs + GVE_TX_MIN_DESC_PREVENT_CACHE_OVERLAP;
+
 	/* Clean if there aren't enough descriptors to send the packet
-	 * while maintaining the cacheline overlap margin.
+	 * while maintaining the cacheline overlap margin.
 	 */
-	if (unlikely(!gve_can_tx(txq, nb_descs, 1))) {
+	if (unlikely(!gve_can_tx(txq, req_descs, 1))) {
 		int nb_to_clean = RTE_MAX(DQO_TX_MULTIPLIER * txq->rs_thresh,
-					  nb_descs);
+					  (int)req_descs);
 
 		gve_tx_clean_descs_dqo(txq, nb_to_clean);
-		if (!gve_can_tx(txq, nb_descs, 1))
+		if (!gve_can_tx(txq, req_descs, 1))
 			break;
 	}
```
The margin is checked but not deducted from `nb_free` -- it is enforced by stopping the burst early. This matches the Linux driver.

---

## PATCH 06/10: net/gve: fix Tx stall on invalid packets

**Errors:**

None.

**Analysis:**

When a packet failed `rte_mbuf_check()` or exceeded the descriptor limit, the burst stopped without consuming the packet. The application retried the same packet indefinitely, stalling the queue. The fix frees such packets and continues:
```c
 	if (rte_mbuf_check(tx_pkt, true, &reason)) {
 		PMD_DRV_DP_LOG(DEBUG, "Invalid mbuf: %s", reason);
-		break;
+		txq->stats.invalid_mbufs++;
+		rte_pktmbuf_free(tx_pkt);
+		continue;
 	}
 
-	/* Drop packet if it doesn't adhere to hardware limits. */
-	if (!gve_tx_validate_descs(tx_pkt, nb_descs, tso)) {
+	if (!gve_tx_validate_descs(txq, tx_pkt, nb_descs, tso)) {
 		txq->stats.too_many_descs++;
-		break;
+		rte_pktmbuf_free(tx_pkt);
+		continue;
 	}
```
Validation is moved before the ring space check so oversized packets are dropped immediately rather than blocking the burst. A new `nb_sent` counter tracks successfully posted packets separately from `nb_tx` (which includes dropped packets) so `nb_pkts - nb_sent` gives the error count. The `invalid_mbufs` counter is added.

---

## PATCH 07/10: net/gve: export Tx drop counters in xstats

**Errors:**

None.

**Analysis:**

`too_many_descs` was counted but missing from the xstats table. The patch adds it together with `invalid_mbufs`:
```c
+	{ "too_many_descs", TX_QUEUE_STATS_OFFSET(too_many_descs) },
+	{ "invalid_mbufs", TX_QUEUE_STATS_OFFSET(invalid_mbufs) },
```

---

## PATCH 08/10: net/gve: reject Tx packets that cannot fit in the ring

**Errors:**

None.

**Analysis:**

A TSO packet needing more descriptors than the ring can ever have free (accounting for the reserved slot, cacheline margin, and posted-but-not-reclaimed descriptors) would fail the space check on every burst and block the queue. The patch adds `gve_tx_max_pkt_descs()` to compute the absolute maximum:
```c
+static inline int
+gve_tx_max_pkt_descs(const struct gve_tx_queue *txq)
+{
+	return (int)txq->nb_tx_desc - 1 - GVE_TX_MIN_DESC_PREVENT_CACHE_OVERLAP -
+		(GVE_TX_MIN_RE_INTERVAL - 1);
+}
```
and checks packets against it before the ring space check:
```c
-gve_tx_validate_descs(struct rte_mbuf *tx_pkt, uint16_t nb_descs, bool is_tso)
+gve_tx_validate_descs(const struct gve_tx_queue *txq, struct rte_mbuf *tx_pkt,
+		      uint16_t nb_descs, bool is_tso)
 {
+	if (nb_descs > gve_tx_max_pkt_descs(txq))
+		return false;
```
Such packets are dropped and counted as `too_many_descs`.

---

## PATCH 09/10: net/gve: enforce DQO Tx packet length limits

**Errors:**

None.

**Analysis:**

Non-TSO packets outside the 17 to 9728 byte range trigger malicious driver detection, which drops the packet and disables the datapath. The patch adds `gve_tx_validate_pkt()`:
```c
+static inline bool
+gve_tx_validate_pkt(const struct rte_mbuf *tx_pkt, bool is_tso)
+{
+	if (!is_tso)
+		return tx_pkt->pkt_len >= GVE_TX_MIN_PKT_SIZE_DQO &&
+		       tx_pkt->pkt_len <= GVE_TX_MAX_PKT_SIZE_DQO;
+
+	return true;
+}
```
and calls it before the ring space check:
```c
+	if (!gve_tx_validate_pkt(tx_pkt, tso)) {
+		txq->stats.invalid_pkts++;
+		rte_pktmbuf_free(tx_pkt);
+		continue;
+	}
```
The new `invalid_pkts` counter is added to xstats. TSO path returns true for now; patch 10 adds TSO validation.

---

## PATCH 10/10: net/gve: enforce DQO TSO header and length limits

**Errors:**

None.

**Analysis:**

TSO packets were not checked against device limits. The device requires MSS 88 to 9728 bytes and payload at most 262143 bytes. Out-of-range values trigger malicious driver detection. A zero `tso_segsz` divided by zero in `gve_tx_validate_descs()`, headers longer than 255 bytes were silently truncated by the 8-bit `header_len` field, and split headers gave `gve_tx_validate_descs()` negative first-segment length. The patch completes `gve_tx_validate_pkt()`:
```c
 static inline bool
 gve_tx_validate_pkt(const struct rte_mbuf *tx_pkt, bool is_tso)
 {
+	uint32_t headlen;
+
 	if (!is_tso)
 		return tx_pkt->pkt_len >= GVE_TX_MIN_PKT_SIZE_DQO &&
 		       tx_pkt->pkt_len <= GVE_TX_MAX_PKT_SIZE_DQO;
 
-	return true;
+	/* Headers must be present, fit the context descriptor, and be
+	 * contained in the first segment; the payload must not be empty.
+	 */
+	headlen = tx_pkt->l2_len + tx_pkt->l3_len + tx_pkt->l4_len;
+	if (tx_pkt->l4_len == 0 || headlen > GVE_TX_MAX_HDR_SIZE_DQO ||
+	    tx_pkt->data_len < headlen || tx_pkt->pkt_len <= headlen)
+		return false;
+
+	return tx_pkt->pkt_len - headlen <= GVE_TX_MAX_TSO_LEN_DQO &&
+	       tx_pkt->tso_segsz >= GVE_TX_MIN_TSO_MSS_DQO &&
+	       tx_pkt->tso_segsz <= GVE_TX_MAX_PKT_SIZE_DQO;
 }
```
Checks: L4 header present, total header length fits 8-bit field, headers in first segment, non-empty payload, payload length within limit, MSS within range. Packets failing these are dropped and counted as `invalid_pkts`.

---

## Summary

All 10 patches are correctness bug fixes with no style, process, or documentation issues. The series systematically fixes DQO Tx queue stalls and datapath shutdowns caused by loop bounds, unhandled completion types, descriptor accounting, invalid packets, and missing device limit checks. Code is clean, comments are clear, and new counters are properly exported in xstats.

Review-Result: CLEAN