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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Data race due to publishing the message slot before context initialization
- [High] Flawed early response mitigation causes premature sender completion and cascading slot corruption
--

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

net: mana: fix stale HWC response after command timeout

This commit fixes an issue where a hardware command timeout leaves a stale
response in the channel, potentially corrupting subsequent requests. It
introduces a per-slot lock, refcount, and a responded flag to safely track
slot ownership between the sender and the CQ interrupt handler.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index adc7ad98ca8d8..2f0dae3539552 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -7,25 +7,58 @@
>  #include <linux/pci.h>
>  #include <linux/vmalloc.h>
>  
> +/* Acquire a free message slot from the inflight bitmap.  Returns
> + * -ETIMEDOUT if a prior HWC command has timed out (preserving the
> + * error code callers expect).
> + */
>  static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
>  {
>  	struct gdma_resource *r = &hwc->inflight_msg_res;
>  	unsigned long flags;
>  	u32 index;
>  
> -	down(&hwc->sema);
> +	for (;;) {
> +		spin_lock_irqsave(&r->lock, flags);
>  
> -	spin_lock_irqsave(&r->lock, flags);
> +		if (hwc->hwc_timed_out) {
> +			spin_unlock_irqrestore(&r->lock, flags);
> +			return -ETIMEDOUT;
> +		}
>  
> -	index = find_first_zero_bit(hwc->inflight_msg_res.map,
> -				    hwc->inflight_msg_res.size);
> +		index = find_first_zero_bit(r->map, r->size);
> +		if (index < r->size) {
> +			struct hwc_caller_ctx *ctx;
> +
> +			bitmap_set(r->map, index, 1);
> +			ctx = &hwc->caller_ctx[index];
> +			reinit_completion(&ctx->comp_event);

[Severity: High]
Could this publish the message slot before the context is fully initialized?

The slot is marked active in the bitmap with bitmap_set() before the
caller_ctx fields (comp_event, refcnt, responded, error) are initialized.

Since mana_hwc_handle_resp() checks the bitmap locklessly via test_bit():

mana_hwc_handle_resp() {
        ...
        if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
                dev_err(hwc->dev, "hwc_rx: invalid msg_id = %u\n", msg_id);
                ...
        }
        ctx = hwc->caller_ctx + msg_id;
        spin_lock(&ctx->lock);
        ...
}

If a stale or duplicate hardware response arrives at this exact moment,
can the interrupt handler observe the published bit and interact with the
partially initialized ctx fields?

> +			/* Take the response-side reference here, under
> +			 * r->lock and together with the slot bitmap bit,
> +			 * so a stale or duplicate response that lands
> +			 * before mana_hwc_send_request() posts the request
> +			 * cannot drop the refcount to zero and free the
> +			 * slot under the sender.  One reference is the
> +			 * sender's; the other is released by
> +			 * mana_hwc_handle_resp().
> +			 */
> +			refcount_set(&ctx->refcnt, 2);
> +			ctx->responded = false;
> +			ctx->msg_id = index;
> +			ctx->error = -EINPROGRESS;
> +			spin_unlock_irqrestore(&r->lock, flags);
> +			break;
> +		}

[ ... ]

> @@ -1039,6 +1119,10 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
>  		dest_vrcq = hwc->pf_dest_vrcq_id;
>  	}
>  
> +	/* handle_resp()'s reference was taken in mana_hwc_get_msg_index(),
> +	 * so hardware responding immediately after the doorbell ring cannot
> +	 * release the slot before this sender is done with it.
> +	 */
>  	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);

[Severity: High]
Does this early response mitigation handle the sender's completion correctly?

If a stale response arrives before mana_hwc_send_request() posts its WQE,
mana_hwc_handle_resp() processes it and calls complete(&ctx->comp_event).

When the sender then reaches wait_for_completion_timeout() right below the
WQE posting step, the completion is already signaled, so it will return
immediately as if successful. 

The sender then drops its reference and frees the slot, even though the newly
posted WQE is still actively being processed by the hardware.

If another thread reuses this slot, it will reset ctx->responded = false.
When the hardware later delivers the real response for the original request,
will mana_hwc_handle_resp() accept it and use it to complete the new thread's
request with incorrect data?

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