|WARNING| [2/2] net/iavf: fix leak of queue to traffic class mapping data

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## PATCH 1/2: net/iavf: fix local memory leaks in TM hierarchy commit

### Errors

None identified.

### Warnings

**Queue-related buffer allocation without `rte_zmalloc_socket()`:**
The patch converts `q_tc_mapping` and `q_bw` allocations from `rte_zmalloc()` to `calloc()`.
While these are temporary/local variables in this function, they are queue configuration structures
that configure hardware queue-to-TC mappings and bandwidth settings.
If these structures need to be accessible by secondary processes or require NUMA-local allocation,
`calloc()` would be incorrect.
However, the commit message states these are temporary variables that don't need hugepage memory,
which appears reasonable for this use case.
Verify that these structures are truly local/temporary and not shared with hardware or secondary processes.

**`<stdlib.h>` include after DPDK headers:**
The patch adds `#include <stdlib.h>` after `#include <rte_tm_driver.h>`.
Per DPDK coding standards, system/libc includes should come before DPDK includes:
```c
/* Current (incorrect order): */
#include <rte_tm_driver.h>
#include <stdlib.h>

/* Should be: */
#include <stdlib.h>
#include <rte_tm_driver.h>
```

### Info

The fix correctly addresses the memory leaks by:
- Freeing `q_tc_mapping` and `q_bw` on all return paths (success, `fail_clear`, and `err`)
- Initializing pointers to `NULL` to make `free()` safe on uninitialized error paths
- Adding error path cleanup for the newly discovered `qtc_map` allocation leak (ret_val = IAVF_ERR_NO_MEMORY; goto fail_clear)

The error path tracing is correct: all allocations are now freed on all exit paths.

---

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

### Errors

None identified.

### Warnings

**`qtc_map` allocation without `rte_zmalloc_socket()`:**
The patch converts `qtc_map` from `rte_zmalloc()` to `calloc()`.
This structure is stored in `vf->qtc_map` and persists across function calls --
it is used later in the datapath to validate queue/TC mappings.
If this needs to be accessible by secondary DPDK processes or requires NUMA-local allocation,
`calloc()` would break secondary process support.

The original code used `rte_zmalloc()`, which allocates from hugepages and is visible to secondary processes.
The new code uses `calloc()`, which allocates from process-private heap.

**Recommendation:** Use `rte_zmalloc_socket()` instead of `calloc()` to preserve secondary process visibility
and NUMA locality:
```c
qtc_map = rte_zmalloc_socket("qtc_map",
    q_tc_mapping->num_tc * sizeof(struct iavf_qtc_map),
    0, SOCKET_ID_ANY);
if (!qtc_map) {
    ret_val = IAVF_ERR_NO_MEMORY;
    goto fail_clear;
}
```

And correspondingly use `rte_free()` instead of `free()` at all cleanup sites:
```c
rte_free(old_qtc_map);
...
rte_free(vf->qtc_map);
```

This assumes the structure needs secondary process visibility.
If it does not (the commit message should state this explicitly), then `calloc()` is acceptable
but the rationale should be documented.

### Info

The leak fixes are correct in structure:
- `old_qtc_map` pointer correctly captures the old value before assignment
- Old map is freed immediately after successful assignment of new map
- New map is freed on the error path (`free(qtc_map)` added to `err:` label)
- Map is freed on device uninit (`iavf_uninit_vf()`) and TM config uninit (`iavf_tm_conf_uninit()`)
- Pointers are set to NULL after freeing to avoid dangling references

The error path tracing is correct: the new `qtc_map` allocation is now freed on all failure paths.

---

## Summary

Both patches correctly fix memory leaks by adding cleanup on all error paths and at uninit time.

**Key concerns:**
1. **Include order** (Patch 1): `<stdlib.h>` should come before DPDK headers
2. **Allocation type** (Both patches): Converting from `rte_zmalloc()` to `calloc()` may break secondary process support
   if these structures need to be shared. Verify whether `qtc_map` in particular needs hugepage backing,
   and if so, use `rte_zmalloc_socket()` + `rte_free()` instead of `calloc()` + `free()`.