|WARNING| [v2,9/9] net/iavf: fix IPsec flow metadata leaks

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## PATCH 1/9: net/iavf: keep device stopped if start fails

### Errors

**Resource leak on queue startup failure:**
The patch moves `adapter->stopped = 0` after `iavf_start_queues()`, but the error path at line 1115 (`goto error;`) does not clean up the IRQ resources allocated by `iavf_config_rx_queues_irqs()` just before it. These resources include:
- Queue-vector map (`vf->qv_map`)
- Interrupt eventfds (enabled via `rte_intr_efd_enable()`)
- Interrupt vector list (allocated via `rte_intr_vec_list_alloc()`)

The error path should mirror the cleanup added in patch 5/9, which was written to fix the same leak for a different failure scenario.

```c
/* At line 1115 after iavf_start_queues() fails */
goto error;

/* The 'error:' label at line 1129 just does 'return -1;' */
/* Should be: */
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;
```

## PATCH 2/9: net/iavf: do not store ethdev pointer

No errors or warnings identified. The change correctly replaces process-local `rte_eth_dev` pointers with port-ID-based lookups.

## PATCH 3/9: net/iavf: fix false success on flow validation

No errors or warnings identified. The fix correctly returns NULL on validation failure instead of returning the engine pointer that caused the false positive.

## PATCH 4/9: net/iavf: fix memory leak in raw pattern parsing

No errors or warnings identified. The fix correctly frees `pkt_buf` when `msk_buf` allocation fails.

## PATCH 5/9: net/iavf: fix memory leaks around IRQ resources

### Errors

**Incorrect variable used in conditional at line 1127:**
The patch introduces `bool wb_on_itr` to cache the capability check, but then uses it inconsistently. At line 1097, the code checks:
```c
if (dev->data->dev_conf.intr_conf.rxq != 0) {
	if (wb_on_itr)
		rte_intr_disable(intr_handle);
	rte_intr_enable(intr_handle);
}
```
This checks `wb_on_itr` to decide whether to disable interrupts before enabling them.

At line 1127 in the new error path, the code does:
```c
if (!wb_on_itr && dev->data->dev_conf.intr_conf.rxq != 0)
	rte_intr_disable(intr_handle);
```
This uses `!wb_on_itr` (inverted), which creates asymmetry. Looking at the original code flow, interrupts are disabled at line 1142 in `iavf_dev_stop()` with:
```c
if (!(vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_WB_ON_ITR) &&
    dev->data->dev_conf.intr_conf.rxq != 0)
	rte_intr_disable(intr_handle);
```
This matches the new error-path condition (capability NOT set), so the logic appears correct. However, the naming is confusing: the variable is called `wb_on_itr` (writeback-on-ITR support is enabled), but the error path checks `!wb_on_itr`. The relationship is:
- WB_ON_ITR supported - interrupts already disabled (via line 1097's conditional disable)
- WB_ON_ITR NOT supported - interrupts enabled at 1099, must be disabled on error

The code is correct but the naming is misleading. No error, but flagging as a clarity issue in case it causes confusion during backport review.

## PATCH 6/9: net/iavf: fix generic flow error-path leaks

No errors or warnings identified. The fix correctly unwinds registered parsers on initialization failure and frees the item array on match-result allocation failure.

## PATCH 7/9: net/iavf: centralize memory management for flows

### Errors

**Use-after-free in iavf_fdir_parse:**
At line 1590, the code path for successful parse with `meta == NULL` does:
```c
if (meta != NULL)
	*meta = filter;
else
	rte_free(filter);
return ret;
```
This frees `filter` when the caller did not request metadata output. However, line 1594 then does:
```c
error:
	rte_free(item);
	rte_free(filter);
	return ret;
```
The error path *always* frees `filter`, even after the successful path already freed it when `meta == NULL`. On success with no metadata output, `filter` is freed twice: once at line 1592, then again at line 1595.

The correct structure is:
```c
	rte_free(item);
	if (meta != NULL)
		*meta = filter;
	else
		rte_free(filter);
	return ret;

error:
	rte_free(item);
	rte_free(filter);
	return ret;
```
Success and error paths should not share the `rte_free(filter)` call.

**Missing null check in iavf_hash_parse_pattern_action:**
At line 1532, the code allocates `rss_cfg`:
```c
rss_cfg = rte_zmalloc("iavf_rss_rule", sizeof(*rss_cfg), 0);
if (rss_cfg == NULL) {
	rte_flow_error_set(error, ENOMEM, ...);
	return -rte_errno;
}
```
If this allocation fails, the function returns -rte_errno immediately. However, earlier allocations and parsing may have left resources allocated. Specifically:
- `ci_flow_check_actions()` at line 1529 may have allocated internal state in `parsed_actions`
- `iavf_hash_parse_pattern()` at line 1536 is not yet called, so no pattern resources are leaked

Looking at `ci_flow_check_actions()` in common code, it does not appear to allocate resources that require cleanup on failure. The current early return is acceptable. No error.

**Inconsistent metadata freeing in generic_flow.c:**
At lines 2110-2119 in `iavf_parse_engine_create()`:
```c
if (engine->create(ad, flow, meta, error) == 0) {
	if (flow->rule != meta)
		rte_free(meta);
	return engine;
}

if (flow->rule != NULL && flow->rule != meta)
	rte_free(flow->rule);
rte_free(meta);
flow->rule = NULL;
meta = NULL;
```
The success path (line 2111) frees `meta` if `flow->rule != meta`, meaning the engine allocated a separate rule object. The error path (lines 2114-2116) frees both `flow->rule` (if it's not `meta`) and `meta` itself.

This appears correct: engines either adopt `meta` as `flow->rule` (no free needed) or allocate a new rule (free `meta`). On error, both are freed. No issue.

## PATCH 8/9: net/iavf: fix null engine for egress security flows

No errors or warnings identified. The patch correctly registers an egress security engine and routes these flows through normal parser and lifecycle callbacks.

## PATCH 9/9: net/iavf: fix IPsec flow metadata leaks

No errors or warnings identified. The fix correctly frees the pattern-match item after parsing and frees the parsed flow item when metadata cannot be returned.

---

## Summary

**PATCH 1/9:**
- **Error:** Queue startup failure leaks IRQ resources (queue-vector map, eventfds, interrupt vector list). The error path at line 1115 should jump to cleanup code that mirrors the cleanup in patch 5/9, not directly to `return -1`.

**PATCH 7/9:**
- **Error:** Use-after-free in `iavf_fdir_parse()`. When `meta == NULL`, `filter` is freed at line 1592, then freed again in the error-path fallthrough at line 1595. The success path must `return ret` before falling into the error label, or the error label must not unconditionally free `filter`.

Review-Result: ERROR