[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

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>

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message describes a bug that the current driver
  cannot reach.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but on cores with
  normal (non-enhanced) descriptors in ring mode, which is the default…

--

net: stmmac: fix XDP TX DMA address for compound RX pages

This commit changes stmmac_xdp_xmit_xdpf() to use virt_to_head_page()
instead of virt_to_page() when it looks up the page_pool DMA address for
the XDP_TX head buffer. The goal is that a data pointer sitting in a tail
page of a compound RX page still resolves to the DMA base of the head 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.

[Severity: Medium]

Can the driver reach this configuration today? stmmac_xdp_set_prog()
refuses to attach a program when the MTU is above ETH_DATA_LEN:

drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c:stmmac_xdp_set_prog() {
    ...
	if (prog && dev->mtu > ETH_DATA_LEN) {
		/* For now, the driver doesn't support XDP functionality with
		 * jumbo frames so we return error.
		 */
		NL_SET_ERR_MSG_MOD(extack, "Jumbo frames not supported");
		return -EOPNOTSUPP;
	}
    ...
}

stmmac_change_mtu() blocks the other order too:

	if (stmmac_xdp_is_enabled(priv) && new_mtu > ETH_DATA_LEN) {
		netdev_dbg(priv->dev, "Jumbo frames not supported for XDP\n");
		return -EINVAL;
	}

When XDP is enabled the MTU is at most 1500, so stmmac_set_bfsize() returns
DEFAULT_BUFSIZE (1536). Then in __alloc_dma_rx_desc_resources():

	dma_buf_sz_pad = stmmac_rx_offset(priv) + dma_conf->dma_buf_sz +
			 SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
	num_pages = DIV_ROUND_UP(dma_buf_sz_pad, PAGE_SIZE);
	...
	pp_params.order = order_base_2(num_pages);

dma_buf_sz_pad stays below PAGE_SIZE, so num_pages is 1 and the order is 0.
stmmac_rx() also sets frame_sz to a single page.

On an order-0 page, virt_to_page() and virt_to_head_page() return the same
struct page. Does this change behaviour at runtime in any configuration the
driver currently allows?

The threshold in the message also looks off. By the driver's own sizing,
dma_buf_sz reaches 4096 once the MTU goes above roughly 2022, not 3000.

The later patch in the series, "net: stmmac: disable TX checksum offload
for jumbo frames on bugged cores", only adds a csum override. It does not
lift the XDP/jumbo restriction, so the described trigger still can't happen
at the end of the series.

The change is reasonable hardening, and it matches the existing
virt_to_head_page(ctx.xdp.data) use in stmmac_rx(). Could the message
describe it as hardening, or as a prerequisite for jumbo or multi-buffer
XDP? Could the Fixes: tag also be reconsidered? As written, stable will
treat this as a fix for a reachable DMA corruption bug.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index b8b4de4c5b1a7..ca61ab449e32b 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);

[Severity: High]

This isn't a bug introduced by this patch, but it has the same symptom.
On cores with normal (non-enhanced) descriptors in ring mode, the default
when chain_mode is not set, can the XDP_TX path in stmmac_xdp_xmit_xdpf()
make the DMA engine fetch part of the frame from an address that was never
programmed?

stmmac_rx() builds the xdp_buff with a full-page frame_sz:

	bufsz = DIV_ROUND_UP(priv->dma_conf.dma_buf_sz, PAGE_SIZE) * PAGE_SIZE;
	...
	xdp_init_buff(&ctx.xdp, bufsz, &rx_q->xdp_rxq);

That means an XDP program can use bpf_xdp_adjust_tail() to grow the frame
well past 2047 bytes and return XDP_TX, even with the MTU at 1500.

Further down in stmmac_xdp_xmit_xdpf(), only TDES2 is programmed:

		stmmac_set_desc_addr(priv, desc, dma_addr);
		stmmac_prepare_tx_desc(priv, desc, !i, len, csum,
				       priv->descriptor_mode, !!i, last_frame,
				       xdp_get_frame_len(xdpf));

