Re: [PATCH v2] net/mlx5: fix uint16 underflow on Rx queue error path

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.