mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* Re: [PATCH net-next v7 1/4] net: mana: prepare HWC ownership for safe reinitialization
@ 2026-10-14  8:15 netdev-bot+sashiko
  0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-10-14  8:15 UTC (permalink / raw)
  To: weh
  Cc: longli, kotaranov, kuba, davem, pabeni, edumazet, andrew+netdev,
	jgg, leon, haiyangz, wei.liu, decui, shradhagupta, horms, ernis,
	stephen, netdev, linux-rdma, linux-hyperv, linux-kernel,
	gargaditya, kees+treewide, kees, mawasthi, weh, edumazet

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 (2) · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] When DESTROY_HWC is not acknowledged,
  mana_hwc_destroy_channel() only calls mana_hwc_fence_channel() and…
- [Low] The commit is described as preparation for dynamic HWC queue
  sizing, but it fixes real bugs in the existing single-channel code.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it breaks the
  reader lifetime model the patch documents.
- [High] This problem wasn't introduced by this patch.

--

net: mana: prepare HWC ownership for safe reinitialization

This patch serializes the ESTABLISH_HWC and DESTROY_HWC shared-memory
transactions with a mutex. It tracks PF ownership with setup_active, and
when DESTROY_HWC is not acknowledged it fences the HWC context but does
not free it. It also snapshots the bootstrap CQ count and id, publishes
cq_table with release/acquire ordering, frees the table only after an RCU
grace period, and destroys the HWC EQ before the CQ and completion buffer.

Link: https://lore.kernel.org/r/20260908035201.402424-2-longli@microsoft.com
Link: https://lore.kernel.org/r/20260908035201.402424-5-longli@microsoft.com
Link: https://lore.kernel.org/r/178910960115.219967.13830871915506436112@kernel.org

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index ae0ed700b3b93..69da65a405a46 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c

[ ... ]

> @@ -765,11 +766,16 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
>  	switch (type) {
>  	case GDMA_EQE_COMPLETION:
>  		cq_id = eqe->details[0] & 0xFFFFFF;
> -		if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
> +		/* The IRQ handler's RCU read-side section protects the table
> +		 * until HWC teardown has fenced its EQ and waited for readers.
> +		 */
> +		cq_table = smp_load_acquire(&gc->cq_table);
> +		if (!cq_table || cq_id >= READ_ONCE(gc->max_num_cqs))
>  			break;
>  
> -		cq = gc->cq_table[cq_id];
> -		if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id))
> +		cq = READ_ONCE(cq_table[cq_id]);
> +		if (!cq || WARN_ON_ONCE(cq->type != GDMA_CQ ||
> +					cq->id != cq_id))
>  			break;
>  
>  		if (cq->cq.callback)

[Severity: High]
This isn't a bug introduced by this patch, but it affects the lifetime
model this comment describes. The comment says table readers are protected
only by the IRQ handler's RCU read-side section. The reader dereferences
cq->type, cq->id and cq->cq.callback without taking a reference.

Does mana_ib follow that model? mana_ib_remove_cq_cb() in
drivers/infiniband/hw/mana/cq.c also writes to this table:

	kfree(gc->cq_table[cq->queue.id]);
	gc->cq_table[cq->queue.id] = NULL;

It frees the object before clearing the slot and never waits for a grace
period. mana_ib_destroy_cq() calls it before the hardware CQ is destroyed,
so the CQ can still raise completion EQEs:

	mana_ib_remove_cq_cb(mdev, cq);

	/* Ignore return code as there is not much we can do about it.
	 * The error message is printed inside.
	 */
	mana_ib_gd_destroy_cq(mdev, cq);

Say an EQ interrupt on another CPU has already loaded the slot here. Can
it then read freed memory and call through cq->cq.callback? A local RDMA
user can reach this by calling ibv_destroy_cq() on an armed CQ.

mana_ib_install_cq_cb() also publishes the object with a plain store:

	gc->cq_table[cq->queue.id] = gdma_cq;

There is no release ordering. On weakly ordered architectures such as
arm64, could the READ_ONCE() reader above see the pointer before type, id
and callback are initialized?

The series does not change cq.c.

[Severity: High]
This is also a pre-existing issue, not introduced by this patch. The cq_id
here comes from the device's EQE. Nothing checks that cq->cq.parent is the
EQ that delivered the event.

