|SUCCESS| [3/3] net/iavf: fix crash in TM ops without QoS capability

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## PATCH 1/3: net/iavf: fix invalid free of queue TC map

### Errors

**Double-free of queue TC map:**
The patch removes a `rte_free(vf->qtc_map)` that was followed by `free(old_qtc_map)` on the same pointer. The commit message correctly identifies this as a double-free bug. After the first `rte_free()` call reads hugepage heap metadata that doesn't exist (because the pointer came from libc `calloc()`), the subsequent `free()` releases the same pointer that `rte_free()` attempted to free. This could cause heap corruption or crashes.

The fix is correct: `vf->qtc_map` is assigned from `qtc_map` (which was allocated with `calloc()`), so only `free(old_qtc_map)` should be called. The `rte_free()` was operating on the wrong heap.

**Line removed:**
```c
rte_free(vf->qtc_map);
```

This is a correctness bug fix.

---

## PATCH 2/3: net/iavf: fix bandwidth config of unshaped queues

### Analysis

The patch moves two assignments (`queue_id` and `tc`) outside the `if (tm_node->shaper_profile)` block so they execute for all queue nodes, not just those with shaper profiles.

**Before:** Queue nodes without shaper profiles left `queue_id` and `tc` uninitialized (zeroed by the allocation), causing the PF to apply default bandwidth to queue 0 repeatedly instead of to the correct queue.

**After:** Every queue node, whether it has a shaper profile or not, gets its `queue_id` and `tc` set correctly before the virtchnl message is sent.

**Lines changed:**
```c
+		q_bw->cfg[node_committed].queue_id = tm_node->id;
+		q_bw->cfg[node_committed].tc = tm_node->tc;
 		if (tm_node->shaper_profile) {
-			q_bw->cfg[node_committed].queue_id = tm_node->id;
 			q_bw->cfg[node_committed].shaper.peak = ...
 			q_bw->cfg[node_committed].shaper.committed = ...
-			q_bw->cfg[node_committed].tc = tm_node->tc;
 		}
```

This is a correctness bug fix. The logic error caused queue bandwidth configuration to be applied to the wrong queue (always queue 0) for unshaped nodes.

---

## PATCH 3/3: net/iavf: fix crash in TM ops without QoS capability

### Errors

**NULL pointer dereference in TM operations:**
When the PF does not grant `VIRTCHNL_VF_OFFLOAD_QOS`, `vf->qos_cap` remains NULL. Four TM operations (`node_add`, `shaper_profile_add`, `capabilities_get`, `node_capabilities_get`) dereference `vf->qos_cap` without checking, causing segmentation faults.

The patch adds a `iavf_tm_qos_check()` helper that returns `-ENOTSUP` when `qos_cap` is NULL, and calls it at the start of each affected function.

**Lines demonstrating the bug (before fix):**
```c
uint16_t tc_nb = vf->qos_cap->num_elem;  /* crashes if qos_cap is NULL */
```

**Fix (example from `iavf_tm_capabilities_get`):**
```c
+	ret = iavf_tm_qos_check(vf, error);
+	if (ret)
+		return ret;
+
 	tc_nb = vf->qos_cap->num_elem;
```

This is a correctness bug fix.

### Warnings

**Boolean variable usage:**
The `iavf_tm_qos_check()` function could use `bool` for its return type since it returns 0 (success) or -ENOTSUP (not supported). However, the existing DPDK TM API functions return `int` for error codes, so matching that convention is acceptable. Not flagging this as it follows the existing pattern in the codebase.

---

## General Comments

All three patches are bug fixes with appropriate `Fixes:` tags and `Cc: stable at dpdk.org`. The commit messages clearly explain the bugs and fixes. The code changes are minimal and targeted.

**Patch 1** fixes a heap corruption bug (double-free with mixed allocators).  
**Patch 2** fixes a logic error that caused bandwidth limits to be applied to the wrong queue.  
**Patch 3** fixes NULL pointer dereferences that crash the application.

All three are high-value correctness fixes with no style or API issues.

Review-Result: CLEAN