Re: [PATCH net-next v6 06/14] net: lan966x: use a dedicated device for DMA operations

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Memory leak of xdp_frame on DMA mapping error or insufficient headroom in lan966x_fdma_xmit_xdpf().
- [High] Unchecked failure of fdma_dcbs_init() leaves pages unallocated and leads to a NULL pointer dereference during cleanup.
- [High] Double addition of XDP_PACKET_HEADROOM causes the IFH and packet start to skip DMA synchronization for CPU.
- [High] Transmitting an xdpf frame uses a corrupted DMA address, leading to transmission of random memory and IOMMU faults.
- [High] Cross-pool page corruption when recycling XDP_TX pages after an MTU change.
- [High] XDP_TX completely ignores XDP program packet adjustments (length and offset).
--

commit 911b52e69be420fb557cf5f643980df6f6523c6c
Author: Daniel Machon <daniel.machon@microchip.com>
Subject: net: lan966x: use a dedicated device for DMA operations

In preparation for the PCIe FDMA implementation, this commit adds a
lan966x->dma_dev pointer and resolves it at probe time. The new pointer
is then used for every DMA operation in the FDMA path. Natively, this is
just lan966x->dev, so there is no functional change for non-PCIe devices.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> @@ -126,7 +126,7 @@ static int lan966x_fdma_rx_alloc(struct lan966x_rx *rx)
>  	if (err)
>  		return err;
>  
> -	err = fdma_alloc_coherent(lan966x->dev, fdma);
> +	err = fdma_alloc_coherent(lan966x->dma_dev, fdma);
>  	if (err) {
>  		page_pool_destroy(rx->page_pool);
>  		return err;
>  	}

[Severity: High]
This is a pre-existing issue, but I noticed that fdma_dcbs_init() is called
further down in lan966x_fdma_rx_alloc() without checking its return value.

If page_pool_dev_alloc_pages() fails inside lan966x_fdma_rx_dataptr_cb(),
fdma_dcbs_init() will return -ENOMEM. Because the error is ignored, device
initialization incorrectly succeeds while rx->page[dcb][db] is left as NULL.

Later, when lan966x_fdma_rx_free_pages() is called during an MTU change or
driver unload, it passes these NULL pointers to page_pool_put_full_page():

    for (j = 0; j < fdma->n_dbs; ++j)
        page_pool_put_full_page(rx->page_pool, rx->page[i][j], false);

Could this cause a NULL pointer dereference and system crash?

[ ... ]