The generic destroy path, mana_gd_destroy_queue()->mana_gd_destroy_cq(),
only does this:

	gc->cq_table[id] = NULL;

Then mana_gd_destroy_queue() frees the queue memory and calls
kfree(queue) without waiting for a grace period.

mana_dealloc_queues() frees the datapath CQs while their parent EQs and
the HWC EQ are still registered:

	mana_destroy_rxqs(apc);
	mana_destroy_txq(apc);
	mana_destroy_eq(apc);

Suppose an EQ interrupt on another CPU loaded the CQ pointer before the
slot was cleared. Could it then read cq->type, cq->id and cq->cq.callback
from freed memory and call the callback? The EQE could be stale, or a
malicious host could post a datapath CQ id on the HWC EQ.

The synchronize_rcu() added in mana_hwc_destroy_channel() only protects
the cq_table allocation itself.

For the HWC CQ, the mana and mana_ib EQs are destroyed in mana_remove()
and mana_rdma_remove() before mana_gd_cleanup(). So the HWC claim in the
commit message holds under normal ordering.

[ ... ]

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 3bca4b683134b..b1d972968f0d7 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -382,16 +386,49 @@ static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)
>  	mana_gd_ring_cq(q_self, SET_ARM_BIT);
>  }
>  
> -static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
> +static int mana_hwc_publish_cq(struct gdma_context *gc,
> +			       struct gdma_queue *cq)
>  {
> -	kfree(hwc_cq->comp_buf);
> +	struct gdma_queue **cq_table = READ_ONCE(gc->cq_table);
> +	u32 id = READ_ONCE(cq->id);
>  
> -	if (hwc_cq->gdma_cq)
> -		mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);

[ ... ]

> +static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
> +{
> +	/* Destroy the EQ first: it deregisters the IRQ and drains in-flight
> +	 * handlers, so none can touch the CQ after it is freed.
> +	 */
>  	if (hwc_cq->gdma_eq)
>  		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
>  
> +	/* Safe to free now that the EQ handler is fenced. */
> +	if (hwc_cq->gdma_cq)
> +		mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
> +
> +	kfree(hwc_cq->comp_buf);
>  	kfree(hwc_cq);
>  }

[Severity: Low]
The commit message calls this preparation for dynamic HWC queue sizing.
This reordering also seems to fix a use-after-free in the existing
single-channel code.

Before this patch, mana_hwc_destroy_cq() freed comp_buf and then the CQ.
The IRQ was only deregistered later, when the EQ was destroyed. An HWC EQ
interrupt could arrive in that window. One example is the
mana_hwc_create_channel() failure unwind after ESTABLISH_HWC timed out
while the PF was still live. The interrupt could then run
mana_hwc_comp_event()->mana_gd_poll_cq() on the freed comp_buf.

The old mana_hwc_destroy_channel() also ignored the return value of
mana_smc_teardown_hwc(). It freed the PF-visible EQ/CQ/RQ/SQ DMA memory
even when DESTROY_HWC was not acknowledged.

The old WARN_ON(cq->id >= gc->max_num_cqs) in
mana_hwc_establish_channel() could also be triggered by the host, which
panics a guest booted with panic_on_warn.

Could these fixes be split out with Fixes: tags, so they can go to net and
stable separately from the reinitialization preparation?

[ ... ]

> @@ -759,6 +829,13 @@ int mana_hwc_create_channel(struct gdma_context *gc)
>  	u16 q_depth_max;
>  	int err;
>  
> +	/* Retry a retained context before assigning queues to the PF again. */
> +	if (gd->driver_data) {
> +		mana_hwc_destroy_channel(gc);
> +		if (gd->driver_data)
> +			return -ETIMEDOUT;
> +	}
> +
>  	hwc = kzalloc_obj(*hwc);

[ ... ]

> @@ -808,30 +885,56 @@ int mana_hwc_create_channel(struct gdma_context *gc)

[ ... ]

