|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

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

### Errors

**Use-after-free risk on reject path:**
The patch initializes `fwd_cmd = NULL` and `req_len = 0` at declaration, then adds a guard `if (encaped)` before the `memcpy()` in `bnxt_hwrm_reject_fwd_resp()`. However, the reject path still dereferences `fwd_cmd` in the error log:
```c
fwd_cmd ? rte_le_to_cpu_16(fwd_cmd->req_type) : 0xFFFF
```
When the early reject is taken (fw_vf_id out of range), `fwd_cmd` remains `NULL` and this ternary correctly handles it. But this pattern is fragile: if a later change removes the `? :` check, the code will dereference NULL. The safer fix is to log only the `fw_vf_id` on the early-reject path and skip the `req_type` entirely when `fwd_cmd` is not yet valid.

Suggested refactor (in `bnxt_cpr.c`):
```c
if (fw_vf_id < bp->pf->first_vf_id ||
    fw_vf_id >= bp->pf->first_vf_id + bp->pf->active_vfs) {
	PMD_DRV_LOG_LINE(ERR,
		"FWD req's source_id 0x%x out of range 0x%x - 0x%x (%d %s)",
		fw_vf_id, bp->pf->first_vf_id,
		bp->pf->first_vf_id + bp->pf->active_vfs - 1,
		bp->pf->active_vfs, bp->pf->active_vfs == 1 ? "VF" : "VFs");
	/* fwd_cmd is NULL here; log only fw_vf_id, no req_type */
	rc = bnxt_hwrm_reject_fwd_resp(bp, fw_vf_id, NULL, 0);
	if (rc)
		PMD_DRV_LOG_LINE(ERR,
			"Failed to send REJECT for out-of-range VF 0x%x.",
			fw_vf_id - bp->pf->first_vf_id);
	return;
}
```
And in the later reject path (after `fwd_cmd` is valid), keep the original log with `fwd_cmd->req_type`.

This eliminates the need for the ternary and makes the code self-documenting: when `fwd_cmd` is NULL, we don't log `req_type` at all.

### Warnings

None.

---

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

### Errors

None. The patch correctly fixes the `rte_le_to_cpu_16()` accessor bug in `bnxt_hwrm_func_resc_qcaps()` and adds appropriate clamping to `__bnxt_hwrm_func_qcaps()`.

### Warnings

**Hardcoded constant instead of manifest constant:**
The patch defines `BNXT_MAX_RING_GRPS` and `BNXT_MAX_L2_CTX` as `65535U` in `bnxt.h`, which is correct. However, the clamp in `__bnxt_hwrm_func_qcaps()` could use `UINT16_MAX` for clarity:
```c
if (bp->max_ring_grps > BNXT_MAX_RING_GRPS)
	bp->max_ring_grps = BNXT_MAX_RING_GRPS;
```
This is acceptable as written (the constants document the domain-specific limit), but `UINT16_MAX` would make the 16-bit nature explicit. Not a bug, just a style preference.

---

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

### Errors

None. The patch correctly fixes the early-termination bug by using `STAILQ_FIRST()`/`STAILQ_REMOVE_HEAD()` to drain the list, which is the correct portable pattern when `STAILQ_FOREACH_SAFE()` is not available.

### Warnings

None.

---

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

### Errors

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

### Warnings

None.

---

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

### Errors

None. The patch correctly moves the `bp->pf->vf_info = vf_info;` assignment before the loop so that `bnxt_free_vf_info()` can find and free the partially-allocated array on error.

### Warnings

None.

---

## Summary

**Must fix (1 Error):**
- Patch 1: The reject-path error log still risks NULL dereference of `fwd_cmd` if the ternary is later removed. Refactor to log only `fw_vf_id` on the early-reject path and skip `req_type` when `fwd_cmd` is NULL.

**Should consider (1 Warning):**
- Patch 2: The clamp constants `BNXT_MAX_RING_GRPS` and `BNXT_MAX_L2_CTX` could optionally use `UINT16_MAX` for explicitness, though the current code is acceptable.

**Patches 2-5 are correct** and fix real resource leaks and correctness bugs with no issues.