|WARNING| [v3, 5/5] net/bnxt: fix VF info alloc error path memory leak

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-29

# DPDK Patch Review: net/bnxt Series v3

## PATCH 1/5: net/bnxt: add VF ID boundary check before usage

### Errors

**Out-of-bounds array access before validation**

The original code computed `vf_id` from firmware-supplied `fw_vf_id` and used it to index `bp->pf->vf_info[]` before checking that `fw_vf_id` was within the valid range. This is a critical correctness bug: an out-of-range VF ID could index past the array bounds and corrupt memory.

The patch correctly moves the range check ahead of the `vf_id` computation and `vf_info[]` access, which fixes the vulnerability.

### Warnings

**Unnecessary NULL initializers mask potential bugs**

```c
struct input *fwd_cmd = NULL;
uint16_t req_len = 0;
```

These initializations are added to silence a compiler warning about uninitialized variables on the reject path, but they hide the real issue: the reject path should not need valid values for `fwd_cmd` or `req_len` because those variables have not been set when the early validation fails.

The real fix is at the use site in `bnxt_hwrm.c`:

```c
if (encaped)
    memcpy(req.encap_request, encaped, ec_size);
```

This NULL-checks `encaped` before use, which is correct. The `fwd_cmd` and `req_len` NULL/0 initializers in `bnxt_cpr.c` are now dead stores on the success path (they are unconditionally overwritten after validation passes). However, they do serve a purpose on the reject path where `fwd_cmd` is dereferenced in the error log:

```c
fwd_cmd ? rte_le_to_cpu_16(fwd_cmd->req_type) : 0xFFFF
```

This ternary is safe with the NULL initializer. The alternative would be to remove the initializers and not log `req_type` on early reject (just log the `fw_vf_id`), but the current approach is defensible as a minor readability trade-off.

**Recommendation:** Acceptable as-is. The NULL initializers do prevent a real dereference issue in the error log on the early-reject path, even though they are dead stores on the success path.

---

## PATCH 2/5: net/bnxt: fix bounds on firmware-reported resource counts

### Info

**v3 correctly fixes the byte-swap bug**

v3 changes the accessor in `bnxt_hwrm_func_resc_qcaps()` from `rte_le_to_cpu_32()` to `rte_le_to_cpu_16()` and removes the clamp that was hiding the real issue in v2. This is the correct fix: `max_hw_ring_grps` is declared `uint16_t` in the `func_resc_qcaps` response (unlike the `uint32_t` field in `func_qcaps`), so reading it with the 32-bit accessor on a big-endian host would swap a 16-bit value as if it were 32-bit, shifting the real value into the wrong half of the register. The clamp in v2 would have forced the garbage result down to 65535 instead of surfacing the correct value.

With the 16-bit accessor, a correctly-read 16-bit value can never exceed 65535, so no clamp is needed on this path. The clamps added in `__bnxt_hwrm_func_qcaps()` remain appropriate because that response's `max_hw_ring_grps` field is `uint32_t` and could legitimately contain a value >65535 that needs to be capped to fit the driver's 16-bit storage.

**Integer overflow clamped correctly**

The `max_l2_ctx` calculation adds two 16-bit values and could overflow 16 bits. The patch widens the addition to 32-bit and clamps the result with `RTE_MIN()` before narrowing back to 16-bit:

```c
uint32_t l2_ctx = bp->max_l2_ctx + bp->max_rx_em_flows;
bp->max_l2_ctx = (uint16_t)RTE_MIN(l2_ctx, (uint32_t)BNXT_MAX_L2_CTX);
```

This is correct. The explicit cast to `uint16_t` after the `RTE_MIN()` is safe because the clamped value is guaranteed <= 65535.

No issues found.

---

## PATCH 3/5: net/bnxt: fix VNIC filter list walk stopping early

### Errors

None. The patch correctly fixes the early-termination bug.

### Info

**v3 correctly identifies the real bug**

The v3 commit message accurately describes the issue: `bnxt_free_filter()` does not actually release memory (it recycles the filter object onto `bp->free_filter_list` after zeroing it), so this was never a use-after-free. The real bug is that zeroing the filter's `next` field causes `STAILQ_FOREACH()`'s implicit advance (`filter = filter->next`) to see NULL after the first iteration, so the loop silently stops after removing one filter. All remaining filters in `vnic->filter` are leaked: their hardware entries are never cleared and the filter objects never return to `bp->free_filter_list`.

