Skip to content

Make TC BPF helpers preserve skb metadata #10216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 16 commits into
base: bpf-next_base
Choose a base branch
from
Open

Make TC BPF helpers preserve skb metadata #10216

wants to merge 16 commits into from
+475 −183

Conversation

@kernel-patches-daemon-bpf
Copy link

@kernel-patches-daemon-bpf kernel-patches-daemon-bpf bot commented Nov 5, 2025

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

@kernel-patches-daemon-bpf
Copy link
Author

kernel-patches-daemon-bpf bot commented Nov 5, 2025

Upstream branch: 4cb4897
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1020081
version: 4

@kernel-patches-review-bot
Copy link

kernel-patches-review-bot bot commented Nov 5, 2025 

commit 9deb452eeba4e886c42cf2575265326b2571f34d
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.

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.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 96714eab9..370ddc61b 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3260,11 +3260,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);

The comment update says callers provide meta_len+len as headroom, but
is this true for all callers?

bpf_skb_proto_4_to_6() calls skb_cow(skb, len_diff) then calls
bpf_skb_net_hdr_push()->bpf_skb_generic_push(). Similarly,
bpf_lwt_seg6_adjust_srh() calls skb_cow_head(skb, len) then calls
bpf_skb_net_hdr_push()->bpf_skb_generic_push().

Neither accounts for meta_len in their headroom calculation. When
skb_postpush_data_move()->skb_data_move() runs, if meta_len >
skb_headroom(skb) after the push, the metadata gets silently cleared
(see the WARN_ON_ONCE check in skb_data_move() at skbuff.h).

Can the metadata be lost when these functions are called on skbs with
metadata?

> @@ -3496,6 +3496,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;
>
> @@ -3506,7 +3507,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/19115341225

@kernel-patches-daemon-bpf
Copy link
Author

kernel-patches-daemon-bpf bot commented Nov 5, 2025

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

@jsitnicki
net: Helper to move packet data and metadata after skb_push/pull
6a6ff81 
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]>
@jsitnicki
net: Preserve metadata on pskb_expand_head
cbb6a10 
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]>
@jsitnicki
bpf: Unclone skb head on bpf_dynptr_write to skb metadata
6b8cbaa 
Currently bpf_dynptr_from_skb_meta() marks the dynptr as read-only when
the skb is cloned, preventing writes to metadata.

Remove this restriction and unclone the skb head on bpf_dynptr_write() to
metadata, now that the metadata is preserved during uncloning. This makes
metadata dynptr consistent with skb dynptr, allowing writes regardless of
whether the skb is cloned.

Signed-off-by: Jakub Sitnicki <[email protected]>
@jsitnicki
vlan: Make vlan_remove_tag return nothing
eaeb4eb 
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]>
@jsitnicki
bpf: Make bpf_skb_vlan_pop helper metadata-safe
4e35ebf 
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]>
@jsitnicki
bpf: Make bpf_skb_vlan_push helper metadata-safe
56fbbcc 
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]>
@jsitnicki
bpf: Make bpf_skb_adjust_room metadata-safe
3b2d1fe 
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]>
@jsitnicki
bpf: Make bpf_skb_change_proto helper metadata-safe
ff37619 
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]>
@jsitnicki
bpf: Make bpf_skb_change_head helper metadata-safe
06b7e3c 
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]>
@jsitnicki
selftests/bpf: Verify skb metadata in BPF instead of userspace
e37d4ce 
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]>
@jsitnicki
selftests/bpf: Dump skb metadata on verification failure
603c002 
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]>
@jsitnicki
selftests/bpf: Expect unclone to preserve skb metadata
2f778b1 
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]>
@jsitnicki
selftests/bpf: Cover skb metadata access after vlan push/pop helper
f199148 
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]>
@kernel-patches-daemon-bpf
Copy link
Author

kernel-patches-daemon-bpf bot commented Nov 6, 2025

Upstream branch: b54a8e1
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1020081
version: 4

@jsitnicki
selftests/bpf: Cover skb metadata access after bpf_skb_adjust_room
818091f 
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]>
@jsitnicki
selftests/bpf: Cover skb metadata access after change_head/tail helper
98d7de3 
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]>
@jsitnicki
selftests/bpf: Cover skb metadata access after bpf_skb_change_proto
e77bc25 
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bpf-next new V4-ci-fail V4

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jsitnicki