Re: [PATCH rdma-next 12/23] RDMA/mlx5: Verify UMR CQ polling against a hostile NIC

[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 5 potential
> issue(s) to consider.
> 
> Critical: 1 · High: 0 · Medium: 2 · Low: 2
> 
> 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).
> 
> - [Critical] RDMA/mlx5: unvalidated error CQE syndromes in
>    mlx5_ib_poll_one_umr()
> - [Medium] When a forged CQE is detected, the waiters are only woken if
>    the forced health work runs, and on the unload and remove paths it…
> - [Medium] mlx5_ib_poll_one_umr() treats a CQE as forged if the top byte
>    of sop_drop_qpn is not MLX5_OPCODE_UMR.
> - [Low] After the forged-CQE path sets cq->poll_one =
>    mlx5_ib_poll_one_stopped, the UMR CQ's consumer index never advances…
> - [Low] The kernel-doc of mlx5_ib_cq_stop_polling() says that on return
>    'no CQE is looked at again'.
> 
> --
> 
> RDMA/mlx5: Verify UMR CQ polling against a hostile NIC
> 
> In adversarial mode, this installs a dedicated poll_one for the UMR CQ.
> A CQE is accepted only if its opcode, QPN and wqe_counter match the oldest
> entry in a shadow FIFO of posted UMR requests, and the completion context
> is then taken from that FIFO. A CQE that fails validation marks the UMR
> path permanently failed, stops polling the CQ and forces the mlx5_core
> health recovery; teardown also stops polling before the UMR QP is
> destroyed.
> 
>> diff --git a/drivers/infiniband/hw/mlx5/cq.c b/drivers/infiniband/hw/mlx5/cq.c
>> index 11c1153747c24..1a98263ec3111 100644
>> --- a/drivers/infiniband/hw/mlx5/cq.c
>> +++ b/drivers/infiniband/hw/mlx5/cq.c
> 
> [ ... ]
> 
>> @@ -608,6 +609,161 @@ static int poll_soft_wc(struct mlx5_ib_cq *cq, int num_entries,
>>   	return npolled;
>>   }
>>   
>> +static int mlx5_ib_poll_one_stopped(struct mlx5_ib_cq *cq,
>> +				    struct mlx5_ib_qp **cur_qp, struct ib_wc *wc)
>> +{
>> +	return -EAGAIN;
>> +}
> 
> [Severity: Medium]
> Could this check flag a legitimate UMR error recovery as forgery?
> 
> The sop check also runs on MLX5_CQE_REQ_ERR CQEs, including WR_FLUSH_ERR
> ones, before the syndrome is examined. In the error CQE layout, this byte
> is the top byte of s_wqe_opcode_qpn in struct mlx5_err_cqe.
> 
> Elsewhere in the tree, handle_good_req() reads it only for successful REQ
> CQEs. Nothing reads s_wqe_opcode for error or flush CQEs.
> 
> The device may not report the UMR opcode in flush CQEs for WQEs it never
> executed. Examples are the WQEs queued behind a failed UMR and the
> barrier WR posted by mlx5r_umr_recover().
> 
> In that case, an honest NIC that hits a recoverable error such as
> LOCAL_PROT_ERR would get the UMR path disabled for good, with -EFAULT for
> every later UMR, plus a forced health recovery.
> 
> This depends on hardware behavior that nothing in the tree confirms or
> rules out. Would it be safer to apply the sop check only to MLX5_CQE_REQ,
> and rely on the full wqe_counter match against the shadow FIFO for error
> CQEs?

The PRM defines the send WQE opcode as valid in a requester error CQE, 
flushed WQEs included, so an honest NIC reports the UMR opcode there as 
well.

