|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

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

### Errors
None.

### Warnings
1. **Conditional compilation directive for error path cleanup**

The cleanup code added under `#ifdef CONFIG_FSL_QMAN_FQ_LOOKUP` is only executed when that config is defined. If `CONFIG_FSL_QMAN_FQ_LOOKUP` was defined during the path that set `fq->key`, but undefined at compile time for the error path, the FQ table entry will leak. Verify that the `#ifdef` at the error path matches all code paths that could have set `fq->key`.

## 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
None.

### 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. **Resource cleanup skipped when device is already closed**

The code now only calls `dpaa_finish()` if `dpaa_valid_dev` reaches zero, but the condition `if (eth_dev->state != RTE_ETH_DEV_UNUSED)` means that if the device is already in `UNUSED` state, `dpaa_eth_dev_close()` and `rte_eth_dev_release_port()` are skipped. This is correct.

However, the original code unconditionally called `dpaa_finish()` when `dpaa_valid_dev` hit zero after decrementing. The new code removes the `dpaa_finish()` call entirely. If `dpaa_finish()` performs global cleanup (freeing shared resources, unmapping memory, closing file descriptors), removing this call causes a resource leak when the last DPAA device is removed.

Suggested fix: Restore the `dpaa_finish()` call inside the `if (!dpaa_valid_dev)` block:

```c
if (!dpaa_valid_dev) {
	rte_mempool_free(dpaa_tx_sg_pool);
	dpaa_finish();  /* Add this back */
}
```

## 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. **Port handle leak on repeated initialization**

In `fm_prev_cleanup()`, the code closes `dpaa_intf.port_handle` if it is non-NULL before opening a new one. However, this occurs inside a loop (`for (i = 0; i < ...)`). If the loop runs multiple times, the first iteration will open a new `port_handle` and then on subsequent iterations the `if (dpaa_intf.port_handle)` check will always be true (because it was just assigned), causing the newly opened handle to be immediately closed and reopened. This pattern is inefficient but not technically a leak if the close succeeds.

More critically: if `fm_port_open()` fails and returns NULL, `dpaa_intf.port_handle` is set to NULL but the old handle is already closed, so no leak occurs from the close. But if `dpaa_intf.port_handle` was non-NULL from a previous successful open and the new `fm_port_open()` fails, we have closed the old handle and now have NULL, which may cause issues if the caller expects the handle to remain valid.

Suggested approach: Only close the handle if we are about to replace it with a different one (different device), not unconditionally on every loop iteration. Or, only close it once before the loop if it is being reinitialized from scratch.

2. **Invalid descriptor count bounds check placement in dpaa_qdma.c**

The added check `if (num == 0 || num > FSL_QDMA_SG_MAX_ENTRY)` comes *after* the loop that computes `num` and *after* code that already dereferences `ft->desc_ssge[num - 1]` and `ft->desc_dsge[num - 1]` when setting the `final` flag. If `num` is zero or exceeds the array bounds, those dereferences will have already caused out-of-bounds access before the check triggers. The check must be placed *before* any array access using `num`.

Suggested fix: Move the bounds check to immediately after the loop that computes `num`, before the `ft->desc_ssge[num - 1].final = 1;` line.

## 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
None.

### Warnings
None.

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

### Errors
None.

### Warnings
1. **Incomplete guard for invalid L3 header**

Adding a check for `mbuf->l3_len == 0` prevents the immediate pointer arithmetic issue, but the function should also verify that `l3_hdr` (computed as `rte_pktmbuf_mtod(mbuf, char *) + mbuf->l2_len`) points to valid data within the mbuf. If `l2_len` is incorrect or the packet is truncated, `l3_hdr` could point outside the mbuf data range. Consider adding a bounds check on `l2_len` and `l3_len` relative to `rte_pktmbuf_data_len(mbuf)`.

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

### Errors
None.

### Warnings
None.

---

## Summary

**Errors found: 3**
- Patch 09: Missing `dpaa_finish()` call when last device is removed (resource leak)
- Patch 11: Port handle management issue in loop (inefficient or may leave state inconsistent on error)
- Patch 11: Bounds check on `num` placed after array accesses, allowing out-of-bounds write

**Warnings: 3**
- Patch 01: Conditional cleanup under `#ifdef` may not match all paths that set `fq->key`
- Patch 11: Port handle close/reopen pattern in loop should be reviewed
- Patch 16: L3 length check should be combined with bounds validation on `l2_len` and `l3_len`