With normal descriptors, the call chain
stmmac_prepare_tx_desc()->ndesc_prepare_tx_desc()->norm_set_tx_desc_len_on_ring()
splits the length between buffer 1 and buffer 2:

	unsigned int buffer1_max_length = BUF_SIZE_2KiB - 1;

	if (unlikely(len > buffer1_max_length)) {
		p->des1 |= cpu_to_le32(FIELD_PREP(TDES1_BUFFER2_SIZE_MASK,
						  len - buffer1_max_length) |
				       FIELD_PREP(TDES1_BUFFER1_SIZE_MASK,
						  buffer1_max_length));

In ring mode, TDES3 holds the buffer 2 address. That is why jumbo_frm() in
ring_mode.c sets it explicitly:

	desc->des3 = cpu_to_le32(lower_32_bits(des2) + BUF_SIZE_4KiB);

The XDP path never writes des3, and ndesc_release_tx_desc() only clears
des0 and des1:

	memset(p, 0, offsetof(struct dma_desc, des2));

So des3 still holds either 0 (from the coherent allocation, or after
clean_desc3() runs on completion) or a stale buffer address. Would the MAC
then read len - 2047 bytes from that address and transmit them?

Without an IOMMU, this could send arbitrary memory out on the wire. With
an IOMMU, the read would fault and TX DMA would be restarted.

ndev->xdp_features is set unconditionally, so nothing blocks XDP on these
cores. The behaviour goes back to be8b38a722e6, which added XDP_TX.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-stmmac-xdp-tx-pre-fixes-v1-0-05fd0c90f7e1%40oss.qualcomm.com

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: 7367 bytes --]

On Oct 13, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] The commit message describes a bug that the current driver
>   cannot reach.
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but on cores with
>   normal (non-enhanced) descriptors in ring mode, which is the default…
> 
> --
> 
> net: stmmac: fix XDP TX DMA address for compound RX pages
> 
> This commit changes stmmac_xdp_xmit_xdpf() to use virt_to_head_page()
> instead of virt_to_page() when it looks up the page_pool DMA address for
> the XDP_TX head buffer. The goal is that a data pointer sitting in a tail
> page of a compound RX page still resolves to the DMA base of the head 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.
> 
> [Severity: Medium]
> 
> Can the driver reach this configuration today? stmmac_xdp_set_prog()
> refuses to attach a program when the MTU is above ETH_DATA_LEN:

Similar to patch 2/2, this is a preliminary fix needed to remove the MTU
limitation when the interface is running in XDP mode.

> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c:stmmac_xdp_set_prog() {
>     ...
> 	if (prog && dev->mtu > ETH_DATA_LEN) {
> 		/* For now, the driver doesn't support XDP functionality with
> 		 * jumbo frames so we return error.
> 		 */
> 		NL_SET_ERR_MSG_MOD(extack, "Jumbo frames not supported");
> 		return -EOPNOTSUPP;
> 	}
>     ...
> }
> 
> stmmac_change_mtu() blocks the other order too:
> 
> 	if (stmmac_xdp_is_enabled(priv) && new_mtu > ETH_DATA_LEN) {
> 		netdev_dbg(priv->dev, "Jumbo frames not supported for XDP\n");
> 		return -EINVAL;
> 	}
> 
> When XDP is enabled the MTU is at most 1500, so stmmac_set_bfsize() returns
> DEFAULT_BUFSIZE (1536). Then in __alloc_dma_rx_desc_resources():
> 
> 	dma_buf_sz_pad = stmmac_rx_offset(priv) + dma_conf->dma_buf_sz +
> 			 SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> 	num_pages = DIV_ROUND_UP(dma_buf_sz_pad, PAGE_SIZE);
> 	...
> 	pp_params.order = order_base_2(num_pages);
> 
> dma_buf_sz_pad stays below PAGE_SIZE, so num_pages is 1 and the order is 0.
> stmmac_rx() also sets frame_sz to a single page.
> 
> On an order-0 page, virt_to_page() and virt_to_head_page() return the same
> struct page. Does this change behaviour at runtime in any configuration the
> driver currently allows?
> 
> The threshold in the message also looks off. By the driver's own sizing,
> dma_buf_sz reaches 4096 once the MTU goes above roughly 2022, not 3000.
> 
> The later patch in the series, "net: stmmac: disable TX checksum offload
> for jumbo frames on bugged cores", only adds a csum override. It does not
> lift the XDP/jumbo restriction, so the described trigger still can't happen
> at the end of the series.
> 
> The change is reasonable hardening, and it matches the existing
> virt_to_head_page(ctx.xdp.data) use in stmmac_rx(). Could the message
> describe it as hardening, or as a prerequisite for jumbo or multi-buffer
> XDP? Could the Fixes: tag also be reconsidered? As written, stable will
> treat this as a fix for a reachable DMA corruption bug.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index b8b4de4c5b1a7..ca61ab449e32b 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);
> 
> [Severity: High]
> 
> This isn't a bug introduced by this patch, but it has the same symptom.
> On cores with normal (non-enhanced) descriptors in ring mode, the default
> when chain_mode is not set, can the XDP_TX path in stmmac_xdp_xmit_xdpf()
> make the DMA engine fetch part of the frame from an address that was never
> programmed?

