net: mana: concurrent HWC requests and dynamic queue depth

drivers/net/ethernet/microsoft/mana/hw_channel.c void mana_hwc_destroy_channel(struct gdma_context *gc) · line 820

[Severity: Medium]

The changelog presents this as preparation work, but doesn't this hunk also change behaviour on an existing failure path?

gc->max_num_cqs is only set once HWC_INIT_DATA_MAX_NUM_CQS arrives in mana_hwc_init_event_handler(). So with the old gate, when ESTABLISH_HWC had already been submitted but establish failed before that data arrived, DESTROY_HWC was skipped entirely.

Two such cases in mana_hwc_establish_channel():

	if (!wait_for_completion_timeout(&hwc->hwc_init_eqe_comp, 60 * HZ))
		return -ETIMEDOUT;

and a failing mana_smc_read_response() inside mana_smc_setup_hwc(). In both cases mana_hwc_create_channel() -> out: mana_hwc_destroy_channel() then went on to free the EQ/CQ/RQ/SQ DMA memory whose frame addresses had already been published to the PF.

mana_smc_teardown_hwc() documents why that ordering matters:

	/* Read shmem response (polling for VF possession) and validate.
	 * For teardown, waiting for response is required to ensure hardware
	 * invalidates MST entries before software frees memory.
	 */

Since the new gate closes that window, should this patch carry a Fixes: tag and state that it fixes the skipped teardown, instead of being described as tracking/preparation only?

drivers/net/ethernet/microsoft/mana/hw_channel.c void mana_hwc_destroy_channel(struct gdma_context *gc) · line 827

[Severity: High]

This isn't a bug introduced by this patch, since the pre-patch code ignored the mana_smc_teardown_hwc() return value as well, but the new gate makes the failing-teardown case reachable for every submitted-but-unconfirmed establish, so it seems worth asking about here.

On failure the return value is only logged, setup_active is left true, and execution falls straight through to:

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

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

	if (hwc->cq)
		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
	...
	kfree(hwc);

Those helpers reach mana_hwc_dealloc_dma_buf() and mana_gd_destroy_queue() -> mana_gd_free_memory() (dma_free_coherent), releasing exactly the four regions whose frame addresses were handed to the PF by the ESTABLISH_HWC writel loop.

mana_smc_teardown_hwc() fails when mana_smc_poll_register() times out after roughly 20 seconds because the PF never returns aperture possession, or when mana_smc_read_response() reports an error or an invalid response. In that state its own comment says the MST entries may still be valid:

* For teardown, waiting for response is required to ensure hardware
* invalidates MST entries before software frees memory.

Can the device then keep writing EQEs/CQEs into pages handed back to the allocator, or into unmapped pages once the IOMMU mapping is gone? The EQ MSI-X is also released while the PF may still raise it, and kfree(hwc) drops the state that would be needed to retry the teardown later.

The commit message says "Resources are still freed if teardown fails" - what makes freeing safe here, given the comment above?

drivers/net/ethernet/microsoft/mana/shm_channel.c int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr, · line 240

[Severity: Medium]

setup_active is stored here (and cleared at function entry) and is read/cleared in mana_hwc_destroy_channel(), with no lock and no READ_ONCE()/WRITE_ONCE(). What serializes the two contexts?

mana_schedule_serv_work() queues the worker on the system workqueue:

	INIT_WORK(&mns_wk->serv_work, mana_serv_func);
	schedule_work(&mns_wk->serv_work);

Neither mana_gd_remove() nor mana_gd_cleanup_device() cancels or flushes it, and the worker path mana_serv_reset() -> mana_gd_suspend()/mana_gd_resume() -> mana_gd_setup() -> mana_hwc_create_channel() -> mana_smc_setup_hwc() runs without device_lock, so it can overlap remove/shutdown/PM.

That gives this interleaving on the single shared-memory aperture:

CPU0 mana_smc_setup_hwc()

	*submitted = true;
	writel() loop			/* ESTABLISH_HWC handed to PF */
	mana_smc_read_response()	/* polling */

