Re: [PATCH net v3 1/6] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy

This puts gc->cq_table under RCU: the base pointer and the entries are
annotated __rcu, the EQ interrupt handler reads them with
rcu_dereference(), publishers use rcu_assign_pointer(), and teardown
clears the slot and calls synchronize_rcu() before the CQ is freed.

A few questions on the teardown latency and on the lifetime rule the new
gdma.h comment documents.

> diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
> index f2547989f4229..2bf4be21cede7 100644
> --- a/drivers/infiniband/hw/mana/cq.c
> +++ b/drivers/infiniband/hw/mana/cq.c

[ ... ]

> @@ -149,23 +157,49 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)

[ ... ]

> +	rcu_assign_pointer(cq_table[cq->queue.id], NULL);
> +
> +	/* Wait for in-flight EQ handlers that may have loaded the old
> +	 * pointer via rcu_dereference() to finish before freeing.
> +	 */
> +	synchronize_rcu();
> +	kfree(gdma_cq);
>  }

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index e8b7ffb47eb98..fa6fcc2475c1a 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c

[ ... ]

> @@ -743,10 +744,11 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
>  	switch (type) {
>  	case GDMA_EQE_COMPLETION:
>  		cq_id = eqe->details[0] & 0xFFFFFF;
> -		if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
> +		cq_table = rcu_dereference(gc->cq_table);
> +		if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs || !cq_table))
>  			break;
>  
> -		cq = gc->cq_table[cq_id];
> +		cq = rcu_dereference(cq_table[cq_id]);
>  		if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id))
>  			break;

[Severity: High]

This isn't a bug introduced by this patch - the baseline had the same
"if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs)) break; cq = gc->cq_table[cq_id];"
pattern - but the patch rewrites these lines and documents the RCU scheme
as the complete safety argument for cq_table, while the bound that guards
every index stays outside the published object.

mana_hwc_establish_channel() freezes the element count at vcalloc() time
and publishes only the pointer, but gc->max_num_cqs keeps being written
straight from a device-supplied EQE payload:

hw_channel.c:mana_hwc_init_event_handler() {
	...
		case HWC_INIT_DATA_MAX_NUM_CQS:
			gd->gdma_context->max_num_cqs = val;
			break;
	...
}

That handler stays installed as the HWC EQ eq.callback for the whole
device lifetime, and mana_gd_process_eqe() dispatches any
GDMA_EQE_HWC_INIT_DATA to it from hard IRQ context, with no coupling to the
currently published table.

If the PF raises gc->max_num_cqs above the allocated element count, does
rcu_dereference(cq_table[cq_id]) then read past the end of the vcalloc'ed
array, and can the value read there be used as
cq->cq.callback(cq->cq.context, cq)?

Separately, the gc->max_num_cqs load carries no address dependency on the
rcu_dereference()d base pointer. Can a reader therefore pair a freshly
published table with a stale bound, producing a spurious hard-IRQ
WARN_ON_ONCE and a silently dropped completion?

Would storing the element count inside or alongside the published
allocation be more robust than trusting a device-writable, non-RCU field?

[ ... ]

