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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] RDMA/mlx5: unvalidated error CQE syndromes in mlx5_ib_poll_one_umr()
- [High] RDMA/mlx5: hung UMR waiters in mlx5_ib_poll_one_umr() on teardown
--

commit da9b5d87a1b071b4bd6eacadaf91a72df89f8c23
Author: Yishai Hadas <yishaih@nvidia.com>

RDMA/mlx5: Verify UMR CQ polling against a hostile NIC

This commit adds a dedicated UMR CQ polling handler in adversarial mode
that validates incoming CQEs against a shadow FIFO of posted UMR requests
and triggers forced health recovery if a forged CQE is detected.

> 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;
> +}

[ ... ]

> +int mlx5_ib_poll_one_umr(struct mlx5_ib_cq *cq, struct mlx5_ib_qp **cur_qp,
> +		      struct ib_wc *wc)
> +{

[ ... ]

> +	/* Pop the shadow FIFO head: validated, dispatch to its waiter */
> +	WRITE_ONCE(umrc->pending[head & (MLX5_UMR_MAX_WR - 1)], NULL);
> +	umrc->pending_head = head + 1;
> +	wc->wr_cqe = &expected->cqe;
> +
> +	wc->qp = &qp->ibqp;
> +	wq->tail = wq->wqe_head[idx] + 1;
> +	/* Advance wq->last_poll, the internal error flush starts from it */
> +	handle_atomics(qp, cqe64, wq->last_poll, idx);
> +
> +	if (opcode == MLX5_CQE_REQ) {
> +		wc->status = IB_WC_SUCCESS;
> +	} else {
> +		/* Copy the CQE, so its syndrome is read once */
> +		err_cqe = *(struct mlx5_err_cqe *)cqe64;
> +		mlx5_handle_error_cqe(dev, &err_cqe, wc);
> +		/* As in the generic path, a real failure starts the recovery */
> +		if (wc->status != IB_WC_WR_FLUSH_ERR)
> +			umrc->state = MLX5_UMR_STATE_RECOVER;
> +	}

[Severity: Critical]
Does mlx5_ib_poll_one_umr() need to validate whether the error syndrome in a
MLX5_CQE_REQ_ERR completion is valid for the current umrc->state?

If umrc->state is MLX5_UMR_STATE_ACTIVE and a hostile NIC reports
MLX5_CQE_SYNDROME_WR_FLUSH_ERR for the oldest pending UMR WQE,
mlx5_ib_poll_one_umr() completes the request with IB_WC_WR_FLUSH_ERR while
leaving umrc->state as MLX5_UMR_STATE_ACTIVE. In
drivers/infiniband/hw/mlx5/umr.c:mlx5r_umr_post_send_wait(), the waiter sees
IB_WC_WR_FLUSH_ERR, continues the loop, sees umrc->state is still
MLX5_UMR_STATE_ACTIVE, and reposts the WQE indefinitely.

Conversely, when multiple UMR requests are in flight concurrently and the NIC
returns a non-flush error syndrome for more than one of them, each of those
waiters wakes up in mlx5r_umr_post_send_wait() with a status other than
IB_WC_WR_FLUSH_ERR and calls mlx5r_umr_recover(). Because mlx5r_umr_recover()
drops umrc->lock before posting the barrier WR and resetting the QP:

drivers/infiniband/hw/mlx5/umr.c:mlx5r_umr_recover()
    mutex_unlock(&umrc->lock);
    err = mlx5r_umr_post_send(umrc->qp, mkey, umr_context, wqe, with_data);
    ...
    wait_for_completion(&umr_context->done);
    attr.qp_state = IB_QPS_RESET;
    err = ib_modify_qp(umrc->qp, &attr, IB_QP_STATE);

multiple threads can enter recovery concurrently. When the first thread
resets the QP via __mlx5_ib_modify_qp(), qp->sq.head, qp->sq.tail,
qp->sq.cur_post, and qp->sq.last_poll are zeroed while the second thread's
barrier WR remains in umrc->pending with a non-zero wq->wqe_head[idx].
Completing that second barrier WR then sets wq->tail = wq->wqe_head[idx] + 1
while wq->head is 0. If a subsequent forged CQE triggers internal error
flushing, sw_comp() computes cur = wq->head - wq->tail (which wraps around as
unsigned int) and completes stale wq->wrid[] entries pointing to
stack-allocated mlx5r_umr_context structures of callers that have already
returned.

Should a flush error while active, or a second non-flush error after
entering recovery, be treated as a forged CQE (or should error CQEs on the
UMR QP be disallowed altogether in adversarial mode)?

> +
> +	return 0;
> +
> +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: High]
Can relying solely on mlx5_trigger_health_work_force() to flush pending UMR
requests leave waiters in mlx5r_umr_post_send_wait() hung during device
teardown, reload, or removal?

During teardown or reload, mlx5_unload_one_devl_locked() and
mlx5_uninit_one() in drivers/net/ethernet/mellanox/mlx5/core/main.c hold
dev->intf_state_mutex while unregistering the IB device, which revokes open
MRs through:

  mlx5_ib_dereg_mr()
    -> mlx5r_handle_mkey_cleanup()
      -> mlx5_umr_revoke_mr_with_lock()
        -> mlx5r_umr_revoke_mr()
          -> mlx5r_umr_post_send_wait()

In addition, remove_one(), shutdown(), and
mlx5_sync_reset_set_reset_requested() call mlx5_drain_health_wq() or
mlx5_stop_health_poll(), setting MLX5_DROP_HEALTH_WORK before mlx5_ib
teardown finishes.

If a forged CQE is detected in mlx5_ib_poll_one_umr() during these flows,
cq->poll_one is replaced with mlx5_ib_poll_one_stopped without completing
the entries in umrc->pending. The queued force_fatal_report_work calls
mlx5_fw_fatal_reporter_err() in
drivers/net/ethernet/mellanox/mlx5/core/health.c:

    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;
    }

Because dev->intf_state_mutex is held by the thread waiting in
wait_for_completion(&umr_context.done), or because MLX5_DROP_HEALTH_WORK is
already set, enter_error_state() and the software CQ flush cannot run,
leaving the UMR waiters blocked forever. Could forged_cqe drain and complete
the remaining contexts in umrc->pending directly?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103163949.1554-1-yishaih@nvidia.com?part=12