|WARNING| [8/8] net/ice: fix DCF port leak on probe failure

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## PATCH 1/8: net/ice: fix DCF hardware init error path

### Errors

**Resource leak - VF VSI map freed too late:**
```c
err_rss:
	rte_free(hw->rss_key);
	rte_free(hw->rss_lut);
err_alloc:
	rte_free(hw->vf_vsi_map);
	hw->vf_vsi_map = NULL;
```
The `err_alloc` label is used when allocation of `vf_res` fails (before the VF VSI map is allocated), so the new free is unreachable from that path. The map is allocated after `err_alloc`, so any error after that point needs a different label. The patch should use `err_rss:` or add a new label between them for the map free.

**Interrupt callback use-after-free risk:**
The patch correctly identifies that `ice_dcf_dev_interrupt_handler` would use freed memory if an interrupt arrives after probe failure. The fix of unregistering the callback before freeing structures is correct.

### Warnings

**NULL pointer assignment after free is unnecessary:**
```c
hw->qos_bw_cfg = NULL;
hw->vf_vsi_map = NULL;
```
Setting freed pointers to NULL in an error path that will discard the entire structure is defensive but not required. The `hw` structure is part of the adapter which is freed by the caller on probe failure, so these assignments have no effect. This is a minor style point, not a correctness issue.

---

## PATCH 2/8: net/ice: fix DCF parent adapter init error path

### Errors

**Missing `ice_flow_uninit()` call is correctly identified, but `uninit_flow` label placement is wrong:**
```c
uninit_flow:
	ice_flow_uninit(parent_adapter);
uninit_hw:
	ice_dcf_uninit_parent_hw(parent_hw);
free_vsi:
	rte_free(parent_adapter->pf.main_vsi);
```
The new `uninit_flow` label falls through to `uninit_hw`, which is correct for cleanup order (flow before hw). However, the `goto uninit_flow` from `ice_flow_init()` failure is wrong: if flow init fails, the flow subsystem is not initialized, so calling `ice_flow_uninit()` on it is incorrect. The existing `goto uninit_hw` was right. The patch should only call `ice_flow_uninit()` when a step *after* successful flow init fails.

The correct fix:
```c
err = ice_flow_init(parent_adapter);
if (err) {
	PMD_INIT_LOG(ERR, "Failed to initialize flow");
	goto uninit_hw;  /* flow not initialized, skip uninit_flow */
}

/* ... further setup ... */
if (some_later_failure) {
	goto uninit_flow;  /* flow is initialized, clean it up */
}

uninit_flow:
	ice_flow_uninit(parent_adapter);
uninit_hw:
	ice_dcf_uninit_parent_hw(parent_hw);
free_vsi:
	rte_free(parent_adapter->pf.main_vsi);
	parent_adapter->pf.main_vsi = NULL;
```

**Main VSI leak on all error paths is correct.**

---

## PATCH 3/8: net/ice: fix DCF queue config buffer leak

### Correctness

**Leak correctly identified and fixed:**
```c
PMD_DRV_LOG(ERR, "RXDID 16 is not supported");
rte_free(vc_config);
return -EINVAL;
```
and
```c
PMD_DRV_LOG(ERR, "RXDID == 0 is not supported");
rte_free(vc_config);
return -EINVAL;
```
Both error paths now free `vc_config` before returning. The fix is correct.

---

## PATCH 4/8: net/ice: fix DCF QoS buffer leak on commit

### Errors

**Function name in commit message does not match function in patch:**
The commit message says "ice_dcf_hierarchy_commit() frees its temporary VF and TC bandwidth buffers only on the error path". However, the patch modifies `ice_dcf_hierarchy_commit()`, and the variable names in the code are `vf_bw` and `tc_bw`, not "buffers". The description is accurate.

**Leak correctly identified and fixed:**
```c
hw->tm_conf.committed = true;
free(vf_bw);
free(tc_bw);
return ret_val;
```
These are temporary local variables allocated in this function for the virtchnl message. Freeing them on success is correct. The error path already frees them via `fail_clear`.

However, this should use `rte_free()`, not `free()`:
```c
vf_bw = rte_zmalloc("vf_bw", vf_bw_size, 0);
tc_bw = rte_zmalloc("tc_bw", tc_bw_size, 0);
```
These are allocated with `rte_zmalloc()`, so they must be freed with `rte_free()`. Using `free()` on `rte_malloc()`'d memory is undefined behavior.

---

## PATCH 5/8: net/ice: fix DCF QoS bandwidth replay config leak

### Errors

**Double-free risk in ice_dcf_sched.c:**
```c
rte_free(hw->qos_bw_cfg[vf_id]);
hw->qos_bw_cfg[vf_id] = rte_zmalloc("vf_bw_cfg", size, 0);
```
and
```c
rte_free(hw->qos_bw_cfg[ICE_DCF_VFID]);
hw->qos_bw_cfg[ICE_DCF_VFID] = rte_zmalloc("tc_bw_cfg", size, 0);
```
The old copy is freed before allocating the new one. This is correct.

However, there is a subtle issue: if `rte_zmalloc()` fails after the free, the pointer in the array becomes a dangling pointer (pointing to freed memory). The error path tries to free all entries in `hw->qos_bw_cfg[]` again:
```c
fail_clear:
	/* ... */
	for (i = 0; i < hw->num_vfs; i++)
		rte_free(hw->qos_bw_cfg[i]);
```
This would double-free the entries that were freed earlier in the function. The fix in ice_dcf.c partially addresses this by only freeing non-NULL entries, but the code in ice_dcf_sched.c should NULL the pointer after freeing:
```c
rte_free(hw->qos_bw_cfg[vf_id]);
hw->qos_bw_cfg[vf_id] = NULL;
hw->qos_bw_cfg[vf_id] = rte_zmalloc("vf_bw_cfg", size, 0);
```