CPU1 mana_hwc_destroy_channel()

	if (hwc->setup_active) {			/* reads true */
		if (!mana_smc_teardown_hwc(...))	/* writels DESTROY_HWC */

Can CPU0 then consume the DESTROY_HWC request header written by CPU1 (a bogus -EPROTO), or CPU1 consume the ESTABLISH_HWC response, leaving the PF/VF handover state inconsistent while both also race on the plain read-modify-write of setup_active?

With the old gc->max_num_cqs > 0 gate this window was closed by accident, because the counter is still zero during the setup handover, so the destroy path issued no shared-memory access at all.

The underlying unserialized create-versus-destroy of the HWC context predates this patch, and later in the series mana_hwc_destroy_channel() gains the comment "The caller must serialize setup and teardown operations". Would it help to state that requirement in this patch, next to the new setup_active field, since this is where the flag starts gating aperture access?

Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908035201.402424-1-longli%40microsoft.com

drivers/net/ethernet/microsoft/mana/hw_channel.c static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len, · line 125

[Severity: Medium]
Should this hunk carry a Fixes: tag rather than being described as preparation?

The commit message says:

    Add per-slot locking, sender/response references and a responded flag;
    make ctx->error signed. Stop copying after buffer withdrawal or
    completion.

"Stop copying after buffer withdrawal or completion" reads like a refactor item, but at the baseline mana_hwc_send_request() released the message slot on timeout while leaving ctx->output_buf and ctx->output_buflen pointing at the caller's response buffer, which is usually on the caller's stack:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_query_hwc_timeout() {
	struct gdma_query_hwc_timeout_resp resp = {};
	...
	err = mana_gd_send_request(gc, sizeof(req), &req, sizeof(resp), &resp);
}

The baseline mana_hwc_handle_resp() then copied a late response into that pointer as soon as the bit was set again by the next sender:

	if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
		...
	}
	ctx = hwc->caller_ctx + msg_id;
	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
	...
	memcpy(ctx->output_buf, resp_msg, resp_len);

With HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH == 1 the same index is always reused, so the write lands in a stack frame that has already returned, and the length check runs against the stale output_buflen.

Is the new output_buf test here, together with the ctx->output_buf = NULL withdrawal in mana_hwc_send_request(), what closes that window? If so, would a Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)") and a mention of the corruption in the changelog make this visible to stable? The changelog notes the work was re-targeted from a "net fixes-only posting" to net-next, which makes the fix easy to miss.

include/net/mana/hw_channel.h struct hwc_caller_ctx { · line 192

[Severity: Low]
These three comments look narrower than what hw_channel.c actually relies on. Could they be extended?

The lock comment mentions output_buf only, but the out: block in mana_hwc_send_request() also uses it to serialize ->responded and to decide who owns the response-side reference:

	spin_lock_irqsave(&ctx->lock, flags);
	ctx->output_buf = NULL;
	drop_resp_ref = !ctx->responded;
	ctx->responded = true;
	spin_unlock_irqrestore(&ctx->lock, flags);

and ctx->error / ctx->status_code are read and written under the same lock in both mana_hwc_handle_resp() and mana_hwc_send_request().

The responded comment says it is set "by the first handle_resp(), or by the sender's timeout path", but the out: block above is also reached from the req_len > tx_wr->buf_len rejection, from mana_hwc_post_tx_wqe() failure, and from the no-wait teardown path where hwc_timeout is 0.

The error comment says "Linux error code (negative errno or 0)", while mana_hwc_get_msg_index() stores -EINPROGRESS as an in-band "no response yet" state that mana_hwc_send_request() special-cases with
if (err != -EINPROGRESS). Should that sentinel be documented next to the field, since every reader of ->error has to know about it?

Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908035201.402424-1-longli%40microsoft.com

drivers/net/ethernet/microsoft/mana/hw_channel.c void mana_hwc_destroy_channel(struct gdma_context *gc) · line 980

[Severity: High]
This is a pre-existing issue, but since this patch moves the call to mana_hwc_destroy_cq(), is it worth fixing a use-after-free bug in that function's teardown path?

In mana_hwc_destroy_cq(), hwc_cq->comp_buf is freed before the EQ is destroyed:

mana_hwc_destroy_cq() {
	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);
	...
}

Because mana_gd_destroy_queue() on the EQ is what performs synchronize_rcu() to wait for concurrent interrupt handlers to finish, a racing interrupt handler on another CPU could still execute mana_hwc_comp_event() after the buffer is freed:

