Re: [PATCH net v2] net: Clear skb metadata in LWT BPF xmit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditionally calling `skb_metadata_clear()` in `bpf_xmit` causes a data race and corrupts metadata for cloned SKBs.
- [High] Uninitialized memory read and metadata corruption in non-BPF encapsulations and `bpf_input`.
--

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

net: Clear skb metadata in LWT BPF xmit

This commit clears the SKB metadata in the LWT BPF xmit path before
invoking the BPF program. It aims to prevent a warning in skb_data_move()
when bpf_skb_change_head() is called on packets with existing metadata and a
non-matching MAC header.

> Drop skb metadata in the LWT BPF xmit path, just before invoking the
> program. Doing it late, rather than in ip[6]_finish_output2(), preserves
> metadata for tc egress on the outgoing device when no LWT BPF program is
> attached - the only scenario where carrying tc-layer metadata through the
> IP output path could be meaningful. Non-BPF LWT encap types do not run
> helpers that touch metadata, so the BPF xmit op is the only site that needs
> the clear.

Is this assumption accurate for non-BPF encapsulations and LWT_IN programs?

When non-BPF encapsulations (like IPIP or SEG6) or LWT_IN programs use
skb_push() to prepend outer headers, they shift skb->data downwards but do
not migrate the metadata block.

Because the metadata pointer is implicitly calculated as skb->data - meta_len,
the new encapsulation header silently overwrites the original metadata memory.
The implicit metadata pointer shifts further into the uninitialized SKB
headroom.

When the packet subsequently reaches TC egress, won't BPF programs accessing
data_meta read uninitialized headroom memory instead of the preserved
metadata?

> diff --git a/net/core/lwt_bpf.c b/net/core/lwt_bpf.c
> index f71ef82a5f3d3..d26cb9bb81893 100644
> --- a/net/core/lwt_bpf.c
> +++ b/net/core/lwt_bpf.c
> @@ -284,6 +284,11 @@ static int bpf_xmit(struct sk_buff *skb)
>  		__be16 proto = skb->protocol;
>  		int ret;
>  
> +		/* LWT BPF programs cannot access skb metadata but can corrupt
> +		 * it with helpers that operate on skb headroom. Drop it.
> +		 */
> +		skb_metadata_clear(skb);

Since bpf_xmit() operates in the LWT IP output path which frequently processes
cloned SKBs (e.g., from multicast forwarding or TCP retransmits), is it safe
to call skb_metadata_clear() here without unsharing the SKB?

skb_metadata_clear() directly modifies skb_shinfo(skb)->meta_len. For cloned
SKBs, the skb_shinfo structure is shared across all clones. Modifying it
without gaining exclusive ownership first (such as via skb_unclone()) will
silently zero out the metadata length for all other clones of the same
packet, causing them to unexpectedly lose their metadata when processed on
concurrent, unrelated network paths.

> +
>  		ret = run_lwt_bpf(skb, &bpf->xmit, dst, CAN_REDIRECT);
>  		switch (ret) {
>  		case BPF_OK:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514-bpf-lwt-drop-skb-metadata-v2-1-458664edc2b5@cloudflare.com?part=1