|WARNING| [v3] 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/170643

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch adds an `ignore_outgoing` devarg to the AF_PACKET PMD to control whether the socket receives packets transmitted by the host. Overall implementation is correct, but there are several minor issues to address.

---

## Errors

None.

---

## Warnings

### 1. Test cleanup on early failure paths
**File:** `app/test/test_pmd_af_packet.c:933-938`

The test creates `rx_port_ign_out_on` but if creation of `rx_port_ign_out_off` fails, the error path `fail_rx_ign_off` does not clean up `rx_port_ign_out_on` before jumping to `fail_rx_ign_on`. The cleanup sequence is correct for successful creation but incomplete for early failure.

**Suggested fix:**
```c
ret = create_af_packet_port("net_af_packet_ign_off",
		"iface=" TAP_DEV_NAME ",ignore_outgoing=0",
		&rx_port_ign_out_off);
if (ret != 0) {
	err = "Failed to create ignore_outgoing=0 port";
	/* Need to clean up rx_port_ign_out_on first */
	rte_eth_dev_stop(rx_port_ign_out_on);
	rte_eth_dev_close(rx_port_ign_out_on);
	rte_vdev_uninit("net_af_packet_ign_on");
	goto fail_rx_ign_on;
}
```

### 2. Test does not free received mbufs
**File:** `app/test/test_pmd_af_packet.c:967-970`

The test calls `do_rx_burst()` to receive packets but never frees the mbufs placed in `bufs[]`. This leaks memory from the mempool.

**Suggested fix:**
```c
while (elapsed < LOOPBACK_TIMEOUT_US) {
	uint16_t n_off = do_rx_burst(rx_port_ign_out_off, 0, bufs, BURST_SIZE);
	if (n_off > 0) {
		rx_off += n_off;
		rte_pktmbuf_free_bulk(bufs, n_off);
	}
	uint16_t n_on = do_rx_burst(rx_port_ign_out_on, 0, bufs, BURST_SIZE);
	if (n_on > 0) {
		rx_on += n_on;
		rte_pktmbuf_free_bulk(bufs, n_on);
	}
	if (rx_off >= nb_tx)
		break;
	rte_delay_us_block(STATS_POLL_INTERVAL_US);
	elapsed += STATS_POLL_INTERVAL_US;
}
```

Also free any packets received during the drain phase (lines 955-956):
```c
uint16_t n;
while ((n = do_rx_burst(rx_port_ign_out_off, 0, bufs, BURST_SIZE)) > 0)
	rte_pktmbuf_free_bulk(bufs, n);
while ((n = do_rx_burst(rx_port_ign_out_on, 0, bufs, BURST_SIZE)) > 0)
	rte_pktmbuf_free_bulk(bufs, n);
```

### 3. Missing bounds check on `ignore_outgoing` devarg
**File:** `drivers/net/af_packet/rte_eth_af_packet.c:1290-1293`

The patch parses `ignore_outgoing` with `parse_uint(..., 1)` which limits the value to 1, but the test at line 1093 verifies that `ignore_outgoing=2` fails. However, the actual validation happens inside `parse_uint()` (checking `max` parameter), so the implementation is correct. The test properly exercises the bounds check.

Actually correct -- no issue here.

### 4. Unclear test assertion logic
**File:** `app/test/test_pmd_af_packet.c:989-991`

The `TEST_ASSERT` calls at the end of `test_af_packet_ignore_outgoing()` will always be reached because earlier `goto fail_setup` jumps skip them. This is correct -- the assertions verify the results after all cleanup is done. The pattern is intentional.

Actually correct -- no issue here.

---

## Info

### 1. Test suite organization
**File:** `app/test/test_pmd_af_packet.c:1206`

The new test is added immediately after `test_af_packet_qdisc_bypass`, which is a good logical grouping since both test socket-level options.

### 2. Documentation completeness
The documentation in `af_packet.rst` and release notes clearly describes the new option. The release notes appropriately mention this is a PMD update, not a library or API change.

### 3. Consistent naming
The devarg name `ignore_outgoing` matches the kernel socket option name `PACKET_IGNORE_OUTGOING`, making the mapping obvious.

---

## Correctness Check Summary

**Use-after-free:** None detected.

**Resource leaks:**
- Test code leaks mbufs received during RX burst (Warning #2).
- Test cleanup path incomplete for early failure (Warning #1).

**Error handling:** Socket option error handling is correct -- the code checks `setsockopt()` return value and propagates errors.

**Bounds checking:** The `parse_uint(..., 1)` call correctly limits `ignore_outgoing` to 0 or 1.