Re: [PATCH 1/3] net: mana: check xdp_rxq registration before unreg in mana_destroy_rxq()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Ignored return values for XDP memory model registration leads to page pool leaks
- [High] Hardware state corruption by requesting destruction of handle 0 in TX queue error path
--

commit 981d87e9d96d90f1ed0564dfd6bbd85f261ecd6d
Author: Dipayaan Roy <dipayanroy@linux.microsoft.com>
net: mana: check xdp_rxq registration before unreg in mana_destroy_rxq()

When mana_create_rxq() fails before xdp_rxq_info_reg() is called, the error
path calls mana_destroy_rxq(). Guard xdp_rxq_info_unreg() with
xdp_rxq_info_is_reg() to prevent unregistering a never-registered xdp_rxq.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index a654b3699c4c..dfb4ba9f7664 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -2520,7 +2520,9 @@ static void mana_destroy_rxq(struct mana_port_context *apc,
>  		napi_disable_locked(napi);
>  		netif_napi_del_locked(napi);
>  	}
> -	xdp_rxq_info_unreg(&rxq->xdp_rxq);
> +
> +	if (xdp_rxq_info_is_reg(&rxq->xdp_rxq))
> +		xdp_rxq_info_unreg(&rxq->xdp_rxq);

This problem wasn't introduced by this patch, but it is a pre-existing logic
error not fixed later in the series.

In mana_create_rxq(), the calls to xdp_rxq_info_reg() and
xdp_rxq_info_reg_mem_model() are wrapped in WARN_ON() but their negative
return codes are never assigned to err or handled:

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_create_rxq() {
    ...
	WARN_ON(xdp_rxq_info_reg(&rxq->xdp_rxq, ndev, rxq_idx,
				 cq->napi.napi_id));
	WARN_ON(xdp_rxq_info_reg_mem_model(&rxq->xdp_rxq, MEM_TYPE_PAGE_POOL,
					   rxq->page_pool));

	napi_enable_locked(&cq->napi);
    ...
}

If xdp_rxq_info_reg_mem_model() fails (e.g., due to an ida_alloc failure
under memory pressure), the warning is printed but the queue creation
successfully completes.

Consequently, rxq->xdp_rxq is left with a zeroed mem_info.type
(MEM_TYPE_PAGE_ORDER0).

When an XDP program drops or redirects packets, will xdp_return_frame() read
the incorrect memory type and call put_page() instead of returning the page
to the page_pool?

Does this permanently leak the page_pool inflight count, resulting in an
indefinite system hang when page_pool_destroy() is called during interface
teardown?

>  	mana_destroy_wq_obj(apc, GDMA_RQ, rxq->rxobj);

This problem wasn't introduced by this patch, but it is a pre-existing flaw.

While a subsequent patch in the series ('net: mana: Skip WQ object destruction
for uninitialized RXQ') fixes a similar issue for RX queues by checking for
INVALID_MANA_HANDLE, the same issue remains unaddressed for TX queues.

In mana_create_txq(), apc->tx_qp is zero-allocated. The initialization loop
assigns tx_object = INVALID_MANA_HANDLE only when it reaches index i.

If initialization fails early (e.g., at i = 0), the error path calls
mana_destroy_txq(), which iterates over all apc->num_queues and
unconditionally calls mana_destroy_wq_obj():

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_create_txq() {
    ...
	apc->tx_qp = kzalloc_objs(struct mana_tx_qp, apc->num_queues);
    ...
	for (i = 0; i < apc->num_queues; i++) {
		apc->tx_qp[i].tx_object = INVALID_MANA_HANDLE;
    ...
		err = mana_create_wq_obj(apc, apc->port_handle, GDMA_SQ,
					 &wq_spec, &cq_spec,
					 &apc->tx_qp[i].tx_object);

		if (err)
			goto out;
    ...
out:
	netdev_err(net, "Failed to create %d TX queues, %d\n",
		   apc->num_queues, err);
	mana_destroy_txq(apc);
	return err;
}

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_destroy_txq() {
    ...
	for (i = 0; i < apc->num_queues; i++) {
    ...
		mana_destroy_wq_obj(apc, GDMA_SQ, apc->tx_qp[i].tx_object);
    ...
}

For all unvisited queues, tx_object will be 0 (from kzalloc).

Since mana_destroy_wq_obj() does not validate the handle, could this
inadvertently ask the hardware to destroy handle 0, which might be a valid,
unrelated queue, and corrupt the hardware state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260430035935.1859220-1-dipayanroy@linux.microsoft.com?part=1