mana_hwc_comp_event() {
	...
	completions = hwc_cq->comp_buf;
	comp_read = mana_gd_poll_cq(q_self, completions, hwc_cq->queue_depth);
	...
}

Could this lead to a use-after-free and memory corruption when mana_gd_poll_cq() writes hardware completion events into the freed completions buffer?

drivers/net/ethernet/microsoft/mana/hw_channel.c static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id) · line 21

[Severity: High]
Can pure slot contention here trigger a device reset on healthy hardware?

The channel is built at HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH == 1 in mana_hwc_create_channel(), so sema_init() grants a single permit and any two concurrent mana_gd_send_request() callers contend for it. The loser gets -ETIMEDOUT, which mana_gd_send_request() propagates unchanged.

Existing callers read that code as "the HWC is dead":

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_probe() {
	if (err == -ETIMEDOUT || err == -EPROTO) {
		...
		schedule_delayed_work(&mana_dev_recovery_work.work, ...);
}

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_serv_reset() {
	ret = mana_gd_resume(pdev);
	if (ret == -ETIMEDOUT || ret == -EPROTO) {
		... mana_serv_rescan(pdev);
}

mana_serv_rescan() calls pci_stop_and_remove_bus_device() followed by pci_rescan_bus(). In mana_en the gf-stats work latches ac->hwc_timeout_occurred = true on -ETIMEDOUT and stops collecting stats permanently.

The admission budget is also the same field the response-timeout path in mana_hwc_send_request() shrinks:

	if (hwc->hwc_timeout > 1)
		hwc->hwc_timeout = 1;

and the field mana_serv_reset() sets to 0, so after either event down_timeout() takes the expired path immediately for a second caller even though the channel is responsive.

Would returning a distinguishable code for contention, such as -EBUSY or -EAGAIN, avoid teaching every caller to reset the device on queue depth pressure? The commit message acknowledges this:

"Admission expiry returns -ETIMEDOUT even for contention, so existing callers may reset a responsive channel."

Checking the end of the series, mana_hwc_get_msg_index() is byte-identical there, so the later dynamic HWC queue depth work does not address this; it only makes exhaustion less likely when the device advertises a depth greater than one.

drivers/net/ethernet/microsoft/mana/hw_channel.c static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id) · line 48

[Severity: Low]
The new field is documented in include/net/mana/hw_channel.h as:

	/* Response-side reference outstanding; protected by lock. */
	bool resp_pending;

but this initializing write runs under hwc->inflight_msg_res.lock only, and ctx->lock is never taken in mana_hwc_get_msg_index(). Every other write to the field (mana_hwc_handle_resp(), the timeout path and the out:
path of mana_hwc_send_request(), and mana_hwc_destroy_channel()) does hold ctx->lock.

On the consumer side, mana_hwc_handle_resp() gates on a lock-free test_bit(msg_id, hwc->inflight_msg_res.map) before acquiring ctx->lock, so the publishing stores to responded/resp_pending and the bitmap bit share no lock or barrier with the reader that consumes them.

Since msg_id comes from the RX buffer and is read with READ_ONCE() in mana_hwc_rx_event_handler() precisely because the host can modify it, a response for the freshly published msg_id can arrive between bitmap_set() and the sender's first spin_lock(&ctx->lock). If that handler observed the new resp_pending == true together with a stale responded == true it would consume the response-side reference, and a later sender timeout would then drop the refcount to zero and reopen the late-response reuse window this patch closes.

Should the comment name inflight_msg_res.lock plus the publication ordering, or should the initialization move under ctx->lock?

drivers/net/ethernet/microsoft/mana/hw_channel.c int mana_hwc_create_channel(struct gdma_context *gc) · line 883

[Severity: Medium]
This isn't a bug introduced by this patch, since the baseline also assigned gd->driver_data = hwc before mana_hwc_init_queues(), but does the new channel_up gate give the impression that lookups are now safe when it is ordered after the blocking acquire?

driver_data is published here, while sema_init() and mana_gd_alloc_res_map() (which allocates the bitmap and initializes inflight_msg_res.lock) only run inside mana_hwc_init_queues() -> mana_hwc_init_inflight_msg(), and caller_ctx plus channel_up = true only appear later in mana_hwc_test_channel().

mana_gd_send_request() gates on nothing else:

	hwc = gc->hwc.driver_data;
	if (!hwc) { ... return -ENODEV; }
	hwc->active_senders++;

so a sender that observes the freshly published pointer reaches mana_hwc_get_msg_index() and executes:

	if (down_timeout(&hwc->sema, msecs_to_jiffies(hwc->hwc_timeout)))

on a kzalloc'ed semaphore (count == 0, wait_list.next/prev == NULL, so ___down_common()'s list_add_tail() walks NULL pointers), then spin_lock_irqsave(&r->lock, flags) on a never-initialized spinlock, all before the channel_up check can reject it.

