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

[Yishai Hadas <yishaih@nvidia.com>]

On 07/11/2026 22:24, netdev-bot+sashiko@kernel.org wrote:
> 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?

Right, V1 will move the state in mlx5_ib_poll_one_umr() with 
cmpxchg(ACTIVE->RECOVER), and treat a non-flush error CQE that fails it 
as forged.

> [ ... ]
> 
>> @@ -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
> 
> [ ... ]
> 

Not needed. ERR stays set, as the final cmpxchg fails, so no new UMR is 
posted, and the stopped CQ completes nothing.

Yishai