**Cleanup in ice_dcf.c is correct but incomplete:**
```c
if (hw->qos_bw_cfg != NULL) {
	for (i = 0; i < hw->num_vfs; i++)
		rte_free(hw->qos_bw_cfg[i]);
}
```
This only frees the per-VF entries (indices `0` to `num_vfs - 1`), but the TC bandwidth config is stored at `hw->qos_bw_cfg[ICE_DCF_VFID]`. If `ICE_DCF_VFID` is not in that range, it leaks. The loop should be:
```c
for (i = 0; i <= hw->num_vfs; i++)
	rte_free(hw->qos_bw_cfg[i]);
```
or explicitly handle both:
```c
for (i = 0; i < hw->num_vfs; i++)
	rte_free(hw->qos_bw_cfg[i]);
rte_free(hw->qos_bw_cfg[ICE_DCF_VFID]);
```

---

## PATCH 6/8: net/ice: fix DCF TM config leak on close

### Correctness

**Correct fix for unconditional TM cleanup:**
```c
if (hw->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_QOS) {
	if (hw->tm_conf.committed)
		ice_dcf_clear_bw(hw);
	ice_dcf_tm_conf_uninit(eth_dev);
}
```
The commit message correctly states that only clearing the bandwidth needs a committed hierarchy. The TM config (nodes and shaper profiles) should be released whenever QoS is supported, regardless of commit status. This is correct.

---

## PATCH 7/8: net/ice: fix DCF representor info leak on close

### Correctness

**Refactoring to split close and teardown is correct:**
```c
static void
ice_dcf_dev_teardown(struct rte_eth_dev *dev)
{
	/* ... existing close logic ... */
	ice_dcf_vf_repr_notify_all(adapter, false);
	(void)ice_dcf_dev_stop(dev);
	ice_free_queues(dev);
	ice_dcf_uninit_parent_adapter(dev);
	ice_dcf_uninit_hw(dev, &adapter->real_hw);
}

static int
ice_dcf_dev_close(struct rte_eth_dev *dev)
{
	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
		return 0;

	ice_dcf_dev_teardown(dev);
	ice_dcf_free_repr_info(dev->data->dev_private);
	return 0;
}
```
The representor info must survive reset (because `ice_dcf_dev_init()` notifies them again), so freeing it only in close is correct.

---

## PATCH 8/8: net/ice: fix DCF port leak on probe failure

### Errors

**Type check before probe is good, but creates a gap:**
```c
if (eth_da.nb_representor_ports && eth_da.type != RTE_ETH_REPRESENTOR_VF)
	return -ENOTSUP;

ret = rte_eth_dev_pci_generic_probe(pci_dev,
				    sizeof(struct ice_dcf_adapter),
				    ice_dcf_dev_init);
if (ret || !eth_da.nb_representor_ports)
	return ret;
```
If `rte_eth_dev_pci_generic_probe()` fails and representor ports were requested, the early return skips the error path that would remove the DCF port. The logic should be:
```c
if (ret) {
	/* probe failed, nothing to clean up */
	return ret;
}
if (!eth_da.nb_representor_ports)
	return 0;  /* success, no representors */
```

**Cleanup loop has off-by-one:**
```c
for (i = 0; i < dcf_adapter->real_hw.num_vfs; i++) {
	struct rte_eth_dev *vf_rep_eth_dev =
		dcf_adapter->repr_infos[i].vf_rep_eth_dev;

	if (vf_rep_eth_dev != NULL)
		rte_eth_dev_destroy(vf_rep_eth_dev, ice_dcf_vf_repr_uninit);
}
```
This destroys all representors, not just the ones created so far. If representor creation failed on the 3rd VF out of 10, this loop destroys representors 0-2 (correct) and then tries to destroy NULL pointers for 3-9 (harmless because of the NULL check, but inefficient). The loop bound should be `dcf_adapter->num_reprs`.

**Missing `ice_dcf_free_repr_info()` in error path:**
The `err_remove_dcf` label calls `rte_eth_dev_pci_generic_remove()`, which calls `ice_dcf_dev_uninit()`. However, `ice_dcf_dev_uninit()` does not free the representor info -- only `ice_dcf_dev_close()` does (as of patch 7). So the error path leaks `repr_infos`. The fix should add:
```c
err_remove_dcf:
	ice_dcf_free_repr_info(dcf_adapter);
	rte_eth_dev_pci_generic_remove(pci_dev, ice_dcf_dev_uninit);
```

---

## Summary

### Patch 1
- **Error**: VF VSI map free is unreachable from the `err_alloc` path; needs a new label or to use `err_rss`.

### Patch 2
- **Error**: `ice_flow_uninit()` called when flow init failed; should only uninit flow if init succeeded.

### Patch 3
- **Clean**: Leak fix is correct.

### Patch 4
- **Error**: Uses `free()` instead of `rte_free()` on `rte_zmalloc()`'d memory.

### Patch 5
- **Error**: Double-free risk if `rte_zmalloc()` fails after freeing old entry (should NULL the pointer).
- **Error**: TC bandwidth config at `hw->qos_bw_cfg[ICE_DCF_VFID]` not freed if `ICE_DCF_VFID >= num_vfs`.

### Patch 6
- **Clean**: Correct fix.

### Patch 7
- **Clean**: Correct refactoring.

### Patch 8
- **Error**: DCF port not removed if probe fails before representor creation.
- **Error**: Cleanup loop iterates over `num_vfs` instead of `num_reprs`.
- **Error**: `repr_infos` not freed in error path.

Review-Result: ERROR