Re: [PATCH net v5 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] Logic flaw in handling stale/duplicate responses causes premature request failure and slot reuse.
- [High] Data race: message slot is published to the inflight bitmap before context initialization is complete.
--

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

net: mana: fix stale HWC response after command timeout

This commit attempts to fix a race condition where an HWC response
arrives after a command timeout by giving each caller context a spinlock
and a refcount. It prevents reusing slots whose responses are still
outstanding.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -7,25 +7,58 @@
>  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]
Can a concurrent stale response observe the bitmap before the context is
fully initialized?

If a stale response arrives for this index on another CPU,
mana_hwc_handle_resp() could see the bit set in the inflight bitmap via
test_bit() and proceed to locklessly read and modify context fields while
they are still being initialized here. 

Because ctx->lock is not held during this initialization,
mana_hwc_handle_resp() might observe an uninitialized state, such as an
empty refcnt. This could cause a refcount underflow when hwc_ctx_put()
is called, or race with reinit_completion().

> +			/* Take the response-side reference here, under
> +			 * r->lock and together with the slot bitmap bit,

[ ... ]

> @@ -116,22 +156,44 @@ 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;
> +	if (ctx->responded) {
> +		/* A response for this slot was already delivered; this is a
> +		 * duplicate or replayed one.  Drop it so the hwc_ctx_put()
> +		 * a first response performs is not done twice, which would
> +		 * over-release the slot while the sender still owns it.
> +		 */
> +		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:
> -	ctx->error = err;
> +	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
> +
> +	if (!err && ctx->output_buf) {
> +		ctx->status_code = resp_msg->status;
> +		memcpy(ctx->output_buf, resp_msg, resp_len);
> +		ctx->error = 0;
> +	} else if (ctx->output_buf) {
> +		/* Record the error only while the sender still owns the
> +		 * request: a non-NULL output_buf means it is still waiting.
> +		 * Once it has timed out (or been force-completed by destroy)
> +		 * it clears output_buf and takes its own error, so a late
> +		 * response must not write ctx->error or the buffer here.
> +		 */
> +		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);

[Severity: High]
Could a stale response that arrives before the sender sets output_buf cause
the new request to fail prematurely?

If a stale response arrives after a message slot is allocated, but before
mana_hwc_send_request() sets output_buf to a non-NULL value,
mana_hwc_handle_resp() will execute this path. Because output_buf is NULL,
ctx->error remains -EINPROGRESS. 

It then sets ctx->responded = true and signals ctx->comp_event. 

When mana_hwc_send_request() subsequently sets output_buf, posts the new
WQE, and waits on the completion event, wait_for_completion_timeout()
will return immediately because it was already completed. 

The sender will then read -EINPROGRESS from ctx->error, assume a failure,
and free the message slot while its newly posted WQE is still actively in
flight. This could potentially lead to data corruption when the real response
arrives and corrupts the next request's buffer.

> +	spin_unlock(&ctx->lock);
> +
> +	hwc_ctx_put(hwc, ctx);
>  }

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