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

[Jakub Sitnicki <jakub@cloudflare.com>]

On Sat, Jun 20, 2026 at 05:09 PM GMT, sashiko-bot@kernel.org wrote:
> 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?

For skb to have metadata we had to go throuhg ip6?_forward to get to LWT
output, so as I mentioned earlier - we don't need to unclone here
because ip6?_forward will skb_cow, which already unclones.

>
>>  		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?

Same as above.

>
>>  		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()?

This is valid. For encaps which use LWT input, we need to unclone, as
this happens before ip6?_forward.

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