|WARNING| [v1,17/17] net/dpaa: fix mbuf leak in SG fd creation

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-18

# DPDK Patch Review

## Summary

Reviewing 17 patches from the series targeting DPAA bus and net drivers. The patches address various bug fixes including error handling, endianness, resource leaks, and incorrect logic.

---

## Patch 01/17: bus/dpaa: fix error handling of qman_create_fq

**Errors:**

1. **Conditional cleanup path may be incomplete**
   The `#ifdef CONFIG_FSL_QMAN_FQ_LOOKUP` guard means that on some build configurations, the FQ table entry will not be cleared on error, but on others it will. This creates inconsistent cleanup behavior across configurations. If the FQ table entry was set before the `err:` label, it should always be cleared on error regardless of build config, or the guard should protect both the allocation and cleanup together. Verify that this conditional cleanup matches the allocation pattern.

**Warnings:**

None.

---

## Patch 02/17: bus/dpaa: fix fqid endianness

**Errors:**

None.

**Warnings:**

None.

---

## Patch 03/17: bus/dpaa: fix error handling in qman_query

**Errors:**

None.

**Warnings:**

None.

---

## Patch 04/17: net/dpaa: fix modify cgr to use index

**Errors:**

None.

**Warnings:**

None.

---

## Patch 05/17: net/dpaa/fmlib: add null check in scheme delete

**Errors:**

None.

**Warnings:**

None.

---

## Patch 06/17: bus/dpaa: fix BMI RX stats register offset

**Errors:**

None.

**Warnings:**

None.

---

## Patch 07/17: bus/dpaa: fix fd leak for ccsr mmap

**Errors:**

1. **File descriptor not closed on mmap failure**
   In `bman_driver.c`, the `close(ccsr_map_fd)` is added after `mmap()` but before the `MAP_FAILED` check. If `mmap()` fails, execution falls through to the error path without closing the fd. The close should occur after the failure check, or the error path should explicitly close the fd.
   ```c
   bman_ccsr_map = mmap(NULL, regs_size, PROT_READ |
                        PROT_WRITE, MAP_SHARED, ccsr_map_fd, phys_addr);
   close(ccsr_map_fd);  /* fd closed before checking mmap result */
   if (bman_ccsr_map == MAP_FAILED) {
       /* Error path - fd already closed, but original code had the fd still open here */
       pr_err("Can not map BMan CCSR base Bman: "
              "0x%x Phys: 0x%" PRIx64 " size 0x%" PRIu64,
              ccsr_map_fd, phys_addr, regs_size);
       return -EINVAL;
   }
   ```
   Actually, reviewing the original code: the fd was declared `static` and never closed, so it leaked. The patch fixes this by closing immediately after mmap. Since `mmap()` increments the fd refcount, closing the fd after a successful mmap is safe. On failure, the fd should still be closed. The current patch is correct as-is because the fd should be closed whether mmap succeeds or fails (mmap on success holds a reference, on failure the fd is just unused). No change needed - the patch is correct.

**Warnings:**

None.

---

## Patch 08/17: bus/dpaa: fix device probe issue

**Errors:**

None.

**Warnings:**

None.

---

## Patch 09/17: net/dpaa: fix device remove

**Errors:**

1. **Missing `dpaa_finish()` call**
   The original code called `dpaa_finish()` when `dpaa_valid_dev` reached zero. The patch removes this call unconditionally. If `dpaa_finish()` performs necessary global cleanup (releasing shared resources, unmapping memory, etc.), removing this call leaks those resources when the last device is removed. Either `dpaa_finish()` should still be called when `dpaa_valid_dev == 0`, or the commit message should explain why it is no longer needed.

**Warnings:**

None.

---

## Patch 10/17: net/dpaa: fix invalid check on interrupt unregister

**Errors:**

None.

**Warnings:**

None.

---

## Patch 11/17: net/dpaa: fix coverity reported issues

**Errors:**

