|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: net/af_packet: add option to ignore outgoing packets

## Summary
Overall this is a well-structured patch that adds a new devarg to the AF_PACKET PMD. The implementation is mostly correct, but there are several issues to address.

---

## Errors

### 1. Error path resource leak in `rte_pmd_init_internals()`
**File:** `drivers/net/af_packet/rte_eth_af_packet.c`  
**Location:** Lines 1031-1039

The new `setsockopt()` call for `PACKET_IGNORE_OUTGOING` jumps to the `error` label on failure, but this occurs inside the queue setup loop after `qsockfd` has been created via `socket()`. The `error` label does not close `qsockfd`, causing a file descriptor leak on this error path.

**Fix:** Ensure `qsockfd` is closed before jumping to `error`:
```c
if (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);
		close(qsockfd);  /* ADD THIS */
		goto error;
	}
}
```

**Note:** This same leak pattern exists for the `PACKET_QDISC_BYPASS` and `PACKET_FANOUT` `setsockopt()` calls earlier in the function (pre-existing bug, not introduced by this patch). Consider fixing all three in a follow-up patch.

---

### 2. Missing error check on `rte_vdev_init()` return value
**File:** `app/test/test_pmd_af_packet.c`  
**Location:** Lines 992-994

```c
ret = rte_vdev_init("net_af_packet_ign_bad",
		    "iface=" TAP_DEV_NAME ",ignore_outgoing=2");
TEST_ASSERT(ret != 0, "Expected failure with ignore_outgoing=2");
```

If `rte_vdev_init()` **succeeds** when it should have failed (returning 0), the test asserts failure correctly. However, if it succeeds, the created vdev is never cleaned up (`rte_vdev_uninit()` is never called), leaking the device and potentially affecting subsequent tests.

**Fix:**
```c
ret = rte_vdev_init("net_af_packet_ign_bad",
		    "iface=" TAP_DEV_NAME ",ignore_outgoing=2");
if (ret == 0) {
	/* Cleanup the device that shouldn't have been created */
	rte_vdev_uninit("net_af_packet_ign_bad");
}
TEST_ASSERT(ret != 0, "Expected failure with ignore_outgoing=2");
```

---

### 3. Test cleanup on assertion failure leaks devices
**File:** `app/test/test_pmd_af_packet.c`  
**Location:** Lines 988-991

The test uses `TEST_ASSERT()` after the `fail_rx_ign_on:` label:
```c
fail_rx_ign_on:
	TEST_ASSERT(err == NULL, "%s", err);
	TEST_ASSERT(rx_off > 0, "Expected packets with ignore_outgoing=0");
	TEST_ASSERT(rx_on == 0, "Expected no packets with ignore_outgoing=1");
```

If any early `goto fail_rx_ign_on` occurs (e.g., port creation failure on line 924), the cleanup for `rx_port_ign_out_on` never happens, but the subsequent `TEST_ASSERT(err == NULL, "%s", err)` will abort the test, skipping all later cleanup code (the `rte_vdev_init()` call and its cleanup).

This is not a resource leak in the traditional sense (the test exits), but it violates the test pattern where cleanup should happen before assertions.

**Fix:** Move all `TEST_ASSERT()` calls to after the cleanup cascade finishes, or structure the test so that cleanup always happens before any assertion that could abort.

---

## Warnings

### 1. Implicit comparison on error variable
**File:** `app/test/test_pmd_af_packet.c`  
**Location:** Line 988

```c
TEST_ASSERT(err == NULL, "%s", err);
```

This is correct (explicit comparison against `NULL`). No issue here.

### 2. Missing check for `parse_uint()` range enforcement
**File:** `drivers/net/af_packet/rte_eth_af_packet.c`  
**Location:** Lines 1290-1293

```c
if (strstr(pair->key, ETH_AF_PACKET_IGNORE_OUTGOING_ARG) != NULL) {
	if (parse_uint(pair->key, pair->value, &ignore_outgoing, 1) < 0)
		return -1;
	continue;
}
```

The `parse_uint()` call uses `1` as the `max` parameter, which should enforce `ignore_outgoing` is 0 or 1. However, you should verify that `parse_uint()` enforces this correctly. Looking at the test (lines 992-994), you test that `ignore_outgoing=2` fails, which is good.

**Recommendation:** Add a comment clarifying the range check:
```c
/* parse_uint enforces 0 or 1 via max parameter */
if (parse_uint(pair->key, pair->value, &ignore_outgoing, 1) < 0)
	return -1;
```

### 3. Test uses blocking delay in busy loop
**File:** `app/test/test_pmd_af_packet.c`  
**Location:** Lines 967-974

```c
while (elapsed < LOOPBACK_TIMEOUT_US) {
	rx_off += do_rx_burst(rx_port_ign_out_off, 0, bufs, BURST_SIZE);
	rx_on += do_rx_burst(rx_port_ign_out_on, 0, bufs, BURST_SIZE);
	if (rx_off >= nb_tx)
		break;
	rte_delay_us_block(STATS_POLL_INTERVAL_US);
	elapsed += STATS_POLL_INTERVAL_US;
}
```

This pattern is consistent with other tests in the file (e.g., `test_af_packet_qdisc_bypass()`), so it's acceptable. No change needed, but note that `rte_delay_us_block()` is a busy-wait and may waste CPU in test environments.

---

## Info / Style Notes

### 1. Test could check packet content
The test verifies that `ignore_outgoing=1` drops packets and `ignore_outgoing=0` receives them, but it does not verify that the **received packets are the transmitted ones**. For a more robust test, you could check packet payload or sequence numbers.

This is optional, but would improve test coverage.

### 2. Comment in test could clarify kernel behavior
**File:** `app/test/test_pmd_af_packet.c`  
**Location:** Lines 916-919

The comment is clear, but you could add a note that `PACKET_OUTGOING` is a kernel packet type (not a DPDK concept) to help future readers unfamiliar with AF_PACKET internals.

### 3. Release notes formatting
**File:** `doc/guides/rel_notes/release_26_11.rst`  
**Location:** Lines 67-71

The release notes entry is well-written and follows the existing format. No issues.

### 4. Documentation clarity
**File:** `doc/guides/nics/af_packet.rst`  
**Location:** Lines 28-30

The documentation clearly explains the new parameter. Consider adding a note about when this is useful (e.g., "Use this when the application only wants to receive external traffic, not packets sent by the host.").

Optional improvement.

---

## Positive Observations

1. **Test coverage:** The patch includes a comprehensive functional test that validates both `ignore_outgoing=0` and `ignore_outgoing=1`, plus bounds checking for invalid values.

2. **Documentation:** Release notes, user guide, and parameter help are all updated consistently.

3. **Backward compatibility:** Default is 0, preserving existing behavior.

4. **Code style:** The patch follows DPDK naming conventions and formatting standards.

5. **Error handling:** The `setsockopt()` call properly checks for errors and logs them.

---

## Recommendations

1. **High Priority:** Fix the file descriptor leak on the `PACKET_IGNORE_OUTGOING` error path (Error #1).

2. **High Priority:** Fix the vdev cleanup in the test when `rte_vdev_init()` unexpectedly succeeds (Error #2).

3. **Medium Priority:** Restructure test cleanup to ensure devices are always cleaned up before assertions (Error #3).

4. **Low Priority:** Add a comment documenting the `parse_uint()` range enforcement (Warning #2).

5. **Optional:** Consider a follow-up patch to fix the pre-existing fd leak for `PACKET_QDISC_BYPASS` and `PACKET_FANOUT` error paths (mentioned in Error #1 note).