|WARNING| net/iavf: fix RSS rule overwriting an existing one

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## ERRORS

**1. Use-after-free on error path**

Line: `return -rte_errno;`

When `iavf_hash_ptype_in_use()` returns true, the code calls `rte_flow_error_set()` then returns `-rte_errno`. However, the `meta` pointer (which is `rss_cfg`) is not freed before returning. This `rss_cfg` was allocated by the caller (`iavf_hash_parse_pattern_action()`) and is leaked on this error path.

The correct pattern in this driver is to return the error and let the caller handle cleanup, OR explicitly free `meta` here. Looking at other error paths in the driver and the flow creation pattern, the caller expects to free `meta` only when the create function succeeds (it becomes `flow->rule`). On error, the create function should free it.

**Fix:** Add `rte_free(rss_cfg);` before `return -rte_errno;`

```c
if (iavf_hash_ptype_in_use(ad, &rss_cfg->proto_hdrs)) {
	PMD_DRV_LOG(ERR, "RSS rule for this packet type already exists");
	rte_flow_error_set(error, EEXIST,
			   RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
			   "RSS rule for this packet type already exists");
	rte_free(rss_cfg);
	return -rte_errno;
}
```

**2. Race condition: TOCTOU in duplicate check**

Lines: `if (iavf_hash_ptype_in_use(ad, &rss_cfg->proto_hdrs))` and `ret = iavf_add_del_rss_cfg(ad, rss_cfg, true);`

The duplicate check releases `vf->flow_ops_lock` before calling `iavf_add_del_rss_cfg()`. Another thread could create a rule with the same packet type in the window between the check and the add operation, defeating the duplicate detection.

The lock needs to be held from the duplicate check through the successful addition of the rule to `vf->flow_list`. However, examining the code structure, `iavf_add_del_rss_cfg()` performs PF communication which may sleep, and the flow is added to the list by the generic flow layer after `iavf_hash_create()` returns, not within this function.

This is a fundamental design issue: the check-then-add sequence cannot be made atomic with the current locking structure. The duplicate check can be defeated by a concurrent create operation. A proper fix would require either:
- Moving the duplicate check into the locked region where the flow is added to the list (in the caller)
- Or accepting that this is best-effort duplicate detection (document it)

Given the constraints, this is a race condition that allows duplicate rules to be created despite the check.

**Fix:** Either document that this is a best-effort check in the commit message, or redesign to hold the lock across the entire create operation (which may require refactoring to avoid sleeping under spinlock). The current implementation has a race window.

---

## WARNINGS

**1. Missing release notes**

This patch fixes a bug where multiple RSS rules with the same packet type could be created, with later rules silently overwriting earlier ones. The fix changes the API behavior by rejecting duplicate packet type rules. This is a significant behavioral change that applications need to be aware of.

**Fix:** Add a release notes entry in `doc/guides/rel_notes/release_26_11.rst` (or current release) under "Bug Fixes" documenting this behavior change.

---

## INFO

**Style observations (no changes required):**

1. The explicit comparison `cfg == NULL` is correct per DPDK style (line 1592).

2. The boolean function `iavf_hash_ptype_in_use()` correctly returns `bool` and uses `true`/`false` rather than `int` and `1`/`0`.

3. The loop comparison `i < hdrs->count` could use explicit `!= 0` check per DPDK style, but implicit comparison on unsigned counters in loop bounds is acceptable and consistent with existing code in the file.

---

Review-Result: ERROR