I am working on this fix with a dedicated patch.

Regards,
Lorenzo

> 
> stmmac_rx() builds the xdp_buff with a full-page frame_sz:
> 
> 	bufsz = DIV_ROUND_UP(priv->dma_conf.dma_buf_sz, PAGE_SIZE) * PAGE_SIZE;
> 	...
> 	xdp_init_buff(&ctx.xdp, bufsz, &rx_q->xdp_rxq);
> 
> That means an XDP program can use bpf_xdp_adjust_tail() to grow the frame
> well past 2047 bytes and return XDP_TX, even with the MTU at 1500.
> 
> Further down in stmmac_xdp_xmit_xdpf(), only TDES2 is programmed:
> 
> 		stmmac_set_desc_addr(priv, desc, dma_addr);
> 		stmmac_prepare_tx_desc(priv, desc, !i, len, csum,
> 				       priv->descriptor_mode, !!i, last_frame,
> 				       xdp_get_frame_len(xdpf));
> 
> With normal descriptors, the call chain
> stmmac_prepare_tx_desc()->ndesc_prepare_tx_desc()->norm_set_tx_desc_len_on_ring()
> splits the length between buffer 1 and buffer 2:
> 
> 	unsigned int buffer1_max_length = BUF_SIZE_2KiB - 1;
> 
> 	if (unlikely(len > buffer1_max_length)) {
> 		p->des1 |= cpu_to_le32(FIELD_PREP(TDES1_BUFFER2_SIZE_MASK,
> 						  len - buffer1_max_length) |
> 				       FIELD_PREP(TDES1_BUFFER1_SIZE_MASK,
> 						  buffer1_max_length));
> 
> In ring mode, TDES3 holds the buffer 2 address. That is why jumbo_frm() in
> ring_mode.c sets it explicitly:
> 
> 	desc->des3 = cpu_to_le32(lower_32_bits(des2) + BUF_SIZE_4KiB);
> 
> The XDP path never writes des3, and ndesc_release_tx_desc() only clears
> des0 and des1:
> 
> 	memset(p, 0, offsetof(struct dma_desc, des2));
> 
> So des3 still holds either 0 (from the coherent allocation, or after
> clean_desc3() runs on completion) or a stale buffer address. Would the MAC
> then read len - 2047 bytes from that address and transmit them?
> 
> Without an IOMMU, this could send arbitrary memory out on the wire. With
> an IOMMU, the read would fault and TX DMA would be restarted.
> 
> ndev->xdp_features is set unconditionally, so nothing blocks XDP on these
> cores. The behaviour goes back to be8b38a722e6, which added XDP_TX.
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-stmmac-xdp-tx-pre-fixes-v1-0-05fd0c90f7e1%40oss.qualcomm.com

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

[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 2/2] net: stmmac: disable TX checksum offload for jumbo frames on bugged cores

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message's premise doesn't match the tree.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  stmmac_xdp_xmit_xdpf() never looks at priv->plat->tx_coe when deciding…

--

net: stmmac: disable TX checksum offload for jumbo frames on bugged cores

This patch turns off TX checksum insertion in stmmac_xdp_xmit_xdpf() for
frames longer than ETH_FRAME_LEN on bugged_jumbo cores. It is meant to
match the stmmac_fix_features() workaround that the skb path already uses.

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

[Severity: Low]
Can this scenario happen in the current tree? XDP still can't be used with
a jumbo MTU. stmmac_xdp_set_prog() refuses to attach a program when the MTU
is above ETH_DATA_LEN:

drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c:stmmac_xdp_set_prog() {
    ...
	if (prog && dev->mtu > ETH_DATA_LEN) {
		/* For now, the driver doesn't support XDP functionality with
		 * jumbo frames so we return error.
		 */
		NL_SET_ERR_MSG_MOD(extack, "Jumbo frames not supported");
		return -EOPNOTSUPP;
	}
    ...
}

stmmac_change_mtu() also refuses a jumbo MTU while XDP is enabled:

	if (stmmac_xdp_is_enabled(priv) && new_mtu > ETH_DATA_LEN) {
		netdev_dbg(priv->dev, "Jumbo frames not supported for XDP\n");
		return -EINVAL;
	}

The redirect case looks limited too. Devmap checks every frame with
xdp_ok_fwd_dev(dev, xdp_get_frame_len(xdpf)), which caps the length at
mtu + hard_header_len + VLAN_HLEN. At MTU 1500 that is 1518 bytes.
Redirected frames therefore reach the new ETH_FRAME_LEN check only when
they are 1515 to 1518 bytes long, which is VLAN-tagged normal-size
traffic, not jumbo.

The one path that seems to reach the new check today is XDP_TX after the
program grows the frame. At MTU 1500, stmmac_rx() still sets frame_sz to
at least PAGE_SIZE:

	bufsz = DIV_ROUND_UP(priv->dma_conf.dma_buf_sz, PAGE_SIZE) * PAGE_SIZE;

A program can therefore use bpf_xdp_adjust_tail() to grow a frame past
1514 bytes and return XDP_TX. stmmac_xdp_xmit_xdpf() never checks that
length against the MTU.

Could the commit message describe the trigger that exists today, and say
whether the bugged_jumbo erratum covers it? If this patch depends on a
separate change that allows XDP at a jumbo MTU, could the message say so?
The Fixes: tag reads as if the problem can be hit now.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ca61ab449e32b..0e29c5d04714c 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;

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. The
comment says this matches stmmac_fix_features(), but that function also
turns off TX checksum offload when tx_coe is unset:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_fix_features() {
    ...
	if (!priv->plat->tx_coe)
		features &= ~NETIF_F_CSUM_MASK;
    ...
}

In stmmac_xdp_xmit_xdpf(), csum comes only from the per-queue flag:

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

stmmac_hw_init() clears tx_coe when threshold DMA mode is forced:

		/* TXCOE doesn't work in thresh DMA mode */
		if (priv->plat->force_thresh_dma_mode)
			priv->plat->tx_coe = false;

stmmac_dma_operation_mode() then runs TX in threshold mode
(txmode = tc).

On platforms with snps,force_thresh_dma_mode, do XDP_TX and ndo_xdp_xmit
frames still request full checksum insertion through
stmmac_prepare_tx_desc() in a DMA mode where the driver treats TX COE as
broken? Should the XDP path also check priv->plat->tx_coe here?

>  	while (true) {
>  		skb_frag_t *frag = i ? &sinfo->frags[i - 1] : NULL;
>  		int len = frag ? skb_frag_size(frag) : xdpf->len;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-stmmac-xdp-tx-pre-fixes-v1-0-05fd0c90f7e1%40oss.qualcomm.com

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

[Lorenzo Bianconi]

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

> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The commit message's premise doesn't match the tree.
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but
>   stmmac_xdp_xmit_xdpf() never looks at priv->plat->tx_coe when deciding…
> 
> --
> 
> net: stmmac: disable TX checksum offload for jumbo frames on bugged cores
> 
> This patch turns off TX checksum insertion in stmmac_xdp_xmit_xdpf() for
> frames longer than ETH_FRAME_LEN on bugged_jumbo cores. It is meant to
> match the stmmac_fix_features() workaround that the skb path already uses.
> 
> > 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.
> 
> [Severity: Low]
> Can this scenario happen in the current tree? XDP still can't be used with
> a jumbo MTU. stmmac_xdp_set_prog() refuses to attach a program when the MTU
> is above ETH_DATA_LEN:

This is actually a preliminary fix needed to remove the MTU limitation when the
device is running in XDP mode.

> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c:stmmac_xdp_set_prog() {
>     ...
> 	if (prog && dev->mtu > ETH_DATA_LEN) {
> 		/* For now, the driver doesn't support XDP functionality with
> 		 * jumbo frames so we return error.
> 		 */
> 		NL_SET_ERR_MSG_MOD(extack, "Jumbo frames not supported");
> 		return -EOPNOTSUPP;
> 	}
>     ...
> }
> 
> stmmac_change_mtu() also refuses a jumbo MTU while XDP is enabled:
> 
> 	if (stmmac_xdp_is_enabled(priv) && new_mtu > ETH_DATA_LEN) {
> 		netdev_dbg(priv->dev, "Jumbo frames not supported for XDP\n");
> 		return -EINVAL;
> 	}
> 
> The redirect case looks limited too. Devmap checks every frame with
> xdp_ok_fwd_dev(dev, xdp_get_frame_len(xdpf)), which caps the length at
> mtu + hard_header_len + VLAN_HLEN. At MTU 1500 that is 1518 bytes.
> Redirected frames therefore reach the new ETH_FRAME_LEN check only when
> they are 1515 to 1518 bytes long, which is VLAN-tagged normal-size
> traffic, not jumbo.
> 
> The one path that seems to reach the new check today is XDP_TX after the
> program grows the frame. At MTU 1500, stmmac_rx() still sets frame_sz to
> at least PAGE_SIZE:
> 
> 	bufsz = DIV_ROUND_UP(priv->dma_conf.dma_buf_sz, PAGE_SIZE) * PAGE_SIZE;
> 
> A program can therefore use bpf_xdp_adjust_tail() to grow a frame past
> 1514 bytes and return XDP_TX. stmmac_xdp_xmit_xdpf() never checks that
> length against the MTU.
> 
> Could the commit message describe the trigger that exists today, and say
> whether the bugged_jumbo erratum covers it? If this patch depends on a
> separate change that allows XDP at a jumbo MTU, could the message say so?
> The Fixes: tag reads as if the problem can be hit now.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index ca61ab449e32b..0e29c5d04714c 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;
> 
> [Severity: Medium]
> This is a pre-existing issue, not one introduced by this patch. The
> comment says this matches stmmac_fix_features(), but that function also
> turns off TX checksum offload when tx_coe is unset:

since this issue has not been introduced by this patch, it needs to be fixed
with a dedicated patch.

Regards,
Lorenzo

> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_fix_features() {
>     ...
> 	if (!priv->plat->tx_coe)
> 		features &= ~NETIF_F_CSUM_MASK;
>     ...
> }
> 
> In stmmac_xdp_xmit_xdpf(), csum comes only from the per-queue flag:
> 
> 	bool csum = !priv->plat->tx_queues_cfg[queue].coe_unsupported;
> 
> stmmac_hw_init() clears tx_coe when threshold DMA mode is forced:
> 
> 		/* TXCOE doesn't work in thresh DMA mode */
> 		if (priv->plat->force_thresh_dma_mode)
> 			priv->plat->tx_coe = false;
> 
> stmmac_dma_operation_mode() then runs TX in threshold mode
> (txmode = tc).
> 
> On platforms with snps,force_thresh_dma_mode, do XDP_TX and ndo_xdp_xmit
> frames still request full checksum insertion through
> stmmac_prepare_tx_desc() in a DMA mode where the driver treats TX COE as
> broken? Should the XDP path also check priv->plat->tx_coe here?
> 
> >  	while (true) {
> >  		skb_frag_t *frag = i ? &sinfo->frags[i - 1] : NULL;
> >  		int len = frag ? skb_frag_size(frag) : xdpf->len;
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-stmmac-xdp-tx-pre-fixes-v1-0-05fd0c90f7e1%40oss.qualcomm.com

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

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