| Message ID | 20190207195416.27082-1-willemdebruijn.kernel@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] bpf: only adjust gso_size on bytestream protocols | expand |
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.
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. :/ Thanks, Daniel
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.
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 95d25b010a257..5a7a8b93a5abf 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -4212,6 +4212,12 @@ static inline bool skb_is_gso_sctp(const struct sk_buff *skb) return skb_shinfo(skb)->gso_type & SKB_GSO_SCTP; } +static inline bool skb_is_gso_tcp(const struct sk_buff *skb) +{ + return skb_is_gso(skb) && + skb_shinfo(skb)->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6); +} + static inline void skb_gso_reset(struct sk_buff *skb) { skb_shinfo(skb)->gso_size = 0; diff --git a/net/core/filter.c b/net/core/filter.c index 7a54dc11ac2d3..f7d0004fc1609 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -2789,8 +2789,7 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb) u32 off = skb_mac_header_len(skb); int ret; - /* SCTP uses GSO_BY_FRAGS, thus cannot adjust it. */ - if (skb_is_gso(skb) && unlikely(skb_is_gso_sctp(skb))) + if (!skb_is_gso_tcp(skb)) return -ENOTSUPP; ret = skb_cow(skb, len_diff); @@ -2831,8 +2830,7 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb) u32 off = skb_mac_header_len(skb); int ret; - /* SCTP uses GSO_BY_FRAGS, thus cannot adjust it. */ - if (skb_is_gso(skb) && unlikely(skb_is_gso_sctp(skb))) + if (!skb_is_gso_tcp(skb)) return -ENOTSUPP; ret = skb_unclone(skb, GFP_ATOMIC); @@ -2957,8 +2955,7 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 len_diff) u32 off = skb_mac_header_len(skb) + bpf_skb_net_base_len(skb); int ret; - /* SCTP uses GSO_BY_FRAGS, thus cannot adjust it. */ - if (skb_is_gso(skb) && unlikely(skb_is_gso_sctp(skb))) + if (!skb_is_gso_tcp(skb)) return -ENOTSUPP; ret = skb_cow(skb, len_diff); @@ -2987,8 +2984,7 @@ static int bpf_skb_net_shrink(struct sk_buff *skb, u32 len_diff) u32 off = skb_mac_header_len(skb) + bpf_skb_net_base_len(skb); int ret; - /* SCTP uses GSO_BY_FRAGS, thus cannot adjust it. */ - if (skb_is_gso(skb) && unlikely(skb_is_gso_sctp(skb))) + if (!skb_is_gso_tcp(skb)) return -ENOTSUPP; ret = skb_unclone(skb, GFP_ATOMIC);