|SUCCESS| net/iavf: fix memory leaks in RSS flow engine

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## ERRORS

**1. Resource leak on error path - missing `rte_free(meta)`**

In `iavf_hash_create()`, when `iavf_add_del_rss_cfg()` fails, the error path at line 1617 returns with `-rte_errno`, but this contradicts the fix's intent. The function stores the error in `rte_errno` inside `iavf_add_del_rss_cfg()`, so returning `-rte_errno` double-negates it, producing a positive error code when the caller expects negative. This pattern appears in the existing code at line 1633 as well in `iavf_hash_destroy()`.

```c
/* BAD - double negation */
return -rte_errno;

/* GOOD - rte_errno is already negative or use explicit value */
return ret;
```

However, checking the context: `iavf_add_del_rss_cfg()` returns a negative error code directly, and the code does `ret = iavf_add_del_rss_cfg(...)`. So `ret` is already negative. Then `rte_flow_error_set(error, -ret, ...)` expects a positive errno (it stores `-ret`), and `return -rte_errno;` is indeed wrong if `rte_errno` was set to a positive value by `rte_flow_error_set()`.

Actually, reviewing more carefully: `rte_flow_error_set()` takes an error code as its second argument and does NOT set `rte_errno`. The return should be `return ret;` not `return -rte_errno;`. But this is pre-existing in both locations and the patch doesn't make it worse.

**Let me reconsider the actual error here:**

Looking at line 1617 in the patched code:
```c
return -rte_errno;
```

But `rte_flow_error_set()` on line 1614 sets the error object, it doesn't necessarily set `rte_errno`. This return statement assumes `rte_errno` contains the error, but the actual error is in `ret` from line 1611. The correct return would be `return ret;` since `ret` is already negative from `iavf_add_del_rss_cfg()`.

This is pre-existing (line 1633 has the same pattern in the destroy function), so it's Info only per the pre-existing code rule. Moving on to issues the patch introduces:

**Actually, focusing on what the patch changes:**

The patch adds `rte_free(meta)` on both error paths in `iavf_hash_create()`, which is correct. It adds `rte_free(rss_cfg)` in `iavf_hash_destroy()`, which is correct. It adds `rte_free(pkt_buf)` in `iavf_hash_parse_raw_pattern()`, which is correct.

**2. Incorrect return value in iavf_hash_destroy() when vf_reset is true**

At line 1636 (pre-patch line numbers) / new line 1647, the function returns `ret` which is initialized to 0 at line 1634. When `vf->vf_reset` is true, the function skips the `iavf_add_del_rss_cfg()` call but still needs to free the resources. The patch correctly adds the free operations after the conditional block. However, the return value `ret` remains 0, which is correct for success. The resources are freed, so returning 0 is appropriate. This is not an error.

**Re-examining for actual errors in the patch:**

The patch changes error codes from `EINVAL` to `ENOMEM` in two locations (lines 1543 and 1599 in the patched version). These are in `iavf_hash_parse_pattern_action()` and `iavf_hash_create()`. Both are allocation failures, so `ENOMEM` is correct. The patch text says "Report ENOMEM instead" which is accurate.

The critical question: does the patch introduce any new bugs?

Looking at the `iavf_hash_destroy()` changes:
- Lines 1648-1649 set `flow->rule = NULL` and call `rte_free(rss_cfg)` unconditionally
- Previously, if `vf->vf_reset` was true, the function returned 0 immediately without freeing anything
- Now it always frees `rss_cfg` regardless of `vf_reset`

Is this safe? If `vf->vf_reset` is true, we skip telling the hardware to delete the RSS config, but we still free the memory. That seems correct for a memory leak fix. The `flow->rule` pointer is set to NULL before the free, which is good practice.

**Actually, there's an issue here:**

```c
flow->rule = NULL;
rte_free(rss_cfg);
```

The `rss_cfg` pointer is assigned from `flow->rule` at line 1636. After setting `flow->rule = NULL`, the `rte_free(rss_cfg)` at line 1649 still has the original pointer value (C passes by value), so this is safe. Not a bug.

**Checking for use-after-free:**

The `rte_free(rss_cfg)` happens after `flow->rule = NULL`, and `rss_cfg` is a local variable that goes out of scope after the free, so no use-after-free here.

**Checking the error path return value again:**

In `iavf_hash_create()`, line 1617:
```c
return -rte_errno;
```

But earlier at line 1614:
```c
rte_flow_error_set(error, -ret, ...);
```

The `rte_flow_error_set()` macro/function signature is:
```c
int rte_flow_error_set(struct rte_flow_error *error, int code, ...);
```

It expects a *positive* errno value as `code`. The code passes `-ret` where `ret` is negative (from `iavf_add_del_rss_cfg()`), so it passes a positive errno, which is correct for `rte_flow_error_set()`.

Then `return -rte_errno;` -- but did `rte_flow_error_set()` set `rte_errno`? Looking at the DPDK source, `rte_flow_error_set()` does NOT modify `rte_errno`; it only modifies the `error` structure. So `rte_errno` here is undefined or stale. The return should be `return ret;`.

But this is **pre-existing** -- the patch doesn't change line 1617 in the new code (it's the same as the old code at that location, just shifted). The patch does NOT introduce this bug.

**Looking at what the patch actually changes:**

1. Adds `rte_free(pkt_buf)` before return in `iavf_hash_parse_raw_pattern()` - **CORRECT**
2. Changes `EINVAL` to `ENOMEM` in two error paths - **CORRECT**
3. Adds `rte_free(meta)` on two error paths in `iavf_hash_create()` - **CORRECT**
4. Restructures `iavf_hash_destroy()` to free `rss_cfg` unconditionally - **CORRECT**
5. Sets `flow->rule = NULL` before freeing - **CORRECT**

## WARNINGS

None.

## INFO

**Pre-existing: Incorrect return value using -rte_errno**

In `iavf_hash_create()` at line 1617 and `iavf_hash_destroy()` at line 1644 (post-patch), both return `-rte_errno` after calling `rte_flow_error_set()`. However, `rte_flow_error_set()` does not modify `rte_errno`; it only sets the `error` structure. The correct return value is `ret` (which is already negative from `iavf_add_del_rss_cfg()`). This is pre-existing and not introduced by this patch.

```c
/* Current (pre-existing issue) */
rte_flow_error_set(error, -ret, RTE_FLOW_ERROR_TYPE_HANDLE, NULL, "...");
return -rte_errno;  /* rte_errno may be stale */

/* Should be */
return ret;  /* ret is already the negative error code */
```

---

The patch correctly fixes the memory leaks described in the commit message. All added frees are on the correct error paths, and the `iavf_hash_destroy()` changes properly clean up resources in all exit paths.

Review-Result: CLEAN