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
