Re: [net-next,v21,3/7] net: mtip: Add buffers management functions to the L2 switch driver

Re: [net-next,v21,3/7] net: mtip: Add buffers management functions to the L2 switch driver

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: mtip: Add buffers management functions to the L2 switch driver

This patch adds buffer allocation and deallocation functions for the
MTIP L2 switch driver, including page pool management for RX buffers
and bounce buffer allocation for TX.

> diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> --- a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> +++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> @@ -888,11 +888,99 @@ static void mtip_get_drvinfo(struct net_device *dev,
>
> static void mtip_free_buffers(struct net_device *dev)
> {
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +	int i;
> +
> +	for (i = 0; i < RX_RING_SIZE; i++) {
> +		if (!fep->page[i])
> +			continue;
> +
> +		page_pool_put_full_page(fep->page_pool,
> +					fep->page[i], false);
> +		fep->page[i] = NULL;
> +	}
> +
> +	page_pool_destroy(fep->page_pool);
> +	fep->page_pool = NULL;
> +
> +	for (i = 0; i < TX_RING_SIZE; i++)
> +		kfree(fep->tx_bounce[i]);

Should fep->tx_bounce[i] be set to NULL after kfree() here? The RX path
above correctly sets fep->page[i] = NULL after releasing the page, but
the TX path does not NULL out the bounce buffer pointers.

If mtip_alloc_buffers() is called again after a partial allocation
failure (e.g., mtip_open() fails and user retries), the stale pointers
in tx_bounce[] from the previous failed allocation could lead to a
double-free when mtip_free_buffers() is called on the retry failure
path.

Consider the sequence: first mtip_open() allocates tx_bounce[0..5] then
fails at index 6. mtip_free_buffers() frees tx_bounce[0..5] but leaves
stale pointers. Second mtip_open() allocates tx_bounce[0..2] then fails
at index 3. mtip_free_buffers() will kfree() the stale pointers at
tx_bounce[3..5].

[ ... ]