Re: [PATCH bpf-next v2 4/4] bpf: more lenient bpf_skb_net_shrink() with BPF_F_ADJ_ROOM_FIXED_GSO

From: "Maciej Żenczykowski" <maze@google.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Linux Network Development Mailing List  <netdev@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	BPF Mailing List <bpf@vger.kernel.org>,
	"David S . Miller" <davem@davemloft.net>,
	Willem de Bruijn <willemb@google.com>
Subject: Re: [PATCH bpf-next v2 4/4] bpf: more lenient bpf_skb_net_shrink() with BPF_F_ADJ_ROOM_FIXED_GSO
Date: Thu, 24 Jun 2021 10:13:50 -0700	[thread overview]
Message-ID: <CANP3RGeLUCQKTiu-tK8WuUO6LyLaP5hJmgbD8Km2J-XU5RMfZQ@mail.gmail.com> (raw)
In-Reply-To: <919e8f26-4b82-9d4c-8973-b2ab2b4bc5bf@iogearbox.net>

On Thu, Jun 24, 2021 at 7:05 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 6/17/21 2:09 AM, Maciej Żenczykowski wrote:
> > From: Maciej Żenczykowski <maze@google.com>
> >
> > This is to more closely match behaviour of bpf_skb_change_proto()
> > which now does not adjust gso_size, and thus thoretically supports
> > all gso types, and does not need to set SKB_GSO_DODGY nor reset
> > gso_segs to zero.
> >
> > Something similar should probably be done with bpf_skb_net_grow(),
> > but that code scares me.
>
> Took in all except this one, would be good to have a complete solution for
> both bpf_skb_net_{shrink,grow}(). If you don't have the cycles, I'll look
> into it.
>
> Thanks,
> Daniel

I very much don't understand all the complexities of all the different
encap/tunneling
stuff that is handled in ..._grow().  In principle I think changing
the gso_size is
probably a bad idea in general, but I'm not at all sure that's a
change we can make now,
without breaking backward compatibility with some userspace somewhere
(not Android
though, we don't currently use either of these helpers yet) or causing
other trouble.

I'd love it if there was some truly good documentation of how all the
fields/offloads
in an skb interact, as I find myself constantly having to figure this
out via code examination,
and never feel like I really truly understand things (or perhaps some
helper function that would
'validate' an skb as well formed, ideally in debug mode we could call
it both before and after
a bpf program mucks with things and check it still passes).
I'm not sure who would be the expert here... you? Willem? Tom? someone else?
As such I'll leave this up to one of you.

I sent the patch for ..._shrink() because that one seemed simple enough.
(I don't really understand why shrink is so much simpler than grow...)

What I will try to send you is an extension to 4<->6 protocol
conversion to deal with the extra
8 bytes of overhead in an ipv6 fragment (48 instead of 40 byte header
converted to/from 20 byte ipv4 frag header).
Though this isn't something I even have ready atm, it's just on a todo
list as a relatively unimportant thing.

- Maciej