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

[Yishai Hadas <yishaih@nvidia.com>]

On 07/11/2026 22:25, netdev-bot+sashiko@kernel.org wrote:
> 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)
> 

The race needs a hostile NIC, as a conforming device raises an event 
only after the CQ is armed. V1 will fix it in mlx5, as only mlx5 
supports adversarial mode: a kernel CQ will ignore events until its 
first arm, using a release/acquire pair. This also covers devr->c0, 
which is never armed.

Yishai