Re: [PATCH net v3 2/6] net: mana: fix HWC RQ/SQ buffer size swap

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mana: fix HWC RQ/SQ buffer size swap

This swaps the sizes used when creating the HWC RQ and SQ so the RQ is
sized by max_resp_msg_size and the SQ by max_req_msg_size, adds
max_resp_msg_size to struct hw_channel_context, and uses it as the RX
slot stride in mana_hwc_rx_event_handler().  It also hoists the
num_inflight_msg / max_req_msg_size / max_resp_msg_size assignments in
mana_hwc_init_queues() above the mana_hwc_create_cq() call.

Two questions, both about the description rather than the mechanics of
the swap.

First, about this part of the changelog:

>     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 (max_req_msg_size).

[Severity: Medium]
Can the described overflow or mis-indexing actually happen in this tree?
mana_hwc_init_queues() has a single caller, mana_hwc_create_channel(),
which passes two constants that are equal:

	err = mana_hwc_init_queues(hwc, HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH,
				   HW_CHANNEL_MAX_REQUEST_SIZE,
				   HW_CHANNEL_MAX_RESPONSE_SIZE);

include/net/mana/hw_channel.h:

	#define HW_CHANNEL_MAX_REQUEST_SIZE  0x1000
	#define HW_CHANNEL_MAX_RESPONSE_SIZE 0x1000

The hardware-reported sizes that mana_hwc_establish_channel() returns are
only handed to mana_hwc_test_channel(); the queues are never re-created
with them, so max_req_msg_size == max_resp_msg_size on every path both
before and after this patch.

Also, mana_hwc_post_rx_wqe() advertises the real slot length to the
device:

	sge->address = (u64)req->buf_sge_addr;
	sge->mem_key = hwc_rxq->msg_buf->gpa_mkey;
	sge->size = req->buf_len;

so the device is not told it may write more than one slot regardless of
max_resp_msg_size.

Given that, would it be worth saying in the changelog that the two sizes
are currently identical and that there is no observable misbehaviour,
i.e. that this is a latent-correctness cleanup?  With the

	Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")

tag and the wording above, a stable backporter is likely to read this as
a live buffer overflow fix.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 409e20caeccdc..cbb56c7647876 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -263,7 +263,7 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
>  
>  	/* Select the RX work request for virtual address and for reposting. */
>  	rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
> -	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_req_msg_size;
> +	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size;
>  
>  	if (rx_req_idx >= hwc_rxq->msg_buf->num_reqs) {
>  		dev_err(hwc->dev, "HWC RX: wrong rx_req_idx=%llu, num_reqs=%u\n",

[ ... ]

> @@ -721,6 +721,15 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
>  	if (err)
>  		return err;
>  
> +	/* Set the queue dimensions before creating the CQ: doing so
> +	 * registers mana_hwc_rx_event_handler(), which divides by
> +	 * hwc->max_resp_msg_size and range-checks hwc->num_inflight_msg.
> +	 * They must be valid before any RX completion can be delivered.
> +	 */
> +	hwc->num_inflight_msg = q_depth;
> +	hwc->max_req_msg_size = max_req_msg_size;
> +	hwc->max_resp_msg_size = max_resp_msg_size;

[Severity: Low]
Does this comment claim more than the reordering provides?  It says the
scalars must be valid before mana_hwc_create_cq() registers
mana_hwc_rx_event_handler(), but in that same window hwc->rxq and
hwc->txq are still NULL, since they are only assigned by the
mana_hwc_create_wq() calls further down.

mana_hwc_rx_event_handler() touches hwc->rxq well before it reaches
either hoisted field:

	struct hwc_wq *hwc_rxq = hwc->rxq;
	...
	if (WARN_ON_ONCE(hwc_rxq->gdma_wq->id != gdma_rxq_id))
		return;
	...
	rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size;

and mana_hwc_handle_resp() uses hwc->caller_ctx, which is only allocated
later in mana_hwc_test_channel().  The EQ callback registered by the same
mana_hwc_create_cq() call, mana_hwc_init_event_handler(), likewise
dereferences hwc->cq, hwc->rxq and hwc->txq:

	hwc->cq->gdma_eq->id = eq_db.eq_id;
	...
	hwc->rxq->gdma_wq->id = val;
	...
	hwc->txq->msg_buf->gpa_mkey = val;

So if a completion really could be delivered while mana_hwc_create_cq()
runs, the hoisted scalars would not help.  The window looks not to be
live anyway, because the device only learns the queue DMA addresses via
mana_smc_setup_hwc() in mana_hwc_establish_channel(), which runs after
mana_hwc_init_queues() has published everything, matching the existing
comment in mana_hwc_create_channel():

	/* mana_hwc_init_queues() only creates the required data structures,
	 * and doesn't touch the HWC device.
	 */

Could the comment and the matching changelog paragraph be reworded to say
the assignments are simply hoisted for clarity, rather than asserting
that RX completions can be safely handled at that point?  As written, a
later reader may take this window to be safe for handler execution.

>  	/* CQ is shared by SQ and RQ, so CQ's queue depth is the sum of SQ
>  	 * queue depth and RQ queue depth.
>  	 */
> @@ -733,23 +742,20 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
>  		goto out;
>  	}
>  
> -	err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_req_msg_size,
> +	err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_resp_msg_size,
>  				 hwc->cq, &hwc->rxq);
>  	if (err) {
>  		dev_err(hwc->dev, "Failed to create HWC RQ: %d\n", err);
>  		goto out;
>  	}
>  
> -	err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_resp_msg_size,
> +	err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_req_msg_size,
>  				 hwc->cq, &hwc->txq);
>  	if (err) {
>  		dev_err(hwc->dev, "Failed to create HWC SQ: %d\n", err);
>  		goto out;
>  	}
>  
> -	hwc->num_inflight_msg = q_depth;
> -	hwc->max_req_msg_size = max_req_msg_size;
> -
>  	return 0;

[ ... ]