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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

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

**Errors:**

None identified.

**Warnings:**

None identified.

**Info:**

This patch correctly moves the `adapter->stopped = 0` assignment to after all startup operations that can fail, ensuring the adapter is not marked running if startup is incomplete. The early-return check in `iavf_dev_stop()` is placed after checking `adapter->closed` but before any operations that should not execute on a stopped device.

---

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

**Errors:**

None identified.

**Warnings:**

None identified.

**Info:**

The patch correctly removes the process-local `eth_dev` pointer from `iavf_info` (which is in shared memory) and replaces it with `rte_eth_devices`-based lookup via `iavf_get_eth_dev()`. All call sites that dereference the old pointer are updated to use the new lookup or to receive the ethdev pointer as a parameter.

---

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

**Errors:**

None identified.

**Warnings:**

None identified.

**Info:**

The fix correctly returns `NULL` on validation failure instead of returning the engine pointer, which was causing `rte_flow_validate()` to accept invalid flows. The logic now only returns the engine after validation succeeds.

---

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

**Errors:**

None identified.

**Warnings:**

None identified.

**Info:**

The patch correctly releases IRQ resources on failed startup:
- `rte_intr_efd_disable()` is called on the allocation error path
- `rte_intr_vec_list_free()` is called on startup failure after IRQ configuration
- The queue-vector map is freed on both error and normal stop

The cleanup order on the `irq_error:` label is correct: disable interrupts if they were enabled (checking the WB_ON_ITR flag), then free the eventfd, vector list, and queue-vector map.

---

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

**Errors:**

None identified.

**Warnings:**

None identified.

**Info:**

The patch correctly unwinds registered parsers on `iavf_flow_init()` failure and frees the pattern-match item array on match-result allocation failure. The cleanup path `iavf_flow_uninit()` is called on error, which will free all parsers registered up to the point of failure.

---

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

**Errors:**

None identified.

**Warnings:**

None identified.

**Info:**

This is a large refactor that centralizes rule memory management in the generic flow layer:
- FDIR now allocates its final rule in the parser; the per-VF scratch configuration is removed
- FSUB adopts the parser-produced rule instead of allocating a copy
- RSS builds the final configuration in the parser; the intermediate `iavf_rss_meta` structure and its unused `raw_ena` field are removed
- Engine `free` callbacks are removed; the generic flow layer frees rules on validate, create failure, destroy, and uninit
- The FDIR parser context is created to pass both the adapter and the filter to the action-check callback

The patch correctly handles the case where `flow->rule != meta` in `iavf_parse_engine_create()`: if the engine's create callback succeeds but did not adopt the metadata (i.e., `flow->rule` is something else), the metadata is freed. On error, both `flow->rule` and `meta` are freed if they differ.

RSS destroy no longer checks `vf->vf_reset` before calling `iavf_add_del_rss_cfg()` because the virtchnl layer (`iavf_execute_vf_cmd()`) already refuses to send commands during reset.

---

## Patch 7: 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 `if (!vf->pf_reset_in_progress)` block, consistent with the existing `iavf_flow_flush()` check. This prevents sending RSS configuration virtchnl messages during PF-initiated reset recovery.

---

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

**Errors:**

None identified.

**Warnings:**

None identified.

**Info:**

The patch removes `vf_reset` checks from `iavf_hash_init()`, `iavf_hash_destroy()`, and `iavf_hash_uninit()`:
- `iavf_hash_init()` only registers a parser and sends no commands, so the check is unnecessary
- `iavf_dev_close()` now skips both flow flush and RSS teardown during PF-initiated reset (patch 7), so the destroy check is redundant
- The virtchnl layer already refuses to send commands during reset, so the uninit check is also redundant

---

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

**Errors:**

None identified.

**Warnings:**

None identified.

**Info:**

The patch registers a separate engine for egress security flows, which previously bypassed the parser and had a NULL `engine` pointer. The new `iavf_ipsec_egress_flow_engine` provides minimal create and destroy callbacks (both no-ops, as these flows own no hardware state), and a parser that validates the action and attribute. The `is_ipsec_egress_flow` boolean is removed from `struct rte_flow`.

The `iavf_flow_is_valid()` check now only needs to verify that `flow->engine` is in the registered engine list. The destroy path no longer special-cases these flows, and the dump path handles `rule == NULL` safely.

---

## Patch 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 in `iavf_ipsec_flow_parse()`. It also frees the parsed flow item when `meta` is NULL (i.e., the caller is validating and does not need the parsed rule). When `item->meta` is NULL or `iavf_ipsec_flow_item_parse()` returns NULL, the function now returns an error instead of silently succeeding.

---

## Final Check

All three passes completed:

1. **Correctness bugs**: All identified leaks and state management issues are correctly fixed. Error paths release all allocated resources in the correct order.

2. **Style/process false positives**: None present. All items are actual findings or informational notes about correct changes.

3. **Evidence**: Every finding is supported by quoted code or a description of the code path analyzed. No speculation about unseen code.

---

Review-Result: CLEAN