|SUCCESS| [7/7] net/ice: fix ACL profile segment leak

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# Review of DPDK Patch Series: Intel ICE Driver Resource Leak Fixes

This patch series addresses resource leaks in the Intel ICE network driver. I'll review each patch for correctness, style compliance, and adherence to DPDK guidelines.

---

## PATCH 1/7: net/ice: fix DDP package double free on init failure

### Errors

**Double-free on error path (Error)**

The patch description and code change are correct. When `ice_init_hw_tbls()` fails, the original code at line 2788 does:
```c
ice_free(hw, hw->pkg_copy);
```
This frees the package copy but leaves `hw->pkg_copy` and `hw->seg` as dangling pointers. The error path then calls `ice_deinit_hw()` (via `goto err_init_fw` at line 2915), which calls `ice_free_seg()`, causing a second free of the same memory.

The fix correctly replaces the bare `ice_free()` with `ice_free_seg()`, which both frees the memory and clears the pointers, preventing the double-free.

**Verdict**: Correct fix for a double-free bug.

---

## PATCH 2/7: net/ice: fix memory leaks on port close

### Warnings

**Pre-existing cleanup pattern (Warning)**

The patch adds cleanup calls to `ice_dev_close()` at lines 3112-3116:
```c
ice_cleanup_fltr_mgmt_struct(hw);
hw->switch_info = NULL;
ice_sched_cleanup_all(hw);
ice_free_hw_tbls(hw);
ice_free_seg(hw);
ice_clear_all_vsi_ctx(hw);
```

These functions free resources that were allocated during `ice_dev_init()`:
- `ice_cleanup_fltr_mgmt_struct()` frees switch filter management structures
- `ice_sched_cleanup_all()` frees scheduler resources
- `ice_free_hw_tbls()` frees hardware tables
- `ice_free_seg()` frees the DDP package copy
- `ice_clear_all_vsi_ctx()` frees VSI contexts

The patch description states these resources leak on every port close and reset because `ice_dev_close()` does not call `ice_deinit_hw()`. However, setting `hw->switch_info = NULL` immediately after calling `ice_cleanup_fltr_mgmt_struct()` could mask bugs where code still tries to access `hw->switch_info` after cleanup. This pattern is acceptable as a safety measure but warrants mention.

The cleanup order appears correct: filters before scheduler, tables before segment.

**Minor style note**: The explicit `hw->switch_info = NULL` is defensive programming that may hide use-after-free bugs, but it matches the existing pattern in `ice_deinit_hw()` and is acceptable.

---

## PATCH 3/7: net/ice: fix DDP filename leak

### Errors

None.

### Analysis

The patch fixes three leak scenarios for `ad->devargs.ddp_filename`:

1. **Leak on init failure**: Added `err_free_devargs` label at line 2919 with cleanup:
   ```c
   free((void *)(uintptr_t)ad->devargs.ddp_filename);
   ad->devargs.ddp_filename = NULL;
   ```
   This frees the string on all error paths after `ice_parse_devargs()`.

2. **Leak on duplicate devarg**: Line 808 adds:
   ```c
   free((void *)(uintptr_t)*filename);
   ```
   Before storing a new `strdup()` result, preventing leak when the same devarg appears multiple times.

3. **Error path rewrites**: Lines 2742, 2751, 2767 change `return -EINVAL` to `ret = -EINVAL; goto err_free_devargs`, ensuring cleanup happens.

The cleanup also nulls the pointer, preventing use-after-free. The cast through `uintptr_t` is correct for freeing a `const char *` field.

**Verdict**: Correct resource leak fixes on all identified paths.

---

## PATCH 4/7: net/ice: fix main VSI leak on init failure

### Errors

None.

### Analysis

The patch adds cleanup for the main VSI on three error paths where `ice_send_driver_ver()`, `ice_flow_init()`, or `ice_reset_fxp_resource()` fails.

Line 2836 changes `goto err_pf_setup` to `goto err_release_vsi`.

Lines 2908-2910 add the new label:
```c
err_release_vsi:
    ice_release_vsi(pf->main_vsi);
    pf->main_vsi = NULL;
```

This ensures the VSI created by `ice_pf_setup()` is released before the MSIX pool cleanup. The pointer is nulled to prevent double-free. The cleanup order is correct: VSI before MSIX pool.

**Verdict**: Correct resource leak fix.

---

## PATCH 5/7: net/ice: fix MAC and VLAN filter leak on VSI release

### Errors

None.

### Warnings

**Freeing filters that failed HW removal (Warning)**