> 
> [ ... ]
> 
>> +forged_cqe:
>> +	/* mlx5_ib_poll_cq() updates the CI only for a returned completion */
>> +	mlx5_cq_set_ci(&cq->mcq);
>> +	/*
>> +	 * The NIC forged a CQE. Fail the UMR path for good, ignore any later
>> +	 * CQE and force the health recovery, as the fatal sensors still read
>> +	 * clean. The internal error flush then completes the waiters. Return
>> +	 * an error, not -EAGAIN, as a CQE was consumed.
>> +	 */
>> +	umrc->state = MLX5_UMR_STATE_ERR;
>> +	cq->poll_one = mlx5_ib_poll_one_stopped;
>> +	mlx5_trigger_health_work_force(dev->mdev);
>> +	return -EIO;
>> +}
> 
> [Severity: Medium]
> What wakes the pending UMR waiters if the forced health work cannot run?
> The commit message says:
> 
>    Recovery moves the device to the internal error state, and the existing
>    flush then completes the pending requests in software and wakes their
>    waiters, so this path does not complete them itself.
> 
> The forced work ends up in mlx5_fw_fatal_reporter_err() in
> drivers/net/ethernet/mellanox/mlx5/core/health.c:
> 
> mlx5_fw_fatal_reporter_err() {
>      ...
> 	mutex_lock(&dev->intf_state_mutex);
> 	if (test_bit(MLX5_DROP_HEALTH_WORK, &health->flags)) {
> 		mlx5_core_err(dev, "health works are not permitted at this stage\n");
> 		mutex_unlock(&dev->intf_state_mutex);
> 		return;
> 	}
>      ...
> }
> 
> Both checks stop it during teardown:
> 
> - remove_one() calls mlx5_drain_health_wq(), which sets
>    MLX5_DROP_HEALTH_WORK, before it calls mlx5_uninit_one().
> 
> - mlx5_uninit_one() and mlx5_unload_one_devl_locked() hold
>    intf_state_mutex while mlx5_unregister_device() or
>    mlx5_detach_device() deletes the mlx5_ib aux device. mlx5r_driver has
>    no .suspend, so the device is always deleted.
> 
> In that window, ib_unregister_device() destroys user MRs, and pooled or
> cacheable mkeys are revoked through UMR:
> 
> mlx5r_handle_mkey_cleanup()
>    mlx5_umr_revoke_mr_with_lock()
>      mlx5r_umr_revoke_mr()
>        mlx5r_umr_post_send_wait()
>          wait_for_completion(&umr_context.done);
> 
> Suppose a forged CQE is detected during this window. The UMR CQ is never
> polled again and the device never enters the internal error state. Is
> anything left that can complete this uninterruptible wait?
> 
> In the intf_state_mutex case, isn't this a deadlock?

The same rare hang can already happen today: a UMR waiter waits for its 
completion or a flush, and a fatal error while unbind runs, after the 
health work is dropped, leaves its WQE unflushed. A forged completion in 
this window reaches the same state, so it is a pre-existing hang, i.e. a 
DoS, which is out of scope of this series.

> 
> [Severity: Critical]
> Can a hostile NIC push the UMR path into an invalid state with error
> CQEs that still pass the shadow FIFO match?
> 
> The CQE is checked against the oldest pending context. Its syndrome is
> then taken at face value, whatever umrc->state is. Two cases look
> reachable by the NIC this patch defends against.
> 
> The first case is a WR_FLUSH_ERR CQE while umrc->state is
> MLX5_UMR_STATE_ACTIVE.

A conforming NIC may flush the UMR QP after an async QP error, without 
an earlier error CQE, so a flush while active is not treated as forged. 
A NIC that keeps flushing the reposts can only hang the caller, i.e. a DoS.

> 
> The second case is a non-flush error for more than one pending WQE.
> Each matching CQE sets MLX5_UMR_STATE_RECOVER and wakes its waiter with
> a non-flush status, so several waiters call mlx5r_umr_recover().
> mlx5r_umr_recover() drops umrc->lock before it posts the barrier WR and
> resets the QP. The recoveries therefore run concurrently, and each one
> posts its own barrier.
> 
> When the first barrier completes, ib_modify_qp(IB_QPS_RESET) reaches
> __mlx5_ib_modify_qp(), which zeroes qp->sq.head, sq.tail, sq.cur_post
> and sq.last_poll. It clears neither umrc->pending nor qp->sq.wqe_head[],
> so the second barrier is still at the head of the shadow FIFO.
> 
> Suppose the NIC now completes that barrier with its old wqe_ctr. The
> uncommitted-WQE check compares a stale wq->wqe_head[idx] with the reset
> wq->head and passes. wq->tail becomes wq->wqe_head[idx] + 1 while
> wq->head is 0.
> 
> A later forged CQE then forces the health recovery. Once the device is
> in the internal error state, mlx5_ib_poll_cq() completes the SQ in
> software. sw_comp() computes wq->head - wq->tail, which has wrapped,
> and walks wq->wrid[]. The entries it dispatches point at
> mlx5r_umr_context structures on the stacks of callers that have
> already returned. mlx5r_umr_done() then writes the status and completes
> the done field in that freed stack memory.
> 
> Should a flush seen in MLX5_UMR_STATE_ACTIVE take the forged_cqe path
> instead? The same question applies to a second non-flush error once the
> state is already MLX5_UMR_STATE_RECOVER.
> 
> Separately, should mlx5r_umr_recover() be serialized? And should the
> shadow FIFO be reset together with the SQ counters on the RESET
> transition?

Right, V1 will accept a non-flush error only while the UMR path is 
active, and treat any further one as forged, so a single recovery runs.

Yishai