|SUCCESS| [v2] net/bnxt/tf_core: fix buffer leaks in RM create DB

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: net/bnxt/tf_core: fix buffer leaks in RM create DB

## ERRORS

**1. Incomplete cleanup on existing error path (line 736)**

The existing `fail:` label frees `req`, `resv`, `db->pool`, frees the RM DB, and frees `req_cnt`,
but does NOT free `query`. This is a resource leak on any error that reaches `fail:`.

**Fix applied:** Patch adds `tfp_free((void *)query);` at line 734.
This is correct.

**2. Missing cleanup of `rm_db` on early error returns**

The patch adds `free_bufs:` to clean up query, req, resv, and req_cnt on early errors.
However, after `rm_db` is allocated (line 613), if the `db` allocation fails (lines 617-620),
the patch frees `rm_db` directly before jumping to `free_bufs`.

This is correct -- `rm_db` is freed, then the temporary buffers are freed.

However, there is a code path where `rm_db` is allocated but NOT freed on error:
**If any error occurs between line 613 (rm_db allocated) and line 617 (db allocation),
the code jumps to `free_bufs`, which does not free `rm_db`.**

Looking at the code between those lines: there are no error paths between rm_db allocation
and the db allocation check, so this is not actually reachable in the current code.

**Not a finding** -- no error path exists between rm_db allocation and the explicit rm_db free.

**3. NULL-initialized pointer freed on never-taken error path**

The new `free_bufs:` label calls `tfp_free((void *)query)` etc. on pointers initialized to `NULL`.
If the first allocation (query, line 490) fails, `query` is still `NULL` when `free_bufs:` is reached.

**Check `tfp_free()` semantics:** DPDK convention (matching `free()` and `rte_free()`)
is that freeing a `NULL` pointer is a no-op. The patch relies on this.

This is safe and is standard DPDK practice.

**Not a finding.**

## WARNINGS

None.

## INFO

**1. Possible simplification: remove `fail:` label**

After this patch, the `fail:` label (line 734) and `free_bufs:` label (line 746)
both free the same temporary buffers: `query`, `req`, `resv`.
The `fail:` path also frees `db->pool`, `rm_db->db`, `rm_db`, and `req_cnt`,
and sets `parms->rm_db = NULL` before returning `-EINVAL`.

The `free_bufs:` path only frees the temporary buffers and returns the preserved error code.

These could potentially be unified into a single cleanup path with conditional frees,
but that would be a refactoring beyond the scope of this bug fix.

Not required for correctness; the current structure is clear.

**2. Error code propagation is correct**

All early error returns now preserve the original error code (`rc`) and propagate it via `free_bufs:`.
The existing `fail:` path returns `-EINVAL` unconditionally, which is pre-existing behavior.

The patch does not change the returned error codes, only ensures cleanup happens.

---

## SUMMARY

The patch correctly fixes multiple resource leaks in `tf_rm_create_db()`:

1. **Query buffer** was never freed -- fixed by adding `tfp_free(query)` on both success and error paths.
2. **Early error returns** leaked whichever buffers were allocated -- fixed by routing them through `free_bufs:`.
3. **`rm_db` leak on db allocation failure** -- fixed by explicit free before jumping to `free_bufs:`.

All cleanup is correct. `tfp_free(NULL)` is safe per DPDK convention.
The patch does not introduce new issues.

Review-Result: CLEAN