RE: [EXTERNAL] Re: [PATCH net v6 5/7] net: mana: fix HWC teardown safety with setup_active flag and destroy ordering

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: fix HWC teardown safety with setup_active flag and destroy
ordering

This adds a setup_active flag to struct hw_channel_context to record that
mana_smc_setup_hwc() has activated device MST entries, reorders
mana_hwc_destroy_channel() so the HWC CQ (and with it the EQ IRQ
deregistration) is destroyed before the TXQ/RXQ buffers are freed, and
changes destroy_channel() to leak the HWC resources when
mana_smc_teardown_hwc() fails.

A few questions on the new flag placement and on the leak-on-failure policy.

quoted

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c

b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 5db8cfe2d8443..959886434d07f 100644

--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

quoted

@@ -783,6 +784,20 @@ static int mana_hwc_establish_channel(struct

gdma_context *gc, u16 *q_depth,

 	init_completion(&hwc->hwc_init_eqe_comp);

+	/* Arm setup_active before issuing the setup command.
+	 * mana_smc_setup_hwc() hands the queue PFNs to the PF, activating
+	 * MST entries so the device can DMA into our queue buffers, before
+	 * it can report a later failure such as a possession-poll timeout.
+	 * Recording it up front guarantees the error path
+	 * (mana_hwc_create_channel() -> mana_hwc_destroy_channel()) still
+	 * tears the HWC down instead of freeing buffers the device may still
+	 * write to.  Setting it for a rare pre-submission failure too is
+	 * harmless -- the teardown is then a no-op the device ignores.  Do
+	 * not also tear down here: a second teardown would double the
+	 * hardware timeout on failure and mask the original error code.
+	 */
+	hwc->setup_active = true;
+
 	err = mana_smc_setup_hwc(&gc->shm_channel, false,
 				 eq->mem_info.dma_handle,
 				 cq->mem_info.dma_handle,

[Severity: High]

Is the claim that "the teardown is then a no-op the device ignores" true for the
most likely pre-submission failure?

mana_smc_setup_hwc() fails at its very first step, before anything is written to
the shared memory aperture:

drivers/net/ethernet/microsoft/mana/shm_channel.c:mana_smc_setup_hwc(
) {
	/* Ensure VF already has possession of shared memory */
	err = mana_smc_poll_register(sc->base, false);
	if (err) {
		dev_err(sc->dev, "Timeout when setting up HWC: %d\n", err);
		return err;
	}
	...
}

mana_smc_teardown_hwc() opens with the same poll:

drivers/net/ethernet/microsoft/mana/shm_channel.c:mana_smc_teardown_h
wc() {
	/* Ensure already has possession of shared memory */
	err = mana_smc_poll_register(sc->base, false);
	if (err) {
		dev_err(sc->dev, "Timeout when tearing down HWC\n");
		return err;
	}
	...
}

So when the possession poll times out, no ESTABLISH_HWC message ever
reached the device, but setup_active is already set, the error path runs
mana_hwc_destroy_channel(), the teardown fails for the identical reason, and
the new "leaking resources" branch returns having freed nothing.

Does that leak the hwc struct, the inflight bitmap, the EQ/CQ/RQ/SQ gdma
queues with their dma_alloc_coherent buffers, comp_buf and both msg_buf
DMA buffers, in a case where the old gc->max_num_cqs > 0 gate skipped the
teardown entirely and freed everything?

Would arming the flag only after mana_smc_setup_hwc() returns success, or
distinguishing "message submitted" from "poll failed before submission", avoid
recording a teardown obligation the device never acquired?

The "no-op the device ignores" wording is dropped in v7. On the substance: arming only after success would reopen the window this patch closes -- mana_smc_setup_hwc() activates the MST entries mid-call, so a failure after activation but before it returns success would leave the flag clear and free buffers the device can still DMA into. Arming before is the safe side. The cost is over-leaking on a possession-poll timeout, where freeing would in fact be safe; telling the two apart needs mana_smc_setup_hwc() to report whether it passed the possession poll -- a shm_channel change I'd keep separate. The result is a one-generation leak on a rare setup timeout, not corruption.

[Severity: Medium]

This is a pre-existing issue and not introduced by this patch, but while reading
mana_hwc_establish_channel(): the bound check

	if (WARN_ON(cq->id >= gc->max_num_cqs))
		return -EPROTO;

	cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
	...
	rcu_assign_pointer(cq_table[cq->id], cq);

re-reads both gc->max_num_cqs and cq->id after the check, while
mana_hwc_init_event_handler() writes both from EQ interrupt context
(HWC_INIT_DATA_MAX_NUM_CQS and HWC_INIT_DATA_CQID).

The later patch in this series, "net: mana: keep max_num_cqs immutable once
cq_table is allocated", snapshots both values into locals with
READ_ONCE() and commits gc->max_num_cqs once, so the check-then-use is
gone by the end of the series. Noted here only for completeness.

Agreed, and as you note the immutable-max_num_cqs patch snapshots both with READ_ONCE() and commits gc->max_num_cqs once, so the check-then-use is gone by the end of the series. Nothing to change here.

quoted

@@ -869,6 +884,20 @@ int mana_hwc_create_channel(struct

gdma_context *gc)

quoted

 	u16 q_depth_max;
 	int err;

+	/* A previous teardown may have failed and deliberately left the old
+	 * HWC context reachable (see mana_hwc_destroy_channel()).  Retry

the

quoted

+	 * teardown now -- the device has since been reset -- before building
+	 * a new channel, so we neither orphan the old context nor stack a
+	 * second channel on one whose DESTROY_HWC never completed.  If it

is

quoted

+	 * still failing, return an error that steers mana_serv_reset() to a
+	 * full PCI rescan instead of silently leaking another generation.
+	 */
+	if (gd->driver_data) {
+		mana_hwc_destroy_channel(gc);
+		if (gd->driver_data)
+			return -ETIMEDOUT;
+	}
+
 	hwc = kzalloc_obj(*hwc);
 	if (!hwc)
 		return -ENOMEM;

[Severity: High]

This retry is the only mechanism that can reclaim a retained generation.
Can it run on the probe failure path?

mana_hwc_destroy_channel() is void, so mana_gd_setup() cannot tell that the
teardown was incomplete and keeps unwinding, and mana_gd_probe() ends
at:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_probe() {
	...
free_gc:
	pci_set_drvdata(pdev, NULL);
	vfree(gc);
	...
}

That frees the gdma_context holding gd->driver_data and gc->shm_channel, so
DESTROY_HWC can never be issued for the retained context afterwards.
mana_gd_remove() also vfree()s gc after mana_gd_cleanup_device().

Since mana_gd_probe() schedules mana_dev_recovery_work every
MANA_SERVICE_PERIOD on -ETIMEDOUT, and mana_serv_reset() escalates to
mana_serv_rescan() (remove plus re-probe), does every cycle in which
DESTROY_HWC does not complete retain a fresh generation, paced by the PF?

For errnos other than -ETIMEDOUT/-EPROTO (for example the -ENOMEM from
the vcalloc in mana_hwc_establish_channel(), which now happens with
setup_active already armed) mana_serv_reset() does no rescan:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_serv_reset() {
	ret = mana_gd_resume(pdev);
	if (ret == -ETIMEDOUT || ret == -EPROTO) {
		/* Perform PCI rescan on device if we failed on HWC */
		dev_err(&pdev->dev, "MANA service: resume failed,
rescanning\n");
		mana_serv_rescan(pdev);
	...
}

Is the retained generation then abandoned with no recovery at all?

Would a bounded quarantine work better here, keeping at most one retained
generation reclaimed by the next successful teardown, or propagating the
teardown error so an owner of gc stays alive until reclamation is safe?

This comment will not be addressed in v7.

Correct that the retry is best-effort: it reclaims on the next create_channel, which the common recovery path (serv_reset -> resume -> create) reaches, but a probe failure that vfree()s gc, or a non-ETIMEDOUT/-EPROTO error that skips the rescan, abandons the retained generation. That is the deliberate cost of leaking rather than freeing buffers the device may DMA into -- a bounded memory leak per failed teardown, not corruption. A bounded quarantine reclaimed by the recovery worker would be an improvement, but it's a recovery-architecture change beyond this fix; I'd do it separately.

quoted

@@ -926,11 +955,38 @@ void mana_hwc_destroy_channel(struct

gdma_context *gc)

quoted

 	if (!hwc)
 		return;

-	/* gc->max_num_cqs is set in mana_hwc_init_event_handler(). If it's
-	 * non-zero, the HWC worked and we should tear down the HWC here.
+	/* Tear down the HWC if setup_hwc previously activated MST entries.
+	 * This is the definitive flag — unlike max_num_cqs which depends
+	 * on the init EQE arriving.
+	 *
+	 * If teardown fails the device may still have active MST entries
+	 * and can DMA into the HWC queue buffers.  Freeing them would risk
+	 * memory corruption on systems without an IOMMU to fault the stale
+	 * DMA, so leak the HWC resources instead of handing the pages back
+	 * to the allocator.  Keep setup_active set so the failure is not
+	 * mistaken for a clean teardown.
 	 */
-	if (gc->max_num_cqs > 0)
-		mana_smc_teardown_hwc(&gc->shm_channel, false);
+	if (hwc->setup_active) {
+		int td_err = mana_smc_teardown_hwc(&gc->shm_channel,

false);

quoted

+
+		if (td_err) {
+			dev_err(gc->dev,
+				"HWC teardown failed: %d, leaking

resources\n",

quoted

+				td_err);
+			return;
+		}
+
+		hwc->setup_active = false;
+	}

[Severity: Low]

This return is taken before mana_hwc_destroy_cq(), so the HWC EQ stays on
gic->eq_list and the CQ stays published in gc->cq_table while
mana_gd_cleanup_device() goes on to mana_gd_remove_irqs().

This does not appear to be a use-after-free. The HWC EQ registration takes no
gdma_irq_context reference:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_register_irq() {
	/* The caller acquired a GIC reference via mana_gd_get_gic().
	 * That refcount prevents mana_gd_put_gic() from erasing this
	 * irq_contexts entry concurrently.
	 */
	gic = xa_load(&gc->irq_contexts, msix_index);
	...
}

so mana_gd_put_gic() reaches refcount_dec_and_test() and calls free_irq(),
which unregisters mana_gd_intr() and waits out in-flight handlers before any
vfree(gc). In the window before that, an interrupt would only touch the
EQ/CQ/RQ/TXQ buffers this branch deliberately did not free.

What is left is stale state: the EQ's list node ends up pointing into a kfree()d gic,
and on a later retried teardown after a resume whose
mana_gd_setup() failed before mana_gd_setup_hwc_irqs(),
mana_gd_deregister_irq() can hit

	msix_index = queue->eq.msix_index;
	if (WARN_ON(msix_index >= gc->num_msix_usable))
		return;

with num_msix_usable == 0 and skip the list_del_rcu()/synchronize_rcu()
before the EQ is freed. Would it be reasonable to deregister the EQ IRQ before
taking the leak branch, so the retained buffers are unreachable from the
interrupt path even in the failure case?

I'm fixing it in v7.

quoted

+
+	/* Tear down the HWC CQ object first — mana_hwc_destroy_cq()
+	 * both unpublishes the CQ from cq_table (+synchronize_rcu) and
+	 * deregisters the HWC EQ from the interrupt handler list (via
+	 * mana_gd_deregister_irq + synchronize_rcu), guaranteeing no
+	 * interrupt handler can access RQ/TXQ buffers after this point.
+	 */
+	if (hwc->cq)
+		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc-
cq);

 	if (hwc->txq)
 		mana_hwc_destroy_wq(hwc, hwc->txq);

[ ... ]

quoted

diff --git a/include/net/mana/hw_channel.h

b/include/net/mana/hw_channel.h index 787c6f96d5b57..8340abd36af61
100644

--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h

@@ -206,6 +206,15 @@ struct hw_channel_context {
 	 */
 	u32 rx_leaked_wqe;

+	/* Set after mana_smc_setup_hwc() succeeds (hardware has active
+	 * MST entries).  Cleared only after mana_smc_teardown_hwc()
+	 * succeeds, on both the recoverable establish_channel path and the
+	 * terminal destroy_channel path.  If teardown fails it stays set:
+	 * establish_channel() skips its retry and destroy_channel() leaks
+	 * the HWC rather than free buffers the device may still DMA into.
+	 */
+	bool setup_active;
+

[Severity: Low]

This isn't a runtime bug, but this comment describes different code than what
the patch implements, and it also contradicts the in-code comment added to
mana_hwc_establish_channel():

"Set after mana_smc_setup_hwc() succeeds" - mana_hwc_establish_channel()
sets it before the call and leaves it set when the call fails.

"Cleared only after mana_smc_teardown_hwc() succeeds, on both the
recoverable establish_channel path and the terminal destroy_channel path" -
mana_hwc_establish_channel() has no mana_smc_teardown_hwc() call and
never clears the flag; the single teardown call site and the single clear are both
in mana_hwc_destroy_channel().

"establish_channel() skips its retry" - mana_hwc_establish_channel() has no
retry.

Are these leftovers from the earlier version of the patch that tore down inline?
Could the comment be updated to match the current placement, so a later
reader does not move the assignment below mana_smc_setup_hwc() and
reopen the window this patch closes?

Fixed in v7. The field comment now reads "set before the setup command, cleared after teardown succeeds" and no longer mentions a clear on the establish path or a retry in establish_channel; it matches the in-code comment and the implementation.

Long

quoted

 	struct hwc_caller_ctx *caller_ctx;
 };