Re: [PATCH net v3 1/2] net: lwtunnel: Drop skb metadata before LWT encapsulation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Calling `skb_metadata_clear(skb)` on a potentially cloned SKB modifies the shared `skb_shinfo` structure without exclusive access, leading to a data race and silent data corruption for other clones.
--

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

net: lwtunnel: Drop skb metadata before LWT encapsulation

This commit clears skb metadata before lwt encapsulation across all dispatch
helpers. It prevents non-bpf lwt encapsulations from overwriting metadata in
the headroom and stops bpf lwt xmit from triggering warnings due to metadata
position during skb_data_move().

> diff --git a/net/core/lwtunnel.c b/net/core/lwtunnel.c
> index f9d76d85d04fd..b01a395d9a966 100644
> --- a/net/core/lwtunnel.c
> +++ b/net/core/lwtunnel.c
> @@ -350,6 +350,8 @@ int lwtunnel_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>  	rcu_read_lock();
>  	ops = rcu_dereference(lwtun_encaps[lwtstate->type]);
>  	if (likely(ops && ops->output)) {
> +		/* Encap pushes outer headers over the metadata; drop it. */
> +		skb_metadata_clear(skb);

[Severity: High]
Does calling skb_metadata_clear() here modify the shared skb_shinfo
structure without ensuring exclusive access?

Since skb_metadata_clear() modifies skb_shinfo(skb)->meta_len, if the
buffer is cloned (for instance, via tc mirred or multicast forwarding),
this might cause a data race and corrupt the metadata for other clones still
processing the packet.

Should we unshare the skb first to safely modify the shared info?

>  		dev_xmit_recursion_inc();
>  		ret = ops->output(net, sk, skb);
>  		dev_xmit_recursion_dec();

[ ... ]

> @@ -404,6 +406,8 @@ int lwtunnel_xmit(struct sk_buff *skb)
>  	rcu_read_lock();
>  	ops = rcu_dereference(lwtun_encaps[lwtstate->type]);
>  	if (likely(ops && ops->xmit)) {
> +		/* Encap pushes outer headers over the metadata; drop it. */
> +		skb_metadata_clear(skb);

[Severity: High]
Could this similarly corrupt shared metadata if the buffer is cloned?

>  		dev_xmit_recursion_inc();
>  		ret = ops->xmit(skb);
>  		dev_xmit_recursion_dec();

[ ... ]

> @@ -455,6 +459,8 @@ int lwtunnel_input(struct sk_buff *skb)
>  	rcu_read_lock();
>  	ops = rcu_dereference(lwtun_encaps[lwtstate->type]);
>  	if (likely(ops && ops->input)) {
> +		/* Encap pushes outer headers over the metadata; drop it. */
> +		skb_metadata_clear(skb);

[Severity: High]
Could this also lead to a data race by modifying skb_shinfo on a potentially
shared buffer before handing it to ops->input()?

>  		dev_xmit_recursion_inc();
>  		ret = ops->input(skb);
>  		dev_xmit_recursion_dec();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619-bpf-lwt-drop-skb-metadata-v3-0-71d6a33ab76b@cloudflare.com?part=1