[PATCH net-next v8 2/4] net: mana: give each HWC message slot its own completion state

[PATCH net-next v8 2/4] net: mana: give each HWC message slot its own completion state

[Wei Hu]

From: Long Li <longli@microsoft.com>

Concurrent requests require the response handler and the waiting sender to
agree on the lifetime and result state of each message slot.

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.

Reinitialize completion and result state under both the allocation and
per-slot locks when acquiring the slot. A response handler can still hold
the per-slot lock after the previous sender has returned its admission
permit, so an unpublished bitmap bit alone is not sufficient protection.
If the response records its result while the timed wait expires, return
that result instead of reporting a false timeout.

Keep the baseline's post-timeout admission behavior at this step rather
than introducing a temporary channel-wide latch that suppresses subsequent
teardown commands. This does not solve late-response correlation after ID
reuse. The next patch adds response ownership, per-slot quarantine and the
associated bounded admission and teardown rules together.

Split response-status handling and request cleanup into helpers so the send
path has structured returns rather than phase-jumping gotos.

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
Signed-off-by: Long Li <longli@microsoft.com>
Signed-off-by: Wei Hu <weh@microsoft.com>
---
 .../net/ethernet/microsoft/mana/hw_channel.c  | 124 +++++++++++++-----
 include/net/mana/hw_channel.h                 |   4 +-
 2 files changed, 94 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index a1c1c6f32caf..03a3a0e9d3f8 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -9,6 +9,7 @@
 static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
 {
 	struct gdma_resource *r = &hwc->inflight_msg_res;
+	struct hwc_caller_ctx *ctx;
 	unsigned long flags;
 	u32 index;
 
@@ -19,8 +20,17 @@ static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
 	index = find_first_zero_bit(hwc->inflight_msg_res.map,
 				    hwc->inflight_msg_res.size);
 
+	ctx = &hwc->caller_ctx[index];
+	spin_lock(&ctx->lock);
+	reinit_completion(&ctx->comp_event);
+	ctx->output_buf = NULL;
+	ctx->output_buflen = 0;
+	ctx->error = -EINPROGRESS;
+	ctx->status_code = 0;
+
 	bitmap_set(hwc->inflight_msg_res.map, index, 1);
 
+	spin_unlock(&ctx->lock);
 	spin_unlock_irqrestore(&r->lock, flags);
 
 	*msg_id = index;
@@ -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);
+	}
+
+	ctx->output_buf = NULL;
 	ctx->error = err;
 
 	/* Must post rx wqe before complete(), otherwise the next rx may
@@ -106,6 +123,7 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
 	mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
 
 	complete(&ctx->comp_event);
+	spin_unlock(&ctx->lock);
 }
 
 static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
@@ -694,8 +712,10 @@ static int mana_hwc_test_channel(struct hw_channel_context *hwc, u16 q_depth,
 	if (!ctx)
 		return -ENOMEM;
 
-	for (i = 0; i < q_depth; ++i)
+	for (i = 0; i < q_depth; ++i) {
 		init_completion(&ctx[i].comp_event);
+		spin_lock_init(&ctx[i].lock);
+	}
 
 	hwc->caller_ctx = ctx;
 
@@ -998,6 +1018,37 @@ void mana_hwc_free_retained_channel(struct gdma_context *gc)
 	gc->hwc.gdma_context = NULL;
 }
 
+static int mana_hwc_response_status(struct hw_channel_context *hwc,
+				    u32 command, int error, u32 status)
+{
+	if (error)
+		return error;
+
+	if (!status || status == GDMA_STATUS_MORE_ENTRIES)
+		return 0;
+
+	if (status == GDMA_STATUS_CMD_UNSUPPORTED)
+		return -EOPNOTSUPP;
+
+	if (command != MANA_QUERY_PHY_STAT)
+		dev_err(hwc->dev, "Command 0x%x failed with status: 0x%x\n",
+			command, status);
+
+	return -EPROTO;
+}
+
+static void mana_hwc_finish_request(struct hw_channel_context *hwc,
+				    struct hwc_caller_ctx *ctx, u16 msg_id)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ctx->lock, flags);
+	ctx->output_buf = NULL;
+	spin_unlock_irqrestore(&ctx->lock, flags);
+
+	mana_hwc_put_msg_index(hwc, msg_id);
+}
+
 int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 			  const void *req, u32 resp_len, void *resp)
 {
@@ -1005,26 +1056,32 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 	struct hwc_wq *txq = hwc->txq;
 	struct gdma_req_hdr *req_msg;
 	struct hwc_caller_ctx *ctx;
+	unsigned long flags;
 	u32 dest_vrcq;
 	u32 dest_vrq;
 	u32 command;
+	u32 status;
 	u16 msg_id;
 	int err;
 
-	mana_hwc_get_msg_index(hwc, &msg_id);
+	err = mana_hwc_get_msg_index(hwc, &msg_id);
+	if (err)
+		return err;
 
 	tx_wr = &txq->msg_buf->reqs[msg_id];
+	ctx = hwc->caller_ctx + msg_id;
 
 	if (req_len > tx_wr->buf_len) {
 		dev_err(hwc->dev, "HWC: req msg size: %d > %d\n", req_len,
 			tx_wr->buf_len);
-		err = -EINVAL;
-		goto out;
+		mana_hwc_finish_request(hwc, ctx, msg_id);
+		return -EINVAL;
 	}
 
-	ctx = hwc->caller_ctx + msg_id;
+	spin_lock_irqsave(&ctx->lock, flags);
 	ctx->output_buf = resp;
 	ctx->output_buflen = resp_len;
+	spin_unlock_irqrestore(&ctx->lock, flags);
 
 	req_msg = (struct gdma_req_hdr *)tx_wr->buf_va;
 	if (req)
@@ -1046,11 +1103,23 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 	err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
 	if (err) {
 		dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n", err);
-		goto out;
+		mana_hwc_finish_request(hwc, ctx, msg_id);
+		return err;
 	}
 
 	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;
 
-		err = -ETIMEDOUT;
-		goto out;
+		mana_hwc_finish_request(hwc, ctx, msg_id);
+		return -ETIMEDOUT;
 	}
 
-	if (ctx->error) {
-		err = ctx->error;
-		goto out;
-	}
+	spin_lock_irqsave(&ctx->lock, flags);
+	ctx->output_buf = NULL;
+	err = ctx->error;
+	status = ctx->status_code;
+	spin_unlock_irqrestore(&ctx->lock, flags);
 
-	if (ctx->status_code && ctx->status_code != GDMA_STATUS_MORE_ENTRIES) {
-		if (ctx->status_code == GDMA_STATUS_CMD_UNSUPPORTED) {
-			err = -EOPNOTSUPP;
-			goto out;
-		}
-		if (command != MANA_QUERY_PHY_STAT)
-			dev_err(hwc->dev, "Command 0x%x failed with status: 0x%x\n",
-				command, ctx->status_code);
-		err = -EPROTO;
-		goto out;
-	}
-out:
-	mana_hwc_put_msg_index(hwc, msg_id);
-	return err;
+	mana_hwc_finish_request(hwc, ctx, msg_id);
+	return mana_hwc_response_status(hwc, command, err, status);
 }
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 1320bdfa9d2a..b732c7cc8d8f 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -168,10 +168,12 @@ struct hwc_wq {
 
 struct hwc_caller_ctx {
 	struct completion comp_event;
+	/* Protects slot initialization, the output buffer and response state. */
+	spinlock_t lock;
 	void *output_buf;
 	u32 output_buflen;
 
-	u32 error; /* Linux error code */
+	int error; /* Linux error code */
 	u32 status_code;
 };
 
-- 
2.43.0

Re: [PATCH net-next v8 2/4] net: mana: give each HWC message slot its own completion state

[netdev-bot+sashiko]

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