|WARNING| net/bnxt: truflow: recover CPM pools for reuse

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

---

## Correctness Issues

### Error - Use-after-free risk in act_get_or_alloc_pool()
**File:** `drivers/net/bnxt/tf_core/v3/tfc_act.c`  
**Lines:** 75-78, 86-87

The function allocates a CMM instance with `rte_zmalloc()` at line 104, but on two error paths (lines 75-78 and 86-87), the function returns without freeing `*cmm`. If the caller does not check for these specific error codes and free the memory, it leaks.

**Suggested fix:**
```c
rc = tfc_cpm_set_cmm_inst(cpm_act, *pool_id, *cmm);
if (unlikely(rc)) {
	PMD_DRV_LOG_LINE(ERR, "tfc_cpm_set_cmm_inst() failed: %d", rc);
	cfa_mm_close(*cmm);  // Close CMM before freeing
	rte_free(*cmm);
	*cmm = NULL;
	return -EINVAL;
}
```

**Also apply to em_get_or_alloc_pool()** at lines 192-196 in `tfc_em.c`.

---

### Error - Resource leak on cfa_mm_open() failure
**File:** `drivers/net/bnxt/tf_core/v3/tfc_act.c` line 111  
**File:** `drivers/net/bnxt/tf_core/v3/tfc_em.c` line 162

After `cfa_mm_open()` fails, the allocated CMM memory is freed, but if `cfa_mm_open()` internally allocates resources before failing, those are not released. DPDK convention is to call `cfa_mm_close()` on a CMM instance before freeing it, even on partial initialization.

**Suggested fix:**
```c
rc = cfa_mm_open(*cmm, &oparms);
if (unlikely(rc)) {
	PMD_DRV_LOG_LINE(ERR, "cfa_mm_open() failed: %d", rc);
	cfa_mm_close(*cmm);  // Close before freeing
	rte_free(*cmm);
	*cmm = NULL;
	return -EINVAL;
}
```

---

### Error - tfc_tbl_scope_pool_alloc() resource leak on error
**File:** `drivers/net/bnxt/tf_core/v3/tfc_act.c` lines 50-55  
**File:** `drivers/net/bnxt/tf_core/v3/tfc_em.c` lines 155-160

If `tfc_tbl_scope_pool_alloc()` succeeds but a subsequent operation (query, malloc, open, set_cmm_inst) fails, the allocated pool from TPM is never freed. The function returns -EINVAL without cleanup, leaking the TPM pool.

**Suggested fix:** Add a cleanup label that frees the pool on post-allocation errors:
```c
rc = tfc_tbl_scope_pool_alloc(tfcp, fid, tsid, CFA_REGION_TYPE_ACT,
			      cmm_info->dir, NULL, pool_id);
if (unlikely(rc)) {
	PMD_DRV_LOG_LINE(ERR, "table scope pool alloc failed: %s", strerror(-rc));
	return -EINVAL;
}

/* ... subsequent operations ... */

cleanup_pool:
	tfc_tbl_scope_pool_free(tfcp, fid, tsid, CFA_REGION_TYPE_ACT,
				cmm_info->dir, *pool_id);
	return -EINVAL;
```

---

### Error - Missing error check on tfc_cpm_set_usage() in cleanup path
**File:** `drivers/net/bnxt/tf_core/v3/tfc_act.c` line 234  
**File:** `drivers/net/bnxt/tf_core/v3/tfc_em.c` line 467, 742

In `tfc_act_alloc()` line 234 and `tfc_em_insert()` line 423, `tfc_cpm_set_usage()` is called and its return value is checked. However, in the cleanup paths (line 234 in tfc_act.c assigns to `rc` but does not propagate), the error is logged but execution continues. If the usage update fails, the CPM's internal state is inconsistent, potentially leading to incorrect pool selection on the next allocation.

**Recommendation:** This is acceptable for cleanup paths where further errors are unlikely to be recoverable. However, consider whether the original error (`rc`) should be preserved rather than overwritten by `cleanup_rc`.

---

## Warnings

### Warning - Potential double-free on repeated error in act_get_or_alloc_pool()
**File:** `drivers/net/bnxt/tf_core/v3/tfc_act.c` lines 104-122

If `cfa_mm_open()` fails, `*cmm` is freed and set to NULL. If `tfc_cpm_set_cmm_inst()` then fails, the code frees `*cmm` again (line 119). However, because `*cmm` was set to NULL on line 113, this is a no-op due to `rte_free(NULL)` being safe. This is not a bug, but the redundant free call is confusing.

**Suggested fix:** Remove the redundant free after setting `*cmm = NULL`:
```c
rc = cfa_mm_open(*cmm, &oparms);
if (unlikely(rc)) {
	PMD_DRV_LOG_LINE(ERR, "cfa_mm_open() failed: %d", rc);
	rte_free(*cmm);
	*cmm = NULL;
	return -EINVAL;
}

rc = tfc_cpm_set_cmm_inst(cpm_act, *pool_id, *cmm);
if (unlikely(rc)) {
	PMD_DRV_LOG_LINE(ERR, "tfc_cpm_set_cmm_inst() failed: %d", rc);
	// No rte_free() here, already NULL or still valid
	return -EINVAL;
}
```

**Also apply to em_get_or_alloc_pool()**.

---

### Warning - qparms.max_records == 0 check insufficient
**File:** `drivers/net/bnxt/tf_core/v3/tfc_act.c` lines 62-67

