|WARNING| [v2, 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/166539

_AI Code Review_

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

# Review of DPDK Patch Series: Intel Driver Memory Leak Fixes

## Summary
This patch series addresses memory leaks in Intel network drivers (iavf and ice). The patches correctly identify and fix resource leaks by adding appropriate cleanup calls on error paths and in uninit/destroy functions. The approach of switching from `rte_malloc`/`rte_zmalloc` to standard `malloc`/`calloc` for control-path structures is appropriate.

---

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

### Errors
None.

### Warnings
None.

### Info
The patch correctly identifies and fixes memory leaks of `q_tc_mapping` and `q_bw` on both success and error paths. The switch from `rte_zmalloc` to `calloc` is appropriate for control-path structures that do not require hugepage backing.

---

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

### Errors
None.

### Warnings

**Missing `NULL` check after `calloc` before dereferencing**
In `iavf_hierarchy_commit()`, the code performs:
```c
qtc_map = calloc(q_tc_mapping->num_tc, sizeof(struct iavf_qtc_map));
if (!qtc_map) {
    ret_val = IAVF_ERR_NO_MEMORY;
    goto fail_clear;
}
```
However, immediately after the allocation check, the code accesses `qtc_map[i]` in a loop:
```c
for (i = 0; i < q_tc_mapping->num_tc; i++) {
    q_tc_mapping->tc[i].req.start_queue_id = index;
    q_tc_mapping->tc[i].req.queue_count = tm_node->reference_count;
    qtc_map[i].tc = tm_node->tc;
    qtc_map[i].start_queue_id = index;
    qtc_map[i].queue_count = tm_node->reference_count;
    index += tm_node->reference_count;
}
```

While the `NULL` check is present and will branch to `fail_clear` on allocation failure, the code structure is correct. This is NOT an error.

**Potential issue with freeing `old_qtc_map` when `qtc_map` allocation fails**
The code declares:
```c
struct iavf_qtc_map *old_qtc_map = vf->qtc_map;
```
Then later:
```c
vf->qtc_map = qtc_map;
free(old_qtc_map);
```

If `calloc(qtc_map)` fails and we goto `fail_clear`, the `old_qtc_map` is not freed in the error path. However, examining the error path:
```c
fail_clear:
    if (clear_on_fail) {
        iavf_tm_conf_uninit(dev);
        iavf_tm_conf_init(dev);
    }
err:
    free(q_bw);
    free(q_tc_mapping);
    free(qtc_map);
    return ret_val;
}
```

The `iavf_tm_conf_uninit(dev)` function (added in this patch) frees `vf->qtc_map`:
```c
free(vf->qtc_map);
vf->qtc_map = NULL;
```

So on allocation failure of `qtc_map`, `vf->qtc_map` still points to the old map, and `iavf_tm_conf_uninit()` will free it if `clear_on_fail` is true. If `clear_on_fail` is false, the old map remains valid, which is correct behavior. The error handling is therefore correct.

### Info
The switch from `rte_zmalloc` to `calloc` for the queue-to-TC mapping structure is appropriate since this is control-path configuration data that does not require hugepage memory.

---

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

### Errors
None.

### Warnings
None.

### Info
The fix correctly adds `rte_free(parser_node)` in the error branch where the node is not added to any list. This prevents leaking the `parser_node` allocation on the error path.

---

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

### Errors
None.

### Warnings
None.

### Info
The patch correctly moves the `free(buf)` call outside the error check so that `buf` is freed unconditionally after the admin queue call. The switch from `ice_malloc` to `calloc` is appropriate for this temporary buffer.

---

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

### Errors
None.

### Warnings
None.

### Info
The fix correctly adds cleanup of `vf->proto_xtr` in `iavf_uninit_vf()` to match its allocation in `iavf_init_proto_xtr()`. The use of `rte_free()` is correct since the allocation uses `rte_zmalloc()`.

---

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

### Errors
None.

### Warnings
None.

### Info
The fix correctly frees `iavf_sctx->crypto_capabilities` before freeing the security context structures. The allocation of `crypto_capabilities` is done with `rte_zmalloc_socket()` in `iavf_security_ctx_create()`, so `rte_free()` is the correct deallocator.

---

## Series-Wide Observations

### Positive Aspects
1. All patches correctly identify resource leaks and add appropriate cleanup
2. Error paths are properly traced and resources are freed on all exit paths
3. The switch from DPDK-specific allocators to standard C allocators is appropriate for control-path structures
4. Proper NULL-pointer safety is maintained (`free(NULL)` is safe, `rte_free(NULL)` is safe)
5. Patches set pointers to NULL after freeing to prevent double-free bugs
6. `Cc: stable at dpdk.org` is correctly included for backportable fixes

### Release Notes
These fixes address memory leaks and do not change API or behavior. Since they are bugfixes backported to stable releases, release notes are not required per the guidelines.

---

## Conclusion

All six patches are correct. The resource leak fixes properly trace all allocation sites to their corresponding free sites on both success and error paths. The code changes do not introduce new bugs and follow DPDK coding conventions.

**Recommendation: Approve series**