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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RDMA/mlx5: unserialized mlx5r_umr_recover() and ERR state overwrite
--

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

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

This commit updates mlx5r_umr_recover() to transition umrc->state using
cmpxchg() when mlx5_ib_adversarial_mode is enabled so that a concurrent
transition to MLX5_UMR_STATE_ERR by the CQ poller is not overwritten back to
MLX5_UMR_STATE_RECOVER or MLX5_UMR_STATE_ACTIVE.

> 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.
> + */
> +static bool mlx5r_umr_move_state(struct umr_common *umrc, unsigned int from,
> +				 unsigned int to)
> +{
> +	if (!mlx5_ib_adversarial_mode) {
> +		umrc->state = to;
> +		return true;
> +	}
> +
> +	return cmpxchg(&umrc->state, from, to) != MLX5_UMR_STATE_ERR;
> +}
> +
>  static int mlx5r_umr_recover(struct mlx5_ib_dev *dev, u32 mkey,
>  			     struct mlx5r_umr_context *umr_context,
>  			     struct mlx5r_umr_wqe *wqe, bool with_data)
> @@ -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);
>  
> @@ -387,7 +408,9 @@ static int mlx5r_umr_recover(struct mlx5_ib_dev *dev, u32 mkey,
>  		goto err;
>  	}
>  
> -	umrc->state = MLX5_UMR_STATE_ACTIVE;
> +	if (!mlx5r_umr_move_state(umrc, MLX5_UMR_STATE_RECOVER,
> +				  MLX5_UMR_STATE_ACTIVE))
> +		return -EFAULT;

[Severity: High]
Can a hostile NIC still cause concurrent mlx5r_umr_recover() executions and
overwrite MLX5_UMR_STATE_ERR back to MLX5_UMR_STATE_ACTIVE?

In mlx5_ib_adversarial_mode, up to 64 threads can concurrently post UMR WRs
in mlx5r_umr_post_send_wait() under umrc->sem, placing their contexts into
umrc->pending[]. If a hostile NIC returns valid-header MLX5_CQE_REQ_ERR
completions (matching qpn, sop == MLX5_OPCODE_UMR, and sequential wqe_ctr)
with a non-flush syndrome (such as MLX5_CQE_SYNDROME_LOCAL_QP_OP_ERR, where
wc->status != IB_WC_WR_FLUSH_ERR) for multiple in-flight WRs,
mlx5_ib_poll_one_umr() accepts each CQE and unconditionally writes
MLX5_UMR_STATE_RECOVER:

drivers/infiniband/hw/mlx5/cq.c:mlx5_ib_poll_one_umr() {
	...
	} 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;
	}
}

Multiple waiters in mlx5r_umr_post_send_wait() then wake up with
umr_context.status != IB_WC_WR_FLUSH_ERR and call mlx5r_umr_recover().
Because mlx5r_umr_recover() only holds umrc->lock across the initial state
check and treats umrc->state == MLX5_UMR_STATE_RECOVER as the normal entry
state without serializing recovery callers, multiple threads can run
mlx5r_umr_recover() at the same time.

This appears to allow two problems:

1. If one recovery thread fails at the err label in mlx5r_umr_recover(), it
sets umrc->state = MLX5_UMR_STATE_ERR without stopping cq->poll_one:

drivers/infiniband/hw/mlx5/umr.c:mlx5r_umr_recover() {
	...
err:
	umrc->state = MLX5_UMR_STATE_ERR;
	return err;
}

When mlx5_ib_poll_one_umr() subsequently processes another non-flush error
CQE for an in-flight WR or barrier WR, its plain store overwrites
MLX5_UMR_STATE_ERR back to MLX5_UMR_STATE_RECOVER, allowing a concurrent
recovery thread's mlx5r_umr_move_state(umrc, MLX5_UMR_STATE_RECOVER,
MLX5_UMR_STATE_ACTIVE) call to succeed and reopen the UMR path to
MLX5_UMR_STATE_ACTIVE.

2. When the first recovery thread's barrier WR completes and it calls
ib_modify_qp(umrc->qp, IB_QPS_RESET), __mlx5_ib_modify_qp() resets the QP
and zeroes qp->sq.head, qp->sq.tail, qp->sq.cur_post, and qp->sq.last_poll
while the second recovery thread's barrier WR is still queued in
umrc->pending and sleeping in wait_for_completion(&umr_context->done). Does
that desynchronize umrc->pending from qp->sq and leave the second thread
hung in wait_for_completion() while holding umrc->sem, since even the
internal-error software flush in sw_comp() walks from last_poll to head?

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