On Sun, 9 Aug 2026 12:48:37 +0300
Maayan Kashani <[email protected]> wrote:
> Avoid post-decrementing the segment index when it is zero in the
> error cleanup path of mlx5_rxq_mempool_register().
>
> Coverity issue: 503770
> Fixes: 8d1cb02da5b7 ("net/mlx5: support selective Rx")
> Cc: [email protected]
>
> Signed-off-by: Maayan Kashani <[email protected]>
> ---
Claude Opus AI review sees dead bodies here...
Review of [PATCH v2] net/mlx5: fix uint16 underflow on Rx queue error path
Warning: undocumented second change, and it is dead code
The commit message describes only the loop restructuring, but the patch
also adds a "seg->mp == NULL" guard around the free:
+ if (seg->mp == NULL) {
+ mlx5_free(seg->null_mbuf);
+ seg->null_mbuf = NULL;
+ }
This guard can never be false for a segment that has a null_mbuf.
seg->null_mbuf is only ever assigned in the mp == NULL branch of the
registration loop (mlx5_trigger.c:179), and rxq_ctrl is allocated with
MLX5_MEM_ZERO (mlx5_rxq.c:1881), so a segment with mp != NULL always has
null_mbuf == NULL. mlx5_free() already returns early on NULL
(drivers/common/mlx5/mlx5_malloc.c:275), so the guard changes nothing.
It also makes this cleanup inconsistent with rxq_free_elts_sprq(),
which frees null_mbuf for every segment unconditionally:
for (i = 0; i < rxq->rxseg_n; i++) {
mlx5_free(rxq->rxseg[i].null_mbuf);
rxq->rxseg[i].null_mbuf = NULL;
}
Suggest dropping the guard so the patch does only what its subject says.
If it is deliberate, say why in the commit message and apply the same
rule in mlx5_rxq.c so the two paths agree.
Info: the underflow has no observable effect
In the original "while (s-- > 0)", s is uint16_t and the comparison uses
the value before the decrement. With s == 0 the body never executes; s
wraps to 0xFFFF but is dead from that point on, so there is no
out-of-bounds index and no misbehaviour. Coverity 503770 is reporting the
wrap of a dead local. The rewrite is behaviour-preserving and fine, but
it would help stable maintainers to state in the commit message that this
is a static-analysis fix with no runtime impact.
Info: no v2 changelog after the "---" separator, so what changed since
[email protected] is not visible to reviewers.
