|WARNING| [v2,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/172341

_AI Code Review_

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

# DPDK Patch Review

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

### Correctness
**Error**: Use-after-free risk in ice_deinit_hw() path

The patch addresses a double-free scenario where `hw->pkg_copy` is freed but pointers remain dangling. The fix changes from `ice_free(hw, hw->pkg_copy)` to `ice_free_seg(hw)`, which should null out pointers.

However, examining the error path:
```c
ret = ice_init_hw_tbls(hw);
if (ret) {
    PMD_INIT_LOG(ERR, "ice_init_hw_tbls failed: %d", ret);
    ice_free_seg(hw);
}
```

After this block, if `!ad->devargs.safe_mode_support`, the code continues to the `err_init_fw` label (based on patch context), which calls `ice_deinit_hw(hw)`. If `ice_free_seg()` already freed and nulled the pointers, then `ice_deinit_hw()` calling `ice_free_seg()` again should be safe (assuming it checks for NULL). The patch assumes `ice_free_seg()` is idempotent.

**Question**: Does `ice_free_seg()` check for NULL before freeing? Without seeing the implementation, there's a risk that calling it twice could still be problematic if it doesn't guard against NULL. However, the patch description states it "clears these pointers", so this should be safe.

This is a valid correctness fix.

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

### Correctness
**Valid fix**: The patch correctly identifies that `ice_dev_close()` was manually tearing down state instead of calling `ice_deinit_hw()`, missing several cleanup steps:
- Switch filter management structures
- DDP package copy
- VSI contexts
- Flow director counter block

The change to call `ice_deinit_hw(hw)` instead of the manual teardown is correct and consolidates cleanup logic.

**Note**: The patch removes explicit calls that were already part of `ice_deinit_hw()`:
- `ice_sched_cleanup_all(hw)`
- `ice_free_hw_tbls(hw)`
- `ice_free(hw, hw->port_info)`
- `ice_shutdown_all_ctrlq(hw, true)`

These are now handled by the single `ice_deinit_hw(hw)` call, which is the correct approach.

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

### Correctness
**Error**: Missing NULL check after strdup() failure

Line 808-809:
```c
free(*filename);
*filename = strdup(value);
```

If `strdup()` returns NULL (out of memory), `*filename` is set to NULL but no error is returned. The caller won't know allocation failed and will proceed with a NULL filename pointer.

```c
/* Current code - missing error check */
free(*filename);
*filename = strdup(value);
return 0;

/* Should be */
free(*filename);
*filename = strdup(value);
if (*filename == NULL)
    return -ENOMEM;
return 0;
```

**Valid fix for existing leak**: The `free(*filename)` before assignment correctly prevents the leak when the devarg is given multiple times, and the new error path cleanup at `err_free_devargs` is correct.

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

### Correctness
**Valid fix**: The patch correctly identifies that when `ice_send_driver_ver()`, `ice_flow_init()`, or `ice_reset_fxp_resource()` fails, the error path jumps to labels that don't release the main VSI created by `ice_pf_setup()`.

The new `err_release_vsi` label properly releases the VSI and nulls the pointer before proceeding to `err_pf_setup`.

**Note**: Setting `pf->main_vsi = NULL` after `ice_release_vsi()` is correct defensive programming to prevent use-after-free.

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

### Correctness
**Valid fix**: The patch correctly identifies that `ice_remove_all_mac_vlan_filters()` can return early, leaving filter entries in the lists that are never freed when the VSI is freed.

The fix adds explicit cleanup:
```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 ensures any remaining filter entries are freed before the VSI is released.

**Note**: 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

### Correctness
**Valid fix**: The patch identifies multiple resource leaks in `ice_fdir_setup()` error paths:

1. Early returns skip cleanup entirely (filter list, counter init failures)
2. Later error paths release queues and VSI but not hash table, hash map, or counter pools

The new error handling chain properly cleans up all resources:
```c
fail_setup_tx:
    ice_fdir_counter_release(pf);
fail_counter:
    ice_fdir_release_filter_list(pf);
fail_filter_list:
    ice_release_vsi(vsi);
    pf->fdir.fdir_vsi = NULL;
```

**Note**: The patch correctly changes early returns to `goto` statements to ensure cleanup happens.

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

### Correctness
**Valid fix**: The patch correctly identifies that `seg` is allocated with `ice_malloc()` but only freed on the error path, leaking on every successful ACL engine init.

The fix changes from heap allocation to stack allocation, which is appropriate since `ice_flow_add_prof()` copies the segment data. This eliminates the need to free it entirely.

**Verification**: The change from:
```c
struct ice_flow_seg_info *seg;
seg = (struct ice_flow_seg_info *)ice_malloc(hw, sizeof(*seg));
```

To:
```c
struct ice_flow_seg_info seg = {0};
```

and updating all `seg` references to `&seg` is correct. The structure is used only within the function scope and doesn't escape.

---

## Pass 1: Correctness bugs verification

All correctness issues have been identified and are included above.

## Pass 2: Style/process false positives removal

No style/process items need removal - all findings are correctness issues.

## Pass 3: Evidence verification

All findings quote actual code lines from the patches and specify the defect clearly.

---

# Summary

**ERRORS**:

### Patch 3/7, handle_ddp_filename_arg()
Missing error check after `strdup()` at line 809:
```c
*filename = strdup(value);
```
If `strdup()` returns NULL due to out-of-memory, the function returns 0 (success) with `*filename` set to NULL. The caller proceeds with a NULL pointer. Should check and return `-ENOMEM` on allocation failure.

---

**Remaining patches (1, 2, 4, 5, 6, 7)**: All correctness fixes are valid and properly address resource leaks and use-after-free scenarios.

Review-Result: ERROR