|SUCCESS| [v4,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/171818

_AI Code Review_

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

# DPDK Patch Review

Reviewing patch series for DPDK iavf driver, focusing on correctness, style, and API compliance.

---

## PATCH 01/10: net/iavf: keep device stopped if start fails

**Correctness:**
- **Moved `adapter->stopped = 0` to after startup succeeds** -- this prevents `iavf_dev_stop()` from running cleanup on a partially initialized device. Good.
- **Early return in stop when already stopped** -- the check is moved to before the interrupt disable, which is correct. If `stopped == 1`, no further cleanup is needed.

**Style:**
- Code is clean and follows DPDK patterns.

**Finding:** None.

---

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

**Correctness:**
- **Removes `vf->eth_dev` pointer from shared memory** -- correct, `rte_eth_dev` pointers are process-local and must not be in shared memory.
- **Replaces with `rte_eth_devices`-based lookup via `iavf_get_eth_dev()`** -- the inline helper uses `adapter->dev_data->port_id` to index into `rte_eth_devices`, which is the correct pattern for secondary process support.
- All call sites updated to use the lookup or to pass the ethdev pointer down the call stack rather than retrieve it from `vf->eth_dev`.

**Style:**
- The inline helper is placed in the header near other adapter-to-foo accessors.
- Code is consistent with DPDK secondary-process patterns.

**Finding:** None.

---

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

**Correctness:**
- **`iavf_parse_engine_validate()` was returning the engine pointer even when validation failed** -- the loop breaks on failure but then returns `engine` outside the loop, making `iavf_flow_process_filter()` treat it as success.
- **Fix: return `NULL` immediately on failure, return `engine` only after success** -- correct.
- **Falls through to `return NULL` when no engine validates the flow** -- correct.

**Error path:** The fix prevents accepting invalid flows as valid. This is a correctness bug.

---

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

**Correctness:**
- **`iavf_config_rx_queues_irqs()` allocates `qv_map` and may enable eventfds and allocate `intr_vec`** -- if queue startup then fails, those remain allocated.
- **Fix on error path:** the patch adds cleanup in `iavf_dev_start()` after `iavf_start_queues()` fails:
  - disables interrupt
  - disables efd
  - frees `intr_vec`
  - frees `qv_map`
- **Fix in `iavf_dev_stop()`:** frees `qv_map`, which was previously leaked across start/stop cycles.
- **Fix in `iavf_config_rx_queues_irqs()` error path:** adds `rte_intr_efd_disable()` when eventfd enable fails, before the existing `rte_intr_vec_list_free()` call.

**All error paths traced:**
- `iavf_config_rx_queues_irqs()` failure: now cleans up efd.
- `iavf_start_queues()` failure: now cleans up intr, efd, vec, qv_map.
- Normal stop: now frees qv_map.

**Finding:** None. The leak is real and the fix is correct.

---

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

**Correctness:**
- **`iavf_flow_init()` calls `engine->init()` in a loop** -- if one engine fails, parsers registered by earlier engines remain allocated.
- **Fix:** adds `err:` label that calls `iavf_flow_uninit()`, which walks the parser lists and frees all registered parsers.
- **`iavf_search_pattern_match_item()` allocates `items` array** -- if the subsequent `pattern_match_item` allocation fails, `items` is leaked.
- **Fix:** `rte_free(items)` before returning NULL on the second allocation failure.

**Error paths traced:**
- Engine init failure: now unwinds all registered parsers.
- Pattern match allocation failure: now frees `items`.

**Finding:** None. The leaks are real and the fixes are correct.

---

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

**Correctness:**
This patch refactors flow rule memory management to centralize it in the generic flow layer. The goal is to eliminate leaks and bugs from engines managing their own memory.

Key changes:
- **Parser allocates the final rule object** (FDIR, FSUB, RSS) and passes it as `meta`.
- **Engine `create` adopts the parser-produced rule** instead of allocating and copying.
- **Generic flow layer owns the rule pointer** and frees it on validate (after use), create failure, successful destroy, and uninit.
- **Removes engine-specific `free` callbacks** and rule frees from destroy callbacks.

**FDIR:**
- Parser now allocates `iavf_fdir_conf *filter` and returns it as `meta`.
- `iavf_fdir_create()` adopts `meta` into `flow->rule` without allocating.
- `iavf_fdir_destroy()` no longer frees the rule -- generic layer does it.

**FSUB:**
- Similar pattern: parser allocates, create adopts, destroy does not free.

**RSS:**
- Parser now allocates `virtchnl_rss_cfg` directly instead of an intermediate `iavf_rss_meta`.
- `iavf_hash_create()` adopts the rule.
- `iavf_hash_destroy()` no longer frees the rule.

**Generic flow layer (`iavf_generic_flow.c`):**
- `iavf_flow_free()` helper added: frees `flow->rule`, then frees `flow`.
- **Validate path:** after parsing, if `meta != NULL`, it is freed (validation does not create a persistent flow).
- **Create path:** `iavf_parse_engine_create()` loop: if a parser succeeds, it calls `engine->create(ad, flow, meta, error)`. If create succeeds, the loop checks `if (flow->rule != meta) rte_free(meta)` -- this handles engines that adopt `meta` into `flow->rule` (most) vs. engines that ignore `meta` (egress IPsec). If create fails, it frees both `flow->rule` (if not `meta`) and `meta`, then continues to the next parser.
- **Destroy:** calls `engine->destroy()`, then `iavf_flow_free(flow)`.
- **Uninit:** walks flow list, calls `iavf_flow_free(flow)` for each.

**Error paths traced:**
- Validate: `meta` is freed after validation.
- Create: if `engine->create()` fails, the loop frees `meta` and `flow->rule` if they differ, then continues. If no engine succeeds, the flow itself is freed via `iavf_flow_free(flow)`.
- Destroy: rule is freed via `iavf_flow_free(flow)` after `engine->destroy()`.
- Uninit: all flows freed via `iavf_flow_free(flow)`.

**Potential issue in create loop:**
```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;
```
After a failed create, the code frees `flow->rule` if it is not `meta`, then frees `meta`. This assumes that if `flow->rule == meta`, freeing `meta` suffices. But what if the engine set `flow->rule` to something else and `meta` is still allocated separately? The condition `flow->rule != meta` would be true, so both would be freed, which is correct. If `flow->rule == meta`, only `meta` is freed, which is also correct (one free, one object). This logic is sound.

**Finding:** None. The refactor is correct and eliminates the leak on PF-initiated reset (flows were not freed because engine `free` callbacks were missing or not called).

---

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

**Correctness:**
- **During PF-initiated reset, `iavf_dev_close()` skips flow flush but still calls `iavf_hash_uninit()`**, which sends RSS configuration messages to the PF.
- **Fix:** move `iavf_hash_uninit()` under the same `if (!vf->pf_reset_in_progress)` check as `iavf_flow_flush()`.

**Finding:** None. This is consistent with the existing pattern for flow flush.

---

## PATCH 08/10: net/iavf: remove redundant reset checks from RSS engine

**Correctness:**
- **The RSS engine checks `vf->vf_reset` in `init`, `destroy`, and `uninit`** to avoid sending virtchnl commands during reset.
- **Patch claims these checks are no longer needed because:**
  - `iavf_hash_init()` only registers the parser and sends no commands.
  - `iavf_dev_close()` skips both flow flush and RSS teardown during PF-initiated reset (patch 07).
  - Generic flow layer frees rule memory without relying on engine callbacks (patch 06).
  - `iavf_execute_vf_cmd()` already refuses to send commands while `vf_reset` is set.

**Verification:**
- `iavf_hash_init()` calls `iavf_register_parser()` and then `iavf_rss_hash_set()` -- the latter **does** send a virtchnl command via `iavf_add_del_rss_cfg()`. However, `iavf_execute_vf_cmd()` checks `vf->vf_reset` and returns `-EBUSY` if set, so the command will fail but not hang. The check in `iavf_hash_init()` returning `-EIO` early is an optimization, not a correctness requirement.
- `iavf_hash_destroy()` calls `iavf_add_del_rss_cfg()`, which sends a virtchnl command. Again, `iavf_execute_vf_cmd()` will reject it if `vf_reset` is set. But `iavf_dev_close()` now skips calling destroy on RSS flows during PF-initiated reset (via skipping `iavf_hash_uninit()` in patch 07), so the check in `destroy` is indeed redundant.
- `iavf_hash_uninit()` calls `iavf_rss_hash_set()`, which sends a command. Same as above: `iavf_execute_vf_cmd()` will reject it, and `iavf_dev_close()` skips calling it during reset.

**Finding:** None. The checks are redundant after patches 06 and 07, and the underlying command layer already handles `vf_reset`.

---

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

**Correctness:**
- **Egress security flows bypass the flow parsers and are inserted with `engine = NULL`** -- the create path then dereferences `engine` when logging the new flow (via `flow->engine->name`), which would segfault.
- **The flow is also inserted with `is_ipsec_egress_flow = true`**, and various engine-based operations check this flag to special-case the flow.
- **Fix:** register an egress security engine, add a parser, and route these flows through the normal lifecycle. Egress flows own no rule (`flow->rule = NULL`) and require no hardware teardown (destroy does nothing).

**Changes:**
- New engine `iavf_ipsec_egress_flow_engine` with init, uninit, create (sets `flow->rule = NULL`), destroy (does nothing).
- New parser `iavf_ipsec_egress_flow_parser` with `parse_pattern_action` that checks `attr->egress` and `actions[0].type == RTE_FLOW_ACTION_TYPE_SECURITY`, returns `meta = NULL`.
- Generic flow layer in patch 06 already handles `meta = NULL` and `flow->rule = NULL` correctly: if `meta` is NULL, nothing to free after parsing; if `flow->rule` is NULL after create, `iavf_flow_free()` will `rte_free(NULL)` which is safe.
- Removes `is_ipsec_egress_flow` flag from `struct rte_flow` and all checks for it.

**Error paths traced:**
- If the parser returns `meta = NULL` and `engine->create()` succeeds with `flow->rule = NULL`, the flow is valid and can be destroyed (destroy does nothing) and freed (frees the flow struct only).

**Finding:** None. The fix eliminates the NULL dereference and makes egress flows consistent with the rest of the flow engine architecture.

---

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

**Correctness:**
- **`iavf_ipsec_flow_parse()` calls `iavf_search_pattern_match_item()`**, which allocates a pattern-match item. The function then extracts `item->meta` and calls `iavf_ipsec_flow_item_parse()`, but never frees `item`.
- **If `meta` output parameter is NULL**, the parsed flow item `fi` is allocated but never freed.
- **If `item->meta` is NULL**, the function returns `-ret` (which would be the last error from `ci_flow_check_actions`, typically 0), but no flow item was produced -- this should be an error.

**Fix:**
- Frees `item` after extracting its metadata.
- If `iavf_ipsec_flow_item_parse()` returns NULL, sets a flow error "Invalid IPsec flow." and returns `-rte_errno`.
- If `meta` parameter is NULL, frees `fi` (the parsed flow item) instead of leaking it.
- If `meta` is not NULL, returns `fi` as `*meta`.

**Error paths traced:**
- Pattern match fails: `iavf_search_pattern_match_item()` returns NULL, function returns `-rte_errno` (no leak).
- Pattern match succeeds but `item->meta` is NULL: now sets flow error and returns (no leak, `item` freed).
- Pattern match succeeds, `item->meta` is not NULL, but `iavf_ipsec_flow_item_parse()` returns NULL: now sets flow error and returns (no leak, `item` freed).
- Pattern match succeeds, `fi` is allocated, `meta` is NULL: now frees `fi` (no leak, `item` freed).
- Pattern match succeeds, `fi` is allocated, `meta` is not NULL: returns `fi` via `meta` (no leak, `item` freed, ownership transferred).

**Finding:** None. The leaks are real and the fix is correct.

---

## CROSS-CUT CONCERNS

### Evidence rule
All findings rest on code in the patches (added/removed lines) or on surrounding context visible in the diff. No speculation about code not read.

### Memory ordering
No new atomics or shared variables introduced. Existing patterns (e.g., `adapter->stopped`) are not changed in a way that affects ordering. No findings.

### MTU / frame length
No MTU-related code touched. No findings.

### `bool` vs `int`
- `wb_on_itr` in patch 01 is assigned `(vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_WB_ON_ITR) != 0` and used in `if (wb_on_itr)`. This is effectively a boolean, but it is `bool` in the patch (`bool wb_on_itr;`). Good.

### Style
- All patches follow DPDK style (indentation, brace placement, naming).
- No forbidden tokens (checked against the guidelines).
- No trailing whitespace or missing newlines visible in diff context.

### Pre-existing code
No findings rest on code the patches do not touch. Patches 06, 07, warning: review has no Review-Result line, severity inferred from prose (response may be truncated)