Re: [PATCH net v3 5/6] 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 so the driver
knows when the device has live MST entries, reorders
mana_hwc_destroy_channel() to destroy the HWC CQ (and thereby deregister
the EQ) before freeing the TXQ/RXQ buffers, and leaks the HWC resources
when mana_smc_teardown_hwc() fails instead of freeing memory the device
may still DMA into.

A few questions on the new error paths below.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index e378b8ec97c9a..1603968d7989a 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -768,20 +769,33 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
>  	if (err)
>  		return err;
>  
> -	if (!wait_for_completion_timeout(&hwc->hwc_init_eqe_comp, 60 * HZ))
> -		return -ETIMEDOUT;
> +	/* setup_hwc activated MST entries — hardware can now DMA into
> +	 * our queue buffers.  If anything below fails, we must tear
> +	 * down before returning so the caller doesn't need to track
> +	 * whether setup_hwc succeeded.
> +	 */
> +	hwc->setup_active = true;

[Severity: High]
This isn't a bug introduced by this patch, but now that setup_active is
the only gate for sending DESTROY_HWC, is the flag armed late?

mana_smc_setup_hwc() in shm_channel.c publishes the EQ/CQ/RQ/SQ frame
addresses to the PF with the last of its writel()s, and only then polls
for the response:

	/* Write 256-message buffer to shared memory (final 32-bit write
	 * triggers HW to set possession bit to PF).
	 */
	dword = (u32 *)shm_buf;
	for (i = 0; i < SMC_APERTURE_DWORDS; i++)
		writel(*dword++, sc->base + i * SMC_BASIC_UNIT);
	...
	err = mana_smc_read_response(...);
	if (err) {
		dev_err(sc->dev, "Error when setting up HWC: %d\n", err);
		return err;
	}

So an error return here does not mean the request never reached the PF.
mana_smc_read_response() can return -ETIMEDOUT after the 20 s poll in
mana_smc_poll_register() (the PF still owns shmem while it processes the
establish) or -EPROTO on a non-zero status.

In that case mana_hwc_establish_channel() returns with setup_active still
false, and the cleanup path frees the very buffers whose addresses were
handed to the PF:

mana_hwc_create_channel()
  out: mana_hwc_destroy_channel()
         if (hwc->setup_active)      /* false, no DESTROY_HWC is sent */
         mana_hwc_destroy_cq()       /* frees EQ/CQ DMA buffers */
         mana_hwc_destroy_wq()       /* frees RQ/SQ DMA buffers */

Should the flag be set before mana_smc_setup_hwc() is called, or on an
ambiguous failure, so that DESTROY_HWC is still attempted in this window?

