|WARNING| [v5,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/171886

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

**Correctness bug - partial state on error path:**

Line 1116: `iavf_phc_sync_alarm_start(dev);` is called before setting `adapter->stopped = 0`, but the new `irq_error` path does not stop the phc_sync alarm before returning.

```c
iavf_phc_sync_alarm_start(dev);
adapter->stopped = 0;

/* ... */

irq_error:
    /* cleanup irq resources */
    /* Missing: iavf_phc_sync_alarm_stop(dev); */
error:
    return -1;
```

If `iavf_start_queues()` fails after `iavf_phc_sync_alarm_start()`, the alarm remains running even though the device never entered the running state. This leaves stale timers active that may reference freed resources when they fire.

**Fix:** Add `iavf_phc_sync_alarm_stop(dev);` at the start of the `irq_error:` block, before the IRQ cleanup.

---

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

### Info

**Pre-existing observation (not introduced by this patch):**

The `iavf_get_eth_dev()` helper assumes `adapter->dev_data` is non-NULL and dereferences it unconditionally. Most call sites occur after the adapter has been fully initialized, where `dev_data` is guaranteed valid. However, if future code calls this helper during early setup or after teardown, the NULL dereference would occur.

This is not a bug introduced by this patch--it is a property of the helper's current usage. Consider adding a NULL check if the helper is ever used outside fully-initialized contexts.

---

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

### Clean

No issues found. The fix correctly returns NULL on validation failure so that `iavf_flow_process_filter()` treats it as an error, and returns the engine pointer only when validation succeeds.

---

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

### Errors

**Missing cleanup - queue-vector map not freed on early failure:**

Line 874: If `rte_intr_vec_list_alloc()` fails, the code jumps to `qv_map_alloc_err`, which calls `rte_intr_vec_list_free()` and `rte_intr_efd_disable()`. However, `vf->qv_map` was allocated earlier at line 867 and is not freed on this path.

```c
vf->qv_map = rte_zmalloc("qv_map", ...);
if (!vf->qv_map) {
    /* ... */
    return -1;  /* leaks nothing, qv_map is NULL */
}

/* ... */

if (!rte_intr_vec_list_alloc(...)) {
    /* ... */
    goto qv_map_alloc_err;  /* qv_map still allocated, not freed */
}

qv_map_alloc_err:
    rte_intr_vec_list_free(intr_handle);
    rte_intr_efd_disable(intr_handle);
    /* Missing: rte_free(vf->qv_map); vf->qv_map = NULL; */
    return -1;
```

**Fix:** Add cleanup of `vf->qv_map` to the `qv_map_alloc_err` label:

```c
qv_map_alloc_err:
    rte_intr_vec_list_free(intr_handle);
    rte_intr_efd_disable(intr_handle);
    rte_free(vf->qv_map);
    vf->qv_map = NULL;
    return -1;
```

---

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

### Clean

No issues found. The patch correctly adds unwinding of registered parsers on engine init failure and frees the items array when match-result allocation fails.

---

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

### Warnings

**Potential logic error - meta freed twice on create failure:**

Lines 2110-2118 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;
```

If `engine->create()` fails and has set `flow->rule = meta`, then:
1. Line 2114 condition `flow->rule != meta` is false, so `flow->rule` is not freed.
2. Line 2115 frees `meta` unconditionally.
3. `flow->rule` now points to freed memory.

This depends on engine behavior. If an engine sets `flow->rule` to the `meta` pointer on a failed create, the above logic would be wrong. However, inspecting the engines in this patch:

- FDIR: sets `flow->rule = filter` only on success (line 368).
- FSUB: sets `flow->rule = filter` only on success (line 85).
- RSS: sets `flow->rule = rss_cfg` only on success (line 1602).

So in practice, `flow->rule` is only set on success, and the error path is safe. The double-check on line 2114 is defensive but not necessary given current engine implementations.

**Suggested improvement (optional):** Simplify to:

```c
if (engine->create(ad, flow, meta, error) == 0) {
    if (flow->rule != meta)
        rte_free(meta);
    return engine;
}

/* On failure, flow->rule is NULL or stale; free both unconditionally */
rte_free(flow->rule);
rte_free(meta);
flow->rule = NULL;
```

This is more robust if a future engine mistakenly sets `flow->rule` on failure.

---

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

### Clean

No issues found. The patch correctly moves RSS teardown under the same `pf_reset_in_progress` check as flow flush, so no virtchnl commands are sent during PF-initiated reset.

---

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

### Clean

No issues found. The patch correctly removes checks that are no longer needed because:
- `iavf_hash_init()` sends no commands.
- `iavf_dev_close()` skips RSS teardown during PF-initiated reset.
- `iavf_execute_vf_cmd()` already refuses commands when `vf_reset` is set.

---

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

### Warnings

**Code duplication - engine init/uninit are identical:**

The egress engine's `iavf_ipsec_egress_flow_init()` and `iavf_ipsec_egress_flow_uninit()` duplicate the capability check and register/unregister calls from the ingress engine, differing only in the parser pointer.

```c
/* Ingress */
static int
iavf_ipsec_flow_init(struct iavf_adapter *ad) {
    /* ... capability check ... */
    return iavf_register_parser(&iavf_ipsec_flow_parser, ad);
}

/* Egress - identical except parser */
static int
iavf_ipsec_egress_flow_init(struct iavf_adapter *ad) {
    /* ... same capability check ... */
    return iavf_register_parser(&iavf_ipsec_egress_flow_parser, ad);
}
```

**Suggested improvement (optional):** A common helper taking the parser as a parameter would reduce duplication:

```c
static int
iavf_ipsec_flow_init_common(struct iavf_adapter *ad,
                             struct iavf_flow_parser *parser) {
    struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(ad);
    if (!vf->vf_res)
        return -EINVAL;
    if (!(vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_INLINE_IPSEC_CRYPTO))
        return -ENOTSUP;
    return iavf_register_parser(parser, ad);
}

static int iavf_ipsec_flow_init(struct iavf_adapter *ad) {
    return iavf_ipsec_flow_init_common(ad, &iavf_ipsec_flow_parser);
}

static int iavf_ipsec_egress_flow_init(struct iavf_adapter *ad) {
    return iavf_ipsec_flow_init_common(ad, &iavf_ipsec_egress_flow_parser);
}
```

This is a minor style improvement, not a correctness issue.

---

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

### Errors

**Logic error - success path may return error:**

Lines 1968-1977:

```c
if (fi == NULL)
    return rte_flow_error_set(error, EINVAL,
            RTE_FLOW_ERROR_TYPE_ACTION, NULL,
            "Invalid IPsec flow.");

if (meta != NULL)
    *meta = fi;
else
    rte_free(fi);

return 0;
```

If `iavf_ipsec_flow_item_parse()` returns NULL (line 1963), the code sets an error and returns non-zero (line 1968). However, `iavf_ipsec_flow_item_parse()` already logs errors internally and returns NULL on failure. The patch does not verify that an error was set by the callee before setting a new one, potentially overwriting a more specific error.

**More importantly:** If `item->meta` is NULL (line 1960), `fi` is never assigned (it remains NULL from line 1945). The code then hits line 1968 and returns an error claiming "Invalid IPsec flow", but this may be a valid case where the pattern-match item has no metadata. The original code returned `-1` unconditionally when `item->meta` was NULL; the patch changes this to an error even if the pattern matched.

**Suggested fix:** Initialize `ret = 0` after the `item` NULL check, and only set an error if `fi` is NULL when metadata is actually required:

```c
item = iavf_search_pattern_match_item(pattern, array, array_len, error);
if (item == NULL)
    return -rte_errno;

ret = 0;
if (item->meta != NULL) {
    const struct rte_security_session *session = parsed_actions.actions[0]->conf;
    uint32_t type = (uint64_t)(item->meta);

    fi = iavf_ipsec_flow_item_parse(ad, pattern, session, type);
    if (fi == NULL)
        ret = rte_flow_error_set(error, EINVAL,
                RTE_FLOW_ERROR_TYPE_ACTION, NULL,
                "Failed to parse IPsec flow.");
}
rte_free(item);

if (ret != 0)
    return ret;

if (meta != NULL)
    *meta = fi;
else
    rte_free(fi);

return 0;
```

---

## Summary

### Errors

1. **PATCH 01/10:** Missing `iavf_phc_sync_alarm_stop()` on `irq_error` path--alarm remains running after start failure.
2. **PATCH 04/10:** `vf->qv_map` not freed when `rte_intr_vec_list_alloc()` fails--memory leak.
3. **PATCH 10/10:** Returns error when `item->meta` is NULL even if pattern matched--may reject valid flows.

### Warnings

1. **PATCH 06/10:** Defensive double-check on `flow->rule != meta` in error path is safe but could be simplified for clarity.
2. **PATCH 09/10:** Code duplication between ingress and egress engine init/uninit could be reduced with a common helper.

### Info

1. **PATCH 02/10:** `iavf_get_eth_dev()` assumes `adapter->dev_data` is non-NULL; safe in current usage but worth noting for future expansion.

---

Review-Result: ERROR