The patch addresses a leak in `ice_release_vsi()`. The existing code calls `ice_remove_all_mac_vlan_filters()` at line 2942, which returns early if the VSI has no MAC filters or if HW removal fails, leaving filter entries allocated.

Lines 2945-2948 add:
```c
RTE_TAILQ_FOREACH_SAFE(m_f, &vsi->mac_list, next, temp)
    rte_free(m_f);
RTE_TAILQ_FOREACH_SAFE(v_f, &vsi->vlan_list, next, temp)
    rte_free(v_f);
```

This frees any filter entries remaining in the lists after `ice_remove_all_mac_vlan_filters()` completes or returns early.

**Note**: The patch description states that `ice_remove_all_mac_vlan_filters()` may leave entries when HW removal fails. The added cleanup ensures software structures are freed even when HW state diverges. This is correct, but it means the VSI may have filters configured in hardware that are no longer tracked in software. This is acceptable as the VSI itself is being released (and presumably the HW VSI state is torn down separately).

**Verdict**: Correct fix for the leak. The HW/SW state divergence is inherent to the error case and not introduced by this patch.

---

## PATCH 6/7: net/ice: fix FDIR resource leak on setup failure

### Errors

None.

### Analysis

`ice_fdir_setup()` allocates several resources:
- Filter list (line 477)
- Counter structures (line 484)
- Tx queue (line 493)
- Rx queue (line 510)
- Hash table and map (implicitly in counter init)
- FDIR VSI (line 465)

The original error paths at lines 480 and 487 return directly, leaking everything allocated before the failure. Other error paths release the queues and VSI but not the hash table, map, or counters.

The patch fixes this:

1. Lines 480-481: Change `return -EINVAL` to `err = -EINVAL; goto fail_filter_list`.
2. Lines 486-487: Change `return -EINVAL` to `err = -EINVAL; goto fail_counter`.
3. Lines 559-562: Add cleanup labels:
   ```c
   fail_counter:
       ice_fdir_counter_release(pf);
   fail_counter:
       ice_fdir_release_filter_list(pf);
   fail_filter_list:
   ```

The cleanup order is correct: counter release before filter list release, both before VSI release.

The patch description mentions that the hash table name stays registered on failure, causing the next setup to fail. This is correct: `rte_hash_create()` in counter init would fail with `-EEXIST` if the name is already registered from a failed previous attempt.

**Verdict**: Correct fix for multiple resource leaks.

---

## PATCH 7/7: net/ice: fix ACL profile segment leak

### Errors

None.

### Analysis

`ice_acl_prof_init()` allocates a flow segment at line 215:
```c
seg = (struct ice_flow_seg_info *)ice_calloc(hw, segs_cnt, sizeof(*seg));
```

The segment is used to build ACL profiles, then passed to `ice_flow_add_prof()` which copies the segment data (lines 284, 296). The original code only frees `seg` on the error path (line 312) but not on success, leaking it every time ACL is initialized.

Line 311 adds:
```c
ice_free(hw, seg);
```
immediately before the success `return 0` at line 312 (now line 312 in the patched code).

The fix is correct: the segment buffer is temporary and should be freed after the profiles are added, whether that succeeds or fails.

**Verdict**: Correct fix for a memory leak on the success path.

---

## General Style and Process Review

### Style Compliance

All patches follow DPDK C style:
- 8-character tabs for indentation
- Braces on correct lines
- Proper goto label indentation
- No trailing whitespace
- Comments use standard format
- Variable declarations at block start

### API and Documentation

These are internal driver bug fixes with no API changes. Release notes are not required per the guidelines (internal fixes, no public API impact).

The Fixes tags are present and reference the commits that introduced the bugs.

### Severity Assessment

All patches fix resource leaks (memory, hardware resources, registration entries). These are correctness bugs that cause cumulative resource exhaustion on repeated init/close cycles or error conditions.

**No style or process violations identified.**

---

## Summary

This patch series correctly fixes multiple resource leaks in the Intel ICE driver:

1. **PATCH 1/7**: Fixes double-free of DDP package on init failure
2. **PATCH 2/7**: Fixes multiple leaks on port close/reset
3. **PATCH 3/7**: Fixes DDP filename leak on error and duplicate devargs
4. **PATCH 4/7**: Fixes main VSI leak on init failure
5. **PATCH 5/7**: Fixes MAC/VLAN filter leak when VSI release skips HW cleanup
6. **PATCH 6/7**: Fixes FDIR resource leaks on setup failure
7. **PATCH 7/7**: Fixes ACL profile segment leak on success path

All patches correctly identify the leak, add cleanup on the appropriate error paths, and maintain proper cleanup ordering. No correctness bugs or style violations were found.

Review-Result: CLEAN