Re: [PATCH net] net: Clear skb metadata before LWT xmit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Modifying `skb_shinfo(skb)->meta_len` via `skb_metadata_clear()` on a potentially cloned SKB without unsharing causes data races and shared memory corruption.
- [High] Unconditional clearing of SKB metadata breaks XDP-to-TC metadata passing for all forwarded packets.
--

commit 9bd10448be11f5fe388ec5590ce62a619d12c1d2
Author: Jakub Sitnicki <jakub@cloudflare.com>

net: Clear skb metadata before LWT xmit

This commit attempts to clear SKB metadata before calling into LWT xmit from
the IPv4 and IPv6 output paths. This is intended to prevent hidden RX
metadata from affecting LWT SKB mutation helpers.

> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index e4790cc7b5c2..3295e043310f 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -220,6 +220,8 @@ static int ip_finish_output2(struct net *net, struct sock *sk, struct sk_buff *s
>  			return -ENOMEM;
>  	}
>  
> +	skb_metadata_clear(skb);

Does this clear metadata for all forwarded packets, rather than just those
destined for LWT xmit?

By placing skb_metadata_clear() outside of the lwtunnel_xmit_redirect()
block, it appears XDP metadata is permanently stripped from all packets on
the output path. Could this break XDP-to-TC egress metadata passing
system-wide when these packets later reach dev_queue_xmit()?

> +
>  	if (lwtunnel_xmit_redirect(dst->lwtstate)) {
>  		int res = lwtunnel_xmit(skb);
>  

[ ... ]

> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 7e92909ab5be..734818463e07 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -111,6 +111,8 @@ static int ip6_finish_output2(struct net *net, struct sock *sk, struct sk_buff *
>  		}
>  	}
>  
> +	skb_metadata_clear(skb);

Can this modify the shared info on a cloned SKB without unsharing it first?

skb_metadata_clear() directly sets skb_shinfo(skb)->meta_len = 0. Earlier
in this function, multicast packets may be cloned:

net/ipv6/ip6_output.c:ip6_finish_output2() {
    ...
		if (!(dev->flags & IFF_LOOPBACK) && sk_mc_loop(sk) &&
		    ...
			struct sk_buff *newskb = skb_clone(skb, GFP_ATOMIC);

			if (newskb)
				NF_HOOK(NFPROTO_IPV6, NF_INET_POST_ROUTING,
    ...
}

If the SKB is cloned and being processed concurrently by NF_HOOK, would
writing to skb_shinfo(skb)->meta_len cause a data race and corrupt the
shared memory for the clone?

> +
>  	if (lwtunnel_xmit_redirect(dst->lwtstate)) {
>  		int res = lwtunnel_xmit(skb);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260428-wip-skb-local-storage-from-scratch-v1-1-8f7ca9b378ce@cloudflare.com?part=1