Netdev Archive on lore.kernel.org
 help / Atom feed
* [PATCH bpf] bpf: only adjust gso_size on bytestream protocols
@ 2019-02-07 19:54 Willem de Bruijn
  2019-02-11  4:00 ` Alexei Starovoitov
  0 siblings, 1 reply; 4+ messages in thread
From: Willem de Bruijn @ 2019-02-07 19:54 UTC (permalink / raw)
  To: netdev; +Cc: ast, daniel, posk.devel, dja, 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH bpf] bpf: only adjust gso_size on bytestream protocols
  2019-02-07 19:54 [PATCH bpf] bpf: only adjust gso_size on bytestream protocols Willem de Bruijn
@ 2019-02-11  4:00 ` Alexei Starovoitov
  2019-02-11 14:58   ` Daniel Borkmann
  0 siblings, 1 reply; 4+ messages in thread
From: Alexei Starovoitov @ 2019-02-11  4:00 UTC (permalink / raw)
  To: Willem de Bruijn; +Cc: netdev, ast, daniel, posk.devel, dja, Willem de Bruijn

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.


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH bpf] bpf: only adjust gso_size on bytestream protocols
  2019-02-11  4:00 ` Alexei Starovoitov
@ 2019-02-11 14:58   ` Daniel Borkmann
  2019-02-11 20:11     ` Willem de Bruijn
  0 siblings, 1 reply; 4+ messages in thread
From: Daniel Borkmann @ 2019-02-11 14:58 UTC (permalink / raw)
  To: Alexei Starovoitov, Willem de Bruijn
  Cc: netdev, ast, posk.devel, dja, Willem de Bruijn

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH bpf] bpf: only adjust gso_size on bytestream protocols
  2019-02-11 14:58   ` Daniel Borkmann
@ 2019-02-11 20:11     ` Willem de Bruijn
  0 siblings, 0 replies; 4+ messages in thread
From: Willem de Bruijn @ 2019-02-11 20:11 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Alexei Starovoitov, Network Development, Alexei Starovoitov,
	Peter Oskolkov, Daniel Axtens, 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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-07 19:54 [PATCH bpf] bpf: only adjust gso_size on bytestream protocols Willem de Bruijn
2019-02-11  4:00 ` Alexei Starovoitov
2019-02-11 14:58   ` Daniel Borkmann
2019-02-11 20:11     ` Willem de Bruijn

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox