mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* Re: [PATCH net-next v8 2/4] net: mana: give each HWC message slot its own completion state
@ 2026-10-20 13:36 netdev-bot+sashiko
  0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-10-20 13:36 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 more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Commit "net: mana: give each HWC message slot its own completion
  state" fixes memory-safety and stale-completion bugs that go back to…

Pre-existing issues:
- [High] Cross-request response confusion after msg_id reuse.
- [Medium] mana_hwc_handle_resp() copies resp_len bytes from
  rx_req->buf_va.
- [Low] hwc->hwc_timeout is read and modified without synchronization.

--

net: mana: give each HWC message slot its own completion state

This patch adds a per-slot spinlock to struct hwc_caller_ctx. The lock
protects the output buffer, the error and the device status, and
output_buf is published only while the sender is waiting.
mana_hwc_get_msg_index() now reinitializes the completion and result
state when a slot is acquired. A response that races the timed wait is
returned instead of a false timeout, and status handling and cleanup are
split into mana_hwc_response_status() and mana_hwc_finish_request().

Link: https://lore.kernel.org/r/20260908035201.402424-3-longli@microsoft.com
Link: https://lore.kernel.org/r/20260908035201.402424-4-longli@microsoft.com
Link: https://lore.kernel.org/r/179196575434.434549.4428132041097324407@kernel.org
Link: https://lore.kernel.org/r/179196575640.434549.9102950927041409151@kernel.org

> Protect the caller's output buffer, Linux error and device status with a
> per-slot lock. Publish the output buffer only while the sender is waiting
> and withdraw it before every return. A late response cannot copy through a
> pointer to caller storage whose lifetime has ended.

[Severity: Low]
Should this patch carry a Fixes: tag?

Before this patch, mana_hwc_get_msg_index() set the bitmap bit before
mana_hwc_send_request() assigned ctx->output_buf. output_buf was also
never cleared when a sender returned.

A late response that passed the lockless test_bit() in
mana_hwc_handle_resp() could then memcpy() into a previous caller's
response buffer. Some callers pass on-stack buffers. For example,
mana_query_phy_stats() in mana_en.c has:

	struct mana_query_phy_stat_resp resp = {};

The old code also never called reinit_completion(). A complete() that
raced a timed-out sender left comp_event.done set. The next user of that
slot would then return immediately with stale error and status_code
values and an unfilled output buffer.

Both problems appear to date back to the original driver:

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")

Without a Fixes: tag, backporters have no signal that this patch closes
a memory corruption bug in released kernels. The complete fix also needs
"net: mana: support concurrent HWC requests" later in the series, which
finishes the late-response correlation work.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index a1c1c6f32cafa..03a3a0e9d3f84 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -90,14 +100,21 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
>  	}
>  
>  	ctx = hwc->caller_ctx + msg_id;
> -	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
> -	if (err)
> -		goto out;
>  
> -	ctx->status_code = resp_msg->status;
> +	spin_lock(&ctx->lock);
> +	if (!ctx->output_buf) {
> +		spin_unlock(&ctx->lock);
> +		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> +		return;
> +	}
>  
> -	memcpy(ctx->output_buf, resp_msg, resp_len);
> -out:
> +	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
> +	if (!err) {
> +		ctx->status_code = resp_msg->status;
> +		memcpy(ctx->output_buf, resp_msg, resp_len);
> +	}

[Severity: Medium]
This isn't a bug introduced by this patch, but can this memcpy() read
past the end of the RX slot?

resp_len is the device-supplied rx_oob->tx_oob_data_size. The only upper
bound in mana_hwc_verify_resp_msg() is the caller's buffer size:

	if (resp_len > caller_ctx->output_buflen)
		return -EPROTO;

resp_len is never compared against rx_req->buf_len, which equals
hwc->max_req_msg_size.

Suppose a caller passes a resp_len larger than the RX slot, and a buggy
or malicious device reports an oversized length. The copy would then
read from adjacent RX slots or past the DMA allocation, and those bytes
would be returned to the caller.

No current caller with such a resp_len was found. This patch only moves
the existing verify and memcpy() under ctx->lock.

Later in the series, "net: mana: add dynamic HWC queue depth with reinit
path" makes mana_hwc_get_msg_index() reject resp_len >
min(hwc->rx_msg_size_limit, hwc->max_req_msg_size) with -EMSGSIZE before
a slot is taken. That closes this.

[ ... ]

> @@ -1046,11 +1103,23 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,

[ ... ]

