Re: [PATCH net-next v7 02/13] net: mana: share the EQ pool across a queue-set swap

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 1 (1) · Low: 2

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

- [Medium] Every Ethernet CQ teardown now adds a blocking
  mana_gd_test_eq() on its parent EQ (mana_deinit_cq()) and an…
- [Low] This feature patch ('share the EQ pool across a queue-set swap')
  bundles two fixes for bugs that can be hit today, with no Fixes: tag,…
- [Low] A comment in struct mana_port_context (include/net/mana/mana.h)
  is out of date.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch's
  new reader protocol does not cover it, and the commit message says it…
- [Medium] This problem wasn't introduced by this patch, but the HWC
  teardown frees CQ callback state before IRQ readers are drained.

--

net: mana: share the EQ pool across a queue-set swap

This commit makes the MANA EQ pool port-owned. The old and new queue sets
in a swap now share EQs, and mana_grow_eqs() grows the pool when needed.

It also moves cq_table publication to release/acquire and adds a per-CQ
EQ flush plus an RCU grace period when a CQ is torn down. EQ
initialization now happens before IRQ registration, and the DMA region is
released when EQ creation fails.

> diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
> index d4e5e3f912689..0b6ad2547e0ef 100644
> --- a/drivers/infiniband/hw/mana/cq.c
> +++ b/drivers/infiniband/hw/mana/cq.c
> @@ -155,7 +155,8 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
>  	gdma_cq->type = GDMA_CQ;
>  	gdma_cq->cq.callback = mana_ib_cq_handler;
>  	gdma_cq->id = cq->queue.id;
> -	gc->cq_table[cq->queue.id] = gdma_cq;
> +	/* Pairs with the acquire load in mana_gd_process_eqe(). */
> +	smp_store_release(&gc->cq_table[cq->queue.id], gdma_cq);
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. This patch
adds a release/acquire and RCU reader protocol for cq_table. Shouldn't the
matching RDMA remover change as well?

mana_ib_remove_cq_cb() still frees the published gdma_queue before it
clears the slot, and it never waits for IRQ readers:

drivers/infiniband/hw/mana/cq.c:mana_ib_remove_cq_cb() {
    ...
	kfree(gc->cq_table[cq->queue.id]);
	gc->cq_table[cq->queue.id] = NULL;
}

mana_ib_destroy_cq() calls it before the hardware CQ is destroyed:

	mana_ib_remove_cq_cb(mdev, cq);
	...
	mana_ib_gd_destroy_cq(mdev, cq);

So the CQ can still raise EQEs at that point. Raw/RSS QPs attach these CQs
to the port's mpc->eqs[] pool, which this patch makes long-lived.

At the same time, mana_gd_process_eqe() runs under rcu_read_lock() in
mana_gd_intr() and does:

	cq = smp_load_acquire(&gc->cq_table[cq_id]);
	if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id))
		break;

	if (cq->cq.callback)
		cq->cq.callback(cq->cq.context, cq);

Suppose another CPU has already loaded the pointer when kfree() runs. Could
it read freed memory and call through a function pointer taken from it?

After destroy returns, the IB core also frees the mana_ib_cq that
mana_ib_cq_handler() dereferences.

The other CQ removers now unpublish, call synchronize_rcu(), and then
free, as mana_gd_destroy_cq() does. This is the last plain cq_table store
left, yet the commit message says:

    The HWC and RDMA sites need the matching stores, not changes to
    their teardown or recovery policies.

Does that hold for mana_ib_remove_cq_cb()? Userspace can reach it through
ibv_destroy_cq() while completions are still arriving on the same EQ. The
code still looks unchanged at the end of the series.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index ae0ed700b3b93..078b3666947a4 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c

[ ... ]

