Re: [PATCH net-next v2 04/13] net: lan966x: add FDMA LLP register write helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing Hardware Initialization in Fallback Path
- [High] Ignored return value of `fdma_dcbs_init()` leads to memory corruption
- [High] Stale destroyed `page_pool` left in XDP RX queues leading to UAF / Double Free
--

commit e1cc739c153cef02ae9d3a24aa4e6dc773ba0fd8
Author: Daniel Machon <daniel.machon@microchip.com>

net: lan966x: add FDMA LLP register write helper

This commit moves the LLP register writes from the channel start/activate
functions into the allocation functions. It also introduces a shared
lan966x_fdma_llp_configure() helper to write the LLP registers.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> index f8ce735a7fc0..6c5761e886d4 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c

[ ... ]

> @@ -127,6 +134,9 @@ static int lan966x_fdma_rx_alloc(struct lan966x_rx *rx)
>  	fdma_dcbs_init(fdma, FDMA_DCB_INFO_DATAL(fdma->db_size),
>  		       FDMA_DCB_STATUS_INTR);

This is existing code, but could the error recovery path here lead to a
use-after-free or double free?

If fdma_alloc_coherent() fails just prior to this, the newly created
page_pool is destroyed. In the lan966x_fdma_reload() fallback path, the old
page_pool pointer is restored in the lan966x->rx structure, but it is never
re-registered with xdp_rxq_info_reg_mem_model().

The xdp_rxq structures will continue holding the mem.id of the destroyed
page_pool. When the queues are eventually torn down, will
xdp_rxq_info_unreg_mem_model() attempt to free the destroyed page_pool?

Also, is it safe to ignore the return value of fdma_dcbs_init() here?

If fdma_dcbs_init() invokes allocation callbacks that fail with -ENOMEM, the
remaining DMA descriptors could be left uninitialized and retain a dataptr
of 0.

By ignoring the failure, won't the driver falsely assume initialization
succeeded and enable hardware DMA, potentially causing the DMA engine to
write incoming frames to physical address 0?

>  
> +	lan966x_fdma_llp_configure(lan966x, (u64)fdma->dma,
> +				   fdma->channel_id);
> +
>  	return 0;
>  }
>  
> @@ -136,14 +146,6 @@ static void lan966x_fdma_rx_start(struct lan966x_rx *rx)
>  	struct fdma *fdma = &rx->fdma;
>  	u32 mask;
>  
> -	/* When activating a channel, first is required to write the first DCB
> -	 * address and then to activate it
> -	 */
> -	lan_wr(lower_32_bits((u64)fdma->dma), lan966x,
> -	       FDMA_DCB_LLP(fdma->channel_id));
> -	lan_wr(upper_32_bits((u64)fdma->dma), lan966x,
> -	       FDMA_DCB_LLP1(fdma->channel_id));
> -

By moving the LLP register writes out of lan966x_fdma_rx_start(), does this
break the fallback path in lan966x_fdma_reload() used for the AMBA interface?

If lan966x_fdma_rx_alloc() fails due to memory pressure, the driver jumps to
the restore label, copies back the old RX state, and calls
lan966x_fdma_rx_start() to re-activate the channel.

Because lan966x_fdma_rx_start() no longer writes the LLP register, and the
channel was previously disabled via a hardware DB_DISCARD, will the hardware
be re-activated without its base descriptor pointer configured, leading to
the DMA fetching from a stale or invalid address?

>  	lan_wr(FDMA_CH_CFG_CH_DCB_DB_CNT_SET(fdma->n_dbs) |
>  	       FDMA_CH_CFG_CH_INTR_DB_EOF_ONLY_SET(1) |
>  	       FDMA_CH_CFG_CH_INJ_PORT_SET(0) |

[ ... ]

> @@ -214,6 +216,9 @@ static int lan966x_fdma_tx_alloc(struct lan966x_tx *tx)
>  
>  	fdma_dcbs_init(fdma, 0, 0);

Similar to the RX side, this is existing code, but does ignoring the return
value of fdma_dcbs_init() here also risk DMA hardware fetching from an
invalid descriptor if allocation fails?

>  
> +	lan966x_fdma_llp_configure(lan966x, (u64)fdma->dma,
> +				   fdma->channel_id);
> +
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260428-lan966x-pci-fdma-v2-0-d3ec66e06202@microchip.com?part=4