> @@ -1053,15 +1055,28 @@ static void mana_gd_create_cq(const struct gdma_queue_spec *spec,
>  static void mana_gd_destroy_cq(struct gdma_context *gc,
>  			       struct gdma_queue *queue)
>  {
> +	struct gdma_queue __rcu **cq_table;
>  	u32 id = queue->id;
>  
> -	if (id >= gc->max_num_cqs)
> +	/* No rcu_read_lock() here: mana_gd_destroy_cq() runs only on the
> +	 * CQ-destroy/teardown path, where the base cq_table is stable.  See
> +	 * the lifecycle note on gdma_context::cq_table in gdma.h for why the
> +	 * "true" predicate is sound.
> +	 */
> +	cq_table = rcu_dereference_protected(gc->cq_table, true);
> +	if (!cq_table || id >= gc->max_num_cqs)
>  		return;
>  
> -	if (!gc->cq_table[id])
> +	if (!rcu_access_pointer(cq_table[id]))
>  		return;
>  
> -	gc->cq_table[id] = NULL;
> +	rcu_assign_pointer(cq_table[id], NULL);
> +
> +	/* Wait for in-flight EQ handlers that may have loaded the old
> +	 * pointer via rcu_dereference() to finish before the caller
> +	 * frees the CQ memory.
> +	 */
> +	synchronize_rcu();
>  }

[Severity: High]

Is one grace period per CQ acceptable on this path? mana_gd_destroy_cq()
is reached once per TX queue and once per RX queue during netdev teardown:

mana_en.c:mana_destroy_txq() {
	for (i = 0; i < apc->num_queues; i++) {
		...
		mana_deinit_cq(apc, &apc->tx_qp[i]->tx_cq);
}

mana_deinit_cq() -> mana_gd_destroy_queue() -> mana_gd_destroy_cq(), and
mana_destroy_rxq() does the same for &rxq->rx_cq.

With MANA_MAX_NUM_QUEUES == 64 that is up to about 128 serialized RCU
grace periods per ifdown, MTU change, ethtool -L, ethtool -G, XDP attach
or queue reset. Those loops run with rtnl_lock() held (mana_detach() has
ASSERT_RTNL(), and the same loops use napi_disable_locked() /
netif_napi_del_locked(), so the netdev instance lock is held too).

Since RTNL is system-wide, does this stall every other network
configuration operation on the machine for the accumulated duration?

The same question applies to mana_ib_remove_cq_cb() in
drivers/infiniband/hw/mana/cq.c, which now blocks for a full grace period
on each user-triggered ib_destroy_cq() and once per ind_tbl entry in the
mana_ib_create_qp_rss() unwind loop.

mana_ib_remove_cq_cb() frees the per-CQ struct gdma_queue itself, so would
kfree_rcu() give the same guarantee without blocking? For the netdev path,
could all slots be cleared first and a single grace period taken per
teardown batch instead of one per CQ?

The changelog describes the change as annotation plus a deferred free;
would it be worth mentioning the new teardown latency so backporters know
CQ-destroy and ifdown timing changes by orders of magnitude?

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index e3c24d50dad07..409e20caeccdc 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -698,11 +699,15 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
>  	if (WARN_ON(cq->id >= gc->max_num_cqs))
>  		return -EPROTO;
>  
> -	gc->cq_table = vcalloc(gc->max_num_cqs, sizeof(struct gdma_queue *));
> -	if (!gc->cq_table)
> +	cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
> +	if (!cq_table)
>  		return -ENOMEM;
>  
> -	gc->cq_table[cq->id] = cq;
> +	rcu_assign_pointer(cq_table[cq->id], cq);
> +	/* Publish the fully-initialised table last; pairs with the
> +	 * rcu_dereference(gc->cq_table) in mana_gd_process_eqe().
> +	 */
> +	rcu_assign_pointer(gc->cq_table, cq_table);

[Severity: High]

This is a pre-existing check-then-use pattern and not introduced by this
patch, but the patch rewrites exactly these lines.

cq->id and gc->max_num_cqs are validated once, then re-read separately -
one for the allocation size, one as the array index. Both fields are
written from the HWC EQ interrupt handler:

hw_channel.c:mana_hwc_init_event_handler() {
	...
		case HWC_INIT_DATA_CQID:
			hwc->cq->gdma_cq->id = val;
			break;
	...
		case HWC_INIT_DATA_MAX_NUM_CQS:
			gd->gdma_context->max_num_cqs = val;
			break;
	...
}

The HWC EQ is already armed and delivering EQEs here, since this code only
runs after GDMA_EQE_HWC_INIT_DONE completed hwc_init_eqe_comp.

If the device emits an extra or duplicate GDMA_EQE_HWC_INIT_DATA in the
window between the WARN_ON() check, the vcalloc() and the indexed store
(smaller MAX_NUM_CQS or larger CQID), do the validated expression and the
used expression diverge, writing the cq pointer at a device-chosen offset
past the end of the fresh vcalloc'ed table?

Would snapshotting both values into locals before the check and using only
the snapshot for the allocation size and the index close this?

[ ... ]

> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> index 0c395917b2144..0b48ded0aecda 100644
> --- a/include/net/mana/gdma.h
> +++ b/include/net/mana/gdma.h
> @@ -418,7 +418,26 @@ struct gdma_context {
>  
>  	/* This maps a CQ index to the queue structure. */
>  	unsigned int		max_num_cqs;
> -	struct gdma_queue	**cq_table;
> +	/* Both the base pointer and each entry are RCU-managed.  The fast
> +	 * path (mana_gd_process_eqe) reads the base via rcu_dereference()
> +	 * under rcu_read_lock(), so the table is freed with
> +	 * rcu_assign_pointer(NULL) + synchronize_rcu() and an in-flight
> +	 * reader can never observe freed memory.
> +	 *
> +	 * The slow paths -- mana_gd_destroy_cq() and the CQ install/remove
> +	 * callers (mana_create_txq/_rxq, mana_ib_install/remove_cq_cb) --
> +	 * instead read the base with rcu_dereference_protected(cq_table,
> +	 * true).  The bare "true" is justified by teardown ordering, not by
> +	 * a lock: the base table is replaced+freed only by
> +	 * mana_hwc_destroy_channel() (and the create-time reinit), and every
> +	 * teardown path first runs mana_remove() + mana_rdma_remove(), which
> +	 * synchronously drain the netdev and the IB device
> +	 * (unregister_netdevice / ib_unregister_device) that bound all
> +	 * install/remove callers; the reinit case runs before either
> +	 * consumer is probed.  So no slow-path caller can run while the base
> +	 * table is being freed.
> +	 */
> +	struct gdma_queue	__rcu * __rcu *cq_table;

[Severity: Medium]

Is the statement "every teardown path first runs mana_remove() +
mana_rdma_remove(), which synchronously drain the netdev and the IB device
(unregister_netdevice / ib_unregister_device)" accurate for the suspend
and shutdown paths?

mana_gd_suspend() (also reached from the reset service work via
mana_serv_reset()) and mana_gd_shutdown() both call
mana_remove(&gc->mana, true), and the suspending branch deliberately keeps
the netdev registered:

mana_en.c:mana_remove() {
	...
		if (suspending) {
			/* No need to unregister the ndev. */
			rtnl_unlock();
			continue;
		}

		unregister_netdevice(ndev);
	...
}

RTNL is then dropped and mana_gd_cleanup_device() ->
mana_hwc_destroy_channel() runs:

	old_cq_table = rcu_replace_pointer(gc->cq_table, NULL, true);
	synchronize_rcu();
	vfree(old_cq_table);

mana_detach(ndev, false) also calls netif_device_detach() while leaving
apc->port_st_save == true, so a concurrent RTNL reconfiguration in that
window (mana_change_mtu(), mana_set_channels(), mana_set_ringparam(), XDP
set) takes the "if (!from_close && !netif_device_present(ndev)) return 0;"
early return in mana_detach() and proceeds into mana_attach() ->
mana_alloc_queues() -> mana_create_txq(), which does:

	cq_table = rcu_dereference_protected(gc->cq_table, true);
	if (WARN_ON(!cq_table || cq->gdma_id >= gc->max_num_cqs)) {
	...
	rcu_assign_pointer(cq_table[cq->gdma_id], cq->gdma_cq);

These installers are writers, not RCU readers, so synchronize_rcu() does
not wait for them, and the !cq_table test is a check rather than
protection. Can this store land in the vfree()d table?

One step I could not confirm: mana_attach() first issues
mana_query_vport_cfg() over the HWC after mana_gd_deregister_device(), and
whether the PF still answers that is device behaviour; on the
mana_serv_reset() path hwc->hwc_timeout is 0 so the command times out, and
on PM suspend userspace is frozen, which leaves the reboot /
mana_gd_shutdown() window as the plausible case. The unsynchronized writer
predates this patch, but the lifetime rule stated here is new, so should
the invariant be corrected (serialize base replacement against the
installers, or run the installers under rcu_read_lock())?

One more on the same comment: mana_gd_destroy_cq() is listed as covered by
this rule, but it is called from inside mana_hwc_destroy_channel() itself
(mana_hwc_destroy_cq() -> mana_gd_destroy_queue() -> mana_gd_destroy_cq()),
a few statements before that function replaces and vfree()s the base
table. Should that intra-function ordering be spelled out here as well,
since it is what actually makes that caller's "true" predicate sound?