Re: [PATCH net v3 5/6] 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 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.

[ ... ]

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

[ ... ]

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

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

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