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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

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