>  	if (!wait_for_completion_timeout(&ctx->comp_event,
> -					 (msecs_to_jiffies(hwc->hwc_timeout)))) {
> +					 msecs_to_jiffies(hwc->hwc_timeout))) {
> +		spin_lock_irqsave(&ctx->lock, flags);
> +		ctx->output_buf = NULL;
> +		err = ctx->error;
> +		status = ctx->status_code;
> +		spin_unlock_irqrestore(&ctx->lock, flags);
> +
> +		if (err != -EINPROGRESS) {
> +			mana_hwc_finish_request(hwc, ctx, msg_id);
> +			return mana_hwc_response_status(hwc, command, err, status);
> +		}
> +
>  		if (hwc->hwc_timeout != 0)
>  			dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
>  				command, hwc->hwc_timeout);
> @@ -1059,27 +1128,16 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
>  		if (hwc->hwc_timeout > 1)
>  			hwc->hwc_timeout = 1;

[Severity: Low]
This is a pre-existing issue, but can this check-and-store overwrite a
concurrent store of 0 to hwc->hwc_timeout?

mana_serv_reset() in gdma_pci.c sets the "never wait" value without any
synchronization:

	/* HWC is not responding in this case, so don't wait */
	hwc->hwc_timeout = 0;

Consider this sequence:

  1. A sender reads a value greater than 1.
  2. mana_serv_reset() stores 0.
  3. The sender stores 1.

Later senders then wait 1 ms instead of not waiting at all, and the
"timed out" log stops being suppressed.

The HWC_DATA_CFG_HWC_TIMEOUT case in mana_hwc_init_event_handler() also
writes hwc->hwc_timeout. The send path reads it several times with plain
loads.

This patch only reformats the wait expression here. Later in the series,
"net: mana: support concurrent HWC requests" moves all accesses to
mana_hwc_timeout_read(), mana_hwc_timeout_reduce(),
mana_hwc_timeout_update() and mana_hwc_timeout_cancel(). It also changes
mana_serv_reset() to call mana_gd_cancel_hwc_timeout().

>  
> -		err = -ETIMEDOUT;
> -		goto out;
> +		mana_hwc_finish_request(hwc, ctx, msg_id);
> +		return -ETIMEDOUT;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but can a late response for a
timed-out command be delivered to the next sender that reuses this
msg_id?

On this path, mana_hwc_finish_request()->mana_hwc_put_msg_index()
releases the slot immediately. mana_hwc_get_msg_index() always hands out
the lowest free index:

	index = find_first_zero_bit(hwc->inflight_msg_res.map,
				    hwc->inflight_msg_res.size);

With the depth-1 bootstrap queues, the next sender always gets the same
slot and publishes its own output_buf.

