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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · 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 patch relies on program order alone to publish the poll
  context before the handler, and that ordering does not hold for…
- [Medium] __ib_alloc_cq() (drivers/infiniband/core/cq.c) still stores
  cq->comp_handler after dev->ops.create_cq() has returned (create_cq at…

--

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

This commit reorders __ib_alloc_cq(). The irq_poll or work context and
comp_wq are now initialized before cq->comp_handler is stored, and the CQ
is armed only after the handler is set. The goal is that a completion
event raised right after ops.create_cq() returns no longer finds an
uninitialized poll context.

> 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

[ ... ]

> @@ -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: Medium]
This store still runs after dev->ops.create_cq() has returned. By then, can
the driver's EQ dispatch already reach the CQ while comp_handler is still
NULL from rdma_zalloc_drv_obj()?

The driver publishes the CQN during create_cq. For example,
mlx5_core_create_cq()->mlx5_eq_add_cq() inserts it into the radix tree,
efa stores it in dev->cqs_xa, and mthca adds it to cq_table.

Several drivers then call the handler without a NULL check. For example,
in drivers/infiniband/hw/mlx4/cq.c:

static void mlx4_ib_cq_comp(struct mlx4_cq *cq)
{
	struct ib_cq *ibcq = &to_mibcq(cq)->ibcq;
	ibcq->comp_handler(ibcq, ibcq->cq_context);
}

efa_process_comp_eqe() and mthca_cq_completion() do the same, and so does
mlx5_ib_cq_comp() at this commit.

So an early event from a non-conforming device would now be a NULL
function pointer call in irq or tasklet context, instead of scheduling an
uninitialized irq_poll or work. The patch also moves the store later than
before, past rdma_dim_init() (which can do a GFP_KERNEL allocation) and the
whole poll context switch, so the window gets a bit longer.

The commit message says:

    A conforming device raises no event for a new,
    empty CQ, but nothing in the core relies on that.

Doesn't the core now rely on every driver NULL-checking comp_handler?

The later commit "RDMA/mlx5: Ignore completion events for a CQ without a
handler" adds that check to mlx5_ib_cq_comp(), which closes the window for
mlx5. mlx4, efa and mthca still call the handler unconditionally at the end
of the series.

__ib_create_cq() in verbs.c, and the uverbs paths, set the handler before
calling into the driver:

	cq->device = device;
	cq->comp_handler = comp_handler;
	...
	ret = device->ops.create_cq(cq, cq_attr, NULL);

Could __ib_alloc_cq() do the same? That would mean running irq_poll_init(),
INIT_WORK(), the comp_wq store and the comp_handler store before
ops.create_cq(), and keeping only ib_req_notify_cq() for afterwards. The
driver's own publication of the CQ would then provide the ordering.

[Severity: High]
Is program order enough to make the poll context visible before the
handler? The irq_poll_init() stores (memset, INIT_LIST_HEAD, weight, poll),
INIT_WORK() and the cq->comp_wq store are all plain stores. This
comp_handler store is plain too, with no smp_store_release() or smp_wmb().

The reader side has no acquire ordering either:

mlx5 EQ irq
  mlx5_cq_tasklet_cb()
    mlx5_ib_cq_comp()
      ibcq->comp_handler()          <- plain load, indirect call
        ib_cq_completion_softirq()
          irq_poll_sched(&cq->iop)
      or
        ib_cq_completion_workqueue()
          queue_work(cq->comp_wq, &cq->work)

An event that arrives before the CQ is armed gets no ordering from the
doorbell write in ib_req_notify_cq().

On weakly ordered CPUs such as arm64 and POWER, can another CPU see the new
comp_handler before the iop, work or comp_wq stores? In the workqueue
cases, the compiler is also free to reorder the adjacent plain stores to
comp_wq and comp_handler.

If that happens, three things could go wrong:

- The reader schedules an iop whose memset and INIT_LIST_HEAD land after
  its own test_and_set_bit() and list_add_tail(). That corrupts the per-cpu
  irq_poll list.
- irq_poll_softirq() calls a stale NULL iop->poll.
- queue_work() is called with a NULL workqueue.

These are the failures the commit message describes.

Adversarial mode is enabled through cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT).
That is true for arm64 realm/pKVM guests and pseries secure guests. The mlx5
UMR CQ allocated in mlx5r_umr_resource_init() with IB_POLL_SOFTIRQ goes
through this path. The plain NULL check that the later commit adds to
mlx5_ib_cq_comp() does not provide acquire ordering.

Would this need smp_store_release() on comp_handler, paired with an acquire
load in the drivers? The other option is to do all of this setup before
ops.create_cq(). Then the driver's publication, such as radix_tree_insert()
under table->lock in mlx5_eq_add_cq(), would order the stores for readers
that look up the CQ.

> +	if (cq->poll_ctx != IB_POLL_DIRECT)
> +		ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);

[ ... ]

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