1. **dpaa_flow.c: port_handle closed then used**
   In `fm_prev_cleanup()`, the patch adds:
   ```c
   if (dpaa_intf.port_handle) {
       fm_port_close(dpaa_intf.port_handle);
       dpaa_intf.port_handle = NULL;
   }
   dpaa_intf.port_handle =
           fm_port_open(&fm_model.fm_port_params[devid]);
   ```
   If `dpaa_intf.port_handle` is non-NULL, it is closed and set to NULL, then immediately overwritten by `fm_port_open()`. This is correct if the intent is to close the old handle before opening a new one. However, verify that `fm_port_open()` is allowed to be called multiple times (i.e., that this function is called in a loop or re-initialization path). If this is a singleton open and the handle should only be opened once, the check should prevent the open if a handle already exists. Confirm the intended pattern.

2. **dpaa_qdma.c: unsigned comparison against FSL_QDMA_SG_MAX_ENTRY**
   The check `num > FSL_QDMA_SG_MAX_ENTRY` is added, but it is unclear from the patch what `num`'s type is. If `num` is unsigned and derived from user input or hardware registers, ensure it cannot wraparound or be manipulated to bypass this check. If `num` comes from untrusted sources, also verify that it is not negative-as-unsigned (though the `num == 0` check handles zero). Confirm `num`'s type and source.

**Warnings:**

None.

---

## Patch 12/17: net/dpaa: fix xstat name for tx undersized counter

**Errors:**

None.

**Warnings:**

None.

---

## Patch 13/17: net/dpaa: fix xstat string typos in BMI stats table

**Errors:**

None.

**Warnings:**

None.

---

## Patch 14/17: net/dpaa: remove duplicate ptype entries

**Errors:**

None.

**Warnings:**

None.

---

## Patch 15/17: net/dpaa: fix wrong buffer in xstats get by id

**Errors:**

1. **Incorrect loop variable in second loop**
   The second loop in `dpaa_xstats_get_by_id()` after the fix reads:
   ```c
   fman_if_bmi_stats_get_all(dev->process_private, values_copy);
   for (j = 0; i < stat_cnt; i++, j++)
       values[i] = values_copy[j];
   ```
   The loop condition is `i < stat_cnt` but `i` was already incremented by the previous loop (which iterated to `DPAA_XSTATS_MAX`). This loop should initialize `j = 0` (which it does) and then iterate while `i < stat_cnt`, incrementing both `i` and `j`. However, the loop uses `i < stat_cnt` which is the total stat count, not the BMI stat count. This will copy data from `values_copy[j]` where `j` iterates from 0 to `(stat_cnt - DPAA_XSTATS_MAX)`, but the BMI stats array size must match this. Verify that `fman_if_bmi_stats_get_all()` fills exactly `stat_cnt - DPAA_XSTATS_MAX` entries into `values_copy`, or this will copy garbage/uninitialized data.

   The original code had `values` in the function call (wrong), and the loop would copy BMI stats starting at the correct offset. The fix corrects the buffer but the loop logic may still be wrong if the bounds are incorrect. Needs verification that `stat_cnt` and the BMI stats count match.

**Warnings:**

None.

---

## Patch 16/17: net/dpaa: fix null l3_len check in checksum offload

**Errors:**

None.

**Warnings:**

None.

---

## Patch 17/17: net/dpaa: fix mbuf leak in SG fd creation

**Errors:**

None.

**Warnings:**

None.

---

## Cross-Cutting Issues

**Errors:**

1. **Missing release notes**
   All patches in this series are bug fixes with `Fixes:` tags and `Cc: stable at dpdk.org`. According to the guidelines, bug fixes for stable backport should have release notes entries in the current release notes file (e.g., `doc/guides/rel_notes/release_26_03.rst` or similar). None of the patches include release notes updates. For significant bug fixes (resource leaks, incorrect logic, memory corruption risks), release notes should document the fix.

**Warnings:**

None.

---

## Summary of Findings

**Critical Issues (must fix before merge):**
- Patch 01: Conditional cleanup may cause inconsistent error handling across build configs
- Patch 09: Removed `dpaa_finish()` call may leak global resources
- Patch 11: `dpaa_qdma.c` bounds check on `num` needs type/source verification; `dpaa_flow.c` port_handle pattern needs clarification
- Patch 15: Loop bounds in xstats BMI copy may be incorrect
- All patches: Missing release notes entries for bug fixes

**Minor Issues:**
- All patches are well-structured and address real bugs
- Commit messages are clear and reference the original commits