Re: [PATCH net v7 6/7] net: mana: fix stale HWC response after command timeout

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a remaining protocol correlation gap. Responses from the hardware are matched to requests solely by the reusable `hwc_msg_id` slot index. If a command times out (or is a zero-wait teardown), its slot is released and can be immediately reused by a new request. If the hardware later responds to the original timed-out command, the response will carry the same `msg_id`. Because the slot now belongs to the new request (and `ctx->output_buf` is published), `mana_hwc_handle_resp()` will incorrectly accept the stale response and copy its payload into the new request's buffer, leading to silent data corruption.
--

--- Patch [6]: [PATCH net v7 6/7] net: mana: fix stale HWC response after command timeout ---
commit 4d60222957c8ccd6181005f7897ef459443e8e8b
Author: Long Li <[email protected]>
net: mana: fix stale HWC response after command timeout

This commit introduces a per-slot spinlock, refcount, and a responded flag
to prevent a late response from writing into a sender's buffer after it has
returned due to a timeout. It also replaces the counting semaphore with a
waitqueue for admission control.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 88188523dcd48..b1269f7da0563 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -106,22 +129,34 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
>  		resp_len = 0;
>  	}
>  
> -	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
> -	if (err)
> -		goto out;
> +	spin_lock(&ctx->lock);
>  
> -	ctx->status_code = resp_msg->status;
> +	/* Honour a response only while the sender owns the slot (output_buf
> +	 * published) and has not already been answered; otherwise drop it as
> +	 * premature, stale or duplicate without touching the refcount.
> +	 */
> +	if (!ctx->output_buf || ctx->responded) {
> +		spin_unlock(&ctx->lock);
> +		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> +		return;
> +	}
> +	ctx->responded = true;
>  
> -	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: High]
This isn't a bug introduced by this patch, but is there a remaining protocol
correlation gap here?

Responses from the hardware appear to be matched to requests solely by the
reusable hwc_msg_id slot index, with no generation counter or unique
transaction ID.

If a command times out (or is a zero-wait teardown), its slot is released
and can be immediately reused by a new request. If the hardware later
responds to the original timed-out command, won't the response carry the
same msg_id?

Because the slot now belongs to the new request (and ctx->output_buf is
published), could mana_hwc_handle_resp() incorrectly accept this stale
response and copy its payload into the new request's buffer, leading to
silent data corruption?

>  	ctx->error = err;
>  
> -	/* Must post rx wqe before complete(), otherwise the next rx may
> -	 * hit no_wqe error.
> +	/* Post RX WQE before completing — the next response may arrive
> +	 * immediately and needs a posted buffer.
>  	 */
>  	mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> -
>  	complete(&ctx->comp_event);
> +	spin_unlock(&ctx->lock);
> +
> +	hwc_ctx_put(hwc, ctx);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6