Skip to content

Conversation

kernel-patches-daemon-bpf[bot]
Copy link

Pull request for series with
subject: Make TC BPF helpers preserve skb metadata
version: 2
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1013279

pskb_expand_head() copies headroom, including skb metadata, into the newly
allocated head, but then clears the metadata. As a result, metadata is lost
when BPF helpers trigger an skb head reallocation.

Let the skb metadata remain in the newly created copy of head.

Signed-off-by: Jakub Sitnicki <[email protected]>
Lay groundwork for fixing BPF helpers available to TC(X) programs.

When skb_push() or skb_pull() is called in a TC(X) ingress BPF program, the
skb metadata must be kept in front of the MAC header. Otherwise, BPF
programs using the __sk_buff->data_meta pseudo-pointer lose access to it.

Introduce a helper that moves both metadata and a specified number of
packet data bytes together, suitable as a drop-in replacement for
memmove().

Signed-off-by: Jakub Sitnicki <[email protected]>
All callers ignore the return value.

Prepare to reorder memmove() after skb_pull() which is a common pattern.

Signed-off-by: Jakub Sitnicki <[email protected]>
Use the metadata-aware helper to move packet bytes after skb_pull(),
ensuring metadata remains valid after calling the BPF helper.

Signed-off-by: Jakub Sitnicki <[email protected]>
Use the metadata-aware helper to move packet bytes after skb_push(),
ensuring metadata remains valid after calling the BPF helper.

Also, take care to reserve sufficient headroom for metadata to fit.

Signed-off-by: Jakub Sitnicki <[email protected]>
bpf_skb_adjust_room() may push or pull bytes from skb->data. In both cases,
skb metadata must be moved accordingly to stay accessible.

Replace existing memmove() calls, which only move payload, with a helper
that also handles metadata. Reserve enough space for metadata to fit after
skb_push.

Signed-off-by: Jakub Sitnicki <[email protected]>
bpf_skb_change_proto reuses the same headroom operations as
bpf_skb_adjust_room, already updated to handle metadata safely.

The remaining step is to ensure that there is sufficient headroom to
accommodate metadata on skb_push().

Signed-off-by: Jakub Sitnicki <[email protected]>
Although bpf_skb_change_head() doesn't move packet data after skb_push(),
skb metadata still needs to be relocated. Use the dedicated helper to
handle it.

Signed-off-by: Jakub Sitnicki <[email protected]>
Move metadata verification into the BPF TC programs. Previously,
userspace read metadata from a map and verified it once at test end.

Now TC programs compare metadata directly using __builtin_memcmp() and
set a test_pass flag. This enables verification at multiple points during
test execution rather than a single final check.

Signed-off-by: Jakub Sitnicki <[email protected]>
Add diagnostic output when metadata verification fails to help with
troubleshooting test failures. Introduce a check_metadata() helper that
prints both expected and received metadata to the BPF program's stderr
stream on mismatch. The userspace test reads and dumps this stream on
failure.

Signed-off-by: Jakub Sitnicki <[email protected]>
Since pskb_expand_head() no longer clears metadata on unclone, update tests
for cloned packets to expect metadata to remain intact.

Also simplify the clone_dynptr_kept_on_{data,meta}_slice_write tests.
Creating an r/w dynptr slice is sufficient to trigger an unclone in the
prologue, so remove the extraneous writes to the data/meta slice.

Signed-off-by: Jakub Sitnicki <[email protected]>
Add a test to verify that skb metadata remains accessible after calling
bpf_skb_vlan_push() and bpf_skb_vlan_pop(), which modify the packet
headroom.

Signed-off-by: Jakub Sitnicki <[email protected]>
Add a test to verify that skb metadata remains accessible after calling
bpf_skb_adjust_room(), which modifies the packet headroom and can trigger
head reallocation.

The helper expects an Ethernet frame carrying an IP packet so switch test
packet identification by source MAC address since we can no longer rely on
Ethernet proto being set to zero.