> @@ -355,7 +355,7 @@ static void lan966x_fdma_tx_clear_buf(struct lan966x *lan966x, int weight)
>  
>  		dcb_buf->used = false;
>  		if (dcb_buf->use_skb) {
> -			dma_unmap_single(lan966x->dev,
> +			dma_unmap_single(lan966x->dma_dev,
>  					 dcb_buf->dma_addr,
>  					 dcb_buf->len,
>  					 DMA_TO_DEVICE);
> @@ -364,7 +364,7 @@ static void lan966x_fdma_tx_clear_buf(struct lan966x *lan966x, int weight)
>  				napi_consume_skb(dcb_buf->data.skb, weight);
>  		} else {
>  			if (dcb_buf->xdp_ndo)
> -				dma_unmap_single(lan966x->dev,
> +				dma_unmap_single(lan966x->dma_dev,
>  						 dcb_buf->dma_addr,
>  						 dcb_buf->len,
>  						 DMA_TO_DEVICE);

[Severity: High]
This isn't a bug introduced by this patch, but does recycling XDP_TX pages
here risk cross-pool page corruption when the MTU changes?

During an MTU change in lan966x_fdma_reload(), a new page pool is created
for RX, and the old one is destroyed. However, the TX channel remains active.
If there are in-flight XDP_TX frames, they will trigger a TX interrupt and
reach the cleanup branch that recycles pages into the current rx->page_pool:

    page_pool_recycle_direct(rx->page_pool, dcb_buf->data.page);

Since the rx->page_pool pointer was updated by the MTU reload, wouldn't this
incorrectly insert pages allocated from the old pool into the new pool's cache,
violating page pool API semantics?

[ ... ]

> @@ -400,7 +400,7 @@ static int lan966x_fdma_rx_check_frame(struct lan966x_rx *rx, u64 *src_port)
>  	if (unlikely(!page))
>  		return FDMA_ERROR;
>  
> -	dma_sync_single_for_cpu(lan966x->dev,
> +	dma_sync_single_for_cpu(lan966x->dma_dev,
>  				(dma_addr_t)db->dataptr + XDP_PACKET_HEADROOM,
>  				FDMA_DCB_STATUS_BLOCKL(db->status),
>  				DMA_FROM_DEVICE);

[Severity: High]
This is a pre-existing issue, but does adding XDP_PACKET_HEADROOM here
cause the IFH to miss DMA synchronization?

During initialization, lan966x_fdma_rx_dataptr_cb() sets db->dataptr to
already include the headroom offset:

    *dataptr = page_pool_get_dma_addr(page) + XDP_PACKET_HEADROOM;

By adding XDP_PACKET_HEADROOM again in dma_sync_single_for_cpu(), the
synchronized region skips the first bytes of the packet data. Could this
cause lan966x_ifh_get_src_port() to read the IFH from a stale un-synchronized
CPU cache?

[ ... ]

> @@ -635,11 +635,11 @@ int lan966x_fdma_xmit_xdpf(struct lan966x_port *port, void *ptr, u32 len)
>  		lan966x_ifh_set_bypass(ifh, 1);
>  		lan966x_ifh_set_port(ifh, BIT_ULL(port->chip_port));
>  
> -		dma_addr = dma_map_single(lan966x->dev,
> +		dma_addr = dma_map_single(lan966x->dma_dev,
>  					  xdpf->data - IFH_LEN_BYTES,
>  					  xdpf->len + IFH_LEN_BYTES,
>  					  DMA_TO_DEVICE);
> -		if (dma_mapping_error(lan966x->dev, dma_addr)) {
> +		if (dma_mapping_error(lan966x->dma_dev, dma_addr)) {
>  			ret = NETDEV_TX_OK;
>  			goto out;
>  		}

[Severity: High]
This is a pre-existing issue, but does this error path leak the XDP frame?

If dma_mapping_error() triggers (or if xdpf->headroom < IFH_LEN_BYTES fails
just above this snippet), the function returns NETDEV_TX_OK without freeing
the frame. The caller, lan966x_xdp_xmit(), sees a successful return and won't
clean up the frame either, leading to memory leaks under load.

[Severity: High]
This isn't a bug introduced by this patch, but does the DMA address get
corrupted during transmission for xdpf frames?

Here, the dma_addr points exactly to the start of the mapped data. However,
when lan966x_fdma_xdp_tx_dataptr_cb() is invoked to set the transmission
pointer, it unconditionally adds the headroom offset again:

    *dataptr = lan966x->tx.dcbs_buf[dcb].dma_addr + XDP_PACKET_HEADROOM;

For xdpf frames, this shifts the start address far past the mapped buffer,
causing the hardware to read unmapped memory and trigger IOMMU faults.

[ ... ]

> @@ -655,7 +655,7 @@ int lan966x_fdma_xmit_xdpf(struct lan966x_port *port, void *ptr, u32 len)
>  		lan966x_ifh_set_port(ifh, BIT_ULL(port->chip_port));
>  
>  		dma_addr = page_pool_get_dma_addr(page);
> -		dma_sync_single_for_device(lan966x->dev,
> +		dma_sync_single_for_device(lan966x->dma_dev,
>  					   dma_addr + XDP_PACKET_HEADROOM,
>  					   len + IFH_LEN_BYTES,
>  					   DMA_TO_DEVICE);

[Severity: High]
This is a pre-existing issue, but does the XDP_TX path ignore XDP program
packet adjustments?

When the XDP program returns XDP_TX in lan966x_xdp_run(), it passes the
original unmodified data length to lan966x_fdma_xmit_xdpf(). Then, for
page-based transmits, the driver unconditionally writes the IFH at a fixed
offset:

    ifh = page_address(page) + XDP_PACKET_HEADROOM;

If the XDP program adjusted the packet length or offset (via
bpf_xdp_adjust_head or bpf_xdp_adjust_tail), the IFH will be written to
the wrong location, and the unmodified packet will be transmitted instead.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-lan966x-pci-fdma-v6-0-6f48dab9d671@microchip.com?part=6