Re: [PATCH rdma-next 13/23] RDMA/mlx5: Keep the UMR QP failed once a forged CQE is detected

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In adversarial mode, mlx5r_umr_recover()
  (drivers/infiniband/hw/mlx5/umr.c) checks for MLX5_UMR_STATE_ERR at…
- [Medium] The new comment on mlx5r_umr_move_state() (umr.c:338-343) and
  the commit message both say that in adversarial mode ERR is a final…

--

RDMA/mlx5: Keep the UMR QP failed once a forged CQE is detected

In adversarial mode, this commit changes mlx5r_umr_recover() to move the
UMR state with cmpxchg(): ACTIVE to RECOVER on entry and RECOVER to
ACTIVE at the end. The goal is to stop recovery from overwriting an
MLX5_UMR_STATE_ERR that the CQ poller sets on a forged CQE. Outside
adversarial mode the transitions are unchanged.

> diff --git a/drivers/infiniband/hw/mlx5/umr.c b/drivers/infiniband/hw/mlx5/umr.c
> index b0f00d4bc93ca..086b397b830c6 100644
> --- a/drivers/infiniband/hw/mlx5/umr.c
> +++ b/drivers/infiniband/hw/mlx5/umr.c
> @@ -335,6 +335,23 @@ static int mlx5r_umr_post_send(struct ib_qp *ibqp, u32 mkey,
>  	return err;
>  }
>  
> +/*
> + * Move the UMR state from @from to @to. In adversarial_mode ERR is a final
> + * state: it can be set concurrently by the CQ poller, so the move is atomic
> + * and does not replace it. Returns false if ERR was found, and true
> + * otherwise. Outside adversarial_mode this is the plain store it always was.
> + */

[Severity: Medium]
Is ERR really final here? Only the transitions in mlx5r_umr_recover()
were converted to cmpxchg(). mlx5_ib_poll_one_umr() in cq.c still does a
plain store for every validated non-flush error CQE:

drivers/infiniband/hw/mlx5/cq.c:mlx5_ib_poll_one_umr() {
    ...
		if (wc->status != IB_WC_WR_FLUSH_ERR)
			umrc->state = MLX5_UMR_STATE_RECOVER;
    ...
}

That store can overwrite the ERR written by the err: label in
mlx5r_umr_recover(), and that label does not stop the poller:

err:
	umrc->state = MLX5_UMR_STATE_ERR;
	return err;

Could a hostile NIC reopen the UMR path with a sequence like this?

1. Waiters A and B each post a UMR while the state is ACTIVE.

2. The NIC returns validated non-flush error CQEs for both. A and B enter
   mlx5r_umr_recover(), find the state already RECOVER, skip the entry
   cmpxchg, and post barriers WQE3 and WQE4.

3. The NIC completes WQE3. A moves the QP to RESET, the NIC fails
   RST2INIT, and A's err: label stores ERR.

4. The NIC returns a non-flush error CQE for WQE4. It passes the opcode,
   QPN, sop, wqe_ctr (FIFO head) and wqe_head checks, so the poller
   stores RECOVER over ERR.

5. B's RESET and RST2RTS succeed, and B's cmpxchg(RECOVER->ACTIVE) also
   succeeds, so the UMR path is ACTIVE again.

The generic store of RECOVER in mlx5_poll_one() has the same problem at
this commit. The later commit "RDMA/mlx5: Block generic CQ poll path in
adversarial mode" closes that path by making mlx5_poll_one() return
-EOPNOTSUPP in adversarial mode before the store is reached.

Would a cmpxchg(ACTIVE->RECOVER) in mlx5_ib_poll_one_umr() fix this? If
not, should the comment say that only an ERR set on a forged CQE is
final?

[ ... ]

> @@ -353,7 +370,11 @@ static int mlx5r_umr_recover(struct mlx5_ib_dev *dev, u32 mkey,
>  	if (umrc->state != MLX5_UMR_STATE_RECOVER) {
>  		mlx5_ib_warn(dev, "UMR recovery encountered an unexpected state=%d\n",
>  			     umrc->state);
> -		umrc->state = MLX5_UMR_STATE_RECOVER;
> +		if (!mlx5r_umr_move_state(umrc, MLX5_UMR_STATE_ACTIVE,
> +					  MLX5_UMR_STATE_RECOVER)) {
> +			mutex_unlock(&umrc->lock);
> +			return -EFAULT;
> +		}
>  	}
>  	mutex_unlock(&umrc->lock);

[Severity: Medium]
What happens if the poller detects a forged CQE after this
mutex_unlock()?

mlx5_ib_poll_one_umr() never takes umrc->lock. It sets ERR under cq->lock
and only queues the forced health work:

	umrc->state = MLX5_UMR_STATE_ERR;
	cq->poll_one = mlx5_ib_poll_one_stopped;
	mlx5_trigger_health_work_force(dev->mdev);

After the unlock, mlx5r_umr_recover() does not look at umrc->state again
until the final cmpxchg, so recovery continues:

mlx5r_umr_recover()
  mlx5r_umr_post_send()
    only checks mdev->state, writes the barrier WQE and rings the doorbell
  wait_for_completion()
  ib_modify_qp(umrc->qp, &attr, IB_QP_STATE)  /* RESET */
    __mlx5_ib_modify_qp()->mlx5_ib_cq_clean() sweeps the stopped UMR CQ
  mlx5r_umr_qp_rst2rts()
    RST2INIT/INIT2RTR/RTR2RTS put the hardware UMR QP back in RTS
  mlx5r_umr_move_state(umrc, MLX5_UMR_STATE_RECOVER, MLX5_UMR_STATE_ACTIVE)
    first point where ERR is seen

The NIC controls CQE timing. It can complete the barrier with a valid CQE
and then forge one while recovery is in the RESET/RTS steps. All of this
can happen before the forced health work moves the device to
MLX5_DEVICE_STATE_INTERNAL_ERROR.

umrc->state stays ERR, so mlx5r_umr_post_send_wait() still refuses new
UMRs. Even so, in this window the subject line "Keep the UMR QP failed",
the existing comment "don't revive the QP" in mlx5r_umr_recover(), and
this part of the commit message don't seem fully met:

  This keeps new UMRs away from the NIC until the forced health recovery
  moves the device to MLX5_DEVICE_STATE_INTERNAL_ERROR

In adversarial mode, should ERR be checked again at these points?

- before the barrier post, perhaps inside mlx5r_umr_post_send() under
  qp->sq.lock
- after wait_for_completion()
- before each ib_modify_qp() step

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103163949.1554-1-yishaih%40nvidia.com