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

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

[Willem de Bruijn]

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>
---
 include/linux/skbuff.h |  6 ++++++
 net/core/filter.c      | 12 ++++--------
 2 files changed, 10 insertions(+), 8 deletions(-)

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);
-- 
2.20.1.611.gfbb209baf1-goog

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

[Alexei Starovoitov]

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.

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

[Daniel Borkmann]

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

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

[Willem de Bruijn]

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.