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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] The fix leaves a memory-ordering race on comp_handler.

--

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

This patch makes mlx5_ib_cq_comp() skip the comp_handler call when no
handler is set. The case it targets is devr->c0, which is never armed, but
a hostile NIC in adversarial mode can still raise a completion event for
it.

> 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]
For devr->c0 this check is enough, because comp_handler stays NULL for
that CQ. Is it also enough for the CQs set up by __ib_alloc_cq()?

The companion commit "RDMA/core: Set the CQ comp_handler after its poll
context is set up" relies on this NULL check as a "poll context ready"
gate. Neither side of that gate has any ordering, though.

On the writer side, the CQ can be reached from the EQ interrupt once
create_cq() puts it in eq->cq_table.tree. The handler is published later
with a plain store:

drivers/infiniband/core/cq.c:__ib_alloc_cq() {
    ...
	ret = dev->ops.create_cq(cq, &cq_attr, NULL);
    ...
	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->comp_handler = comp_handler;
    ...
}

On the reader side, mlx5_eq_comp_int() gets the CQ with mlx5_eq_cq_get(),
which is an RCU radix_tree_lookup() plus a refcount increment. That orders
against the tree insertion, but not against the stores made after it.
mlx5_ib_cq_comp() then loads comp_handler twice with plain loads.

Adversarial mode is enabled when cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)
is true. That includes arm64 Realm and pKVM guests, and those CPUs are
weakly ordered. Take the UMR CQ, which mlx5r_umr_resource_init() allocates
with IB_POLL_SOFTIRQ.

Could another CPU see comp_handler == ib_cq_completion_softirq before the
memset(), INIT_LIST_HEAD(), weight and poll stores from irq_poll_init()
are visible? If so, irq_poll_sched() would link iop onto the per-CPU list.
The delayed irq_poll_init() stores could then overwrite iop->state and
iop->list while iop is still on that list.

Possible results are per-CPU list corruption, irq_poll_softirq() spinning
on a stale entry, or a NULL iop->poll call. For the workqueue cases, the
compiler is also free to move the plain comp_handler store ahead of the
INIT_WORK() and cq->comp_wq stores. That could lead to queue_work() with a
NULL workqueue or an uninitialized work item.

The wmb() in the later ib_req_notify_cq() only helps if the device waits
for the arm doorbell, and a hostile device does not wait.

Would it make sense to publish the handler with:

	smp_store_release(&cq->comp_handler, comp_handler);

in __ib_alloc_cq(), and read it once in mlx5_ib_cq_comp() with:

	ib_comp_handler h = smp_load_acquire(&ibcq->comp_handler);

	if (h)
		h(ibcq, ibcq->cq_context);

That also removes the double fetch. KCSAN would likely report the current
plain check against the plain store as a data race. Later patches in the
series do not change this code.

>  }
>  
>  static void mlx5_ib_cq_event(struct mlx5_core_cq *mcq, enum mlx5_event type)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103163949.1554-1-yishaih%40nvidia.com