Re: [PATCH net-next 2/7] net: mana: fix HWC RQ/SQ buffer size swap

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free and data corruption due to premature timeout return in mana_hwc_send_request without canceling hardware transaction.
- [High] Missing locking allows concurrent WQE ring corruption in mana_hwc_send_request.
--

--- Patch [2]: [PATCH net-next 2/7] net: mana: fix HWC RQ/SQ buffer size swap ---
commit a1baa394548fbd57903fe3f07805307884c7c9c0
Author: Long Li <longli@microsoft.com>

net: mana: fix HWC RQ/SQ buffer size swap

The HWC RQ receives responses and the SQ sends requests, but
mana_hwc_init_queues() sized the RQ with max_req_msg_size and the SQ with
max_resp_msg_size -- backwards. A response larger than the undersized RQ
buffer could overflow it, and mana_hwc_rx_event_handler() recovered the
RX slot index by dividing by the wrong size.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 409e20caeccdc..3f011ebbe7b3e 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -749,6 +749,7 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
>  
>  	hwc->num_inflight_msg = q_depth;
>  	hwc->max_req_msg_size = max_req_msg_size;
> +	hwc->max_resp_msg_size = max_resp_msg_size;
>  
>  	return 0;
>  out:

[Severity: High]
This isn't a bug introduced by this patch, but could this timeout path lead
to a use-after-free and data corruption?

If a caller invokes mana_hwc_send_request() and it times out, it frees the
msg_id slot and returns without canceling the hardware transaction:

mana_hwc_send_request() {
    if (!wait_for_completion_timeout(&ctx->comp_event, ...)) {
        err = -ETIMEDOUT;
        goto out;
    }
    ...
out:
    mana_hwc_put_msg_index(hwc, msg_id);
}

If the caller provided a stack-allocated response buffer, its stack frame
will be popped when mana_hwc_send_request() returns. When the delayed
hardware response eventually arrives, will mana_hwc_handle_resp() blindly
accept it and overwrite the now-freed stack address?

mana_hwc_handle_resp() {
    ...
    memcpy(ctx->output_buf, resp_msg, resp_len);
}

Alternatively, if another thread reuses the freed msg_id, could it receive
this delayed response instead of its own?

[Severity: High]
This is also a pre-existing issue, but does this code allow concurrent
WQE ring corruption?

Multiple threads can concurrently invoke mana_hwc_send_request(), such as
from parallel InfiniBand requests. This leads to mana_hwc_post_tx_wqe(),
which appears to call mana_gd_post_and_ring() locklessly:

mana_hwc_post_tx_wqe() {
    ...
    err = mana_gd_post_and_ring(hwc_txq->gdma_wq, &req->wqe_req, NULL);
}

If multiple threads execute this concurrently, won't they read the same
wq->head in mana_gd_post_work_request(), overwrite the exact same WQE
slots, and corrupt the shared queue state?

mana_gd_post_work_request() {
    wqe_ptr = mana_gd_get_wqe_ptr(wq, wq->head);
    ...
    mana_gd_write_sgl(wq, wqe_ptr, wqe_req);
    wq->head += wqe_size / GDMA_WQE_BU_SIZE;
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715032942.3945317-1-longli@microsoft.com?part=2