|WARNING| [RFT] net/mlx5: fix out-of-bounds stride index in vector Rx

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## ERRORS

**1. Resource leak on error path**

Line: `rte_pktmbuf_free_seg(elts[i]);`

After `rte_pktmbuf_free_seg(elts[i])` at line 241, the code continues the loop without advancing the output array. The caller expects `rxq_copy_mprq_mbuf_v()` to populate the mbuf array it provides, but this dropped packet leaves a hole: `elts[i]` is freed, but `elts[i]` itself (the pointer slot) is not removed from the array the caller sees. The caller will later access the freed mbuf, causing use-after-free.

The function must either compact the array (shift remaining entries down) or return the actual number of valid mbufs processed. The current code does neither.

**2. Statistics decrement on `#ifdef MLX5_PMD_SOFT_COUNTERS`**

Lines:
```c
#ifdef MLX5_PMD_SOFT_COUNTERS
			rxq->stats.ipackets -= 1;
			rxq->stats.ibytes -= elts[i]->pkt_len;
#endif
```

The `ipackets` and `ibytes` counters were incremented earlier in the Rx path before this function is called. Decrementing them here is correct IF those increments happened unconditionally. However, the decrement is conditional on `MLX5_PMD_SOFT_COUNTERS`, while the increment may have been unconditional or under a different guard. This creates a mismatch: when `MLX5_PMD_SOFT_COUNTERS` is not defined, the increment stands but the decrement is skipped, leaving the statistics wrong.

Verify that the increment is also guarded by `#ifdef MLX5_PMD_SOFT_COUNTERS` in the same way. If the increment is unconditional or under a different condition, the decrement must match.

**3. `idropped` counter increment outside `#ifdef MLX5_PMD_SOFT_COUNTERS`**

Line: `++rxq->stats.idropped;`

The `idropped` increment is unconditional, while `ipackets` and `ibytes` adjustments are conditional on `MLX5_PMD_SOFT_COUNTERS`. This inconsistency means builds without `MLX5_PMD_SOFT_COUNTERS` will increment `idropped` but not decrement `ipackets`/`ibytes`, producing inconsistent statistics.

Move the `idropped` increment inside the `#ifdef MLX5_PMD_SOFT_COUNTERS` block so all statistics adjustments are guarded uniformly, or verify that `idropped` is intentionally always-on and the current guards are correct.

---

## WARNINGS

**1. Missing bounds check rationale in commit message**

The patch adds a check `rxq->consumed_strd + strd_cnt > strd_n` to prevent out-of-bounds access. The commit message states that "the per-packet stride count comes from pkt_len, which may not match the strides the device consumed," but does not explain *why* this mismatch occurs or what the device is doing differently. For a fix to a data-corruption bug, the commit message should explain the root cause: where does `pkt_len` come from, and why can the device consume more strides than `pkt_len` implies?

Add a sentence explaining the device behavior that causes `consumed_strd` to drift (e.g., hardware consumed extra strides due to padding, descriptor format, or error cases).

**2. Counter adjustment uses mbuf field after free**

Line: `rxq->stats.ibytes -= elts[i]->pkt_len;`

This line reads `elts[i]->pkt_len` after calling `rte_pktmbuf_free_seg(elts[i])`. The mbuf has been freed, so `elts[i]` is now a dangling pointer. Reading `pkt_len` from freed memory is use-after-free. The value may be correct (the memory has not been reused yet), but it is undefined behavior.

Read `elts[i]->pkt_len` into a local variable before calling `rte_pktmbuf_free_seg()`, then use the saved value for the statistics adjustment:

```c
uint32_t dropped_len = elts[i]->pkt_len;
rte_pktmbuf_free_seg(elts[i]);
#ifdef MLX5_PMD_SOFT_COUNTERS
rxq->stats.ipackets -= 1;
rxq->stats.ibytes -= dropped_len;
#endif
```

**3. Return value or output array handling not addressed**

The function signature is not shown, but the fact that it walks an `elts[]` array and calls `mprq_buf_to_pkt()` to populate mbufs suggests the caller expects a contiguous array of valid mbufs on return. When a packet is dropped, the code does not adjust the output array or return value to reflect the drop. This will cause the caller to process a stale or freed mbuf pointer.

Either:
- Shift the remaining `elts[]` entries down to fill the hole and decrement the count returned to the caller, or
- Document that dropped packets leave `NULL` or invalid entries and update the caller to skip them

The current code does neither, which is a contract violation.

---

## INFO

None.

---

## Summary

The patch correctly identifies the out-of-bounds condition but the error path has three issues: it frees the mbuf without removing it from the array (caller will use-after-free), it reads the freed mbuf's `pkt_len` for statistics (use-after-free), and the statistics adjustment guards may not match the increment site. Additionally, the caller's expectation of a contiguous valid mbuf array is broken.

Review-Result: ERROR