|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

## ERRORS

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

**Error path resource leaks (multiple instances)**

The error handling is incomplete even after this patch.

Line `goto err_intr;` at line 856 (when VLAN offload V2 fails) now correctly jumps to the new cleanup label. However:

1. **Missing cleanup of `hw->rss_key` and `hw->rss_lut`**: The new `err_intr:` label does not free `hw->rss_key` and `hw->rss_lut`, which were allocated earlier in the function. The cleanup must fall through to `err_rss:` rather than jumping past it.

2. **Missing cleanup of `hw->vf_res`**: All error paths after `hw->vf_res` is allocated must free it. The patch adds `rte_free(hw->vf_vsi_map)` at `err_alloc:`, but this label is reached by `goto err_alloc;` (not shown in patch context) which presumably occurs before `hw->vf_vsi_map` is allocated. If `hw->vf_vsi_map` allocation fails, jumping to `err_alloc:` would then attempt to free the not-yet-allocated map.

**Suggested fix for the new error label**:
```c
err_intr:
	ice_dcf_disable_irq0(hw);
	rte_intr_disable(pci_dev->intr_handle);
	rte_intr_callback_unregister(pci_dev->intr_handle,
				     ice_dcf_dev_interrupt_handler, hw);
	rte_free(hw->qos_bw_cfg);
	hw->qos_bw_cfg = NULL;
	/* fall through to free RSS resources */
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;
	rte_free(hw->vf_res);
err_api:
	iavf_shutdown_adminq(&hw->avf);
```

The current patch creates a label that skips RSS cleanup when the VLAN offload step fails.

---

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

**Label ordering causes incomplete cleanup**

Line 494 (new line after patch): `goto uninit_flow;` when `ice_flow_init()` fails.

The new `uninit_flow:` label at line 507 calls `ice_flow_uninit()`, but `ice_flow_uninit()` should only be called if `ice_flow_init()` **succeeded**. When `ice_flow_init()` itself fails, calling `ice_flow_uninit()` on a partially-initialized flow subsystem may access uninitialized state or double-free resources that `ice_flow_init()` already cleaned up internally on its error path.

The `ice_flow_init()` failure path should jump directly to `uninit_hw:`, not to `uninit_flow:`.

**Suggested fix**:
```c
	err = ice_flow_init(parent_adapter);
	if (err) {
		PMD_INIT_LOG(ERR, "Failed to initialize flow");
		goto uninit_hw;  /* do not call uninit on failed init */
	}
```

The `uninit_flow:` label is only needed on the success path's error exit (e.g., if MAC address add failed).

---

## WARNINGS

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

**Missing error path cleanup**

The patch correctly frees `vf_bw` and `tc_bw` on the success path (line 852-853), but the `fail_clear:` error path (line 856 onward, not shown in diff) does not free them. If the hierarchy commit fails after allocating these buffers, they leak.

The cleanup should be:
```c
fail_clear:
	/* ... existing cleanup ... */
	free(vf_bw);
	free(tc_bw);
	return ret_val;
```

---

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

**Partial resource ownership model**

The refactoring splits close into `ice_dcf_dev_teardown()` (freed on every close/reset) and representor info (freed only on final close). However:

1. **Representor info allocation and notification are coupled**: `ice_dcf_init_repr_info()` allocates the info array, and later in `ice_dcf_dev_init()` the code notifies representors. `ice_dcf_dev_reset()` calls `ice_dcf_dev_teardown()` which calls `ice_dcf_vf_repr_notify_all(adapter, false)`, then re-initializes which calls `ice_dcf_vf_repr_notify_all(adapter, true)` again.

   The comment at line 1683 states "representor info survives ice_dcf_dev_reset", but the representor info **contents** (the `vf_rep_eth_dev` pointers) are set to NULL by the representor close/destroy in the error path of patch 8. This creates an inconsistency: the info array survives, but its contents are cleared.

2. **Unnecessary NULL check after free**: Line 1684 calls `ice_dcf_free_repr_info(dev->data->dev_private);` unconditionally. If `ice_dcf_free_repr_info()` internally checks for NULL, this is acceptable; but if not, and if `ice_dcf_dev_close()` can be called multiple times (DPDK does not forbid it), this could be a double-free.

**Recommendation**: Document in a comment that `ice_dcf_dev_close()` must not be called twice without an intervening `ice_dcf_dev_init()`, or ensure `ice_dcf_free_repr_info()` and `ice_dcf_dev_teardown()` are idempotent.

---

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

**Error path calls generic remove on partially-initialized device**

Line 2148: `rte_eth_dev_pci_generic_remove(pci_dev, ice_dcf_dev_uninit);`

This is called when probe has partially succeeded (DCF port created, some representors may have been created). The generic remove will call `ice_dcf_dev_uninit()`, which expects the device to be in a fully initialized or cleanly closed state. If representor creation failed partway through, `ice_dcf_dev_uninit()` may attempt to clean up state that was never set up (e.g., flow rules, queues).

**More robust approach**: After destroying the representors in the loop (lines 2140-2144), call `ice_dcf_dev_close()` on the DCF ethdev to run the normal teardown, **then** call the generic remove. Or, ensure `ice_dcf_dev_uninit()` is safe to call on a partially-initialized adapter (checks all pointers, does not assume prior `ice_dcf_dev_init()` success).

**Suggested fix**:
```c
err_remove_dcf:
	ice_dcf_free_repr_info(dcf_adapter);  /* free the info we allocated */
	(void)ice_dcf_dev_close(dcf_ethdev);  /* clean teardown */
	rte_eth_dev_pci_generic_remove(pci_dev, ice_dcf_dev_uninit);
```

This ensures the DCF port goes through the normal close path before uninit.

---

## INFO

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

Clean fix. The `rte_free(vc_config);` before each early return correctly prevents the leak.

---

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

The added `rte_free()` before each new allocation (lines 816 and 844) prevents the leak on repeated commits. The loop in `ice_dcf_uninit_hw()` (lines 923-925) correctly frees all entries before freeing the array.

**Minor note**: The loop iterates to `hw->num_vfs`, but the TC node bandwidth is stored at index `ICE_DCF_VFID`. If `ICE_DCF_VFID >= hw->num_vfs`, the TC entry is not freed. Verify that `ICE_DCF_VFID < hw->num_vfs` or adjust the loop bound to `hw->num_vfs + 1` (or to the actual array size).

---

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

The change correctly moves `ice_dcf_tm_conf_uninit()` outside the `if (hw->tm_conf.committed)` block, so uncommitted TM state is also freed. The `ice_dcf_clear_bw()` call remains conditional, as clearing bandwidth requires a committed hierarchy.

---

## SUMMARY

The series correctly identifies and fixes several resource leaks in the ice DCF driver's error paths and close/reset flows. However, **Patches 1 and 2 have correctness bugs** in their error handling that must be fixed. Patch 4 has a missing cleanup on the error path (Warning). Patches 7 and 8 have design issues in their error recovery that should be addressed.

Review-Result: ERROR