|WARNING| [v2] net/af_packet: add option to ignore outgoing packets

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: net/af_packet: add option to ignore outgoing packets

## Summary

This patch adds an `ignore_outgoing` option to the AF_PACKET PMD to enable the `PACKET_IGNORE_OUTGOING` socket option on Linux 4.20+. The implementation is mostly correct, but there are several issues to address.

---

## ERRORS

### 1. Compilation failure when `PACKET_IGNORE_OUTGOING` is not defined

**Location:** `drivers/net/af_packet/rte_eth_af_packet.c:1031-1041`

The code wraps `setsockopt(PACKET_IGNORE_OUTGOING)` in `#if defined(PACKET_IGNORE_OUTGOING)` but silently succeeds when the macro is undefined. If a user explicitly sets `ignore_outgoing=1` on a system without kernel support, the PMD accepts the configuration but does not actually enable the feature.

**Fix:** Return an error when `ignore_outgoing=1` is requested but `PACKET_IGNORE_OUTGOING` is unavailable:

```c
if (ignore_outgoing) {
#if defined(PACKET_IGNORE_OUTGOING)
	rc = setsockopt(qsockfd, SOL_PACKET, PACKET_IGNORE_OUTGOING,
			&ignore_outgoing, sizeof(ignore_outgoing));
	if (rc == -1) {
		PMD_LOG_ERRNO(ERR,
			"%s: could not set PACKET_IGNORE_OUTGOING on AF_PACKET socket for %s",
			name, pair->value);
		goto error;
	}
#else
	PMD_LOG(ERR,
		"%s: PACKET_IGNORE_OUTGOING not supported (kernel < 4.20)",
		name);
	goto error;
#endif
}
```

---

### 2. Resource leak on error path in test

**Location:** `app/test/test_pmd_af_packet.c:968-972`

When `alloc_tx_mbufs()` or `do_tx_burst()` fails, the test sets `err` and jumps to cleanup but does not free the allocated mbufs. If `alloc_tx_mbufs()` succeeds but `nb_tx == 0`, the mbufs in `bufs[]` leak.

**Fix:** Free mbufs on this error path:

```c
allocated = alloc_tx_mbufs(bufs, 4);
nb_tx = do_tx_burst(tx_port, 0, bufs, allocated);
if (allocated == 0 || nb_tx == 0) {
	if (allocated > 0) {
		for (unsigned int i = 0; i < allocated; i++)
			rte_pktmbuf_free(bufs[i]);
	}
	err = "TX setup failed";
	goto out;
}
```

---

### 3. Mbuf leak in polling loop

**Location:** `app/test/test_pmd_af_packet.c:977-985`

The test receives mbufs via `do_rx_burst()` and increments `rx_off`/`rx_on` counters but never frees the received mbufs. All mbufs received in the polling loop leak.

**Fix:** Free mbufs immediately after receiving:

```c
while (elapsed < LOOPBACK_TIMEOUT_US) {
	uint16_t n_off = do_rx_burst(rx_port_ign_out_off, 0, bufs, BURST_SIZE);
	for (uint16_t i = 0; i < n_off; i++)
		rte_pktmbuf_free(bufs[i]);
	rx_off += n_off;

	uint16_t n_on = do_rx_burst(rx_port_ign_out_on, 0, bufs, BURST_SIZE);
	for (uint16_t i = 0; i < n_on; i++)
		rte_pktmbuf_free(bufs[i]);
	rx_on += n_on;

	if (rx_off >= nb_tx)
		break;
	rte_delay_us_block(STATS_POLL_INTERVAL_US);
	elapsed += STATS_POLL_INTERVAL_US;
}
```

---

### 4. Mbuf leak in drain loops at test start

**Location:** `app/test/test_pmd_af_packet.c:964-967`

The test drains both ports before starting the actual test but does not free the received mbufs:

```c
while (do_rx_burst(rx_port_ign_out_off, 0, bufs, BURST_SIZE) > 0)
	;
while (do_rx_burst(rx_port_ign_out_on, 0, bufs, BURST_SIZE) > 0)
	;
```

**Fix:** Free mbufs in the drain loops:

