|WARNING| net/txgbe: keep flow rules per port

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-11-08

# DPDK Patch Review

## Errors

**1. Resource leak on error path in `txgbe_flow_create()`**

Line 3491 (error path):
```c
out:
	TAILQ_REMOVE(&adapter->flow_list, txgbe_flow_mem_ptr, entries);
	rte_flow_error_set(error, -ret,
			   RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
			   "Failed to create flow.");
```

The error path removes `txgbe_flow_mem_ptr` from the list and sets an error, but does not free either `txgbe_flow_mem_ptr` or `flow`. Both were allocated earlier (lines 3155-3164) and must be freed here to avoid a memory leak.

**Suggested fix:**
```c
out:
	TAILQ_REMOVE(&adapter->flow_list, txgbe_flow_mem_ptr, entries);
	rte_free(txgbe_flow_mem_ptr);
	rte_free(flow);
	rte_flow_error_set(error, -ret,
			   RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
			   "Failed to create flow.");
```

---

**2. Error path after `fdir_out:` may leak `fdir_rule_ptr`**

Line 3406 (fdir_out label):
```c
fdir_out:
	if (!fdir_configured && first_mask) {
		fdir_conf->mask = fdir_rule.mask;
		fdir_conf->mode = RTE_FDIR_MODE_NONE;
	}
```

If the code reaches `fdir_out` after allocating `fdir_rule_ptr` (line 3332) but before successful insertion into the flow, `fdir_rule_ptr` is not freed. The allocation occurs at:

```c
fdir_rule_ptr = rte_zmalloc("txgbe_fdir_filter",
		sizeof(struct txgbe_fdir_rule_ele), 0);
```

then if `txgbe_fdir_filter_program()` fails (line 3341 or 3347), the code jumps to `fdir_out:`, leaving `fdir_rule_ptr` allocated but never freed or assigned to the flow.

**Suggested fix:**
Add cleanup before the label:
```c
fdir_out:
	if (fdir_rule_ptr != NULL)
		rte_free(fdir_rule_ptr);
	if (!fdir_configured && first_mask) {
```

---

**3. Inconsistent `n_flows` decrement bounds checking**

Line 3639:
```c
		if (fdir_info->n_flows > 0)
			fdir_info->n_flows--;
```

This checks `n_flows > 0` before decrementing, which is defensive. However, line 3640 immediately tests `n_flows == 0` to decide whether to clear the FDIR state. If `n_flows` was already 0 when this path is reached (due to a bug elsewhere or double-destroy), the counter stays at 0 but the state is never cleared.

The correct logic should decrement unconditionally (trusting that the flow was counted when created) and then check if it reached zero. Alternatively, if paranoia is warranted, log an error if `n_flows` is already zero.

**Suggested fix:**
```c
		if (fdir_info->n_flows > 0) {
			fdir_info->n_flows--;
		} else {
			PMD_DRV_LOG(ERR, "FDIR flow count underflow");
		}
		if (fdir_info->n_flows == 0) {
```

---

## Warnings

**1. Missing release notes entry**

This patch fixes a significant bug (flow rules on one port being freed when another port is closed) and changes internal data structure layout. The fix should be documented in the current release notes under the "Fixed Issues" section for the txgbe PMD.

**Suggested action:** Add a release notes entry in `doc/guides/rel_notes/release_XX_YY.rst` (where XX_YY is the current release) describing the fix.

---

**2. Potential double-free risk in `txgbe_flow_destroy()` if called twice**

Lines 3587-3610: After successfully destroying a flow and freeing `pmd_flow->rule`, the code frees `flow` itself (line 3611). However, if the application calls `rte_flow_destroy()` twice with the same handle (a usage error), the second call will access freed memory when it dereferences `pmd_flow->filter_type` (line 3516) before the port-membership check.

While this is an application bug, the new port-membership check (lines 3607-3615) protects against cross-port destroys but does not protect against double-destroy on the same port. Consider setting `pmd_flow->rule` to NULL after freeing it, or documenting that double-destroy is undefined behavior.

**Suggested fix (optional hardening):**
After destroying the rule-specific structure, set `pmd_flow->rule = NULL` so a second destroy attempt can be detected. Alternatively, document that reusing a destroyed flow handle is undefined behavior.

---

## Info

**1. Code simplification opportunity in `txgbe_filterlist_flush()`**

Lines 3107-3114: The function now only needs to free the `txgbe_flow_mem` list and reset `n_flows`. The individual filter type lists no longer exist. The remaining code is concise and correct. No change needed, but the function could be renamed to `txgbe_flow_list_flush()` in a future refactor to better reflect its new purpose.

---

## Summary

The patch successfully refactors flow rule storage from file-scope lists to per-port lists, fixing the cross-port interference bug described in the commit message. The primary concerns are:
- Error path resource leaks in `txgbe_flow_create()` (two allocations not freed on error)
- Potential `fdir_rule_ptr` leak if `txgbe_fdir_filter_program()` fails
- Minor inconsistency in `n_flows` decrement checking

These must be addressed before merge.

Review-Result: ERROR