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

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

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

On Oct 30, 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 · 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?

I guess the reported issue is fixed in the following patch: "net: stmmac: fix
VLAN tag stripping on short and non-linear RX frames".

Regards,
Lorenzo

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

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