Message-ID: <87tt2cr8eb.fsf@cloudflare.com>
Date: Tue, 12 Aug 2025 15:12:28 +0200
From: Jakub Sitnicki <jakub@...udflare.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: Alexei Starovoitov <ast@...nel.org>,  Andrii Nakryiko
 <andrii@...nel.org>,  Arthur Fabre <arthur@...hurfabre.com>,  Daniel
 Borkmann <daniel@...earbox.net>,  Eduard Zingerman <eddyz87@...il.com>,
  Eric Dumazet <edumazet@...gle.com>,  Jakub Kicinski <kuba@...nel.org>,
  Jesper Dangaard Brouer <hawk@...nel.org>,  Jesse Brandeburg
 <jbrandeburg@...udflare.com>,  Joanne Koong <joannelkoong@...il.com>,
  Lorenzo Bianconi <lorenzo@...nel.org>,  Toke Høiland-Jørgensen
 <thoiland@...hat.com>,  Yan Zhai <yan@...udflare.com>,
  kernel-team@...udflare.com,  netdev@...r.kernel.org,
  bpf@...r.kernel.org,  Stanislav Fomichev <sdf@...ichev.me>
Subject: Re: [PATCH bpf-next v6 9/9] selftests/bpf: Cover metadata access
 from a modified skb clone

On Fri, Aug 08, 2025 at 02:31 PM -07, Martin KaFai Lau wrote:
> 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.

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;
		if (dynptr_type == BPF_DYNPTR_TYPE_SKB_META)
			env->seen_direct_write = true;

		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.

Powered by blists - more mailing lists