|WARNING| [v5,10/10] net/iavf: fix IPsec flow metadata leaks

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review - series.patch

Reviewing 10 patches from the iavf driver against DPDK contribution guidelines.

---

## Patch 1/10: net/iavf: keep device stopped if start fails

**Correctness:**

The patch addresses a state management bug where `adapter->stopped` was cleared before operations that could fail. If start failed after that point, a subsequent stop would execute cleanup on a partially-initialized device.

The fix is correct: move the `adapter->stopped = 0` assignment to after all failable operations complete successfully, and check the flag at the top of `iavf_dev_stop()` before attempting any cleanup.

However, there is a **resource leak on the error path**:

```c
+irq_error:
+	if (!wb_on_itr && dev->data->dev_conf.intr_conf.rxq != 0)
+		rte_intr_disable(intr_handle);
+	rte_intr_efd_disable(intr_handle);
+	rte_intr_vec_list_free(intr_handle);
+	rte_free(vf->qv_map);
+	vf->qv_map = NULL;
 error:
 	return -1;
```

The `irq_error` label cleans up IRQ resources but then **falls through** to the `error` label. Any cleanup allocated before the IRQ configuration step (lines 1047-1089: queue setup, map queues, enable queues) is not unwound. Specifically:

- Queues have been started (`iavf_start_queues()` was not called yet at this point, but `iavf_switch_queue()` was)
- VF resources have been configured but not torn down

The original code had the same issue (both paths returned -1 without cleanup), but this patch adds a new error label without addressing it. Since the patch does not introduce the leak (it was pre-existing), this is acceptable, but worth noting as a remaining issue.

**Style:** Clean.

---

## Patch 2/10: net/iavf: do not store ethdev pointer

**Correctness:**

The patch removes the process-local `rte_eth_dev` pointer from `struct iavf_info` (which lives in shared memory) and replaces accesses with a lookup via `rte_eth_devices[port_id]`.

The `iavf_get_eth_dev()` helper is correct. All call sites are updated consistently. No issues found.

**Style:** Clean.

---

## Patch 3/10: net/iavf: fix false success on flow validation

**Correctness:**

The bug is clear: `iavf_parse_engine_validate()` sets an error on validation failure but returns the engine pointer anyway. The caller treats non-NULL as success.

The fix is correct: return `NULL` on failure, return the engine only after validation succeeds, and return `NULL` if no engine matched.

**Style:** Clean.

---

## Patch 4/10: net/iavf: fix memory leaks around IRQ resources

**Correctness:**

The patch identifies two leaks:

1. On `iavf_config_rx_queues_irqs()` failure, the interrupt vector list and eventfds are not freed.
2. After a normal stop, `vf->qv_map` is not freed, leaking it across start/stop cycles.

The error-path fix at line 984 is correct: on failure to allocate the vector list, jump to `qv_map_alloc_err` which now also calls `rte_intr_efd_disable()` before freeing the vector list.

The stop-path fix at lines 1163-1164 is correct: free `vf->qv_map` and NULL the pointer.

The new `irq_error` label added in `iavf_dev_start()` (lines 1125-1129) correctly cleans up IRQ resources on queue-start failure. The `wb_on_itr` variable is initialized early (line 1045) so the conditional cleanup is safe.

**However, there is an incomplete fix:**

The original `error` label in `iavf_dev_start()` is still present and reachable from earlier failures (e.g., `iavf_init_rss()` failure at line 1098). Those paths now skip the new IRQ cleanup that happens at `irq_error`, meaning the leak still exists on those earlier error paths.

**Error:** The patch does not fix the leak for all error paths. Failures after `iavf_config_rx_queues_irqs()` but before `iavf_start_queues()` (lines 1093-1110) will leak the IRQ resources. The `goto error` at line 1100 should be `goto irq_error`.

Quoting the problematic line:

```c
	if (iavf_init_rss(ad) != 0) {
		PMD_DRV_LOG(ERR, "Failed to configure RSS");
		goto error;  /* BUG: should be goto irq_error */
	}
```

**Style:** Clean.

---

## Patch 5/10: net/iavf: fix generic flow error-path leaks

**Correctness:**

The patch addresses two leaks:

1. In `iavf_flow_init()`, if an engine's `init` callback fails, parsers registered by earlier engines remain allocated. The fix adds an `err` label that calls `iavf_flow_uninit()` to unwind.

2. In `iavf_search_pattern_match_item()`, if allocation of `pattern_match_item` fails, the `items` array is leaked. The fix adds `rte_free(items)` before returning NULL.

