Re: [PATCH net 4/6] net: stmmac: copy XDP metadata in AF_XDP zero-copy RX

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · 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] At this commit, the commit message says
  stmmac_construct_skb_zc() 'keeps the metadata aligned when…

--

net: stmmac: copy XDP metadata in AF_XDP zero-copy RX

This commit changes stmmac_construct_skb_zc() so that it copies the XDP
metadata in [xdp->data_meta, xdp->data) into the new skb together with
the packet data. The metadata ends up right before skb->data, and
meta_len is recorded with skb_metadata_set().

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 30a2774d722b7..8a2faf50d6957 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5519,7 +5519,8 @@ static struct sk_buff *stmmac_construct_skb_zc(struct stmmac_channel *ch,
>  		return NULL;
>  
>  	skb_reserve(skb, xdp->data - xdp->data_hard_start);
> -	memcpy(__skb_put(skb, datasize), xdp->data, datasize);
> +	__skb_put(skb, datasize);
> +	memcpy(skb->data - metasize, xdp->data_meta, datasize + metasize);
>  	if (metasize)
>  		skb_metadata_set(skb, metasize);

[Severity: Medium]
The commit message says this change:

  "keeps the metadata aligned when stmmac_rx_vlan() relocates it with the
  MAC header while stripping a tag."

Is that true at this commit? Here, stmmac_rx_vlan() still moves only the
two MAC addresses:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx_vlan() {
    ...
		vlanid = ntohs(veth->h_vlan_TCI);
		memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2);
		skb_pull(skb, VLAN_HLEN);
    ...
}

The software pop path runs when hw_vlan_en is false, CTAG or STAG RX
offload is enabled, and a tagged frame arrives. In that case,
stmmac_dispatch_skb_zc() calls stmmac_rx_vlan() before eth_type_trans():

    stmmac_rx_vlan(priv->dev, skb);
    skb->protocol = eth_type_trans(skb, priv->dev);

The metadata stays at [old_data - metasize, old_data), but mac_header is
set to old_data + VLAN_HLEN. skb_metadata_end() returns skb_mac_header(),
so the metadata window seen by TC BPF and the stack is shifted by 4
bytes.

Wouldn't that drop the first 4 bytes the XDP program wrote, and return
the first 4 bytes of the destination MAC as the last 4 bytes of
metadata?

The next patch in the series, "net: stmmac: fix VLAN tag stripping on
short and non-linear RX frames", changes stmmac_rx_vlan() to move
meta_len + ETH_ALEN * 2 bytes starting at skb->data - meta_len. That
fixes this by the end of the series.

Until then, this commit is a bisection window. A stable backport that
takes this patch without the next one would also have the problem.
Would it make sense to reorder the two patches? Another option is to
reword the commit message so it doesn't depend on a change that hasn't
landed yet.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com