|WARNING| [v3,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/171743

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

None identified.

### Warnings

**Missing Cc: stable at dpdk.org**
The Fixes tag points to commit `69dd4c3d0898` which is old enough to warrant backporting. The patch includes `Cc: stable at dpdk.org` in the commit message, so this is correct.

### Info

None.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

The patch correctly addresses storing `rte_eth_dev` pointers in shared memory by replacing the stored pointer with a lookup function `iavf_get_eth_dev()`. The lookup uses `dev_data->port_id` to index into the process-local `rte_eth_devices` array, which is the correct pattern for primary/secondary process architectures.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

The fix correctly addresses the logic error where validation failure was not propagating a NULL return. The pattern now returns NULL on failure and returns the engine pointer only after validation succeeds.

---

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

### Errors

**Missing error-path cleanup after `iavf_config_rx_queues_irqs()` failure**

Line 1094 in `iavf_dev_start()`:
```c
if (iavf_start_queues(dev) != 0) {
    PMD_DRV_LOG(ERR, "enable queues failed");
    goto irq_error;
}
```

The `irq_error` label (added at line 1126) cleans up IRQ resources allocated by `iavf_config_rx_queues_irqs()`, but the earlier failure path at line 1086 does not:

```c
if (iavf_config_rx_queues_irqs(dev, intr_handle) != 0) {
    PMD_DRV_LOG(ERR, "Failed to configure irqs");
    goto error;  /* BUG: should goto irq_error or inline cleanup */
}
```

When `iavf_config_rx_queues_irqs()` succeeds but something before `iavf_start_queues()` fails (lines 1088-1107), IRQ resources are leaked because the code jumps to the `error` label at line 1133, which does not clean them up. Only failures after line 1112 correctly reach `irq_error` at line 1126.

**Suggested fix**: Either move the `irq_error` label's cleanup above all failure paths that follow `iavf_config_rx_queues_irqs()`, or make the `goto error;` at line 1089 jump to `irq_error` instead.

### Warnings

None identified.

### Info

The patch correctly addresses leaks of the queue-vector map and interrupt resources on normal stop and failed startup. The addition of `rte_intr_efd_disable()` in the error path is correct.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

The patch correctly unwinds registered parsers when engine initialization fails and frees the item array when match-result allocation fails. The error handling is now complete.

---

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

### Errors

**Use-after-free in `iavf_parse_engine_create()` error path**

Lines 2110-2118 in `iavf_generic_flow.c`:
```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;
```

If `engine->create()` fails and has set `flow->rule` to point to `meta`, the code then frees both `flow->rule` and `meta` (line 2115-2117). Since they point to the same allocation, this is a double-free. The conditional at line 2114 checks `flow->rule != meta`, but if they are equal, `flow->rule` is left dangling after `rte_free(meta)` at line 2117.

**Suggested fix**: After `rte_free(meta)` at line 2117, also set `flow->rule = NULL` when `flow->rule == meta`, or restructure the cleanup to avoid the double-free scenario.

### Warnings

None identified.

### Info

The patch makes substantial changes to centralize rule lifetime management. The core idea of having parsers allocate the final rule object and letting generic flow code manage its lifetime is sound. The FDIR, FSUB, and RSS engines are correctly updated to not manage their own rule memory.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

The patch correctly moves `iavf_hash_uninit()` inside the `!vf->pf_reset_in_progress` check to match the existing pattern for flow flush. This prevents sending RSS configuration commands during PF-initiated reset recovery.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

The removal of `vf_reset` checks is justified: the generic flow layer now handles reset conditions, and the earlier patches ensure RSS teardown is skipped during PF-initiated reset. The `iavf_execute_vf_cmd()` check provides the final safety net.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

The patch correctly addresses the NULL `engine` pointer issue for egress security flows by registering a proper engine and routing these flows through normal parser and lifecycle callbacks. The no-op `create` and `destroy` callbacks are appropriate since egress flows own no hardware state.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

The patch correctly frees the pattern-match item after extracting its metadata and frees the parsed flow item when no metadata output is requested. The error handling is improved with proper propagation of pattern-match failure.

---

## Final Summary

One error was identified in patch 4/10 where an error path following `iavf_config_rx_queues_irqs()` does not clean up IRQ resources. One error was identified in patch 6/10 where a potential double-free or use-after-free exists in the `iavf_parse_engine_create()` error path.

Review-Result: ERROR