Re: [PATCH net 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: 3746 bytes --]

On Oct 27, 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] 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.

I agree, I will reword the commit message in v2.

Regards,
Lorenzo

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

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