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

[Yishai Hadas <yishaih@nvidia.com>]

On 04/11/2026 18:43, sashiko-bot@kernel.org wrote:
> 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()?
> 

Right, a hostile NIC can hit this on a weakly ordered CPU. For a 
conforming device the arm already orders these stores, so the core is
fine as is. v1 will fix it in mlx5, the only driver with adversarial 
mode: a kernel CQ will ignore events until its first arm, using a 
release/acquire pair.

Yishai