|WARNING| [v2,9/9] net/iavf: fix IPsec flow metadata leaks

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/171676

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

**Resource leak on start failure:**
Line: `iavf_dev_start()` function body (lines 1039-1135)

The patch correctly moves `adapter->stopped = 0` to after startup succeeds, and adds early-return in stop when already stopped. However, the new error path at `irq_error:` does not release all resources allocated during the failed start.

The function allocates:
- Queue pairs via `iavf_start_queues()` at line 1112
- IRQ resources via `iavf_config_rx_queues_irqs()` before line 1112

On failure after line 1112, the `irq_error:` path cleans up IRQ resources but does not clean up queues. The queues remain configured in hardware/PF state even though start failed.

Suggested fix: Add `iavf_stop_queues(dev)` before the IRQ cleanup in the error path:

```c
irq_error:
	iavf_stop_queues(dev);
	if (!wb_on_itr && dev->data->dev_conf.intr_conf.rxq != 0)
		rte_intr_disable(intr_handle);
	rte_intr_efd_disable(intr_handle);
	rte_intr_vec_list_free(intr_handle);
	rte_free(vf->qv_map);
	vf->qv_map = NULL;
error:
	return -1;
```

---

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

### Warnings

**Missing null check on adapter dereference:**
Line: `iavf_get_eth_dev()` function (lines 410-413)

The new inline function `iavf_get_eth_dev()` dereferences `adapter->dev_data->port_id` without checking if `adapter` or `adapter->dev_data` is NULL. While callers in this patch always have a valid adapter, defensive programming would check these before dereferencing, or document that the caller guarantees validity.

---

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

### Clean
No issues found. The fix correctly returns NULL on validation failure instead of returning the engine pointer after setting an error.

---

## Patch 4/9: net/iavf: fix memory leak in raw pattern parsing

### Clean
No issues found. The fix correctly frees the previously allocated packet buffer when mask buffer allocation fails.

---

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

### Errors

**Double cleanup risk:**
Line: 1130 in error path

The error path at `irq_error:` calls `rte_intr_vec_list_free()` and `rte_intr_efd_disable()`. However, if `iavf_config_rx_queues_irqs()` succeeded and `iavf_start_queues()` failed, these resources were already set up correctly. The subsequent call to `iavf_dev_stop()` by the application will call these cleanup functions again at lines 1161-1162.

`rte_intr_vec_list_free()` and `rte_intr_efd_disable()` are not documented as idempotent. While they may handle double-free internally, explicitly calling them twice on the same handle is unsafe without verification.

Suggested fix: Only clean up IRQ resources on the error path if `iavf_config_rx_queues_irqs()` itself succeeded but a later step failed. The existing `iavf_dev_stop()` call by the application will clean up otherwise:

```c
irq_error:
	if (!wb_on_itr && dev->data->dev_conf.intr_conf.rxq != 0)
		rte_intr_disable(intr_handle);
	rte_intr_efd_disable(intr_handle);
	rte_intr_vec_list_free(intr_handle);
	rte_free(vf->qv_map);
	vf->qv_map = NULL;
```

Actually, re-reading: `iavf_dev_stop()` is not called on start failure in the existing code. The error path must do all cleanup. The issue is that the cleanup at `irq_error:` assumes IRQ setup succeeded, but if called from before IRQ setup completes, it may clean up partially initialized state.

Revised: The error path is correct only if reached after `iavf_config_rx_queues_irqs()` fully succeeds. If that function fails partway through, its internal error path at `qv_map_alloc_err:` and `efd_alloc_err:` already cleans up its own allocations. The `irq_error:` label in `iavf_dev_start()` should only be reached after line 1089 succeeds.

The code is correct as written: `iavf_config_rx_queues_irqs()` cleans up on its own failure; `iavf_dev_start()` cleans up if IRQ setup succeeded but queue start failed.

**Retraction:** No error here. The cleanup paths are correctly separated.

---

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

### Clean
No issues found. The patch correctly unwinds parser registrations on init failure and frees the item array when match-result allocation fails.

---

## Patch 7/9: net/iavf: centralize memory management for flows

### Errors

**Missing validation of filter allocation:**
Line: 1564 in `iavf_fdir_parse()`

