|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 ignore_outgoing option

## Summary

This patch adds an `ignore_outgoing` option to the AF_PACKET PMD to filter out host-originated packets. The implementation is generally sound, but there are several issues to address.

---

## Errors

### 1. Resource leak on error path in `rte_pmd_init_internals()`

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

The `setsockopt()` call for `PACKET_IGNORE_OUTGOING` has an error path that jumps to the `error` label, but the function has already allocated multiple resources before this point (socket `qsockfd` opened at line 966, `mmap()` allocations in the existing code). The existing error handling appears adequate, but verify that the `error:` label properly cleans up all allocated resources including the socket that was just configured.

**Review the error cleanup path:** Ensure `qsockfd` is closed and any prior queue allocations are freed when this error occurs.

---

### 2. Test does not verify that transmitted packets reached the peer

**File:** `app/test/test_pmd_af_packet.c`  
**Location:** Lines 963-974

```c
nb_tx = do_tx_burst(tx_port, 0, bufs, allocated);
if (allocated == 0 || nb_tx == 0) {
    err = "TX setup failed";
    goto out;
}

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;
```

The test checks `nb_tx` (number of packets accepted by the transmit function), but does not verify that these packets were actually transmitted by the kernel. If the kernel drops them or the TAP interface is down, the test would wait for `rx_off` packets that never arrive, then timeout and pass spuriously because `rx_on == 0` (nothing was received on either port).

**Suggested fix:** After the TX burst, add a check of TX statistics or verify that the TAP interface is up and the packets were actually sent. Alternatively, make the test fail if `rx_off` does not reach `nb_tx` before timeout:

```c
if (rx_off < nb_tx) {
    err = "Did not receive expected packets on ignore_outgoing=0 port";
    goto out;
}
```

---

### 3. Potential undefined behavior if `PACKET_IGNORE_OUTGOING` is not defined

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

```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, ...);
        goto error;
    }
#endif
}
```

If `PACKET_IGNORE_OUTGOING` is not defined (kernel headers < 4.20), but the user passes `ignore_outgoing=1`, the code silently ignores the setting. The port is created successfully but does not have the requested behavior. This is misleading.

**Suggested fix:** Return an error if the feature is requested but not available:

```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: ignore_outgoing requested but PACKET_IGNORE_OUTGOING not supported (kernel < 4.20)",
        name);
    goto error;
#endif
}
```

---

## Warnings

### 1. `parse_uint()` called with max value 1 but no validation of the result

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

```c
if (parse_uint(pair->key, pair->value, &ignore_outgoing, 1) < 0)
    return -1;
```

The `parse_uint()` call constrains `ignore_outgoing` to the range [0, 1], which is correct. However, the existing code for `qdisc_bypass` on line 1287 uses the same pattern, so this is consistent with the existing style. If `parse_uint()` enforces the max bound, this is fine.

**Note:** The test at line 1193 correctly verifies that `ignore_outgoing=2` is rejected. Ensure that `parse_uint()` actually enforces the maximum value; if it does, this is not an issue.

---

### 2. Test cleanup order may cause port operations on stopped devices

**File:** `app/test/test_pmd_af_packet.c`  
**Location:** Lines 1076-1090

```c
out:
    rte_eth_dev_stop(tx_port);
    rte_eth_dev_close(tx_port);
    rte_vdev_uninit("net_af_packet_ign_tx");
out_rx_port_ign_out_off:
    rte_eth_dev_stop(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_stop(rx_port_ign_out_on);
    rte_eth_dev_close(rx_port_ign_out_on);
    rte_vdev_uninit("net_af_packet_ign_on");
```

If the test jumps to `out:` after all three ports have been created and configured, all three ports will be stopped and closed. However, if the test jumps to `out_rx_port_ign_out_off:`, the `tx_port` code will attempt to stop/close a port that may not have been created. This is only safe if `rte_eth_dev_stop()` and `rte_eth_dev_close()` tolerate invalid port IDs.

**Check:** Verify that the port IDs are initialized to invalid values (e.g., `RTE_MAX_ETHPORTS`) if port creation fails, or add guards:

```c
if (tx_port < RTE_MAX_ETHPORTS) {
    rte_eth_dev_stop(tx_port);
    rte_eth_dev_close(tx_port);
    rte_vdev_uninit("net_af_packet_ign_tx");
}
```

Looking at the existing test code, this pattern appears consistent with other tests in the file (e.g., `test_af_packet_qdisc_bypass()` at lines 867-887). If other tests use the same pattern without issue, this is acceptable.

---

### 3. Missing mbuf cleanup in test receive loop

**File:** `app/test/test_pmd_af_packet.c`  
**Location:** Lines 968-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;
}
```

The `do_rx_burst()` calls fill the `bufs[]` array with received mbufs, but the loop never frees them. This leaks the mbufs. Each iteration overwrites `bufs[]`, but the pointers to the previously received mbufs are lost.

**Suggested fix:** Free the received mbufs after counting them:

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

---

## Info

### 1. Consider logging when `ignore_outgoing` is requested but not supported

**File:** `drivers/net/af_packet/rte_eth_af_packet.c`  
**Location:** Line 1031

Even with the error handling suggested above, consider adding an INFO-level log when the feature is successfully enabled, similar to how `fanout_mode` is logged at lines 1318-1322. This helps users verify their configuration was applied:

```c
if (ignore_outgoing)
    PMD_LOG(INFO, "%s: PACKET_IGNORE_OUTGOING enabled", name);
```

---

### 2. Test skip message could be more informative

**File:** `app/test/test_pmd_af_packet.c`  
**Location:** Line 944

```c
printf("SKIPPED: ignore_outgoing may not be supported\n");
```

The message "may not be supported" is vague. Consider:

```c
printf("SKIPPED: ignore_outgoing option not supported (requires Linux kernel >= 4.20)\n");
```

---

### 3. Documentation could mention the behavior difference

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

The documentation states "the socket does not receive packets transmitted by the host," which is accurate. Consider adding a note that the default behavior (0) differs from typical PMDs, which do not receive host-originated packets:

```rst
*   ``ignore_outgoing`` - set PACKET_IGNORE_OUTGOING so the socket does not
    receive packets transmitted by the host on the same interface (optional,
    default 0 for backward compatibility; note that this differs from most
    other PMDs, which do not receive host-originated traffic; requires Linux
    kernel >= 4.20);
```

This helps users understand why the default is 0 rather than 1.

---

## Positive Notes

- The feature is well-tested with a comprehensive functional test
- The test correctly verifies both the positive case (ignore_outgoing=1 filters packets) and the negative case (ignore_outgoing=0 does not)
- The test includes validation that invalid values (ignore_outgoing=2) are rejected
- Release notes are properly updated
- The parameter parsing follows the existing pattern for `qdisc_bypass`
- The `.mailmap` entry is added correctly

---

## Conclusion

The patch is fundamentally sound but requires fixes for:
1. **Error handling** when `PACKET_IGNORE_OUTGOING` is not available
2. **Test robustness** (verify TX actually occurred, fix mbuf leak)
3. Minor improvements to error paths and logging

Please address the items in the **Errors** section before merging.