**STAILQ_FOREACH_SAFE() portability issue**

The v3 description mentions that `STAILQ_FOREACH_SAFE()` is not portable: glibc's `sys/queue.h` has no `STAILQ` _SAFE variant, and DPDK's `RTE_TAILQ_FOREACH_SAFE()` wrapper is only for `TAILQ`, not `STAILQ`. The chosen fix--using `STAILQ_FIRST()`/`STAILQ_REMOVE_HEAD()`--is functionally equivalent for this always-drain-the-head pattern and is the correct portable solution.

```c
while (!STAILQ_EMPTY(&vnic->filter)) {
    filter = STAILQ_FIRST(&vnic->filter);
    rc = bnxt_clear_one_vnic_filter(bp, filter);
    STAILQ_REMOVE_HEAD(&vnic->filter, next);
    bnxt_free_filter(bp, filter);
}
```

This is correct. The loop terminates when the list is empty, and each iteration removes the head before freeing it, so the list advance does not depend on a zeroed-out `next` field.

No issues found.

---

## PATCH 4/5: net/bnxt: fix memory leak in VF VNIC query error path

### Errors

None. The patch correctly adds the missing `rte_free(vnic_ids)` on the error path.

### Info

The fix is straightforward: `vnic_ids` is allocated before calling `bnxt_hwrm_func_vf_vnic_query()`, but if that call fails (`num_vnic_ids < 0`), the original code returned without freeing `vnic_ids`. The patch correctly frees it before returning the error code.

```c
if (num_vnic_ids < 0) {
    rte_free(vnic_ids);
    return num_vnic_ids;
}
```

No issues found.

---

## PATCH 5/5: net/bnxt: fix VF info alloc error path memory leak

### Errors

None. The patch correctly assigns `bp->pf->vf_info` before the nested allocations so the cleanup function can free partial allocations on error.

### Info

The bug: `bp->pf->vf_info` was assigned *after* the loop that allocates `vf_info[i].vlan_table` and `vf_info[i].mc_list` for each VF. If any of those nested allocations failed, the code called `bnxt_free_vf_info()`, which needs `bp->pf->vf_info` to be set in order to walk the array and free the already-allocated entries. Without that pointer, `bnxt_free_vf_info()` would see `bp->pf->vf_info == NULL` and skip the cleanup, leaking the partial allocations.

The fix moves `bp->pf->vf_info = vf_info;` ahead of the loop, so it is always set before any nested allocation that could fail and trigger the cleanup path.

```c
bp->pf->max_vfs = max_vfs;
bp->pf->vf_info = vf_info;  /* moved here from after the loop */
for (i = 0; i < max_vfs; i++) {
    vf_info[i].vlan_table = rte_zmalloc(...);
    if (vf_info[i].vlan_table == NULL)
        goto err;
    ...
}
```

This is correct. The cleanup function can now correctly free any partially-allocated `vlan_table` or `mc_list` entries because `bp->pf->vf_info` is already set when `goto err` is taken.

No issues found.

---

## SUMMARY

### Patch 1/5
- **Errors:** None (the out-of-bounds access is fixed by moving the validation).
- **Warnings:** The NULL initializers for `fwd_cmd` and `req_len` are dead stores on the success path but do prevent a dereference issue in the error log on the early-reject path. Acceptable as-is.

### Patch 2/5
- **Errors:** None.
- **Warnings:** None.
- **Info:** v3 correctly fixes the byte-swap bug by using `rte_le_to_cpu_16()` instead of `rte_le_to_cpu_32()` and removes the unnecessary clamp. Integer overflow in `max_l2_ctx` addition is correctly handled by widening to 32-bit and clamping.

### Patch 3/5
- **Errors:** None.
- **Warnings:** None.
- **Info:** The `STAILQ_FIRST()`/`STAILQ_REMOVE_HEAD()` loop is the correct portable solution for the early-termination leak.

### Patch 4/5
- **Errors:** None.
- **Warnings:** None.
- **Info:** Correct fix for error-path memory leak.

### Patch 5/5
- **Errors:** None.
- **Warnings:** None.
- **Info:** Correct fix for cleanup-path memory leak caused by late pointer assignment.

---

All five patches correctly fix real correctness bugs (out-of-bounds access, integer overflow/byte-swap, early-termination leak, error-path leaks). The series is in good shape.