Re: [PATCH bpf-next v2 11/15] selftests/bpf: Expect unclone to preserve skb metadata
From: Martin KaFai Lau <martin.lau@linux.dev>
Date: 2025-10-22 23:12:58
Also in:
bpf
On 10/19/25 5:45 AM, Jakub Sitnicki wrote:
quoted hunk ↗ jump to hunk
@@ -447,12 +448,14 @@ int clone_dynptr_empty_on_meta_slice_write(struct __sk_buff *ctx) /* * Check that skb_meta dynptr is read-only before prog writes to packet payload - * using dynptr_write helper. Applies only to cloned skbs. + * using dynptr_write helper, and becomes read-write afterwards. Applies only to + * cloned skbs. */ SEC("tc") -int clone_dynptr_rdonly_before_data_dynptr_write(struct __sk_buff *ctx) +int clone_dynptr_rdonly_before_data_dynptr_write_then_rw(struct __sk_buff *ctx) { struct bpf_dynptr data, meta; + __u8 meta_have[META_SIZE]; const struct ethhdr *eth; bpf_dynptr_from_skb(ctx, 0, &data);@@ -465,15 +468,23 @@ int clone_dynptr_rdonly_before_data_dynptr_write(struct __sk_buff *ctx) /* Expect read-only metadata before unclone */ bpf_dynptr_from_skb_meta(ctx, 0, &meta); - if (!bpf_dynptr_is_rdonly(&meta) || bpf_dynptr_size(&meta) != META_SIZE) + if (!bpf_dynptr_is_rdonly(&meta))
Can the bpf_dynptr_set_rdonly() be lifted from the bpf_dynptr_from_skb_meta()? iiuc, the remaining thing left should be handling a cloned skb in __bpf_dynptr_write()? The __bpf_skb_store_bytes() is using bpf_try_make_writable, so maybe something similar can be done for the BPF_DYNPTR_TYPE_SKB_META?
+ goto out; + + bpf_dynptr_read(meta_have, META_SIZE, &meta, 0, 0); + if (!check_metadata(meta_have)) goto out; /* Helper write to payload will unclone the packet */ bpf_dynptr_write(&data, offsetof(struct ethhdr, h_proto), "x", 1, 0); - /* Expect no metadata after unclone */ + /* Expect r/w metadata after unclone */ bpf_dynptr_from_skb_meta(ctx, 0, &meta); - if (bpf_dynptr_is_rdonly(&meta) || bpf_dynptr_size(&meta) != 0) + if (bpf_dynptr_is_rdonly(&meta))
then it does not have to rely on the bpf_dynptr_write(&data, ...) above to make the metadata writable. I have a high level question about the set. I assume the skb_data_move() in patch 2 will be useful in the future to preserve the metadata across the stack. Preserving the metadata across different tc progs (which this set does) is nice to have but it is not the end goal. Can you shed some light on the plan for building on top of this set?