The code checks if `qparms.max_records` is zero and logs a warning, setting it to 1. However, the check happens after division by `max_pools`, meaning if `mem_cfg->rec_cnt` is nonzero but less than `max_pools`, the quotient is zero and the warning triggers. The warning message is misleading ("rec_cnt=0 for tsid ACT") when `rec_cnt` is actually nonzero but small.

**Suggested fix:**
```c
qparms.max_records = mem_cfg->rec_cnt / max_pools;
if (unlikely(qparms.max_records == 0)) {
	PMD_DRV_LOG_LINE(WARNING,
			 "rec_cnt (%u) < max_pools (%u), using 1 record per pool",
			 mem_cfg->rec_cnt, max_pools);
	qparms.max_records = 1;
}
```

---

### Warning - cfa_mm_query() error handling incomplete
**File:** `drivers/net/bnxt/tf_core/v3/tfc_act.c` line 68  
**File:** `drivers/net/bnxt/tf_core/v3/tfc_em.c` line 149

After `cfa_mm_query()` fails, the function returns `rc` directly without freeing the TPM pool allocated earlier. This is the same pool leak issue noted in the Error section above.

---

### Warning - Uninitialized variable in act_get_or_alloc_pool()
**File:** `drivers/net/bnxt/tf_core/v3/tfc_act.c` line 104  
**File:** `drivers/net/bnxt/tf_core/v3/tfc_em.c` line 176

The variable `*cmm` is assigned the result of `rte_zmalloc()` at line 104. However, if this allocation fails (returns NULL), the code logs an error and returns -ENOMEM, leaving `*cmm` as NULL. The caller in `tfc_act_alloc()` then uses `cmm` at line 218 (`cfa_mm_alloc(cmm, &aparms)`). If `cmm` is NULL, this is a NULL pointer dereference.

**However**, the calling code at line 195 checks `if (unlikely(rc))` after calling `act_get_or_alloc_pool()`, so if allocation fails, the function returns before reaching line 218. This is **not** a bug.

**Do NOT flag this** (correcting my analysis: the control flow prevents the dereference).

---

## Style and Formatting

### Info - Hardcoded Ethernet overhead assumption
**File:** `drivers/net/bnxt/tf_core/v3/tfc_act.c` line 32

The comment and context suggest ACT records are not directly related to MTU or Ethernet framing. This is likely acceptable for ACT (action records), but if any code in the patch computes frame sizes for MTU validation, verify it uses per-device overhead via `dev_info.max_rx_pktlen - dev_info.max_mtu`, not a hardcoded constant.

**No issue found in this patch** for Ethernet overhead (ACT is orthogonal to MTU).

---

### Info - Comment formatting
**File:** `drivers/net/bnxt/hcapi/cfa_v3/mm/cfa_mm.c` lines 432-442

The multi-line comment explaining the `free_list_empty` flag exceeds 80 columns on some lines. DPDK guidelines allow up to 100 characters, so this is acceptable. No change needed.

---

### Info - Boolean logic for all_used
**File:** `drivers/net/bnxt/hcapi/cfa_v3/mm/cfa_mm.c` lines 537-542

The compound boolean expression is clear and explicit. No issue.

---

## Documentation

### Error - Missing Doxygen for cfa_mm_free_blk_count()
**File:** `drivers/net/bnxt/hcapi/cfa_v3/mm/include/cfa_mm.h` lines 164-185

The new API function `cfa_mm_free_blk_count()` has Doxygen documentation. However, the `@return` description does not specify the return value on NULL input or invalid signature.

**Suggested fix:** Match the implementation at `cfa_mm.c` line 636:
```c
 * @return
 *   Number of free blocks in list_0; 0 on invalid input (NULL cmm or bad signature)
```

---

### Warning - Release notes not updated
**File:** Not present in patch

The patch fixes a significant bug (pool exhaustion under churn) and adds a new API (`cfa_mm_free_blk_count()`). However, no release notes are included. For a stable backport (`Cc: stable at dpdk.org`), release notes are typically not required for bugfixes, but documenting the fix helps users understand the impact.

**Recommendation:** Add a brief note in `doc/guides/rel_notes/release_XX_YY.rst` (current release) under "Fixed Issues" describing the pool recovery fix.

---

## Process

### Info - Cc: stable at dpdk.org present
The patch correctly includes `Cc: stable at dpdk.org` for backporting to LTS.

---

## Summary of Required Fixes

1. **Error - Resource leaks in act_get_or_alloc_pool() and em_get_or_alloc_pool()**: Free `*cmm` on error paths before returning (lines 75-78, 86-87 in tfc_act.c; lines 192-196 in tfc_em.c).

2. **Error - TPM pool leak on post-allocation errors**: If operations after `tfc_tbl_scope_pool_alloc()` fail, free the pool before returning.

3. **Error - cfa_mm_open() cleanup incomplete**: Call `cfa_mm_close()` before `rte_free()` on CMM instances to release internal resources.

4. **Warning - Misleading warning message**: Clarify the `qparms.max_records == 0` warning to reflect the actual condition (rec_cnt < max_pools).

5. **Warning - Redundant free calls**: Remove redundant `rte_free(*cmm)` after setting `*cmm = NULL` to avoid confusion.

---

**Final note:** The patch implements a sound recovery mechanism for pool exhaustion. The correctness issues are primarily resource leaks on error paths where cleanup is incomplete. Once these are addressed, the logic is robust.