|WARNING| [v3, 6/6] net/iavf: fix leak of IPsec crypto capabilities array

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/6: net/iavf: fix local memory leaks in TM hierarchy commit

### Errors

**Resource leak on error path - qtc_map not freed on all failure paths**

The patch adds `free(qtc_map)` only to the `err:` label but not to the `fail_clear:` label. When an error occurs after `qtc_map` is allocated but before reaching the success path, control flows to `fail_clear:` which calls `iavf_tm_conf_uninit(dev)` and then falls through to `err:`. However, if allocation of `qtc_map` succeeds but the virtchannel call fails, `qtc_map` is leaked because `fail_clear:` doesn't free it before calling `iavf_tm_conf_uninit()`.

The fix is incomplete. The `free(qtc_map)` should be added to both error labels or the code should ensure `qtc_map` is always freed on any error path after allocation.

```c
fail_clear:
	/* Clear all the traffic manager configuration */
	if (clear_on_fail) {
		iavf_tm_conf_uninit(dev);
		iavf_tm_conf_init(dev);
	}
	free(qtc_map);  /* MISSING - add this line */
err:
	free(q_bw);
	free(q_tc_mapping);
	free(qtc_map);
	return ret_val;
```

### Warnings

**Unnecessary NULL initialization of local variables**

The patch initializes `q_tc_mapping` and `q_bw` to `NULL` at declaration, then immediately overwrites them with `calloc()` results. Since these variables are not read before assignment, the initialization to `NULL` is unnecessary and should be removed per the guidelines on avoiding dead initialization.

```c
/* Current code */
struct virtchnl_queue_tc_mapping *q_tc_mapping = NULL;
struct virtchnl_queues_bw_cfg *q_bw = NULL;

/* Suggested */
struct virtchnl_queue_tc_mapping *q_tc_mapping;
struct virtchnl_queues_bw_cfg *q_bw;
```

---

## Patch 2/6: net/iavf: fix leak of queue to traffic class mapping data

### No Issues Found

The patch correctly:
- Frees `qtc_map` in `iavf_tm_conf_uninit()` and `iavf_uninit_vf()`
- Saves `old_qtc_map` before assigning new `qtc_map`
- Frees `old_qtc_map` after successful assignment
- Frees newly allocated `qtc_map` on error path
- Replaces `rte_zmalloc` with `calloc` appropriately

---

## Patch 3/6: net/iavf: fix memory leak on error when adding flow parser

### No Issues Found

The patch correctly adds `rte_free(parser_node)` on the error leg where the node is not added to any TAILQ. The fix is correct and complete.

---

## Patch 4/6: net/ice: fix buffer leak in config of Tx queue TM node

### No Issues Found

The patch correctly:
- Moves `free(buf)` to unconditionally execute after the adminq call
- Replaces `ice_malloc` with `calloc` appropriately
- Removes the incorrect `ice_free(hw, buf)` from the error path (which became unreachable after the unconditional free was added)

---

## Patch 5/6: net/iavf: fix leak of flex metadata extraction field

### Errors

**Double-free risk in iavf_dev_close()**

The patch adds a free of `vf->proto_xtr` in both `iavf_uninit_vf()` and `iavf_dev_close()`. Examining the call graph:

`iavf_dev_close()` calls `iavf_dev_stop()` which calls `iavf_uninit_vf()`. This means that when `iavf_dev_close()` executes:

1. `iavf_dev_stop()` is called
2. Which calls `iavf_uninit_vf()`
3. Which frees `vf->proto_xtr` and sets it to `NULL`
4. Control returns to `iavf_dev_close()`
5. Which then tries to free `vf->proto_xtr` again

While the code sets `vf->proto_xtr = NULL` after freeing, and `rte_free(NULL)` is safe, this pattern is still a code smell indicating the free is in the wrong place or duplicated unnecessarily.

The correct fix is to free `vf->proto_xtr` **only** in `iavf_uninit_vf()`, not in both places. Remove the free from `iavf_dev_close()`.

---

## Patch 6/6: net/iavf: fix leak of IPsec crypto capabilities array

### No Issues Found

The patch correctly adds `rte_free(iavf_sctx->crypto_capabilities)` before freeing `iavf_sctx` itself in `iavf_security_ctx_destroy()`. The fix properly frees the nested allocation before the containing structure.

---

## Summary

**Patches requiring fixes:**
- Patch 1/6: Error - incomplete error path cleanup for `qtc_map`; Warning - unnecessary NULL initialization
- Patch 5/6: Error - potential double-free pattern in `iavf_dev_close()`

**Patches that are correct:**
- Patch 2/6: Correct
- Patch 3/6: Correct
- Patch 4/6: Correct
- Patch 6/6: Correct