The same window exists in the depth-renegotiation path added later in the series, which calls mana_hwc_destroy_queues() (frees the bitmap, kfrees caller_ctx) and then mana_hwc_init_queues() again while driver_data stays published.

I could not exhibit a concurrent sender, since mana_hwc_create_channel() runs from mana_gd_setup() during probe and resume where the clients have been removed or not yet probed. Is the requirement stated by the two new comments ("queue initialization below must precede senders", "The setup caller must exclude concurrent HWC teardown") enforced anywhere in code?

[ ... ]

drivers/net/ethernet/microsoft/mana/hw_channel.c void mana_hwc_destroy_channel(struct gdma_context *gc) · line 874

[Severity: High]
This is a pre-existing issue in the helper rather than something this patch introduces, but since the comment here now makes mana_hwc_destroy_cq() the fence for the interrupt handler, does that helper actually fence anything before it frees the buffers the handler writes?

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

Only the last step reaches the barrier, via mana_gd_destroy_eq() -> mana_gd_deregister_irq():

	list_del_rcu(&eq->entry);
	...
	synchronize_rcu();

Meanwhile the hard-IRQ callback writes into the buffer that was already freed and dereferences the queue that was already destroyed:

	completions = hwc_cq->comp_buf;
	comp_read = mana_gd_poll_cq(q_self, completions, hwc_cq->queue_depth);

An interrupt taken in that interval reaches mana_gd_intr() -> the HWC EQ, which is still on gic->eq_list -> mana_gd_process_eqe() -> mana_hwc_comp_event(), so can this write into the kfree()d comp_buf?

The gc->max_num_cqs = 0 store just above helps for the completion path, because mana_gd_process_eqe() does:

	if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
		break;

but that plain store carries no barrier or RCU synchronization, so an interrupt already inside mana_gd_process_eqe() on another CPU can still pass the check, and the EQ-callback path has no such guard at all:
mana_hwc_init_event_handler() dereferences hwc->cq (for example GDMA_EQE_HWC_INIT_EQ_ID_DB writing hwc->cq->gdma_eq->id) after it has been kfree()d, and hwc->cq is not set to NULL here.

The window is also reachable when mana_smc_teardown_hwc() fails, since mana_hwc_destroy_channel() only logs "Failed to tear down HWC" and continues while the device may still be generating HWC EQEs and CQEs.

Would destroying and deregistering the EQ first, and freeing comp_buf and the CQ last, close this?

[ ... ]

drivers/net/ethernet/microsoft/mana/hw_channel.c int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len, · line 1111

[Severity: Low]
This hunk, together with the resp_pending handling in mana_hwc_handle_resp(), fixes a real misassociation on existing kernels:
the baseline timeout path did

out:

	mana_hwc_put_msg_index(hwc, msg_id);

while the hardware response was still outstanding, and mana_hwc_handle_resp() dispatches purely on msg_id, so the next request could take the same slot and be completed with the previous command's response and buffer contents.

Should this carry a Fixes: tag and a description of the observable failure? As posted the fix is bundled with unrelated behavioural changes (admission timeout, sender draining, SQ posting serialization), so stable maintainers have nothing to select on.

[ ... ]

