Re: [PATCH rdma-next 16/23] RDMA/mlx5: Ignore completion events for a CQ without a handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RDMA/mlx5: missing acquire/release on comp_handler in mlx5_ib_cq_comp()
--

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

RDMA/mlx5: Ignore completion events for a CQ without a handler

This commit skips calling ibcq->comp_handler in mlx5_ib_cq_comp() when no
completion handler is set, preventing a NULL function pointer call if a
hostile NIC raises an unsolicited completion event on a CQ without a
handler.

> diff --git a/drivers/infiniband/hw/mlx5/cq.c b/drivers/infiniband/hw/mlx5/cq.c
> index 5cc86c3f93f3e..f955eb762051d 100644
> --- a/drivers/infiniband/hw/mlx5/cq.c
> +++ b/drivers/infiniband/hw/mlx5/cq.c
> @@ -46,7 +46,12 @@ static void mlx5_ib_cq_comp(struct mlx5_core_cq *cq, struct mlx5_eqe *eqe)
>  {
>  	struct ib_cq *ibcq = &to_mibcq(cq)->ibcq;
>  
> -	ibcq->comp_handler(ibcq, ibcq->cq_context);
> +	/*
> +	 * A CQ created without a handler (devr->c0) is never armed, but a
> +	 * hostile NIC can still raise a completion event for it.
> +	 */
> +	if (ibcq->comp_handler)
> +		ibcq->comp_handler(ibcq, ibcq->cq_context);

[Severity: High]
When a kernel CQ is allocated via __ib_alloc_cq() (such as dev->umrc.cq in
mlx5r_umr_resource_init()), dev->ops.create_cq() publishes the CQ into the
completion EQ radix tree via mlx5_ib_create_cq() -> mlx5_core_create_cq() ->
mlx5_eq_add_cq() while cq->comp_handler is still NULL.

After create_cq() returns, __ib_alloc_cq() initializes cq->dim and the poll
context before setting cq->comp_handler:

drivers/infiniband/core/cq.c:__ib_alloc_cq() {
	...
	ret = dev->ops.create_cq(cq, &cq_attr, NULL);
	if (ret)
		goto out_free_wc;

	rdma_dim_init(cq);

	switch (cq->poll_ctx) {
	case IB_POLL_DIRECT:
		comp_handler = ib_cq_completion_direct;
		break;
	case IB_POLL_SOFTIRQ:
		comp_handler = ib_cq_completion_softirq;
		irq_poll_init(&cq->iop, IB_POLL_BUDGET_IRQ, ib_poll_handler);
		break;
	case IB_POLL_WORKQUEUE:
	case IB_POLL_UNBOUND_WORKQUEUE:
		comp_handler = ib_cq_completion_workqueue;
		INIT_WORK(&cq->work, ib_cq_poll_work);
		cq->comp_wq = (cq->poll_ctx == IB_POLL_WORKQUEUE) ?
				ib_comp_wq : ib_comp_unbound_wq;
		break;
	...
	}

	cq->comp_handler = comp_handler;
}

If a hostile NIC raises an unsolicited completion event for the newly
created CQN on another CPU while __ib_alloc_cq() is setting up the poll
context, mlx5_eq_comp_int() -> mlx5_ib_cq_comp() reads and invokes
ibcq->comp_handler with plain loads.

Because ibcq in mlx5_ib_cq_comp() is derived from cq (&to_mibcq(cq)->ibcq)
rather than from the loaded ibcq->comp_handler pointer, there is no address
dependency between reading ibcq->comp_handler and accessing fields of ibcq
inside the completion handler, and neither irq_poll_init() nor INIT_WORK()
provides a release barrier before cq->comp_handler = comp_handler.

Could a weakly ordered CPU or the compiler reorder the stores to cq->iop,
cq->work, cq->comp_wq, or cq->dim with the store and load of comp_handler,
causing ib_cq_completion_softirq() or ib_cq_completion_workqueue() to see
uninitialized poll context fields (such as a NULL iop->poll or cq->comp_wq)?
Should __ib_alloc_cq() use smp_store_release(&cq->comp_handler,
comp_handler) paired with smp_load_acquire(&ibcq->comp_handler) in
mlx5_ib_cq_comp()?

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