Re: [PATCH net v2 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] In this patch, stmmac_construct_skb_zc()
  (drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:5521-5525) copies…

--

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

This patch changes stmmac_construct_skb_zc() to copy the XDP metadata in
[xdp->data_meta, xdp->data) into the new skb together with the packet
data. The metadata sits right before skb->data, and its length 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 71858af6fd66d..d9fc2464a160a 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]
Does the metadata stay valid for VLAN tagged frames that are stripped in
software?

The commit message says:

  This keeps data_meta valid for the stack and TC BPF after eth_type_trans()
  anchors mac_header.

When priv->hw->hw_vlan_en is false, stmmac_dispatch_skb_zc() calls
stmmac_rx_vlan() after the skb is built and before eth_type_trans() runs:

stmmac_dispatch_skb_zc() {
    skb = stmmac_construct_skb_zc(ch, xdp);
    ...
    else
        /* Driver level stripping. */
        stmmac_rx_vlan(priv->dev, skb);
    skb->protocol = eth_type_trans(skb, priv->dev);
    ...
}

For an 802.1Q frame with NETIF_F_HW_VLAN_CTAG_RX set (or 802.1AD with
NETIF_F_HW_VLAN_STAG_RX), stmmac_rx_vlan() moves only the MAC addresses
and then pulls the tag:

stmmac_rx_vlan() {
    ...
    memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2);
    skb_pull(skb, VLAN_HLEN);
    ...
}

The metadata stays at [D - metasize, D), where D is the original frame
start. eth_type_trans() then sets mac_header to D + VLAN_HLEN. Since
skb_metadata_end() is skb_mac_header(), TC BPF and the stack would see
the window [D + 4 - metasize, D + 4).

Wouldn't that window lose the first 4 bytes of metadata and end with 4
stale destination MAC bytes?

This can happen with AF_XDP zero-copy, an XDP program that calls
bpf_xdp_adjust_meta() and returns XDP_PASS, and a VLAN tagged frame on a
core where hw_vlan_en is false. stmmac_dvr_probe() sets
NETIF_F_HW_VLAN_CTAG_RX for all cores but only sets hw_vlan_en for xmac
cores, so dwmac100/dwmac1000 use software stripping by default.

Before this patch the metadata was not copied at all, so this does not
make things worse than they were. A later patch in the same 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 before skb_pull(), which fixes this case.

Could the commit message mention that the VLAN stripping path is fixed
separately, or could the two patches be reordered?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027-stmmac-xdp-fixes-v2-0-10eae8a093a8%40oss.qualcomm.com