Re: [PATCH net] net: stmmac: restore NET_IP_ALIGN in the non-XDP RX path

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: stmmac: restore NET_IP_ALIGN in the non-XDP RX path

This adds NET_IP_ALIGN back to the RX DMA offset returned by
stmmac_rx_offset() for the non-XDP case, so that the page pool page handed
to the stack as the skb head places the IP header at a 4-byte aligned
address again. Without it, ARM32 takes an alignment exception in
icmp_echo() for any received echo request.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index a71f0df263785..2a57415be25cf 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1529,7 +1529,7 @@ static unsigned int stmmac_rx_offset(struct stmmac_priv *priv)
>  	if (stmmac_xdp_is_enabled(priv))
>  		return XDP_PACKET_HEADROOM;
>  
> -	return NET_SKB_PAD;
> +	return NET_SKB_PAD + NET_IP_ALIGN;
>  }

[Severity: High]
Should the XDP branch of stmmac_rx_offset() get the same treatment?

When an XDP program is attached, stmmac_rx_offset() returns bare
XDP_PACKET_HEADROOM (256) with no NET_IP_ALIGN, and an XDP_PASS packet goes
to the stack through exactly the code this patch is fixing, in stmmac_rx():

	skb = napi_build_skb(page_address(buf->page),
			     rx_q->napi_skb_frag_size);
	...
	/* XDP program may adjust header */
	head_pad_len = ctx.xdp.data - ctx.xdp.data_hard_start;
	skb_reserve(skb, head_pad_len);
	skb_put(skb, buf1_len);

For a program that returns XDP_PASS without touching the head, head_pad_len
is 256, so the Ethernet header starts at 256, the IP header at 270 and the
ICMP or TCP header at 290:

	290 % 4 == 2

which is the same 2 mod 4 misalignment class as the 98 in the reported
crash, so the icmp_echo() panic quoted in the commit message looks
reproducible on the same board by attaching an XDP_PASS program.

The 2021 commit a955318fe67e ("stmmac: align RX buffers") cited in the
commit message changed both returns (XDP_PACKET_HEADROOM + NET_IP_ALIGN as
well); here only one return changes, leaving the two branches of the same
helper with different alignment guarantees.

Page pool sizing is derived from stmmac_rx_offset() for both branches 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);

so wouldn't the extra two bytes be accounted for in the XDP case too?

If the intent is to fix only the non-XDP configuration, could the commit
message say so explicitly, since stable backporters and anyone running XDP
on ARM32 would still hit the panic?

Cross-instance finding from sashiko-gemini (bb586704729c8cac27b6f004c29082ebde7dad94593b943d82d5139137628cdb):
[Severity: Critical]
Hardware DMA address truncation and silent packet corruption due to unaligned RX offset.
-- 
pw-bot: cr