Re: [PATCH bpf] bpf: only adjust gso_size on bytestream protocols

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Network Development <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Peter Oskolkov <posk.devel@gmail.com>,
	Daniel Axtens <dja@axtens.net>,
	Willem de Bruijn <willemb@google.com>
Subject: Re: [PATCH bpf] bpf: only adjust gso_size on bytestream protocols
Date: Mon, 11 Feb 2019 15:11:30 -0500	[thread overview]
Message-ID: <CAF=yD-Kkcfz6xcMKu28b4nZEmv_k7NV3yGspfM3WY69P9rqXMg@mail.gmail.com> (raw)
In-Reply-To: <25e33f72-b04a-3229-ce8e-2320d29b8ee6@iogearbox.net>

On Mon, Feb 11, 2019 at 9:58 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Hi Willem,
>
> On 02/11/2019 05:00 AM, Alexei Starovoitov wrote:
> > On Thu, Feb 07, 2019 at 02:54:16PM -0500, Willem de Bruijn wrote:
> >> From: Willem de Bruijn <willemb@google.com>
> >>
> >> bpf_skb_change_proto and bpf_skb_adjust_room change skb header length.
> >> For GSO packets they adjust gso_size to maintain the same MTU.
> >>
> >> The gso size can only be safely adjusted on bytestream protocols.
> >> Commit d02f51cbcf12 ("bpf: fix bpf_skb_adjust_net/bpf_skb_proto_xlat
> >> to deal with gso sctp skbs") excluded SKB_GSO_SCTP.
> >>
> >> Since then type SKB_GSO_UDP_L4 has been added, whose contents are one
> >> gso_size unit per datagram. Also exclude these.
> >>
> >> Move from a blacklist to a whitelist check to future proof against
> >> additional such new GSO types, e.g., for fraglist based GRO.
> >>
> >> Fixes: bec1f6f69736 ("udp: generate gso with UDP_SEGMENT")
> >> Signed-off-by: Willem de Bruijn <willemb@google.com>
> >
> > Applied to bpf tree.
> > I agree that whitelist approach is the most appropriate.
>
> What would be needed to get UDP GSO working with nat64 work above? I don't
> really mind about SCTP, but sucks that this doesn't guarantee full support
> for TCP *and* UDP at least. :/

The easy part is shrinking headers in bpf_skb_net_shrink and
bpf_skb_proto_6_to_4. Those are safe if they adjust gso_size only if
skb_is_gso_tcp(skb).

Growing headers, whether with nat64 or in-review BPF_LWT_ENCAP_IP, is
fine if the original gso_size was chosen sufficiently below MSS to
account for the possible transformation.

Though this is not so cheap to verify here. But the same MTU concern
exists for non-GSO packets. Those are also adjusted unconditionally,
as far as I can tell. We do not need to add an MTU check solely for GSO.

For both GSO and non-GSO, for egress transformation, an admin
inserting such BPF programs can at least add an explicit route mtu to
force processes to limit the size they generate. Analogous to how tunnel
devices derive their mtu from their destination device minus encap headers.