```c
uint16_t nb_rx;
while ((nb_rx = do_rx_burst(rx_port_ign_out_off, 0, bufs, BURST_SIZE)) > 0) {
	for (uint16_t i = 0; i < nb_rx; i++)
		rte_pktmbuf_free(bufs[i]);
}
while ((nb_rx = do_rx_burst(rx_port_ign_out_on, 0, bufs, BURST_SIZE)) > 0) {
	for (uint16_t i = 0; i < nb_rx; i++)
		rte_pktmbuf_free(bufs[i]);
}
```

---

### 5. Missing error check on `parse_uint()`

**Location:** `drivers/net/af_packet/rte_eth_af_packet.c:1292-1295`

The code calls `parse_uint()` with `max=1` (enforcing boolean values 0 or 1) and returns `-1` on failure, but this does not propagate a meaningful error and lacks cleanup of resources allocated earlier in `rte_eth_from_packet()`.

However, reviewing the function context, `rte_eth_from_packet()` does not allocate resources before parsing arguments, so returning `-1` is acceptable here. The test case at the end of `test_af_packet_ignore_outgoing()` verifies that `ignore_outgoing=2` is rejected, confirming the bounds check works.

**No change needed** -- error propagation is correct in this context.

---

## WARNINGS

### 1. Port stop/close ordering

**Location:** `app/test/test_pmd_af_packet.c:986-996`

The test calls `rte_eth_dev_stop()` followed by `rte_eth_dev_close()` on each port, then calls `rte_vdev_uninit()`. While this works, the typical DPDK pattern is to close the device first (which internally stops it), then uninit the vdev. This is a minor style issue, not a functional bug.

**Suggested improvement (not required):**

```c
out:
	rte_eth_dev_close(tx_port);
	rte_vdev_uninit("net_af_packet_ign_tx");
out_rx_port_ign_out_off:
	rte_eth_dev_close(rx_port_ign_out_off);
	rte_vdev_uninit("net_af_packet_ign_off");
out_rx_port_ign_out_on:
	rte_eth_dev_close(rx_port_ign_out_on);
	rte_vdev_uninit("net_af_packet_ign_on");
```

---

### 2. Test uses `unsigned int` for counters instead of `uint16_t`

**Location:** `app/test/test_pmd_af_packet.c:924`

The test declares `rx_off` and `rx_on` as `unsigned int`, but `do_rx_burst()` returns `uint16_t`. For consistency with DPDK APIs and other test code, consider using `uint16_t` or `uint32_t`.

This is a minor style preference and does not affect correctness.

---

### 3. Unsigned comparison `>= 0` is always true (dead code detection)

**Location:** `drivers/net/af_packet/rte_eth_af_packet.c:1293`

The condition `if (parse_uint(pair->key, pair->value, &ignore_outgoing, 1) < 0)` is correct. However, note that `ignore_outgoing` is declared as `unsigned int`, so any check like `if (ignore_outgoing >= 0)` would always be true. The current code does not have this bug, but be aware of it if modifying the validation logic.

---

## STYLE OBSERVATIONS

### 1. Comment style

**Location:** `app/test/test_pmd_af_packet.c:917-920`

The multi-line comment uses the correct DPDK style:

```c
/*
 * Test: Ignore outgoing packets configuration
 * TX on the TAP with qdisc_bypass=0 produces PACKET_OUTGOING frames.
 * A peer with ignore_outgoing=0 should see them; ignore_outgoing=1 must not.
 */
```

This is good.

---

### 2. Parameter string alignment

**Location:** `drivers/net/af_packet/rte_eth_af_packet.c:1425-1429`

The parameter string is correctly updated with the new `ignore_outgoing` option. Formatting is consistent with the existing style.

---

## DOCUMENTATION

### 1. Release notes are correctly updated

The patch adds a new feature section to `doc/guides/rel_notes/release_26_11.rst` documenting the new option and kernel requirement. This is appropriate.

### 2. AF_PACKET documentation updated

The `doc/guides/nics/af_packet.rst` file correctly documents the new `ignore_outgoing` parameter, its default value, and kernel version requirement.

---

## FINAL ASSESSMENT

**Errors:** 4 resource leaks (test code), 1 silent configuration failure (feature not applied when unsupported)

**Warnings:** Minor style and pattern preferences

**Overall:** The feature is well-designed and the documentation is good. The primary issues are **resource leaks in the test** (mbufs not freed) and **silent failure when `PACKET_IGNORE_OUTGOING` is unavailable**. Fix these, and the patch will be ready.