> @@ -1030,17 +1031,17 @@ static int mana_gd_create_eq(struct gdma_dev *gd,
>  		return -EINVAL;
>  	}
>  
> +	queue->eq.callback = spec->eq.callback;
> +	queue->eq.context = spec->eq.context;
> +	queue->head |= INITIALIZED_OWNER_BIT(log2_num_entries);
> +	queue->eq.log2_throttle_limit = spec->eq.log2_throttle_limit ?: 1;
> +
>  	err = mana_gd_register_irq(queue, spec);
>  	if (err) {
>  		dev_err(dev, "Failed to register irq: %d\n", err);
>  		return err;
>  	}
>  
> -	queue->eq.callback = spec->eq.callback;
> -	queue->eq.context = spec->eq.context;
> -	queue->head |= INITIALIZED_OWNER_BIT(log2_num_entries);
> -	queue->eq.log2_throttle_limit = spec->eq.log2_throttle_limit ?: 1;
> -
>  	if (create_hwq) {
>  		err = mana_gd_create_hw_eq(gc, queue);
>  		if (err)

[ ... ]

> @@ -1273,8 +1273,10 @@ int mana_gd_create_mana_eq(struct gdma_dev *gd,
>  	queue->gdma_dev = gd;
>  
>  	err = mana_gd_create_eq(gd, spec, true, queue);
> -	if (err)
> +	if (err) {
> +		mana_gd_destroy_dma_region(gc, gmi->dma_region_handle);
>  		goto out;
> +	}
>  
>  	*queue_ptr = queue;
>  	return 0;

[Severity: Low]
This isn't a bug, but both of these changes look like fixes for problems
that exist today, separate from the queue-set swap. The commit message
says the reorder ensures "a live shared IRQ cannot observe an
uninitialized EQ". It also says the DMA region release "closes an
existing unwind gap".

Before this patch, mana_gd_create_eq() linked the queue into gic->eq_list
with list_add_rcu() in mana_gd_register_irq(). That happened before
eq.callback, eq.context, the owner bit and log2_throttle_limit were set.
With msi_sharing, mana_gd_intr() could walk the new EQ while its head and
owner phase were still zero.

Also, when mana_gd_create_eq() failed, mana_gd_create_mana_eq() only
called mana_gd_free_memory(). That leaked the DMA region registered by
mana_gd_create_dma_region().

Both paths are reachable without any queue-set swap, through
mana_alloc_queues()->mana_create_eq() and through RDMA's
mana_ib_create_eqs().

Would it make sense to split these into separate patches with Fixes: tags,
so stable can pick them up?

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 3bca4b683134b..aefb8646cd79c 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -702,7 +702,8 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
>  	if (!gc->cq_table)
>  		return -ENOMEM;
>  
> -	gc->cq_table[cq->id] = cq;
> +	/* Pairs with the acquire load in mana_gd_process_eqe(). */
> +	smp_store_release(&gc->cq_table[cq->id], cq);
>  
>  	return 0;
>  }

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch, but the
HWC teardown path doesn't get the new grace period either.
mana_hwc_destroy_channel() does:

	if (gc->max_num_cqs > 0) {
		mana_smc_teardown_hwc(&gc->shm_channel, false);
		gc->max_num_cqs = 0;
	}

Because max_num_cqs is now 0, mana_gd_destroy_cq() returns early on its
id >= gc->max_num_cqs check and skips synchronize_rcu(). Then
mana_hwc_destroy_cq() does:

	kfree(hwc_cq->comp_buf);

	if (hwc_cq->gdma_cq)
		mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);

	if (hwc_cq->gdma_eq)
		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);

The EQ is deregistered and RCU-synchronized only after comp_buf and the
gdma_cq have already been freed.

An HWC EQ handler could still be running mana_hwc_comp_event() after
mana_smc_teardown_hwc() returns. That can happen with threaded or
preempted IRQ handling, or with a late HWC response during removal,
suspend or reset. Can that handler then touch the freed comp_buf through:

	completions = hwc_cq->comp_buf;