Both fixes are correct. The unwind path is appropriate. No issues found.

**Style:** Clean.

---

## Patch 6/10: net/iavf: centralize memory management for flows

**Correctness:**

This is a large refactor that centralizes flow rule memory management in the generic flow layer. Key changes:

1. FDIR: allocate the final rule in the parser, use it for action checks, remove per-VF scratch config.
2. FSUB: adopt the parser-produced rule instead of allocating a second copy.
3. RSS: build the final `virtchnl_rss_cfg` in the parser, remove the intermediate `iavf_rss_meta` struct.
4. All engines: stop managing their own rule memory; let the generic layer free rules.

The changes to `iavf_parse_engine_create()` (lines 2099-2119) introduce a **use-after-free risk**:

```c
	RTE_TAILQ_FOREACH_SAFE(parser_node, parser_list, node, temp) {
		meta = NULL;
		if (parser_node->parser->parse_pattern_action(ad,
				parser_node->parser->array,
				parser_node->parser->array_len,
				pattern, actions, attr, &meta, error))
			continue;
		engine = parser_node->parser->engine;

		RTE_ASSERT(engine->create != NULL);
		if (engine->create(ad, flow, meta, error) == 0) {
			if (flow->rule != meta)
				rte_free(meta);  /* free if engine didn't adopt */
			return engine;
		}

		if (flow->rule != NULL && flow->rule != meta)
			rte_free(flow->rule);  /* POTENTIAL USE-AFTER-FREE */
		rte_free(meta);
		flow->rule = NULL;
		meta = NULL;
	}
```

If `engine->create()` fails and sets `flow->rule` to a value that was allocated earlier in the loop (by a previous parser), then at line 2117 the code frees `flow->rule`, which may still be pointed to by `meta` from an earlier iteration.

Actually, re-reading the code: each iteration sets `meta = NULL` at the top, and the parser allocates a new `meta` each time. So `flow->rule` can only be set by the current engine's `create` callback. If `create` fails, `flow->rule` is either NULL, equal to `meta` (engine adopted it), or something the engine allocated separately. The check `flow->rule != meta` ensures we don't double-free. This is correct.

**However, there is a potential leak in the FDIR parser** (`iavf_fdir_parse()`):

```c
	filter = rte_zmalloc("iavf_fdir_meta", sizeof(*filter), 0);
	if (filter == NULL) {
		rte_flow_error_set(error, ENOMEM,
				   RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
				   "Failed to allocate FDIR metadata.");
		return -rte_errno;
	}

	ctx.ad = ad;
	ctx.filter = filter;
	param.driver_ctx = &ctx;

	ret = ci_flow_check_attr(attr, NULL, error);
	if (ret)
		goto error;  /* OK: filter freed */

	ret = ci_flow_check_actions(actions, &param, &parsed_actions, error);
	if (ret)
		goto error;  /* OK: filter freed */

	item = iavf_search_pattern_match_item(pattern, array, array_len, error);
	if (item == NULL) {
		ret = -rte_errno;
		goto error;  /* OK: filter freed */
	}

	ret = iavf_fdir_parse_pattern(ad, pattern, item->input_set_mask,
				      error, filter);
	if (ret)
		goto error;  /* OK: filter freed, but item not freed */

	rte_free(item);
	if (meta != NULL)
		*meta = filter;
	else
		rte_free(filter);
	return ret;

error:
	rte_free(item);  /* item may be NULL */
	rte_free(filter);
	return ret;
```

At line 1591, if `iavf_fdir_parse_pattern()` fails, the code jumps to `error`, which frees `item` and `filter`. But `item` was allocated at line 1584. If `iavf_fdir_parse_pattern()` fails, `item` is leaked **only if the allocation succeeded**. Actually, looking at the error label: `rte_free(item)` is called. So the leak is **not present**. The code is correct.

Wait, let me re-check the flow:

- Line 1584: `item = iavf_search_pattern_match_item(...)` allocates `item`.
- Line 1585: if `item == NULL`, jump to `error` (line 1588). At the error label, `rte_free(item)` is called, which is safe for NULL.
- Line 1590: `ret = iavf_fdir_parse_pattern(...)`
- Line 1591: if `ret` is non-zero, jump to `error`.
- Line 1597 (error label): `rte_free(item)` is called.

So `item` **is** freed on the error path. No leak. I was mistaken.

**Correctness issues in RSS parser** (`iavf_hash_parse_raw_pattern()`):

