|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 01/10: net/iavf: keep device stopped if start fails

### Errors

None identified.

### Warnings

None identified.

### Info

This patch correctly addresses the state-machine issue where `adapter->stopped` was cleared before operations that could fail, allowing a subsequent `iavf_dev_stop()` to perform cleanup on a device that never fully started. The fix moves the flag update to after all fallible operations succeed and adds an early-return check in `stop` to handle devices already stopped. The approach is correct and improves robustness.

---

## PATCH 02/10: net/iavf: do not store ethdev pointer

### Errors

None identified.

### Warnings

None identified.

### Info

The patch removes the process-local `eth_dev` pointer from shared memory (`iavf_info`) and replaces it with lookups via `rte_eth_devices[port_id]`. The `iavf_get_eth_dev()` helper is safe because `port_id` is part of the shared data structure and stable across processes. Call sites are updated consistently and the change is mechanical. This correctly addresses the primary/secondary process memory model.

---

## PATCH 03/10: net/iavf: fix false success on flow validation

### Errors

None identified.

### Warnings

None identified.

### Info

The fix is correct: `iavf_parse_engine_validate()` was returning the engine pointer on both success and failure, and the caller treated non-NULL as success. The patch changes the function to return `NULL` on validation failure and return the engine only after validation succeeds. This eliminates the false-positive validation acceptance.

---

## PATCH 04/10: net/iavf: fix memory leaks around IRQ resources

### Errors

**Error 1: Missing `rte_free(vf->qv_map)` in the error path of `iavf_config_rx_queues_irqs()`**

`iavf_config_rx_queues_irqs()` allocates `vf->qv_map` at line 846:

```c
vf->qv_map = rte_zmalloc("qv_map", vf->msix_base * sizeof(struct virtchnl_vector_map), 0);
```

On the error paths added by this patch (`efd_alloc_err` and `qv_map_alloc_err`), the patch cleans up `intr_handle` resources but does not free `vf->qv_map`. If `rte_intr_efd_enable()` or `rte_intr_vec_list_alloc()` fails after the `qv_map` allocation succeeds, `qv_map` is leaked.

**Fix:** Add `rte_free(vf->qv_map); vf->qv_map = NULL;` before `return -1;` in both new error labels.

**Error 2: IRQ cleanup in `iavf_dev_start()` error path may be incorrect**

In the new `irq_error:` label, the patch adds:

```c
if (!wb_on_itr && dev->data->dev_conf.intr_conf.rxq != 0)
	rte_intr_disable(intr_handle);
```

However, earlier in the function, when `intr_conf.rxq != 0`, the code does:

```c
if (wb_on_itr)
	rte_intr_disable(intr_handle);
rte_intr_enable(intr_handle);
```

So `rte_intr_enable()` is called unconditionally when `intr_conf.rxq != 0`, but the error-path disabling is conditional on `!wb_on_itr`. This asymmetry suggests the error path may not fully reverse the setup.

**Fix:** The cleanup should mirror the enable logic: `if (dev->data->dev_conf.intr_conf.rxq != 0) rte_intr_disable(intr_handle);` without the `wb_on_itr` condition.

### Warnings

None identified.

---

## PATCH 05/10: net/iavf: fix generic flow error-path leaks

### Errors

None identified.

### Warnings

None identified.

### Info

The patch correctly unwinds parser registrations on engine init failure in `iavf_flow_init()` and frees the allocated item array when match-result allocation fails in `iavf_search_pattern_match_item()`. Both leaks are real and the fixes are correct.

---

## PATCH 06/10: net/iavf: centralize memory management for flows

### Errors

None identified.

### Warnings

None identified.

### Info

This is a substantial refactor that centralizes rule memory management in the generic flow layer. The key changes are:

- FDIR: allocates the rule in the parser, removes the per-VF scratch configuration, and adopts the parser-produced rule in `create`.
- FSUB: adopts the parser-produced rule instead of allocating a second object.
- RSS: builds the final `virtchnl_rss_cfg` in the parser and removes the intermediate `iavf_rss_meta` type.
- Generic flow: frees rules on validate, create failure, successful destroy, and uninit; removes engine-specific `free` callbacks.

The refactor eliminates the leak during PF-initiated reset (where engines without a `free` callback would leak their rules) and centralizes lifetime in one place. The mechanical changes to each engine are correct and the new ownership model is consistent.

---

## PATCH 07/10: net/iavf: skip RSS teardown during PF-initiated reset

### Errors

None identified.

### Warnings

None identified.

### Info

The patch moves `iavf_hash_uninit()` under the same `!vf->pf_reset_in_progress` check as `iavf_flow_flush()`, so that no RSS configuration messages are sent during PF-initiated reset recovery. This is correct: the PF has already torn down the configuration, so attempting to delete it via virtchnl will fail or hang.

---

## PATCH 08/10: 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()` because:

- `iavf_hash_init()` sends no commands.
- `iavf_dev_close()` skips both flow flush and RSS teardown during reset.
- `iavf_execute_vf_cmd()` already refuses to send commands when `vf_reset` is set.

The reasoning is sound and the checks are indeed redundant given the higher-level guards.

---

## PATCH 09/10: net/iavf: fix null engine for egress security flows

### Errors

None identified.

### Warnings

None identified.

### Info

The patch registers a new `IAVF_FLOW_ENGINE_IPSEC_EGRESS` engine for egress security flows, which previously bypassed the parser and were inserted with a `NULL` engine pointer. The new engine has no-op `create` and `destroy` callbacks (egress flows own no rule and require no hardware teardown) and a parser that validates the action and attribute but does not produce metadata. This correctly integrates egress flows into the normal lifecycle and fixes the NULL dereference in `iavf_flow_create()` logging.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

The patch fixes two leaks in `iavf_ipsec_flow_parse()`:

1. The pattern-match item is now freed after extracting its metadata.
2. The parsed flow item is freed when no metadata output is requested (`meta == NULL`).

The patch also propagates pattern-match failure and reports an error when no flow item was produced. All three fixes are correct.

---

## Summary

**Two errors found:**

1. **Patch 04**: Missing `rte_free(vf->qv_map)` in the error paths of `iavf_config_rx_queues_irqs()` (both `efd_alloc_err` and `qv_map_alloc_err` labels).
2. **Patch 04**: Asymmetric interrupt cleanup logic in `iavf_dev_start()` error path -- `rte_intr_disable()` should be called unconditionally when `intr_conf.rxq != 0`, without the `wb_on_itr` condition.

All other patches are correct. The series as a whole substantially improves memory management and robustness in the iavf driver.

Review-Result: ERROR