include/net/mana/gdma.h struct gdma_context { · line 478

[Severity: Medium]
Does taking hwc_lock around the hwc_timeout accesses in mana_need_log() and mana_serv_reset() provide any mutual exclusion, given that the remaining writers and readers of that field do not take it?

Writers without the lock:

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_init_event_handler() {
		case HWC_DATA_CFG_HWC_TIMEOUT:
			hwc->hwc_timeout = val;
}

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_send_request() {
			if (hwc->hwc_timeout > 1)
				hwc->hwc_timeout = 1;
}

and mana_gd_verify_vf_version() passes &hwc->hwc_timeout to mana_gd_query_hwc_timeout(), which writes through it unlocked.

Readers without the lock include wait_ms = hwc->hwc_timeout and the new admission budget:

	if (down_timeout(&hwc->sema, msecs_to_jiffies(hwc->hwc_timeout)))

The first of those writers runs in hard-IRQ context, so spin_lock_irqsave() in mana_need_log() only masks the local CPU and does not exclude the same store on another CPU.

There is also an intent problem now that the field is a blocking budget:
the value is sampled before sleeping, so mana_serv_reset() setting

	hwc->hwc_timeout = 0;

under hwc_lock, specifically so that nothing waits, cannot shorten a wait already in progress, and a reconfiguration EQE can restore a long timeout in the middle of a reset cycle.

Would a single IRQ-safe lock for the field, or an explicit READ_ONCE()/WRITE_ONCE() policy with defined update precedence, be a better fit than the current "Not all timeout writers use it"?

drivers/net/ethernet/microsoft/mana/gdma_main.c static void mana_gd_create_cq(const struct gdma_queue_spec *spec, · line 1273

[Severity: High]
This isn't a bug introduced by this patch, but can this lead to a use-after-free in the EQ interrupt handler?

The CQ memory is freed by the caller (mana_gd_destroy_queue()) immediately after mana_gd_destroy_cq() unpublishes it from the table, without waiting for an RCU grace period.

Concurrently, the EQ interrupt processor (mana_gd_process_eqe()) runs under rcu_read_lock(), reads the CQ pointer from the table, and dereferences it. If the CQ is freed during this window, the interrupt handler will access freed memory. Should this wait for an RCU grace period before freeing the queue?

drivers/net/ethernet/microsoft/mana/hw_channel.c static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth, · line 890

[Severity: High]
This is a pre-existing issue, but does this queue destruction order cause a use-after-free during teardown?

mana_hwc_destroy_cq() frees the CQ object before it destroys the EQ. The EQ remains active and can still receive events from the hardware. If a teardown fails or times out (which is actively exercised by the new reinit_fallback logic) and an initialization event arrives after the CQ is freed but before the EQ is disabled, the EQ interrupt handler will dereference the freed CQ when processing HWC_INIT_DATA_CQID.

drivers/net/ethernet/microsoft/mana/hw_channel.c int mana_hwc_create_channel(struct gdma_context *gc) · line 965

[Severity: Critical]
Does capping the hardware-reported HWC queue depth without negotiating it with the host cause a ring buffer size mismatch?

The driver limits the queue depth to HW_CHANNEL_MAX_QUEUE_DEPTH (128) and allocates DMA rings based on this limit. However, the mana_smc_setup_hwc() protocol lacks a queue size field, meaning this restriction isn't communicated to the host hardware.

If the hardware advertises a larger depth (e.g., 256), will it write up to 256 entries into the rings and exceed the 128 allocated entries, leading to an out-of-bounds memory write?

drivers/net/ethernet/microsoft/mana/hw_channel.c int mana_hwc_create_channel(struct gdma_context *gc) · line 1015

[Severity: Critical]
Does this validation check incorrectly allow the out-of-bounds access?

By verifying q_depth_max < hwc->num_inflight_msg, it permits the hardware depth (q_depth_max) to be larger than the driver's allocated depth (hwc->num_inflight_msg). Should this reject the configuration if the hardware depth exceeds the allocated depth?

drivers/net/ethernet/microsoft/mana/hw_channel.c static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth, · line 791

[Severity: High]
Is it safe to set gc->hwc.doorbell to INVALID_DOORBELL here while the HWC EQ is already registered on the shared msix 0 interrupt?

mana_hwc_init_queues() has already run mana_gd_create_eq() -> mana_gd_register_irq() -> list_add_rcu(&queue->entry, &gic->eq_list), so mana_gd_process_eq_events() can run at any point after mana_smc_setup_hwc() writes the shared-memory request. At the end of every batch it arms unconditionally:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_process_eq_events() {
	...
	mana_gd_ring_doorbell(gc, eq->gdma_dev->doorbell, eq->type, eq->id,
			      head, SET_ARM_BIT);
	...
}

and mana_gd_ring_doorbell() forms the MMIO address with no validity check:

	void __iomem *addr = gc->db_page_base + gc->db_page_size * db_index;
	...
	writeq(e.as_uint64, addr);

Only GDMA_EQE_HWC_INIT_EQ_ID_DB repopulates gd->doorbell in mana_hwc_init_event_handler(). If the first delivered batch does not start with that event, does the writeq() run with db_index == 0xFFFFFFFF, i.e. roughly 17 TiB past the ioremapped BAR0 window?

The field is also written from hard IRQ context (gd->doorbell = eq_db.doorbell;) while this store is a plain process-context store with no WRITE_ONCE() or barrier. Since the reinit path calls mana_hwc_establish_channel() two or three times per probe, does this multiply the window rather than close it?

drivers/net/ethernet/microsoft/mana/hw_channel.c static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth, · line 817

[Severity: High]
This isn't a bug introduced by this patch, but since a doorbell sanity check is being added right here, could it also cover the BAR range?

The value comes from a 16-bit device-controlled field and is stored with no validation:

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_init_event_handler() {
	...
	eq_db.as_uint32 = event->details[0];
	hwc->cq->gdma_eq->id = eq_db.eq_id;
	gd->doorbell = eq_db.doorbell;
	...
}

It is then consumed unchecked by mana_gd_ring_doorbell() from the EQ rearm, from mana_gd_ring_cq() on the HWC CQ and from mana_gd_wq_ring_doorbell() on the HWC SQ/RQ. The driver already has the corresponding check for the PF-reported db_id:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_register_device() {
	...
	if (gc->db_page_off + gc->db_page_size * ((u64)resp.db_id + 1) > gc->bar0_size) {
		dev_err(gc->dev, "Doorbell ID %u out of range\n", resp.db_id);
		return -EPROTO;
	}
	...
}

With a 4 KiB doorbell page, a reported index of 0x8000 gives a 128 MiB offset, well outside a typical VF BAR0. Can the writeq() then land outside the ioremapped region at a device-chosen offset? The commit message notes the gap ("does not cover BAR bounds"), so was leaving it open intentional?

drivers/net/ethernet/microsoft/mana/hw_channel.c static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth, · line 892

[Severity: Medium]
This is a pre-existing ordering issue rather than something this patch introduces, but the comment says the CQ helper deregisters the EQ before returning, and that only happens as the last step:

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_destroy_cq() {
	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 list_del_rcu()/synchronize_rcu() pair that actually drains in-flight EQ users lives in mana_gd_deregister_irq(), reached only from the final gdma_eq destroy. During the first two frees the EQ is still on gic->eq_list.

Can an interrupt in that window reach mana_hwc_comp_event() and use the already freed comp_buf?

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_comp_event() {
	...
	completions = hwc_cq->comp_buf;
	comp_read = mana_gd_poll_cq(q_self, completions, hwc_cq->queue_depth);
	...
}

An HWC init EQE in the same window would instead reach mana_hwc_init_event_handler(), which writes hwc->cq->gdma_eq->id and hwc->cq->gdma_cq->id while those objects are being freed, since hwc->cq is only cleared after mana_hwc_destroy_queues() returns. Would destroying the EQ first, before comp_buf and the gdma_cq, close this?

drivers/net/ethernet/microsoft/mana/hw_channel.c int mana_hwc_create_channel(struct gdma_context *gc) · line 1001

[Severity: High]
Is the store order here inverted with respect to what the readers check?

The pre-existing teardown in mana_hwc_destroy_channel() clears the bound first and only drops the pointer at the very end (gc->max_num_cqs = 0; ... vfree(gc->cq_table); gc->cq_table = NULL;). Here the pointer goes away first, leaving a state where the bound check passes but the pointer is NULL.

The surviving hard IRQ consumer validates only the bound:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_process_eqe() {
	...
	cq_id = eqe->details[0] & 0xFFFFFF;
	if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
		break;

	cq = gc->cq_table[cq_id];
	...
}

That is exactly the case the patch guards against in mana_gd_destroy_cq() with READ_ONCE() plus a NULL check, but mana_gd_process_eqe() has neither.

Beyond the adjacent-store window, is there a much longer one? The subsequent mana_hwc_init_queues() re-registers the HWC EQ on msix 0 before mana_hwc_establish_channel() runs, and the hard IRQ init handler sets gd->gdma_context->max_num_cqs = val from HWC_INIT_DATA_MAX_NUM_CQS while gc->cq_table is still NULL. The vcalloc() that repopulates it only happens after the up-to-60 s wait_for_completion_timeout() returns. Would a GDMA_EQE_COMPLETION delivered in that window pass the bound check and dereference NULL plus cq_id * 8, then call through the loaded value as cq->cq.callback?

Also, since the reclaim now relies on synchronize_rcu(), should these be WRITE_ONCE()/rcu_assign_pointer() stores rather than plain ones?

drivers/net/ethernet/microsoft/mana/hw_channel.c int mana_hwc_create_channel(struct gdma_context *gc) · line 1047

[Severity: High]
Does this abort actually avoid freeing memory the device may still own?

The goto out lands in mana_hwc_destroy_channel(), which retries the teardown once and then continues regardless of the result:

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_destroy_channel() {
	...
	if (hwc->setup_active) {
		if (!mana_smc_teardown_hwc(&gc->shm_channel, false))
			hwc->setup_active = false;
		else
			dev_err(hwc->dev, "Failed to tear down HWC\n");
	}
	gc->max_num_cqs = 0;

	/* Deregister the HWC EQ before freeing the work queues. */
	if (hwc->cq)
		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
	...
}

So a third failure is logged and then mana_hwc_destroy_cq() and mana_hwc_destroy_wq() run mana_gd_destroy_queue() -> mana_gd_free_memory() -> dma_free_coherent(), plus vfree(gc->cq_table) and kfree(hwc), with hwc->setup_active still true.

mana_smc_teardown_hwc() states the requirement explicitly:

drivers/net/ethernet/microsoft/mana/shm_channel.c:mana_smc_teardown_hwc() {
	...
	/* Read shmem response (polling for VF possession) and validate.
	 * For teardown, waiting for response is required to ensure hardware
	 * invalidates MST entries before software frees memory.
	 */
	...
}

Can the coherent EQ/CQ/SQ/RQ rings and HWC message buffers then be returned to the page allocator while the hardware MST entries still map them, and with hwc freed so no later safe teardown is possible? The ignore-the-failure behaviour in mana_hwc_destroy_channel() predates this series, but this abort path is newly reachable, and the commit message says the intent is to "never test queues after an indeterminate destroy result".

[ ... ]

include/net/mana/gdma.h enum { · line 697

[Severity: Medium]
The commit message says "Advertise support in the driver-version request." Can this flag gate the new behaviour at all?

GDMA_DRV_CAP_FLAGS1 is only transmitted by mana_gd_verify_vf_version(), which runs after the HWC has been created:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_setup() {
	...
	err = mana_hwc_create_channel(gc);
	if (err)
		goto remove_irq;

	err = mana_gd_verify_vf_version(pdev);
	...
}

By then mana_hwc_create_channel() has already consumed HWC_INIT_DATA_QUEUE_DEPTH and performed the DESTROY_HWC plus second ESTABLISH_HWC cycle, and the verify request itself travels over the rebuilt channel. So the rebuild in mana_hwc_create_channel() is taken purely from the device-reported depth:

	if (q_depth_max > HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH) {

Unlike other capabilities in this driver, there is no PF acknowledgment check (GDMA_PF_CAP_FLAG_1_* in gc->pf_cap_flags1) before acting.

In the baseline, hwc_init_q_depth_max was stored but never acted on, since mana_hwc_test_channel() was always called with HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH. Does existing hardware that already reports a depth above one now perform an extra shared-memory teardown/establish per probe, each with mana_smc_poll_register() polling and a 60 * HZ wait_for_completion_timeout()? And can probe now fail through the reinit_fallback goto out paths where it previously succeeded at depth one?

Could the commit message describe the flag as informational only, and mention the behaviour change on PF firmware that never saw the new bit?

[ ... ]

Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908035201.402424-1-longli%40microsoft.com