|SUCCESS| [v2,8/8] net/ice: fix DCF port leak on probe failure

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

**Resource leak on error path**
Lines `err_rss:` and `err_alloc:` labels do not release `hw->vf_vsi_map` when allocated resources before it are freed, causing a leak when any error after getting the VSI map occurs.
```c
err_alloc:
	rte_free(hw->vf_res);
err_api:
	iavf_shutdown_adminq(&hw->avf);
```
The patch correctly adds:
```c
err_alloc:
	rte_free(hw->vf_vsi_map);
	hw->vf_vsi_map = NULL;
	rte_free(hw->vf_res);
```

**Use-after-free risk from interrupt callback**
When VLAN offload V2 capability fetch fails, the function returns without disabling the interrupt or unregistering the callback. The DCF hardware structure passed to `ice_dcf_dev_interrupt_handler` is freed when probe fails, so a later interrupt would access freed memory.
The patch correctly adds the `err_intr:` label that disables IRQ0, disables and unregisters the interrupt, and frees the QoS bandwidth config before proceeding to the RSS cleanup.

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

### Errors

**Resource leak on error path**
`ice_dcf_init_parent_adapter()` allocates `parent_adapter->pf.main_vsi` but none of the error paths free it. When `ice_dcf_init_parent_hw()` or `ice_flow_init()` fails, the VSI leaks.
The patch correctly adds a `free_vsi:` label that releases it.

**Missing cleanup on flow init failure**
When `ice_flow_init()` fails, the error path jumps to `uninit_hw:` which calls `ice_dcf_uninit_parent_hw()` but skips `ice_flow_uninit()`, so the DDP parser and any flow engine state set up before the failure are not released.
The patch correctly adds `uninit_flow:` label calling `ice_flow_uninit()` before `uninit_hw:`.

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

### Errors

**Memory leak on error return**
`ice_dcf_configure_queues()` allocates `vc_config` but returns `-EINVAL` without freeing it when the required RXDID is not supported (lines 1234 and 1247 in the original code). The message leaks on each failed port start.
The patch correctly adds `rte_free(vc_config);` before both early returns.

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

### Errors

**Memory leak on success path**
`ice_dcf_hierarchy_commit()` allocates temporary `vf_bw` and `tc_bw` buffers and frees them only on the error path (`fail_clear:`), so every successful commit leaks them.
The patch correctly adds `free(vf_bw); free(tc_bw);` after setting `hw->tm_conf.committed = true;` on the success path.

Note: The patch uses `free()` not `rte_free()` because the buffers are allocated with `calloc()` earlier in the function (not shown in diff but visible in context).

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

### Errors

**Memory leak and race condition in bandwidth config replacement**
`ice_dcf_hierarchy_commit()` allocates new copies of VF and TC bandwidth config and assigns them directly to `hw->qos_bw_cfg[vf_id]` and `hw->qos_bw_cfg[ICE_DCF_VFID]` without freeing the previous copies, causing a leak on every commit after the first. Additionally, the VF reset thread in `ice_dcf_vsi_update_service_handler()` may be reading these configs concurrently (it holds `vsi_update_lock` while replaying), so the new copy becomes visible before it is filled in, and there is no synchronization preventing a use-after-free if the reset thread is reading the old copy when it is freed.

The patch introduces `ice_dcf_replace_qos_bw_cfg()` which:
1. Fills the new copy first (before the function is called)
2. Swaps it in under `vsi_update_lock` (the same lock the VF reset thread holds)
3. Frees the old copy after releasing the lock

This correctly prevents both the leak and the race.

**Memory leak in uninit**
`ice_dcf_uninit_hw()` frees the `hw->qos_bw_cfg` array but not the individual config copies within it, so they leak on close and reset.
The patch correctly adds a loop freeing each `hw->qos_bw_cfg[i]` before freeing the array.

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

### Errors

**Resource leak when hierarchy not committed**
`ice_dcf_uninit_hw()` releases TM nodes and shaper profiles only when `hw->tm_conf.committed` is true. If the application adds nodes or profiles but never commits them, or the commit fails without clearing them, they leak on close and reset.
The patch correctly moves `ice_dcf_tm_conf_uninit(eth_dev);` outside the `if (hw->tm_conf.committed)` block, so TM config is released whenever QoS is supported.

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

### Errors

**Memory leak on close**
The representor info array allocated in `ice_dcf_init_repr_info()` is only freed by `ice_dcf_dev_uninit()`, which runs on device removal. Closing the DCF port with `rte_eth_dev_close()` releases the port without freeing the representor info, so it leaks.

The patch correctly:
1. Extracts the close logic into `ice_dcf_dev_teardown()` (which does not free repr info)
2. Calls `ice_dcf_dev_teardown()` from both `ice_dcf_dev_close()` and `ice_dcf_dev_reset()`
3. Adds `ice_dcf_free_repr_info()` in `ice_dcf_dev_close()` only (reset needs the info to survive for re-init)

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

### Errors

**Port leak on probe failure**
When representors are requested, `eth_ice_dcf_pci_probe()` creates the DCF port via `rte_eth_dev_pci_generic_probe()` before handling representor creation. If a later step fails (unsupported representor type, invalid VF ID, or failed representor creation), the probe returns an error but leaves the DCF port and any representors already created in place. As the probe failed, the PCI bus also unmaps the device, so these ports are leaked and cannot be closed safely.

The patch correctly:
1. Moves the `eth_da.type != RTE_ETH_REPRESENTOR_VF` check before creating the DCF port (early return, no cleanup needed)
2. On failure after DCF port creation, destroys any representors created so far (loop over `repr_infos[i].vf_rep_eth_dev`, calling `rte_eth_dev_destroy()` for non-NULL entries)
3. Calls `rte_eth_dev_pci_generic_remove()` to remove the DCF port
4. Changes the error path from multiple early returns to `goto err_remove_dcf;` for centralized cleanup

---

## Summary

All eight patches address correctness bugs: resource leaks on error paths (patches 1-3, 8), resource leaks on success paths (patch 4), resource leaks and race conditions in concurrent access (patch 5), and resource leaks on cleanup paths (patches 6-7). Every finding is a use-after-free, resource leak, race condition, or missing cleanup that would cause incorrect behavior or crashes in production.

The fixes are correct and follow DPDK patterns:
- Error labels are ordered from most-to-least cleanup needed
- Resources are freed in reverse allocation order
- NULL checks after free prevent double-free on re-entry
- Locks are held during pointer swaps to prevent concurrent access races
- Cleanup is centralized via goto on error paths

No style, process, or documentation issues were identified. The code follows DPDK conventions, and the Fixes/Cc tags are present.

Review-Result: CLEAN