|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 Linux `PACKET_IGNORE_OUTGOING` socket option (available in Linux 4.20+). The implementation is generally correct but has several issues that must be addressed.

---

## Errors

### 1. Resource leak on setsockopt error path

**File:** `drivers/net/af_packet/rte_eth_af_packet.c`  
**Line:** ~1028-1040

The `PACKET_IGNORE_OUTGOING` setsockopt error path does a `goto error` after opening `qsockfd`, but there is no indication that the socket is closed in the error handler. Trace the cleanup in the `error:` label to verify that `qsockfd` is properly closed. If not, this is a file descriptor leak.

**Suggested fix:**
Verify the `error:` label at the end of `rte_pmd_init_internals()` closes all sockets in `(*internals)->rx_queue[q].sockfd` that have been opened so far. If the socket is not added to that array before this error path, it will leak. Either ensure the socket is in the array before this check, or explicitly close it:

```c
if (rc == -1) {
	PMD_LOG_ERRNO(ERR, "...");
	close(qsockfd);  /* explicit close before goto if not yet tracked */
	goto error;
}
```

---

### 2. parse_uint with max=1 allows only 0 and 1, but no validation message explains this

**File:** `drivers/net/af_packet/rte_eth_af_packet.c`  
**Line:** ~1292-1295

The code calls `parse_uint(pair->key, pair->value, &ignore_outgoing, 1)` with max=1, which correctly restricts the value to 0 or 1. However, when the user passes `ignore_outgoing=2`, the test expects an error message but none is logged. The `parse_uint` function returns -1 on out-of-range, but the caller just propagates it without logging why.

**Suggested fix:**
Either:
- Log an error message before returning -1:
  ```c
  if (parse_uint(pair->key, pair->value, &ignore_outgoing, 1) < 0) {
      PMD_LOG(ERR, "%s: ignore_outgoing must be 0 or 1", name);
      return -1;
  }
  ```
- Or verify `parse_uint` already logs errors internally (check its implementation). If it does not, add the message.

---

### 3. Missing check whether PACKET_IGNORE_OUTGOING is defined before using it in parse_uint

**File:** `drivers/net/af_packet/rte_eth_af_packet.c`  
**Line:** ~1289-1295

The code unconditionally parses `ignore_outgoing` from the devargs, then only uses `#if defined(PACKET_IGNORE_OUTGOING)` inside `rte_pmd_init_internals`. This means:
- On kernels < 4.20, a user can pass `ignore_outgoing=1` and it will be silently ignored (no error, no warning).
- The test `test_af_packet_ignore_outgoing()` will skip if creation fails, but a user won't get any feedback that their configuration was ignored.

**Suggested fix:**
Either:
- Return an error if `ignore_outgoing` is set but `PACKET_IGNORE_OUTGOING` is not defined:
  ```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;
  #if !defined(PACKET_IGNORE_OUTGOING)
      if (ignore_outgoing != 0) {
          PMD_LOG(ERR, "%s: ignore_outgoing requires Linux >= 4.20", name);
          return -1;
      }
  #endif
      continue;
  }
  ```
- Or at minimum log a warning if the feature is requested but unavailable.

---

## Warnings

### 1. Test does not verify packet content or counts match expectations

**File:** `app/test/test_pmd_af_packet.c`  
**Line:** ~913-1010

The test `test_af_packet_ignore_outgoing()` transmits `nb_tx` packets, waits for loopback, then asserts:
- `rx_off > 0` (at least one packet received on the port with `ignore_outgoing=0`)
- `rx_on == 0` (no packets received on the port with `ignore_outgoing=1`)

However, it does not verify that `rx_off == nb_tx` or provide any diagnostic information about how many packets were received versus transmitted. If the test becomes flaky (e.g., due to kernel buffer drops or timing), the output won't indicate whether only some packets were seen.

**Suggested fix:**
Add a diagnostic message or stricter assertion:
```c
TEST_ASSERT(rx_off == nb_tx,
    "Expected %u packets on ignore_outgoing=0 port, got %u", nb_tx, rx_off);
```

If occasional packet loss is acceptable in the test environment, at least log the counts for debugging.

---

### 2. Missing release notes entry for test addition

**File:** `doc/guides/rel_notes/release_26_11.rst`  
**Line:** ~67-71

The release notes mention the new driver feature, but do not mention the new test case `test_af_packet_ignore_outgoing`. While test additions are not strictly required in release notes, a functional test for a kernel-version-dependent feature is worth mentioning for users who want to verify compatibility.

**Suggested fix:**
Add a bullet under the AF_PACKET driver section:
```
* Added functional test ``test_af_packet_ignore_outgoing`` to verify the
  new ``ignore_outgoing`` option.
```

---

### 3. No documentation of behavior when feature is unavailable

**File:** `doc/guides/nics/af_packet.rst`  
**Line:** ~28-30

The documentation states "requires Linux kernel >= 4.20" but does not explain what happens if the user sets `ignore_outgoing=1` on an older kernel. Does initialization fail? Is it silently ignored?

**Suggested fix:**
Clarify the behavior:
```
*   ``ignore_outgoing`` - set PACKET_IGNORE_OUTGOING so the socket does not
    receive packets transmitted by the host on the same interface (optional,
    default 0; requires Linux kernel >= 4.20; setting to 1 on older kernels
    will cause device initialization to fail / be silently ignored [choose one]).
```

---

## Info

### 1. Test could be more robust by flushing Rx queues earlier

**File:** `app/test/test_pmd_af_packet.c`  
**Line:** ~955-957

The test flushes Rx queues with two `while` loops before transmitting packets. However, it does this *after* configuring all three ports. Any residual traffic generated during port configuration (e.g., link-local multicast, ARP) might still arrive after the flush.

**Suggested improvement:**
Move the Rx flush to immediately before the `alloc_tx_mbufs()` call to minimize the window for spurious packets, or add a second flush after a short delay.

---

### 2. Variable name rx_on/rx_off could be more descriptive

**File:** `app/test/test_pmd_af_packet.c`  
**Line:** ~925

The variable names `rx_on` and `rx_off` are concise but ambiguous. Consider renaming:
```c
unsigned int rx_ign_off = 0, rx_ign_on = 0;
```
This makes it immediately clear which port the variable corresponds to.

---

### 3. Magic number 4 in alloc_tx_mbufs

**File:** `app/test/test_pmd_af_packet.c`  
**Line:** ~960

The test allocates 4 mbufs (`alloc_tx_mbufs(bufs, 4)`) with no explanation. While this is probably sufficient for the test, a named constant or comment would clarify intent:
```c
#define IGNORE_OUTGOING_TEST_PKT_COUNT 4
allocated = alloc_tx_mbufs(bufs, IGNORE_OUTGOING_TEST_PKT_COUNT);
```

---

## Conclusion

**Overall:** The patch correctly implements the feature and includes a functional test. The primary concerns are:
1. Potential file descriptor leak on error path (Error #1) -- **must verify and fix**
2. Silent failure when `ignore_outgoing=1` is set on old kernels (Error #3) -- **should return error or log warning**
3. Missing user feedback when parse_uint rejects a value (Error #2) -- **should log error**

After addressing these issues, the patch will be ready for integration.