or touch the freed gdma_cq? The commit message deliberately leaves the
HWC teardown policy alone, so this only points out the remaining gap.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 7586972e78e09..4b2c8039b8fa3 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> @@ -2653,12 +2712,25 @@ static void mana_schedule_napi(void *context, struct gdma_queue *gdma_queue)
>  
>  static void mana_deinit_cq(struct mana_port_context *apc, struct mana_cq *cq)
>  {
> -	struct gdma_dev *gd = apc->ac->gdma_dev;
> +	struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
> +	struct gdma_queue *gdma_cq = cq->gdma_cq;
> +	struct gdma_queue *eq;
> +	int err;
>  
> -	if (!cq->gdma_cq)
> +	if (!gdma_cq)
>  		return;
>  
> -	mana_gd_destroy_queue(gd->gdma_context, cq->gdma_cq);
> +	eq = gdma_cq->cq.parent;
> +	if (gdma_cq->id < gc->max_num_cqs && eq &&
> +	    eq->id != INVALID_QUEUE_ID) {
> +		/* Flush queued events after WQ teardown, before removing this CQ. */
> +		err = mana_gd_test_eq(gc, eq);
> +		if (err && mana_en_need_log(apc, err))
> +			netdev_err(apc->ndev, "Failed to flush EQ %u for CQ %u: %d\n",
> +				   eq->id, gdma_cq->id, err);
> +	}
> +
> +	mana_gd_destroy_queue(gc, gdma_cq);
>  }

[Severity: Medium]
How much slower does teardown get with this? Every Ethernet CQ now does a
blocking mana_gd_test_eq() on its parent EQ here. It also does an
unconditional synchronize_rcu() in mana_gd_destroy_cq():

	cmpxchg(&gc->cq_table[id], queue, NULL);
	synchronize_rcu();

Both run one CQ at a time from mana_destroy_txq() and mana_destroy_rxq().
They hold RTNL and the netdev instance lock, and they run on close,
detach, reset, create unwind and queue-set free.

mana_gd_test_eq() takes gc->eq_test_event_mutex, sends
GDMA_GENERATE_TEST_EQE over the HWC, and waits up to 30 * HZ. TX CQ i and
RX CQ i share EQ i. After that, mana_destroy_eq()->mana_gd_destroy_eq()
flushes each EQ again and adds another grace period in
mana_gd_deregister_irq().

So teardown seems to go from N test-EQ round trips and N grace periods to
3N of each. synchronize_rcu() now also runs for CQs that were never
published; the old code returned early when the slot was empty.

A failed flush is only logged. Suppose an EQ stops delivering test events
while the HWC still answers. Wouldn't each CQ on that EQ wait the full 30
seconds, and then mana_gd_destroy_eq() wait again?

Would one flush per distinct EQ, plus one grace period after all slots
are cleared, be enough here? This still seems to be the case at the end
of the series.

[ ... ]

> diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
> index 1d91b62299f56..12a65145cd3fd 100644
> --- a/include/net/mana/mana.h
> +++ b/include/net/mana/mana.h
> @@ -574,7 +574,9 @@ struct mana_port_context {
>  
>  	u8 mac_addr[ETH_ALEN];
>  
> +	/* Port-owned EQ pool: max_queues slots, num_eqs populated. */
>  	struct mana_eq *eqs;
> +	unsigned int num_eqs;
>  	struct dentry *mana_eqs_debugfs;
>  
>  	enum TRI_STATE rss_state;

[Severity: Low]
This isn't a bug, but an older comment further down in struct
mana_port_context still says:

	/* Create num_queues EQs, SQs, SQ-CQs, RQs and RQ-CQs, respectively. */
	unsigned int max_queues;
	unsigned int num_queues;

mana_destroy_eq() now iterates over num_eqs, which can exceed num_queues
after mana_grow_eqs(). Should that comment be updated as well? It is
still there at the end of the series.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791710735.git.weh%40linux.microsoft.com