Signed-off-by: Jakub Sitnicki <[email protected]>
Add a test to verify that skb metadata remains accessible after calling
bpf_skb_change_head() and bpf_skb_change_tail(), which modify packet
headroom/tailroom and can trigger head reallocation.

Signed-off-by: Jakub Sitnicki <[email protected]>
Add a test to verify that skb metadata remains accessible after calling
bpf_skb_change_proto(), which modifies packet headroom to accommodate
different IP header sizes.

Signed-off-by: Jakub Sitnicki <[email protected]>
@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 7361c86
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1013279
version: 2

@kernel-patches-review-bot
Copy link

commit 75a8bedbf981b66deb77608d0bd258cdfe7866f5
Author: Jakub Sitnicki <[email protected]>

bpf: Make bpf_skb_adjust_room metadata-safe

bpf_skb_adjust_room() may push or pull bytes from skb->data. In both
cases, skb metadata must be moved accordingly to stay accessible. The
commit replaces memmove() with helpers that handle metadata.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 76628df1f..5e1a52694 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3253,11 +3253,11 @@ static void bpf_skb_change_protocol(struct sk_buff *skb, u16 proto)
>
>  static int bpf_skb_generic_push(struct sk_buff *skb, u32 off, u32 len)
>  {
> -	/* Caller already did skb_cow() with len as headroom,
> +	/* Caller already did skb_cow() with meta_len+len as headroom,
>  	 * so no need to do it here.
>  	 */
>  	skb_push(skb, len);
> -	memmove(skb->data, skb->data + len, off);
> +	skb_postpush_data_move(skb, len, off);
>  	memset(skb->data + off, 0, len);
>
>  	/* No skb_postpush_rcsum(skb, skb->data + off, len)

The updated comment indicates bpf_skb_generic_push() now requires callers
to reserve meta_len+len headroom. However, some callers were not updated:

1. bpf_skb_proto_4_to_6() calls skb_cow(skb, len_diff) without meta_len,
   then calls bpf_skb_net_hdr_push()->bpf_skb_generic_push().

2. bpf_lwt_seg6_adjust_srh() calls skb_cow_head(skb, len) without meta_len,
   then calls bpf_skb_net_hdr_push()->bpf_skb_generic_push().

When metadata exists (skb_metadata_len() > 0) and insufficient headroom is
reserved, skb_data_move() will detect the condition with WARN_ON_ONCE and
clear the metadata, causing data loss.

Can these call paths lose metadata? Should they reserve meta_len as well?

>  @@ -3281,7 +3281,7 @@ static int bpf_skb_generic_pop(struct sk_buff *skb, u32 off, u32 len)
>  	old_data = skb->data;
>  	__skb_pull(skb, len);
>  	skb_postpull_rcsum(skb, old_data + off, len);
> -	memmove(skb->data, old_data, off);
> +	skb_postpull_data_move(skb, len, off);
>
>  	return 0;
>  }
> @@ -3489,6 +3489,7 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
>  	u8 inner_mac_len = flags >> BPF_ADJ_ROOM_ENCAP_L2_SHIFT;
>  	bool encap = flags & BPF_F_ADJ_ROOM_ENCAP_L3_MASK;
>  	u16 mac_len = 0, inner_net = 0, inner_trans = 0;
> +	const u8 meta_len = skb_metadata_len(skb);
>  	unsigned int gso_type = SKB_GSO_DODGY;
>  	int ret;
>
> @@ -3499,7 +3500,7 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
>  			return -ENOTSUPP;
>  	}
>
> -	ret = skb_cow_head(skb, len_diff);
> +	ret = skb_cow_head(skb, meta_len + len_diff);
>  	if (unlikely(ret < 0))
>  		return ret;



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: bpf: Make bpf_skb_adjust_room metadata-safe
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/18630684133

@kernel-patches-daemon-bpf
Copy link
Author

Forwarding comment 3419651280 via email
In-Reply-To: [email protected]
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant