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

[Jakub Sitnicki <jakub@cloudflare.com>]

On Tue, Aug 12, 2025 at 11:20 AM -07, Martin KaFai Lau wrote:
> On 8/12/25 6:12 AM, Jakub Sitnicki wrote:
>>> 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.
>> All makes sense.
>> To keep things simple and consistent, it would be best to have a single
>> unclone (bpf_try_make_writable) point caused by a write to metadata
>> through an skb clone.
>> Today, the unclone in the prologue can already be triggered by a write
>> to data_meta from a dead branch. Despite being useless, since
>> pskb_expand_head resets meta_len.
>> We also need the prologue unclone for bpf_dynptr_slice_rdwr created from
>> an skb_meta dynptr, because creating a slice does not invalidate packet
>> pointers by contract.
>> So I'm thinking it makes sense to unclone in the prologue if we see a
>> potential bpf_dynptr_write to skb_meta dynptr as well. This could be
>> done by tweaking check_helper_call to set the seen_direct_write flag:
>> static int check_helper_call(...)
>> {
>>          // ...
>>         	switch (func_id) {
>>          // ...
>> 	case BPF_FUNC_dynptr_write:
>> 	{
>>                  // ...
>> 		dynptr_type = dynptr_get_type(env, reg);
>>                  // ...
>> 		if (dynptr_type == BPF_DYNPTR_TYPE_SKB ||
>> 		    dynptr_type == BPF_DYNPTR_TYPE_SKB_META)
>> 			changes_data = true;
>
> This looks ok.
>
>> 		if (dynptr_type == BPF_DYNPTR_TYPE_SKB_META)
>> 			env->seen_direct_write = true;
>
> The "seen_direct_write = true;" addition will be gone from the verifier
> eventually when pskb_expand_head can keep the data_meta (?). Right, there are
> existing cases that the prologue call might be unnecessary. However, I don't
> think it should be the reason that it can set "seen_direct_write" on top of the
> "changes_data". I think it is confusing.
>
>> 		break;
>> 	}
>>          // ...
>> }
>> That would my the plan for the next iteration, if it sounds sensible.
>> As for keeping metadata intact past a pskb_expand_head call, on second
>> thought, I'd leave that for the next patch set, to keep the patch count
>> within single digits.
>
> If the plan is to make pskb_expand_head support the data_meta later, just set
> the rdonly bit in the bpf_dynptr_from_skb_meta now. Then the future
> pskb_expand_head change will be a clean change in netdev and filter.c, and no
> need to revert the "seen_direct_write" changes from the verifier.

I was planning to keep the "seen_direct_write" change once
pskb_expand_head is patched to preserve the metadata. That way
bpf_dynptr_write(&meta, ...) could remain just a memmove.

But to be fair, at this point I don't have the code worked out, so who
knows how things are going to eventually play out.

All right. Let me make the skb_meta dynptr read-only for skb clones for
now. That sounds like a clean cut compromise since it's a corner case
which we expect no one cares about at the moment.

Thanks for guidance.