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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

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

**Errors:**

None.

**Warnings:**

None.

**Info:**

The fix correctly replaces a bare `ice_free(hw, hw->pkg_copy)` with `ice_free_seg(hw)`, which also clears the `hw->pkg_copy` and `hw->seg` pointers. This prevents the double-free when the error path calls `ice_deinit_hw()` -> `ice_free_seg()` again. The fix is sound.

---

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

**Errors:**

1. **Possible use-after-free of `hw->switch_info`**  
   **Line:** `hw->switch_info = NULL;`  
   **Issue:** The patch sets `hw->switch_info` to NULL immediately after calling `ice_cleanup_fltr_mgmt_struct(hw)`. If `ice_cleanup_fltr_mgmt_struct()` does not free `hw->switch_info` itself (only the filter lists it points to), then the pointer is leaked before being cleared. If it *does* free `hw->switch_info`, then the patch correctly clears a dangling pointer. Without reading `ice_cleanup_fltr_mgmt_struct()`, I cannot determine which is true. However, the pattern of clearing a pointer after cleanup is defensive against later accesses. If `ice_cleanup_fltr_mgmt_struct()` does free it, the code is correct. If it does not, the `hw->switch_info` structure itself leaks.  
   **Suggested fix:** Verify that `ice_cleanup_fltr_mgmt_struct()` frees the `hw->switch_info` structure itself, not just its contents. If it does not, add `ice_free(hw, hw->switch_info);` before the NULL assignment.

**Warnings:**

None.

**Info:**

The patch adds cleanup of filter management structures, DDP package, and VSI contexts on close, which were previously missing. The order of operations (filter cleanup, scheduler cleanup, hardware tables, segment free, VSI context clear) appears logical.

---

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

**Errors:**

None.

**Warnings:**

None.

**Info:**

The patch correctly frees the duplicated `ddp_filename` string on all init error paths and before overwriting it in `handle_ddp_filename_arg()`. The use of `(void *)(uintptr_t)` cast to discard `const` is consistent with the existing close path. The pattern is correct.

---

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

**Errors:**

None.

**Warnings:**

None.

**Info:**

The patch adds `ice_release_vsi(pf->main_vsi)` and clears the pointer on the error paths where `ice_send_driver_ver()`, `ice_flow_init()`, or `ice_reset_fxp_resource()` fails. This correctly releases the VSI allocated by `ice_pf_setup()`. The label naming (`err_release_vsi:`) is consistent with the existing style.

---

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

**Errors:**

1. **Incorrect allocator mismatch**  
   **Lines:** `rte_free(m_f);` and `rte_free(v_f);`  
   **Issue:** The patch frees filter entries with `rte_free()`, but without seeing the allocation sites, I cannot verify this matches the allocator used. If filters are allocated with `rte_zmalloc()` or similar, `rte_free()` is correct. If they are allocated with `malloc()`, `free()` is correct. DPDK's `rte_free()` should handle both (`rte_malloc()` and `rte_zmalloc()` are interchangeable with `rte_free()`), but the pattern must match what was used at allocation.  
   **Suggested action:** Verify that MAC and VLAN filter entries are allocated with `rte_malloc()` or `rte_zmalloc()`. If they use standard `malloc()`, change to `free()`.

**Warnings:**

None.

**Info:**

The patch adds cleanup of filter entries that `ice_remove_all_mac_vlan_filters()` left in the lists, either because it returned early or because hardware removal failed. The use of `RTE_TAILQ_FOREACH_SAFE` is correct for list traversal with deletion.

---

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

**Errors:**

None.

**Warnings:**

None.

**Info:**

The patch adds cleanup of the filter list, counter pools, hash table, and hash map on all `ice_fdir_setup()` error paths. The gotos (`fail_counter:`, `fail_filter_list:`) correctly chain the cleanup in reverse order of allocation. The `err = -EINVAL;` assignments are correct (though redundant in one case where `err` is already set by the failing function, but harmless).

---

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

**Errors:**

None.

**Warnings:**

None.

**Info:**

The patch frees the `seg` buffer after successful profile addition. The comment in the fix ("ice_flow_add_prof() copies the segments") confirms that the buffer is no longer needed. The existing error path already freed it, so adding the free on success is correct.

---

## Summary

All seven patches address resource leaks (memory, hardware state) on error paths and during teardown. The fixes are generally correct, with two items requiring verification:

1. **Patch 2:** Confirm that `ice_cleanup_fltr_mgmt_struct()` frees the `hw->switch_info` structure itself, not just its contents.
2. **Patch 5:** Confirm that MAC/VLAN filter entries are allocated with `rte_malloc()` or `rte_zmalloc()`, matching the `rte_free()` used in the patch.

Both are correctness concerns about code not visible in the patch.

Review-Result: ERROR