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

[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: 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.

> 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

[ ... ]

> @@ -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_hwc() {
	/* 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?

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

> @@ -869,6 +884,20 @@ int mana_hwc_create_channel(struct gdma_context *gc)
>  	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
> +	 * 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
> +	 * 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?

> @@ -926,11 +955,38 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
>  	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);
> +
> +		if (td_err) {
> +			dev_err(gc->dev,
> +				"HWC teardown failed: %d, leaking resources\n",
> +				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?

> +
> +	/* 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);

[ ... ]

> 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?

>  	struct hwc_caller_ctx *caller_ctx;
>  };