|WARNING| net/af_packet: add capture direction option

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: net/af_packet: add capture direction option

## Summary

This patch adds a `capture_dir` option to the AF_PACKET PMD to control packet filtering (ingress, egress, or both). The implementation is mostly correct but has one **Error**-level correctness bug and several style/documentation issues.

---

## Errors

### 1. Resource leak on early return paths in `eth_af_packet_rx()`

**File:** `drivers/net/af_packet/rte_eth_af_packet.c`  
**Lines:** 203-212

The `goto release_frame` paths skip allocation and jump directly to the frame release code. However, when VLAN stripping or timestamping is enabled, the existing frame release path expects an `mbuf` variable to be initialized. The label `release_frame` appears later in the function at a point where `mbuf` has been allocated. If `goto release_frame` is reached before `mbuf = rte_pktmbuf_alloc()`, and the code path attempts to free or manipulate `mbuf`, this is a use-after-free or null dereference.

**Review of the existing code context:** The original `eth_af_packet_rx()` function allocates `mbuf` before any `goto release_frame`. The new capture_dir check is placed **before** the allocation, so the `goto release_frame` jumps to cleanup code that may reference an uninitialized `mbuf`.

**Suggested fix:**

Move the capture_dir filtering to after the mbuf allocation, or ensure the `release_frame` label does not access `mbuf` when it is NULL. Preferred approach: move the filter check after allocation.

```c
/* allocate the next mbuf */
mbuf = rte_pktmbuf_alloc(pkt_q->mb_pool);
if (unlikely(mbuf == NULL)) {
	/* ... */
}

/* drop frames that do not match capture_dir */
if (pkt_q->capture_dir != RTE_AF_PACKET_CAPTURE_DIR_INOUT) {
	sll = (struct sockaddr_ll *)((char *)ppd + TPACKET_ALIGN(sizeof(*ppd)));
	if (sll->sll_pkttype == PACKET_OUTGOING) {
		if (pkt_q->capture_dir == RTE_AF_PACKET_CAPTURE_DIR_IN) {
			rte_pktmbuf_free(mbuf);
			goto release_frame;
		}
	} else {
		if (pkt_q->capture_dir == RTE_AF_PACKET_CAPTURE_DIR_OUT) {
			rte_pktmbuf_free(mbuf);
			goto release_frame;
		}
	}
}
```

---

## Warnings

### 1. Test leaks resources on early failure

**File:** `app/test/test_pmd_af_packet.c`  
**Lines:** 933-990

The test function `test_af_packet_capture_dir()` allocates TX port and RX ports, then has multiple early error paths (`goto out`). If an error occurs after `create_af_packet_port()` but before the port is added to the cleanup list, the port is not freed. The test attempts to track `n_rx` to limit cleanup, but if port creation succeeds and configuration fails, `n_rx` is incremented and cleanup will try to stop/close a port that was never successfully configured.

**Suggested fix:**

Initialize all port IDs to `RTE_MAX_ETHPORTS` (invalid). Only clean up ports whose IDs are valid. Set port ID to invalid after cleanup.

```c
for (m = 0; m < RTE_DIM(modes); m++)
	rx_ports[m] = RTE_MAX_ETHPORTS;
tx_port = RTE_MAX_ETHPORTS;

/* ... */

out:
	for (i = 0; i < RTE_DIM(modes); i++) {
		if (rx_ports[i] < RTE_MAX_ETHPORTS) {
			rte_eth_dev_stop(rx_ports[i]);
			rte_eth_dev_close(rx_ports[i]);
			rte_vdev_uninit(names[i]);
		}
	}
	if (tx_port < RTE_MAX_ETHPORTS) {
		rte_eth_dev_stop(tx_port);
		rte_eth_dev_close(tx_port);
		rte_vdev_uninit("net_af_packet_cap_tx");
	}
```

### 2. Test does not free allocated mbufs on failure

**File:** `app/test/test_pmd_af_packet.c`  
**Lines:** 933-990

When `alloc_tx_mbufs()` succeeds but TX fails or the test exits early, the allocated mbufs are not freed. After the `alloc_tx_mbufs(bufs, 4)` call, if an error path is taken, `bufs` array may contain allocated mbufs that are never transmitted or freed.

**Suggested fix:**

Track whether mbufs are allocated. Free them on error paths:

```c
if (allocated > 0 && nb_tx < allocated) {
	for (i = nb_tx; i < allocated; i++)
		rte_pktmbuf_free(bufs[i]);
}
```

### 3. Missing Doxygen comment for new enum

**File:** `drivers/net/af_packet/rte_eth_af_packet.c`  
**Lines:** 42-52

The new `enum rte_af_packet_capture_dir` is internal to the driver (not in a header), but DPDK style encourages documentation even for internal types when they represent user-visible options.

**Suggested fix:**

Add a comment block above the enum explaining what each value means.

```c
/**
 * Packet capture direction for AF_PACKET PMD.
 * Controls which packets are received on an AF_PACKET socket.
 */
enum rte_af_packet_capture_dir {
	RTE_AF_PACKET_CAPTURE_DIR_INVALID = -1,
	RTE_AF_PACKET_CAPTURE_DIR_IN,      /**< Ingress only */
	RTE_AF_PACKET_CAPTURE_DIR_OUT,     /**< Egress only */
	RTE_AF_PACKET_CAPTURE_DIR_INOUT,   /**< Both directions */
};
```

---

## Info

### 1. Implicit NULL comparison in `get_capture_dir()`

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

```c
if (!capture_dir)
```

DPDK style requires explicit comparison:

```c
if (capture_dir == NULL)
```

### 2. Duplicate or missing blank line

**File:** `drivers/net/af_packet/rte_eth_af_packet.c`  
**Lines:** 889-891

There is no blank line between function `get_fanout()` and `get_capture_dir()`. DPDK style requires a blank line between function definitions.

**Suggested fix:**

```c
}

static enum rte_af_packet_capture_dir
```

### 3. Type size mismatch for enum storage

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

```c
uint8_t capture_dir;
```

The enum `rte_af_packet_capture_dir` may be signed (contains `-1`), but it is stored in a `uint8_t`. This works but is semantically inconsistent.

**Suggested fix:**

```c
enum rte_af_packet_capture_dir capture_dir;
```

Or, if memory layout is critical:

```c
int8_t capture_dir;
```

### 4. Test could assert more specific expected counts

**File:** `app/test/test_pmd_af_packet.c`  
**Lines:** 992-994

The test asserts `rx[1] > 0` and `rx[2] > 0`, but `nb_tx` is known. Checking `rx[1] == nb_tx` would catch partial reception bugs.

**Suggested fix:**

```c
TEST_ASSERT(rx[1] == nb_tx, "Expected all %u packets with capture_dir=out, got %u", nb_tx, rx[1]);
TEST_ASSERT(rx[2] == nb_tx, "Expected all %u packets with capture_dir=inout, got %u", nb_tx, rx[2]);
```

---

## Positive Notes

- The feature is well-motivated and matches libpcap behavior.
- The test coverage is comprehensive (all three modes plus invalid input).
- Documentation is updated (user guide, release notes, param string).
- The approach (checking `sll_pkttype`) is correct per libpcap's `linux_check_direction()`.
- Release notes are properly placed in the current release file.

---

## Recommendations

1. **Fix the Error**: Move the capture_dir check after mbuf allocation to avoid potential use-after-free or null dereference.
2. **Fix test cleanup**: Ensure ports are only cleaned up if successfully created, and free allocated mbufs on error paths.
3. **Apply style fixes**: Explicit NULL comparison, blank line between functions, consistent enum storage type.