>  
>  	*q_depth = hwc->hwc_init_q_depth_max;
>  	*max_req_msg_size = hwc->hwc_init_max_req_msg_size;
>  	*max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;
>  
>  	/* Both were set in mana_hwc_init_event_handler(). */
> -	if (WARN_ON(cq->id >= gc->max_num_cqs))
> -		return -EPROTO;
> +	if (WARN_ON(cq->id >= gc->max_num_cqs)) {
> +		err = -EPROTO;
> +		goto teardown;
> +	}
>  
>  	cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
> -	if (!cq_table)
> -		return -ENOMEM;
> +	if (!cq_table) {
> +		err = -ENOMEM;
> +		goto teardown;
> +	}
>  
>  	rcu_assign_pointer(cq_table[cq->id], cq);
>  	/* Publish the fully-initialised table last; pairs with the
> @@ -790,6 +804,16 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
>  	rcu_assign_pointer(gc->cq_table, cq_table);
>  
>  	return 0;
> +
> +teardown:
> +	{
> +		int td_err = mana_smc_teardown_hwc(&gc->shm_channel, false);
> +
> +		if (!td_err)
> +			hwc->setup_active = false;
> +
> +		return td_err ? td_err : err;
> +	}
>  }

[ ... ]

> @@ -907,11 +931,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;
> +		}

[Severity: Medium]
Does this early return also leak the objects the hardware cannot reach?

It skips the whole tail of mana_hwc_destroy_channel():

	kfree(hwc->caller_ctx);
	hwc->caller_ctx = NULL;
	mana_gd_free_res_map(&hwc->inflight_msg_res);
	...
	kfree(hwc);
	gc->hwc.driver_data = NULL;
	gc->hwc.gdma_context = NULL;

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

The comment in hw_channel.h calls this "the terminal destroy_channel
path", but mana_gd_cleanup_device() is also reached from
mana_gd_suspend(), from the mana_gd_probe() error path and from the
mana_gd_resume() error path, all on the same gdma_context.
mana_serv_reset() runs mana_gd_suspend() followed by mana_gd_resume()
exactly when the HWC is not responding (it sets hwc->hwc_timeout = 0),
which is when the DESTROY_HWC poll is most likely to time out.

On the following re-create, mana_hwc_create_channel() drops the last
pointer to the abandoned context:

	hwc = kzalloc_obj(*hwc);
	...
	gd->driver_data = hwc;

so hwc, hwc->caller_ctx and the inflight_msg bitmap become unreachable,
and mana_hwc_establish_channel() ends with a bare

	rcu_assign_pointer(gc->cq_table, cq_table);

which neither captures, synchronize_rcu()s nor vfree()s the table still
published by the previous instance. Would that leak one vcalloc'd
max_num_cqs table per failed reset cycle?

That also seems to differ from the contract this patch adds in gdma.h,
which says the base table is "replaced+freed only by
mana_hwc_destroy_channel() (and the create-time reinit)" — the
create-time reinit only replaces it. Would freeing these after
mana_gd_remove_irqs(), or at least having establish_channel() free or
warn on an already-published cq_table, keep the two consistent?

> +
> +		hwc->setup_active = false;
> +	}
> +
> +	/* 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);
> @@ -919,17 +970,6 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
>  	if (hwc->rxq)
>  		mana_hwc_destroy_wq(hwc, hwc->rxq);
>  
> -	if (hwc->cq)
> -		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
> -
> -	/* Reset only after mana_hwc_destroy_cq() above has run with a valid
> -	 * max_num_cqs so mana_gd_destroy_cq() clears the CQ table slot and
> -	 * waits out in-flight EQ handlers (synchronize_rcu) before the CQ is
> -	 * freed.  Clearing it earlier would make that path early-return and
> -	 * skip the slot clear, leaving a dangling cq_table entry.
> -	 */
> -	gc->max_num_cqs = 0;
> -
>  	kfree(hwc->caller_ctx);
>  	hwc->caller_ctx = NULL;
>  
> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> index 0b48ded0aecda..74eccc80cf6b4 100644
> --- a/include/net/mana/gdma.h
> +++ b/include/net/mana/gdma.h
> @@ -416,7 +416,13 @@ struct gdma_context {
>  	/* L2 MTU */
>  	u16 adapter_mtu;
>  
> -	/* This maps a CQ index to the queue structure. */
> +	/* Size of cq_table, i.e. the largest valid CQ index + 1.  Set once
> +	 * when cq_table is allocated and treated as immutable for the
> +	 * table's lifetime (a bound only) -- it is never reset on teardown.
> +	 * cq_table == NULL is the sole "table torn down" signal, so every
> +	 * cq_table[id] access must guard with both !cq_table (gone) and
> +	 * id >= max_num_cqs (out of bounds).
> +	 */
>  	unsigned int		max_num_cqs;

[Severity: High]
This is a pre-existing issue, but is the "set once ... treated as
immutable" invariant actually enforced anywhere?

The only writer is mana_hwc_init_event_handler(), which stores the
device-supplied EQE payload unconditionally, with no check that
gc->cq_table is already non-NULL:

	case HWC_INIT_DATA_MAX_NUM_CQS:
		gd->gdma_context->max_num_cqs = val;
		break;

The size passed to vcalloc(gc->max_num_cqs, sizeof(*cq_table)) in
mana_hwc_establish_channel() is not recorded anywhere else, and the HWC
EQ callback stays registered for the channel's whole lifetime, so a later
GDMA_EQE_HWC_INIT_DATA event can raise the bound above the allocated
size. Then in mana_gd_process_eqe(), in hard IRQ context:

	cq_table = rcu_dereference(gc->cq_table);
	if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs || !cq_table))
		break;

	cq = rcu_dereference(cq_table[cq_id]);
	if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id))

Can this read past the end of the vmalloc'd array and then dereference
whatever pointer-sized value it found (cq->type, cq->id, and on success
the indirect call cq->cq.callback)? The same bound guards the
rcu_assign_pointer() writes in mana_create_txq()/mana_create_rxq() and in
mana_ib_install_cq_cb():

	cq_table = rcu_dereference_protected(gc->cq_table, true);
	if (!cq_table || cq->queue.id >= gc->max_num_cqs)
		return -EINVAL;

Given that this patch makes the bound load-bearing by removing its reset,
would it be worth latching the allocated size in its own field, or
ignoring HWC_INIT_DATA_MAX_NUM_CQS once cq_table is non-NULL, alongside
the new documentation?

> diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
> index 73671f479399a..3d8543acb5cc0 100644
> --- a/include/net/mana/hw_channel.h
> +++ b/include/net/mana/hw_channel.h
> @@ -200,6 +200,15 @@ struct hw_channel_context {
>  	u32 pf_dest_vrcq_id;
>  	u32 hwc_timeout;
>  
> +	/* 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: Medium]
setup_active records device-scoped state (MST entries programmed for this
VF), but it lives in the per-instance hw_channel_context. Does the leak
path lose that record?

After the teardown failure, mana_hwc_destroy_channel() returns with
setup_active still true on a context that mana_gd_cleanup_device() then
walks away from (it also goes on to call mana_gd_remove_irqs()). On the
next attempt:

mana_gd_resume()
  mana_gd_setup()
    mana_hwc_create_channel()
      hwc = kzalloc_obj(*hwc);      /* setup_active == false */
      gd->driver_data = hwc;        /* old context dropped */
      mana_hwc_establish_channel()  /* second ESTABLISH_HWC */

Since shm_channel.c pairs ESTABLISH_HWC with DESTROY_HWC, does this leave
the first HWC's MST entries armed for the device's lifetime, with a later
clean teardown sending only one DESTROY_HWC? That looks like it defeats
the stated intent that "the failure is not mistaken for a clean
teardown".

Would keeping the flag in struct gdma_context (which survives
suspend/resume, as shm_channel does), or refusing to re-establish while a
previous HWC is known live, keep the driver's record in step with the
device?

Cross-instance finding from sashiko-gemini (b1a5d3a1fd2b96eddef89c85fad512dd15f2a00ad5dda70d873898d2dd806258):
[Severity: Medium]
Redundant HWC teardown in `mana_hwc_establish_channel()` causes a double hardware timeout (up to 120s stall) and masks the original initialization error code.