[PATCH net-next 0/2] stmmac: some XDP fixes in XDP_TX path

[PATCH net-next 0/2] stmmac: some XDP fixes in XDP_TX path

[Lorenzo Bianconi]

- Fix page address lookup for compound pages in XDP_TX path.
- Disable TX checksum offload for jumbo frames in XDP_TX path.

---
Lorenzo Bianconi (2):
      net: stmmac: fix XDP TX DMA address for compound RX pages
      net: stmmac: disable TX checksum offload for jumbo frames on bugged cores

 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
---
base-commit: 29f144016f03608abfddcd6ebaeabafd7aeaadd2
change-id: 20261012-stmmac-xdp-tx-pre-fixes-37fd4800b74c

Best regards,
-- 
Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

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

[Lorenzo Bianconi]

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.

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.

Fixes: be8b38a722e6 ("net: stmmac: Add support for XDP_TX action")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@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

[PATCH net-next 2/2] net: stmmac: disable TX checksum offload for jumbo frames on bugged cores

[Lorenzo Bianconi]

stmmac_xdp_xmit_xdpf() decides TX checksum insertion from the queue COE
capability only:

	csum = !priv->plat->tx_queues_cfg[queue].coe_unsupported;

and lets the enhanced descriptor set TX_CIC_FULL accordingly. On the
bugged_jumbo cores (snps,dwmac-3.40a, snps,dwmac-3.610 and
snps,dwmac-3.710) hardware checksum insertion is broken for jumbo
frames; the skb path works around it by clearing NETIF_F_CSUM_MASK in
stmmac_fix_features() when the MTU exceeds ETH_DATA_LEN, so skbs are
never marked CHECKSUM_PARTIAL and no checksum is inserted. The XDP TX
path bypasses that workaround, so once XDP is allowed at MTU >
ETH_DATA_LEN, jumbo frames sent via XDP_TX or redirected into the
device would leave the interface with a full checksum insertion enabled
on those cores - exactly what the workaround is meant to avoid.

Disable checksum insertion for oversized frames on bugged_jumbo cores in
the XDP TX path too.

Fixes: be8b38a722e6 ("net: stmmac: Add support for XDP_TX action")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index ca61ab449e32..0e29c5d04714 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5276,6 +5276,14 @@ static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
 		return STMMAC_XDP_CONSUMED;
 	}
 
+	/* Disable TX checksum offload for jumbo frames on cores where
+	 * checksum insertion is broken for oversized frames, matching the
+	 * stmmac_fix_features() workaround used by the skb path.
+	 */
+	if (priv->plat->bugged_jumbo &&
+	    xdp_get_frame_len(xdpf) > ETH_FRAME_LEN)
+		csum = false;
+
 	while (true) {
 		skb_frag_t *frag = i ? &sinfo->frags[i - 1] : NULL;
 		int len = frag ? skb_frag_size(frag) : xdpf->len;

-- 
2.56.0

Re: [PATCH net-next 0/2] stmmac: some XDP fixes in XDP_TX path

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net-next 0/2] stmmac: some XDP fixes in XDP_TX path

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1317 bytes --]

> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.

these issues have been reported by sashiko.

> 
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.

they are real problems spotted by sashiko.

> 
>  - What hardware the change was tested on. For driver fixes please
>    mention the device (and if relevant firmware version) used for
>    testing, or say that the change was not tested on real hardware.

I tested them on a Qualcomm Rb3-gen2 board.

Regards,
Lorenzo

> 
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Daniel Machon]

Hi Lorenzo,

> 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?

> 
> 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@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

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2767 bytes --]

> 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().

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@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [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@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@microchip.com>