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

[netdev-bot+sashiko@kernel.org]

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] The patch closes memory-safety and wrong-result bugs that are
  reachable in the baseline, but it carries no Fixes: tag and no stable…

Pre-existing issues:
- [Medium] At this commit, a late response for a request that already
  timed out can be delivered to the next request that reuses the same…
- [Medium] At this commit, mana_hwc_handle_resp() copies resp_len bytes,
  a value the device supplies via rx_oob->tx_oob_data_size, from the RX…
- [Medium] At this commit, the timeout path in mana_hwc_send_request()
  frees the msg_id slot through mana_hwc_finish_request() while the SQ…
- [Medium] mana_hwc_rx_event_handler() returns without calling
  mana_hwc_post_rx_wqe(hwc->rxq, rx_req) when the device-supplied…

--

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

This adds a per-slot spinlock to struct hwc_caller_ctx. The lock protects
the caller's output buffer, the Linux error and the device status.

The output buffer is published only while the sender waits and is
withdrawn before every return. Completion and result state are
reinitialized when a slot is acquired, and status handling and cleanup
move into helpers.

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.
>
> Reinitialize completion and result state under both the allocation and
> per-slot locks when acquiring the slot.

[Severity: Low]
These changes fix bugs that can already be hit in the baseline, but the
commit has no Fixes: tag and no stable note. Should it have them?

Before this patch, mana_hwc_send_request() set ctx->output_buf = resp and
never cleared it. On timeout it called mana_hwc_put_msg_index() and
returned. A late response in mana_hwc_handle_resp() could then memcpy()
through ctx->output_buf into the stack frame of a caller that had already
returned.

The baseline also never reinitialized comp_event after the
init_completion() in mana_hwc_test_channel(). A complete() that landed
after a timeout carried over to the next user of the slot. That user then
returned immediately with stale error and status and an unfilled response
buffer.

Both become easier to hit once hwc_timeout drops to 1 ms after the first
timeout.

Would something like this be appropriate?

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

This commit only partly closes the late-response problem. Could the
message also tell backporters that the follow-up "net: mana: support
concurrent HWC requests" is needed too?

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 9e0964072af9a..6f0ccdeffc4f9 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?

Here resp_len comes from the device through rx_oob->tx_oob_data_size.
mana_hwc_verify_resp_msg() only checks it against the caller's buffer:

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

Nothing in mana_hwc_send_request() keeps the caller's resp_len (which
becomes output_buflen) at or below rx_req->buf_len, which is
max_req_msg_size. If a caller passes a larger resp_len, a buggy or
malicious device could make this copy pull adjacent RX DMA slots into the
caller's buffer.

The later patch "net: mana: add dynamic HWC queue depth with reinit path"
fixes this. There, mana_hwc_get_msg_index() rejects any resp_len above
min(hwc->rx_msg_size_limit, hwc->max_req_msg_size).

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

[Severity: Medium]
This isn't a bug introduced by this patch, but the caller of this
function, mana_hwc_rx_event_handler(), drops the RX buffer when the device
sends an out-of-range msg_id:

	msg_id = READ_ONCE(resp->response.hwc_msg_id);
	if (msg_id >= hwc->num_inflight_msg) {
		dev_err(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
		return;
	}

rx_req is already known at this point. Should this path call
mana_hwc_post_rx_wqe(hwc->rxq, rx_req) before it returns?

As written, each such completion permanently removes one buffer from the
HWC RQ. After q_depth of them the RQ is empty, and later responses have no
buffer to land in. This still looks present at the end of the series.

[ ... ]

> @@ -1048,11 +1105,24 @@ 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))) {

[ ... ]

>  		if (hwc->hwc_timeout != 0)
>  			dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
>  				command, hwc->hwc_timeout);
> @@ -1061,27 +1131,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;

[Severity: Medium]
This isn't a bug introduced by this patch, but can a late response for a
request that timed out still reach the next request that reuses the same
msg_id?

Sender A times out and calls
mana_hwc_finish_request()->mana_hwc_put_msg_index(), which clears the
bitmap bit right away. Sender B then gets the same index from
find_first_zero_bit(), publishes ctx->output_buf = resp_B and posts its
request.

When A's late response arrives, mana_hwc_handle_resp() checks only the
msg_id and whether ctx->output_buf is non-NULL:

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

Both checks pass for B's slot. A's response would then be copied into B's
buffer and complete B, and B would return A's data and status.

The commit message says this is not solved here. The later patch "net:
mana: support concurrent HWC requests" handles it by keeping a timed-out
slot quarantined. The slot stays reserved until the late response arrives
or mana_hwc_stop_channel() cancels it.

[Severity: Medium]
This is a pre-existing issue, but is the TX DMA buffer for this msg_id safe
to reuse at this point?

The SQ WQE posted by mana_hwc_post_tx_wqe() points at
txq->msg_buf->reqs[msg_id].buf_va, and the device may not have consumed it
yet. mana_hwc_get_msg_index() uses find_first_zero_bit(), so the next
sender usually gets the same index and overwrites that buffer:

	req_msg = (struct gdma_req_hdr *)tx_wr->buf_va;
	if (req)
		memcpy(req_msg, req, req_len);

Could the device then read a mix of the old and new request contents?

The later patch "net: mana: support concurrent HWC requests" keeps the
slot's bitmap bit, and with it the TX buffer, until the late response
arrives or mana_hwc_stop_channel() cancels it.

>  	}

[ ... ]

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