On Thu, Aug 07, 2025 at 05:33 PM -07, Martin KaFai Lau wrote:
> On 8/4/25 5:52 AM, Jakub Sitnicki wrote:
>> +/* Check that skb_meta dynptr is empty */
>> +SEC("tc")
>> +int ing_cls_dynptr_empty(struct __sk_buff *ctx)
>> +{
>> + struct bpf_dynptr data, meta;
>> + struct ethhdr *eth;
>> +
>> + bpf_dynptr_from_skb(ctx, 0, &data);
>> + eth = bpf_dynptr_slice_rdwr(&data, 0, NULL, sizeof(*eth));
>
> If this is bpf_dynptr_slice() instead of bpf_dynptr_slice_rdwr() and...
>
>> + if (!eth)
>> + goto out;
>> + /* Ignore non-test packets */
>> + if (eth->h_proto != 0)
>> + goto out;
>> + /* Packet write to trigger unclone in prologue */
>> + eth->h_proto = 42;
>
> ... remove this eth->h_proto write.
>
> Then bpf_dynptr_write() will succeed. like,
>
> bpf_dynptr_from_skb(ctx, 0, &data);
> eth = bpf_dynptr_slice(&data, 0, NULL, sizeof(*eth));
> if (!eth)
> goto out;
>
> /* Ignore non-test packets */
> if (eth->h_proto != 0)
> goto out;
>
> bpf_dynptr_from_skb_meta(ctx, 0, &meta);
> /* Expect write to fail because skb is a clone. */
> err = bpf_dynptr_write(&meta, 0, (void *)eth, sizeof(*eth), 0);
>
> The bpf_dynptr_write for a skb dynptr will do the pskb_expand_head(). The
> skb_meta dynptr write is only a memmove. It probably can also do
> pskb_expand_head() and change it to keep the data_meta.
>
> Another option is to set the DYNPTR_RDONLY_BIT in bpf_dynptr_from_skb_meta() for
> a clone skb. This restriction can be removed in the future.
Ah, crap. Forgot that bpf_dynptr_write->bpf_skb_store_bytes calls
bpf_try_make_writable(skb) behind the scenes.
OK, so the head page copy for skb clone happens either in BPF prologue
or lazily inside bpf_dynptr_write() call today.
Best if I make it consistent for skb_meta from the start, no?
Happy to take a shot at tweaking pskb_expand_head() to keep the metadata
in tact, while at it.
>
>> +
>> + /* Expect no metadata */
>> + bpf_dynptr_from_skb_meta(ctx, 0, &meta);
>> + if (bpf_dynptr_size(&meta) > 0)
>> + goto out;
>> +
>> + test_pass = true;
>> +out:
>> + return TC_ACT_SHOT;
>> +}