The function writes to `rss_cfg->proto_hdrs.raw.spec`, `mask`, and `pkt_len` (lines 952-954). These fields are accessed without checking if `rss_cfg` is NULL. However, the only caller is `iavf_hash_parse_pattern_action()`, which allocates `rss_cfg` at line 1532 and returns early if allocation fails, so the NULL dereference cannot occur. The code is correct.

**RSS destroy in `iavf_hash_destroy()`** (line 1608): the function no longer checks `vf->vf_reset`. The commit message says "the generic flow layer frees rule memory without relying on engine callbacks" and "`iavf_execute_vf_cmd()` already refuses to send commands while `vf_reset` is set." However, the patch **does not add a check** to `iavf_add_del_rss_cfg()` to refuse commands during reset. Let me check if `iavf_execute_vf_cmd()` does this.

Looking at `iavf_execute_vf_cmd()` in `iavf_vchnl.c` (not in this patch, but must be in the tree): I don't have the full tree, but the commit message claims it checks `vf_reset`. If that's true, the code is correct. If not, this is a bug.

Actually, looking at patch 7/10 (preview): it modifies `iavf_dev_close()` to skip `iavf_hash_uninit()` when `pf_reset_in_progress` is true. And patch 8/10 (preview) removes the `vf_reset` checks from the RSS engine entirely, with the justification that "`iavf_execute_vf_cmd()` already refuses to send commands while `vf_reset` is set."

So the chain of reasoning is:
- Patch 6 removes the `vf_reset` check from `iavf_hash_destroy()`.
- Patch 7 skips `iavf_hash_uninit()` during PF reset.
- Patch 8 removes remaining `vf_reset` checks from RSS init/destroy/uninit.
- All rely on `iavf_execute_vf_cmd()` blocking commands during reset.

**If `iavf_execute_vf_cmd()` does not block commands during reset, this is a bug.** I cannot verify this without the full tree. However, the commit message explicitly states this is the case, so I will assume it is correct. If it is not, the bug is in the commit message and the assumption, not in the code itself.

**Style:** Clean.

---

## Patch 7/10: net/iavf: skip RSS teardown during PF-initiated reset

**Correctness:**

The patch moves the `iavf_hash_uninit()` call inside the `!pf_reset_in_progress` check, so RSS configuration messages are not sent during PF-initiated reset recovery.

This is correct. The check is consistent with the existing flow flush behavior. No issues found.

**Style:** Clean.

---

## Patch 8/10: net/iavf: remove redundant reset checks from RSS engine

**Correctness:**

The patch removes `vf_reset` checks from `iavf_hash_init()`, `iavf_hash_destroy()`, and `iavf_hash_uninit()`, relying on:

1. `iavf_hash_init()` sends no commands (only registers the parser).
2. `iavf_dev_close()` skips RSS teardown during PF reset (patch 7).
3. `iavf_execute_vf_cmd()` blocks commands during reset.

Point 1 is verifiable: `iavf_hash_init()` calls `iavf_register_parser()`, which does not send virtchnl commands. Correct.

Point 2 is added by patch 7. Correct.

Point 3 is stated but not shown. As noted above, I assume this is correct per the commit message.

The patch is correct given the stated assumptions.

**Style:** Clean.

---

## Patch 9/10: net/iavf: fix null engine for egress security flows

**Correctness:**

The patch registers a new `iavf_ipsec_egress_flow_engine` for egress IPsec flows, which previously bypassed the parser and were inserted with a NULL `engine` pointer. This caused a NULL dereference in logging (line 2270 of the old code: `flow->engine->name` when `flow->engine` was NULL).

The new egress engine:
- Parses only the action list (security action) and attribute (requires egress).
- Owns no rule (`flow->rule = NULL`).
- `create` and `destroy` are no-ops.

The pattern is ignored (egress flows don't match incoming traffic), so the parse function does not call `iavf_search_pattern_match_item()`. This is correct.

The changes to `iavf_flow_dev_dump()` (lines 2426-2435) handle flows with NULL `rule` gracefully. Correct.

The changes to `iavf_register_parser()` and `iavf_unregister_parser()` add the egress engine type to the IPsec crypto parser list. Correct.

**Style:** Clean.

---

## Patch 10/10: net/iavf: fix IPsec flow metadata leaks

**Correctness:**

The patch fixes two leaks in `iavf_ipsec_flow_parsewarning: review has no Review-Result line, severity inferred from prose (response may be truncated)