>  void mana_hwc_destroy_channel(struct gdma_context *gc)
>  {
>  	struct hw_channel_context *hwc = gc->hwc.driver_data;
> +	struct gdma_queue **old_cq_table;
> +	int err;
>  
>  	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.
> +	/* An unacknowledged destroy leaves the PF's mappings live.  Fence
> +	 * software dispatch, but retain every PF-visible allocation.
>  	 */
> -	if (gc->max_num_cqs > 0) {
> -		mana_smc_teardown_hwc(&gc->shm_channel, false);
> -		gc->max_num_cqs = 0;
> +	err = mana_smc_teardown_hwc(&gc->shm_channel, false,
> +				    &hwc->setup_active);
> +	if (err) {
> +		dev_err(hwc->dev,
> +			"HWC teardown failed: %d, retaining PF-visible resources\n",
> +			err);
> +		mana_hwc_fence_channel(gc, hwc);
> +		return;
>  	}

[Severity: Medium]
Does this leak the retained hw_channel_context when the gdma_context
itself is freed?

On this path gc->hwc.driver_data still points at hwc. The only code that
releases it is the retry at the top of mana_hwc_create_channel(), and that
only runs if the same gc is set up again. mana_gd_cleanup() returns as if
teardown had completed and does not tell its callers that a context was
retained.

mana_gd_remove() in gdma_pci.c then does:

	mana_gd_cleanup(gc);

	xa_destroy(&gc->irq_contexts);

	pci_iounmap(pdev, gc->bar0_va);

	vfree(gc);

The mana_gd_probe() unwind does the same through its free_gc label. On an
unresponsive HWC, mana_serv_reset() goes through suspend, then resume
returns -ETIMEDOUT, then mana_serv_rescan() removes the device.

Each of these paths loses the following:

  hwc
  caller_ctx
  the inflight_msg_res bitmap
  the gc->cq_table vmalloc
  comp_buf
  the hwc_cq/hwc_wq/gdma_queue wrappers
  the DMA-coherent queue and message buffers

The PF can never reach the software-only allocations, so keeping them
does not appear to buy any safety.

If the PF stays unresponsive, the probe recovery loop repeats: probe fails
with -ETIMEDOUT, recovery work runs a rescan, and probe runs again. Each
pass would leak one full context.

The final patch in the series keeps this behaviour through
mana_hwc_release_channel()->mana_hwc_retain_channel(), and gdma_pci.c is
not changed.

Could the software-only state be freed here? Alternatively, could the
retained ownership be reported to the callers of mana_gd_cleanup()?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791561995.git.weh%40linux.microsoft.com

^ permalink raw reply	[flat|nested] 2+ messages in thread
* [PATCH net-next v7 0/4] net: mana: concurrent HWC requests and dynamic queue depth
@ 2026-10-13  8:05 Wei Hu
  2026-10-13  8:05 ` [PATCH net-next v7 1/4] net: mana: prepare HWC ownership for safe reinitialization Wei Hu
  0 siblings, 1 reply; 2+ messages in thread
From: Wei Hu @ 2026-10-13  8:05 UTC (permalink / raw)
  To: longli, kotaranov, kuba, davem, pabeni, edumazet, andrew+netdev,
	jgg, leon, haiyangz, wei.liu, decui, shradhagupta, horms, ernis,
	stephen
  Cc: netdev, linux-rdma, linux-hyperv, linux-kernel, gargaditya,
	kees+treewide, kees, mawasthi, weh, edumazet

This series picks up Long Li's v5 MANA Hardware Channel (HWC) work,
addresses its review findings, and retains its four-patch feature lineage.
Safety changes required by concurrency and repeated HWC reinitialization
are folded into the patch which owns each behavior rather than posted as a
separate fixes series.

I have taken over follow-up upstreaming from Long. Long remains the author
of every patch, his existing sign-off is preserved, and I added mine for
the changes and submission custody.

Patch 1 prepares HWC ownership for safe reinitialization. It serializes
complete shared-memory transactions, tracks possible PF ownership,
retains mappings after an unacknowledged destroy, fences the HWC EQ before
releasing CQ state, and safely publishes and withdraws the HWC CQ table.

Patch 2 gives each slot independent completion state, withdraws caller
storage before sender return, handles the response-versus-timeout race,
and prevents depth-one slot reuse after a genuine timeout.

Patch 3 adds concurrent requests with bounded admission, serialized SQ
posting, separate sender/response ownership, stale-response quarantine,
coherent timeout updates, private setup publication, and sender draining.

Patch 4 rebuilds the depth-one bootstrap queues at validated depths from 2
through 128. It validates both reports, message limits and doorbells,
keeps the untouched bootstrap channel for depth one or reports above 128,
and restores fresh bootstrap queues after a failed rebuild only when
teardown was acknowledged.

Generic Ethernet/RDMA CQ callback refcounting and generic service-work
versus device-lifecycle serialization are independent pre-existing work
and are intentionally excluded. Malformed-RX handling, duplicate-success
correlation, and broad device-controlled allocation/timeout policy caps
also remain separate follow-ups.

This series is independent of the separately posted queue-set v6 series.
That series addresses the pre-existing generic Ethernet CQ reclamation
gap discussed in the HWC v6 review. The CQ-publication hunks overlap;
whichever series lands second will need reconciliation preserving both
table-publication and individual-CQ lifetime guarantees.
https://lore.kernel.org/all/cover.1790795005.git.weh@linux.microsoft.com/

Changes in v7 (v6 -> v7):
 - Rebase onto net-next d8674294aefef02266c4d47ad10131f1bffbe534,
   following Simon Horman's request after the v6 CI application failure.
 - Preserve the upstream GDMA core/PCI transport split and CDX support.
   Initialize the HWC lifetime lock on both transport probe paths and
   perform PCI recovery's timeout cancellation through a GDMA core helper
   which protects the published HWC pointer across the module boundary.
 - Retain the four-patch lineage and the v6 ownership, request, timeout,
   publication, depth-validation and fallback policies.

Rebase request:
https://lore.kernel.org/all/20261009151906.GF83879@horms.kernel.org/

Changes in v6 (v5 -> v6):
 - Keep the four-patch v5 structure while folding review-required HWC
   ownership, teardown, timeout, publication and fallback safety into
   their owning feature patches.
 - Serialize complete ESTABLISH_HWC and DESTROY_HWC shared-memory
   transactions. Record possible PF ownership before MMIO submission and
   clear it only after DESTROY_HWC is acknowledged.
 - Retain PF-visible queues after an indeterminate destroy. Fence the
   dedicated HWC EQ and withdraw its CQ from dispatch without freeing the
   mappings, then retry teardown before assigning fresh queues.
 - Destroy the HWC EQ before CQ and completion state. Release-publish the
   CQ table and wait for IRQ-side RCU readers before freeing it, without
   adding per-completion refcount operations.
 - Withdraw the caller's response buffer before return and quarantine a
   timed-out slot until its late response arrives or teardown cancels it.
 - Return -EBUSY for healthy slot pressure and reserve -ETIMEDOUT for an
   admitted request which receives no response.
 - Publish slots under the inflight lock, protect response state with the
   per-slot lock, and retain sender/response references until both sides
   finish.
 - Synchronize hwc_timeout updates and publish the HWC only after queues,
   admission state, caller contexts and the direct EQ test are ready.
 - Replace the goto-heavy request and channel-creation flows called out in
   review with focused admission, submission, wait, build, validation,
   rebuild and fallback helpers.
 - Treat reported depth as a maximum: rebuild exactly for 2 through 128,
   retain untouched bootstrap queues for 1 or above 128, and require the
   rebuilt report to repeat the selected depth.
 - Enforce directional request/response maxima against both the report
   and allocated buffers. Rebuild only for exact 4096-byte reports while
   permitting bootstrap operation for other nonzero maxima.
 - Validate BAR page geometry and every stored doorbell. Invalidate the
   old HWC doorbell before re-establishment and suppress MMIO while the
   invalid sentinel is present, without adding full validation to each
   data-path doorbell ring.
 - Remove the proposed capability bit because driver-version
   advertisement occurs after HWC establishment and cannot negotiate
   creation-time behavior.
 - Preserve current net-next behavior which always uses the
   hardware-reported dest_vrq_id and dest_vrcq_id.

Changes in v5 (v4 -> v5):
 - Shortened comments and documented the existing error paths in patches
   1 and 2.
 - Rejected apparent success from a response accepted before request
   submission.
 - Guarded CQ unpublication when establishment failed before cq_table
   allocation.
 - Required successful retry teardown before creating fresh bootstrap
   queues after an initial destroy failure.

Changes in v4 (return to net-next after the v3 split):
 - Reworked the feature series into four standalone patches for handover,
   per-slot completion state, concurrency and dynamic depth.
 - Added bounded admission, guarded sender accounting, timeout quarantine,
   queue-depth bounds, message-buffer sizing and rebuild validation.
 - Reset routing IDs for each establish and rejected a missing doorbell.

Changes in v3 (v2 -> net v3, historical fixes-only posting):
 - Split the combined series after maintainer feedback, posting fixes to
   net and deferring concurrency and dynamic depth.
 - Added response-buffer withdrawal and stale-response handling.
 - Removed pcie_flr() teardown fallback and retained resources after an
   unacknowledged destroy.

Changes in v2 (v1 -> v2):
 - Bounds-checked the SGE pointer derived from inline OOB size.
 - Protected active-sender accounting and drained senders during
   teardown and failure exits.
 - Rebased on net-next e354f7d60f14.

v1:
 - Combined CQ-table lifetime, queue sizing, RX validation, teardown,
   concurrent-request and dynamic-depth work in seven patches.

Validation:
 - Exact application and strict checkpatch for all four patches.
 - Baseline and every intermediate commit built with GCC/Clang W=1,
   including GDMA core, PCI, CDX COMPILE_TEST and RDMA target objects.
 - Full baseline/release kernel and module builds on the new base.
 - Sparse: no new diagnostics relative to baseline. Smatch remains
   limited by identical baseline frontend and array-bound diagnostics;
   it is not counted as a clean pass.
 - Fresh baseline/release runs on two Azure MANA VMs: concurrent PD/MR
   and native RNIC PD/CQ/MR resource controls, channel/MTU changes,
   three PCI remove/rescan cycles per VM, and three samples each of
   forward/reverse TCP and 64-byte UDP. No new kernel warning or
   functional failure in these runs; safe kernels/configurations restored.
 - These runs do not establish negotiated multi-request depth, connected
   RDMA data-plane operation, CDX hardware support or fresh firmware-fault,
   fallback, sanitizer/lockdep coverage. Prior v6 fault evidence is not
   presented as fresh v7 execution.

v6:
https://lore.kernel.org/all/cover.1790665894.git.weh@linux.microsoft.com/
v5:
https://lore.kernel.org/all/20260908035201.402424-1-longli@microsoft.com/
v4:
https://lore.kernel.org/all/20260901200018.3194525-1-longli@microsoft.com/
v3:
https://lore.kernel.org/all/20260803234355.636038-1-longli@microsoft.com/
v2:
https://lore.kernel.org/all/20260721234339.1476932-1-longli@microsoft.com/
v1:
https://lore.kernel.org/all/20260715032942.3945317-1-longli@microsoft.com/

Long Li (4):
  net: mana: prepare HWC ownership for safe reinitialization
  net: mana: give each HWC message slot its own completion state
  net: mana: support concurrent HWC requests
  net: mana: add dynamic HWC queue depth with reinit path

 .../net/ethernet/microsoft/mana/gdma_cdx.c    |   1 +
 .../net/ethernet/microsoft/mana/gdma_main.c   | 140 ++-
 .../net/ethernet/microsoft/mana/gdma_pci.c    |  11 +-
 .../net/ethernet/microsoft/mana/hw_channel.c  | 928 +++++++++++++++---
 .../net/ethernet/microsoft/mana/shm_channel.c |  58 +-
 include/net/mana/gdma.h                       |  16 +
 include/net/mana/hw_channel.h                 |  56 +-
 include/net/mana/shm_channel.h                |  10 +-
 8 files changed, 1019 insertions(+), 201 deletions(-)


base-commit: d8674294aefef02266c4d47ad10131f1bffbe534
-- 
2.43.0


^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-10-14  8:15 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-14  8:15 [PATCH net-next v7 1/4] net: mana: prepare HWC ownership for safe reinitialization netdev-bot+sashiko
  -- strict thread matches above, loose matches on Subject: below --
2026-10-13  8:05 [PATCH net-next v7 0/4] net: mana: concurrent HWC requests and dynamic queue depth Wei Hu
2026-10-13  8:05 ` [PATCH net-next v7 1/4] net: mana: prepare HWC ownership for safe reinitialization Wei Hu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®