When the firmware response for the old command arrives,
mana_hwc_handle_resp() checks only the bitmap bit and that output_buf is
non-NULL:

	spin_lock(&ctx->lock);
	if (!ctx->output_buf) {

The old payload is then copied into the new sender's buffer, and the new
sender completes with the old command's status.

A caller such as mana_gd_destroy_dma_region() could then return success
for the wrong transaction. If the old response is shorter, the tail of
the buffer is left stale.

The commit message acknowledges this ("This does not solve late-response
correlation after ID reuse"). It is fixed later in the series by "net:
mana: support concurrent HWC requests". That patch keeps the
response-side reference (refcnt and resp_pending) and the admission
permit on a timed-out slot. The bitmap bit is then cleared only after
the late response is consumed, or at teardown.

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

^ permalink raw reply	[flat|nested] 2+ messages in thread
* [PATCH net-next v8 0/4] net: mana: concurrent HWC requests and dynamic queue depth
@ 2026-10-16 12:06 Wei Hu
  2026-10-16 12:06 ` [PATCH net-next v8 2/4] net: mana: give each HWC message slot its own completion state Wei Hu
  0 siblings, 1 reply; 2+ messages in thread
From: Wei Hu @ 2026-10-16 12:06 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 adds concurrent HWC request ownership and negotiated HWC
queue depth to MANA. It starts from depth-one bootstrap queues, keeps
setup private until the final channel is tested, and preserves the
distinction between CPU lifetime and PF ownership of DMA mappings.

I am continuing Long Li's four-patch series. Original per-patch author
identity, dates and sign-offs are retained.

Changes in v8 (v7 -> v8):
 - Rebase onto net-next 05b5019b01fb9443b09ae4ed34af1135a80a84c8.
 - Withdraw the table and drain RCU readers before freeing the HWC
   bundle, in patch 1. Add final PCI/CDX software-only disposal of
   retained state without reclaiming unacknowledged PF-owned DMA.
 - Initialize caller state under the per-slot lock. Do not introduce
   a temporary channel-wide timeout latch in intermediate patch 2.
 - Preserve recovery signaling when abandoned responses exhaust
   admission, while healthy saturation remains ordinary pressure.
 - Include the candidate functional test in bounded bootstrap fallback
   and preserve a live retry budget without reviving cancellation.
 - Share the effective TX budget with GDMA/RDMA request producers;
   bound RDMA batches without changing RX allocation/indexing strides.
 - Preserve stopped-channel errors before admission and fail malformed
   doorbell initialization without depending on a later INIT_DONE.
 - Correct cleanup comments and commit-message rationale. Review both
   Sashiko report streams rather than treating either as exhaustive.

This series remains independent of queue-set replacement. That work
addresses individual Ethernet CQ publication/retirement. The shared
CQ-table hunks must be reconciled while preserving both contracts,
whichever series lands second. Neither series is silently stacked.
https://lore.kernel.org/all/cover.1790795005.git.weh@linux.microsoft.com/

Known separate concerns include individual RDMA/datapath CQ lifetime,
malformed-response RX reposting and wider DMA failure/device-lifecycle
policy. This revision does not claim to fix them. Standalone net/stable
backports of older-kernel fixes require separate extraction/validation.

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:
Source and build qualification:
Per-intermediate GCC/Clang W=1 builds, patch style checks, focused
actual-source C cases and paired full kernel/image/module qualification
are complete. Sparse retains a baseline warning; Smatch retains baseline
frontend diagnostics and is not counted as a clean pass.

Ordinary baseline/release coverage on VM5 and VM6 is complete.
Each VM/fixture has 1,600 type-2 PD/MR and 320 type-3 PD/CQ/MR operations
across the initial and three post-PCI rounds, native channels 8->4->8,
native/upper MTUs 1500->1400->1500, three PCI remove/rescan cycles,
bounded ping and three-second TCP/1Mbit UDP with positive receivers.
Both VMs returned to their exact SAFE baselines. Owned fixtures/helpers
and processes were removed; guest and central reservations were released.

Evidence is explicitly composed, not one uninterrupted four-trial run.
The completed VM5 baseline uses retained exact runtime receipts and a
corrected evaluation of iperf3's duplicate, identical UDP bitrate metadata.
That original harness attempt remains PROFILE_FAILED in its history.
VM5 release and both VM6 fixtures completed in a new three-trial run.
Fixture identity and SAFE continuity are checked across the two scopes.

No measured simultaneous outstanding firmware-command count, negotiated
depth proof, performance comparison, connected RDMA workload, CDX hardware,
fault injection, sanitizer, DIM or queue-set qualification is claimed.

The final intermediate-history correction leaves the entire final
source tree identical to the release build snapshot a1454f411cdc.
Evidence retains the literal build commit and kernelrelease rather
than relabeling an image with a later history-only commit ID.

v7:
https://lore.kernel.org/all/cover.1791561995.git.weh@linux.microsoft.com/
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

 drivers/infiniband/hw/mana/main.c             |   19 +-
 .../net/ethernet/microsoft/mana/gdma_cdx.c    |    5 +-
 .../net/ethernet/microsoft/mana/gdma_main.c   |  154 ++-
 .../net/ethernet/microsoft/mana/gdma_pci.c    |   15 +-
 .../net/ethernet/microsoft/mana/hw_channel.c  | 1019 ++++++++++++++---
 .../net/ethernet/microsoft/mana/shm_channel.c |   58 +-
 include/net/mana/gdma.h                       |   17 +
 include/net/mana/hw_channel.h                 |   69 +-
 include/net/mana/shm_channel.h                |   10 +-
 9 files changed, 1150 insertions(+), 216 deletions(-)


base-commit: 05b5019b01fb9443b09ae4ed34af1135a80a84c8
-- 
2.43.0


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2026-10-20 13:36 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-20 13:36 [PATCH net-next v8 2/4] net: mana: give each HWC message slot its own completion state netdev-bot+sashiko
  -- strict thread matches above, loose matches on Subject: below --
2026-10-16 12:06 [PATCH net-next v8 0/4] net: mana: concurrent HWC requests and dynamic queue depth Wei Hu
2026-10-16 12:06 ` [PATCH net-next v8 2/4] net: mana: give each HWC message slot its own completion state 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®