Re: [PATCH rdma-next 15/23] RDMA/core: Set the CQ comp_handler after its poll context is set up

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RDMA/core: comp_handler initialized after create_cq() in __ib_alloc_cq()
--

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

RDMA/core: Set the CQ comp_handler after its poll context is set up

This commit updates __ib_alloc_cq() to initialize the CQ poll context
(irq_poll, work_struct, and comp_wq) before setting cq->comp_handler and
arming the CQ via ib_req_notify_cq().

> diff --git a/drivers/infiniband/core/cq.c b/drivers/infiniband/core/cq.c
> index 12304c9a9403e..42e4d272f5ade 100644
> --- a/drivers/infiniband/core/cq.c
> +++ b/drivers/infiniband/core/cq.c
> @@ -217,6 +217,7 @@ struct ib_cq *__ib_alloc_cq(struct ib_device *dev, void *private, int nr_cqe,
>  		.cqe		= nr_cqe,
>  		.comp_vector	= comp_vector,
>  	};
> +	ib_comp_handler comp_handler;
>  	struct ib_cq *cq;
>  	int ret = -ENOMEM;
>  
> @@ -248,19 +249,16 @@ struct ib_cq *__ib_alloc_cq(struct ib_device *dev, void *private, int nr_cqe,
>  
>  	switch (cq->poll_ctx) {
>  	case IB_POLL_DIRECT:
> -		cq->comp_handler = ib_cq_completion_direct;
> +		comp_handler = ib_cq_completion_direct;
>  		break;
>  	case IB_POLL_SOFTIRQ:
> -		cq->comp_handler = ib_cq_completion_softirq;
> -
> +		comp_handler = ib_cq_completion_softirq;
>  		irq_poll_init(&cq->iop, IB_POLL_BUDGET_IRQ, ib_poll_handler);
> -		ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
>  		break;
>  	case IB_POLL_WORKQUEUE:
>  	case IB_POLL_UNBOUND_WORKQUEUE:
> -		cq->comp_handler = ib_cq_completion_workqueue;
> +		comp_handler = ib_cq_completion_workqueue;
>  		INIT_WORK(&cq->work, ib_cq_poll_work);
> -		ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
>  		cq->comp_wq = (cq->poll_ctx == IB_POLL_WORKQUEUE) ?
>  				ib_comp_wq : ib_comp_unbound_wq;
>  		break;
> @@ -269,6 +267,15 @@ struct ib_cq *__ib_alloc_cq(struct ib_device *dev, void *private, int nr_cqe,
>  		goto out_destroy_cq;
>  	}
>  
> +	/*
> +	 * The CQ is reachable from the completion interrupt once created. Set
> +	 * the handler only after the poll context it schedules is set up, and
> +	 * arm the CQ only after that.
> +	 */
> +	cq->comp_handler = comp_handler;

[Severity: High]
Since __ib_alloc_cq() calls dev->ops.create_cq(cq, &cq_attr, NULL) before
initializing the poll context and setting cq->comp_handler, can a completion
interrupt arriving right after dev->ops.create_cq() registers the CQ in the
device EQ table race with this initialization?

Unlike __ib_create_cq(), ib_uverbs_create_cq(), and
UVERBS_HANDLER(UVERBS_METHOD_CQ_CREATE), which set cq->comp_handler before
calling dev->ops.create_cq(), __ib_alloc_cq() calls dev->ops.create_cq() while
cq->comp_handler is still NULL:

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) {
	...
}

At this commit, mlx5_ib_cq_comp() calls ibcq->comp_handler() without a NULL
check (until the subsequent commit "RDMA/mlx5: Ignore completion events for a
CQ without a handler" adds an if (ibcq->comp_handler) check in mlx5):

drivers/infiniband/hw/mlx5/cq.c:mlx5_ib_cq_comp() {
	struct ib_cq *ibcq = &to_mibcq(cq)->ibcq;

	ibcq->comp_handler(ibcq, ibcq->cq_context);
}

Other RDMA drivers (such as efa_process_comp_eqe() in
drivers/infiniband/hw/efa/efa_main.c, mlx4_ib_cq_comp(),
mthca_cq_completion(), and cxgb4) also unconditionally invoke
cq->ibcq.comp_handler() without a NULL check once the CQ is inserted into the
device CQ table during create_cq(), and cq->comp_handler is never set if the
default branch in switch (cq->poll_ctx) jumps to out_destroy_cq.

In addition, even in drivers that check if (ibcq->comp_handler) in their
completion interrupt handler (such as mlx5_ib_cq_comp() after the follow-up
commit and hns_roce_cq_completion()), the CQ is already reachable on other
CPUs while irq_poll_init(&cq->iop, ...), INIT_WORK(&cq->work, ...), and
cq->comp_wq are being initialized. Because cq->comp_handler = comp_handler is
a plain store without release ordering (and the interrupt handler reads
ibcq->comp_handler without acquire ordering), can compiler or CPU store
reordering on weakly ordered architectures allow an interrupt handler on
another CPU to observe a non-NULL cq->comp_handler before the stores to
cq->iop, cq->work, or cq->comp_wq are visible?

Now that ib_req_notify_cq() is called after the switch (cq->poll_ctx) block,
would it be safer to initialize the poll context (cq->iop, cq->work, and
cq->comp_wq) and set cq->comp_handler before calling dev->ops.create_cq(),
leaving only rdma_dim_init() and ib_req_notify_cq() after create_cq()
succeeds?

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