[PATCH net-next 1/2] net: stmmac: fix XDP TX DMA address for compound RX pages

[Daniel Machon]

Hi Lorenzo,

> > Hi Lorenzo,
> 
> Hi Daniel,
> 
> > 
> > > When RX buffers are compound pages (order > 0), only the head page
> > > carries the DMA address programmed by the page pool
> > > (page_pool_get_dma_addr()). stmmac_xdp_xmit_xdpf() computed the head
> > > buffer's bus address from the page that happens to contain the data:
> > > 
> > > 	page = frag ? skb_frag_page(frag)
> > > 		    : virt_to_page(xdpf->data);
> > > 	dma_addr = page_pool_get_dma_addr(page);
> > > 	...
> > > 
> > > RX pages become compound as soon as dma_buf_sz needs more than one page
> > > (pp_params.order = order_base_2(num_pages) > 0), which happens from an
> > > MTU around 3000 bytes once dma_buf_sz reaches 4096. If an XDP program
> > > moves the data forward with bpf_xdp_adjust_head() by more than about a
> > > page, xdpf->data points into a tail page whose struct page has no DMA
> > > address: page_pool_get_dma_addr() then returns a bogus bus address, and
> > > dma_sync_single_for_device() together with the TX descriptor transmit
> > > from the wrong location.
> > 
> > Looks to me like mtu > ETH_DATA_LAN is rejected when setting a prog, and also
> > on mtu change when a prog is attached. So can this even happen today?
> 
> yes, these issues are real but at the moment we can't trigger them since we
> have the MTU limitation in stmmac_change_mtu()/stmmac_xdp_set_prog().
> These are preliminary fixes in order to remove this limitation.
> There is a leftover issue we still need to fix about jumbo frames in
> stmmac_xdp_xmit_xdpf().

Thanks for clarifying.

> 
> Regards,
> Lorenzo
> 
> > 
> > > 
> > > Use virt_to_head_page() to read the DMA base of the buffer and keep the
> > > offset relative to the head page (sizeof(*xdpf) + xdpf->headroom), which
> > > is correct no matter which page the data ends up in.
> > 
> > I think the fix looks correct.
> > 
> > > 
> > > Fixes: be8b38a722e6 ("net: stmmac: Add support for XDP_TX action")
> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at oss.qualcomm.com>
> > > ---
> > >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > index b8b4de4c5b1a..ca61ab449e32 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > @@ -5301,7 +5301,7 @@ static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
> > >  			struct page *page;
> > >  
> > >  			page = frag ? skb_frag_page(frag)
> > > -				    : virt_to_page(xdpf->data);
> > > +				    : virt_to_head_page(xdpf->data);
> > >  			dma_addr = page_pool_get_dma_addr(page);
> > >  			if (frag)
> > >  				dma_addr += skb_frag_off(frag);
> > > 
> > > -- 
> > > 2.56.0
> > > 
> > 
> > /Daniel

Reviewed-by: Daniel Machon <daniel.machon at microchip.com>