Re: [PATCH bpf-next v6 9/9] selftests/bpf: Cover metadata access from a modified skb clone

[Martin KaFai Lau <martin.lau@linux.dev>]

On 8/8/25 4:41 AM, Jakub Sitnicki wrote:
> 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.

There is no write helper for the data_meta now. It must directly write to 
skb->data_meta, so data_meta is a read-only for a clone now. I guess the current 
use case is mostly for tc to read the data_meta immediately after the xdp prog 
has added it (fwiw, it is how we tried to use it also), so it is usually not a 
clone (?). Not even sure if it currently has a write use case considering, 1) 
there is no bpf_"skb"_adjust_meta, and 2) the upper layer cannot use it.

No strong opinion to either copy the metadata on a clone or set the dynptr 
rdonly for a clone. I am ok with either way.

A brain dump:
On one hand, it is hard to comment without visibility on how will it look like 
when data_meta can be preserved in the future, e.g. what may be the overhead but 
there is flags in bpf_dynptr_from_skb_meta and bpf_dynptr_write, so there is 
some flexibility. On the other hand, having a copy will be less surprise on the 
clone skb like what we have discovered in this and the earlier email thread but 
I suspect there is actually no write use case on the skb data_meta now.