|WARNING| [v4, 3/3] net/af_packet: use kernel to filter multi-queue Rx

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/3: net/af_packet: filter Rx of outgoing packets in the PMD

### Errors

None.

### Warnings

**Test infrastructure - single TEST_ASSERT on non-`bool` variable:**
Line in test code: `TEST_ASSERT(err == NULL, "%s", err);`

The `err` variable is a `const char *`. The test framework expects explicit NULL comparison for pointer types. This should be:
```c
TEST_ASSERT_NULL(err, "%s", err != NULL ? err : "");
```
However, the pattern `err == NULL` is explicit, so this is borderline acceptable. The bigger issue is that when `err` is NULL, the printf will pass NULL to `%s`, which is undefined behavior. Consider:
```c
TEST_ASSERT(err == NULL, "%s", err != NULL ? err : "(no error)");
```

**Unnecessary initialization - rx_pkts variable:**
```c
unsigned int rx_pkts = 0, allocated;
```
The `rx_pkts` variable is set to 0 but then unconditionally accumulated in the receive loop. The initial 0 is meaningful here (it's the starting count), so this is actually correct and the warning does not apply.

**Inconsistent comparison style in test:**
```c
if (allocated == 0 || nb_tx == 0) {
```
This is an explicit comparison (correct per guidelines). No issue.

**Missing release notes for test-only changes:**
The guidelines state "Release notes are NOT required for: Test-only changes (unit tests, functional tests)". However, this patch adds a new driver feature (`ignore_outgoing`), not just tests. The release notes are correctly included for the feature itself. No issue.

### Info

**Test structure - acceptable pattern:**
The test helper function `test_af_packet_ignore_outgoing_queues()` returns `TEST_SUCCESS`, `TEST_SKIPPED`, or failure via `TEST_ASSERT`. This is correct usage of the test framework.

**Port cleanup - acceptable pattern:**
The test uses `have_rx` and `have_tx` flags to track which resources need cleanup. This is a correct pattern for handling partial initialization.

---

## Patch 2/3: net/af_packet: use kernel to filter single-queue Rx

### Errors

**Error path resource leak:**
Lines 1041-1051 in `rte_pmd_init_internals()`:
```c
if (ignore_outgoing != 0 && nb_queues == 1) {
    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;
    }
}
```

The `setsockopt()` call occurs inside the `for (q = 0; q < nb_queues; q++)` loop (line 1010). For each iteration, a socket `qsockfd` is opened (line 1012) and file descriptors from previous iterations are stored in `(*internals)->tx_queue[q].sockfd` and `(*internals)->rx_queue[q].sockfd` (lines 1029, 1060).

If the `setsockopt()` fails for queue `q > 0`, the `goto error` jumps to the error cleanup path (line 1152). The cleanup loop starting at line 1155 only iterates `for (unsigned q = 0; q < (*eth_dev)->data->nb_tx_queues; q++)`, which was set earlier in the function. However, at the point of failure, `(*eth_dev)->data->nb_tx_queues` has already been set to `nb_queues` (line 1106), and some queues have been successfully set up.

**But** - inspecting the error path more carefully:
- Line 1155: `for (unsigned q = 0; q < (*eth_dev)->data->nb_tx_queues; q++)`
- This closes sockets for queues 0 through `nb_tx_queues - 1`
- If we fail at queue `q`, we have already stored socket fds for queues `0..q-1`
- The current queue's socket `qsockfd` was opened but not yet stored in the internals structure (storage happens later at lines 1029 and 1060)

So the current queue's `qsockfd` is **leaked** when `setsockopt()` fails. It needs to be closed before `goto error`.

**Fix:**
```c
if (rc == -1) {
    close(qsockfd);
    PMD_LOG_ERRNO(ERR,
        "%s: could not set PACKET_IGNORE_OUTGOING "
        "on AF_PACKET socket for %s",
        name, pair->value);
    goto error;
}
```

### Warnings

None.

---

## Patch 3/3: net/af_packet: use kernel to filter multi-queue Rx

### Errors

None.

### Warnings

**Kernel version comment accuracy:**
Line 1004-1006:
```c
/*
 * Set the flag opportunistically if multiple queues are requested.
 * It takes effect from Linux v6.2. Older kernels ignore it and the
 * PMD filters instead.
 */
```

The comment accurately describes the behavior. The fallback mechanism (PMD-side filtering) is still in place from patch 1, so this is correct. No issue.

**Fanout flag positioning:**
The `PACKET_FANOUT_FLAG_IGNORE_OUTGOING` is OR'd at the high 16 bits via `<< 16`. This is correct per the Linux kernel API: `fanout_arg` is a 32-bit value where the low 16 bits are the fanout type/mode and the high 16 bits are flags. No issue.

---

## Summary

**Critical Issue (Patch 2/3):**
- **Error:** Resource leak of `qsockfd` when `setsockopt(PACKET_IGNORE_OUTGOING)` fails. The socket must be closed before jumping to the error path.

**Minor Issues:**
- **Warning (Patch 1/3):** Test code passes potentially-NULL pointer to `%s` format specifier, which is undefined behavior. Should guard with `err != NULL ? err : ""`.

**All Other Items:** Code is correct. The patches implement a reasonable feature (filtering outgoing packets) with appropriate kernel-side optimization and PMD-side fallback. Test coverage is comprehensive. Documentation and release notes are present and accurate.