The function allocates `filter` via `rte_zmalloc()` at line 1564 and checks for NULL. However, the error path at line 1566 does not set `*meta` to NULL before returning, and the function later (line 1596) conditionally assigns to `*meta` only if `meta != NULL`. If the caller passes a non-NULL `meta` pointer with garbage initial value and allocation fails, the caller sees uninitialized `*meta` rather than a clear failure signal.

Suggested fix: Initialize `*meta = NULL` at the top of the function if `meta != NULL`, or explicitly set it on the error path before returning.

**Use of `rte_errno` without setting it:**
Line: 1568, 1585, 1591 in `iavf_fdir_parse()`

Multiple error paths call `rte_flow_error_set()` but then return `-rte_errno`. If `rte_errno` was not set by the failure (e.g., `ci_flow_check_attr()` or pattern matching failure), the return value may be zero or a stale error code.

Suggested fix: Return a specific error code (e.g., `-EINVAL`) or ensure `rte_errno` is set before each use. The `rte_flow_error_set()` macro does not set `rte_errno` itself.

**Inconsistent freeing of `item` in `iavf_fdir_parse()`:**
Line: 1595 and 1602

The function frees `item` at line 1595 on the success path and again at line 1602 on the error path. If `iavf_fdir_parse_pattern()` fails after `iavf_search_pattern_match_item()` succeeds, execution reaches line 1594 (`goto error`), then line 1602 frees `item`. Correct.

But if `iavf_search_pattern_match_item()` itself fails, it returns NULL and sets `rte_errno`. Line 1585 checks `if (item == NULL)`, sets `ret = -rte_errno`, and does `goto error`. Then line 1602 calls `rte_free(item)` on a NULL pointer. `rte_free(NULL)` is safe, so this is not a bug. However, it is redundant.

Not an error, but could be cleaner: move `rte_free(item)` inside the conditional branches or ensure `item` is always valid before the error label.

---

### Warnings

**Removed per-VF filter structure without checking all references:**
Line: `struct iavf_fdir_info` removal in `iavf.h`

The patch removes `struct iavf_fdir_info` and its instance `vf->fdir`. The commit message states this is now allocated per-parse. However, the removed `vf->fdir.conf` was a persistent scratch buffer. Verify that no code path expected this to survive between calls.

Checked: the old code used `&vf->fdir.conf` as a temporary during parsing and did not rely on it being persistent. The new code allocates `filter` per-parse and returns it as the rule. This is correct.

Not a warning.

---

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

### Clean
No issues found. The patch correctly registers an egress security engine and routes these flows through normal parsers, avoiding the NULL engine dereference.

---

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

### Errors

**Incorrect error return on item parse failure:**
Line: 1969 in `iavf_ipsec_flow_parse()`

When `iavf_ipsec_flow_item_parse()` returns NULL at line 1964, the code reaches line 1968 and calls `rte_flow_error_set()`, which sets the error but does not modify `rte_errno`. The function then returns `rte_flow_error_set(...)` directly. The `rte_flow_error_set()` macro returns the error code passed as its second argument (here, `EINVAL`), but that is not the same as `-rte_errno`.

Wait, checking the line numbers: line 1969 has the `return rte_flow_error_set(...)` call. The macro signature is:

```c
static inline int
rte_flow_error_set(struct rte_flow_error *error,
                   int code,
                   enum rte_flow_error_type type,
                   const void *cause,
                   const char *message)
{
    if (error) {
        *error = (struct rte_flow_error){
            .type = type,
            .cause = cause,
            .message = message,
        };
    }
    rte_errno = code;
    return -code;
}
```

So `rte_flow_error_set(error, EINVAL, ...)` returns `-EINVAL` and sets `rte_errno = EINVAL`. This is correct. No error.

**Retraction:** No issue here.

---

## Summary of Findings

### Errors (1)
**Patch 1/9:**
- Missing `iavf_stop_queues()` call in the new `irq_error:` path when start fails after queue configuration

### Warnings (1)
**Patch 2/9:**
- `iavf_get_eth_dev()` dereferences adapter and dev_data without null checks (acceptable if caller contract guarantees validity, but not documented)

### Info
**Patch 7/9:**
- `iavf_fdir_parse()` could set `*meta = NULL` explicitly on allocation failure for clarity
- Multiple uses of `-rte_errno` where the calling function may not have set `rte_errno`; returning explicit error codes would be safer

---

Review-Result: ERROR