[PATCH net 0/2] geneve, vxlan: Don't set exceptions if skb->len < mtu

This series fixes the exception abuse described in 2/2, and 1/2
is just a preparatory change to make 2/2 less ugly.

Stefano Brivio (2):
  geneve, vxlan: Don't check skb_dst() twice
  geneve, vxlan: Don't set exceptions if skb->len < mtu

 drivers/net/geneve.c | 14 +++-----------
 drivers/net/vxlan.c  | 12 ++----------
 include/net/dst.h    | 10 ++++++++++
 3 files changed, 15 insertions(+), 21 deletions(-)

-- 
2.19.1

Commit f15ca723c1eb ("net: don't call update_pmtu unconditionally") avoids
that we try updating PMTU for a non-existent destination, but didn't clean
up cases where the check was already explicit. Drop those redundant checks.

Signed-off-by: Stefano Brivio <redacted>
Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/geneve.c | 15 ++++-----------
 drivers/net/vxlan.c  | 12 ++----------
 2 files changed, 6 insertions(+), 21 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 6acb6b5718b9..61c4bfbeb41c 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c

@@ -830,12 +830,8 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	if (IS_ERR(rt))
 		return PTR_ERR(rt);
 
-	if (skb_dst(skb)) {
-		int mtu = dst_mtu(&rt->dst) - GENEVE_IPV4_HLEN -
-			  info->options_len;
-
-		skb_dst_update_pmtu(skb, mtu);
-	}
+	skb_dst_update_pmtu(skb, dst_mtu(&rt->dst) -
+				 GENEVE_IPV4_HLEN - info->options_len);
 
 	sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true);
 	if (geneve->collect_md) {

@@ -876,11 +872,8 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	if (IS_ERR(dst))
 		return PTR_ERR(dst);
 
-	if (skb_dst(skb)) {
-		int mtu = dst_mtu(dst) - GENEVE_IPV6_HLEN - info->options_len;
-
-		skb_dst_update_pmtu(skb, mtu);
-	}
+	skb_dst_update_pmtu(skb, dst_mtu(dst) -
+				 GENEVE_IPV6_HLEN - info->options_len);
 
 	sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true);
 	if (geneve->collect_md) {

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 2b8da2b7e721..22e0ce592e07 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c

@@ -2194,11 +2194,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		}
 
 		ndst = &rt->dst;
-		if (skb_dst(skb)) {
-			int mtu = dst_mtu(ndst) - VXLAN_HEADROOM;
-
-			skb_dst_update_pmtu(skb, mtu);
-		}
+		skb_dst_update_pmtu(skb, dst_mtu(ndst) - VXLAN_HEADROOM);
 
 		tos = ip_tunnel_ecn_encap(tos, old_iph, skb);
 		ttl = ttl ? : ip4_dst_hoplimit(&rt->dst);

@@ -2235,11 +2231,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 				goto out_unlock;
 		}
 
-		if (skb_dst(skb)) {
-			int mtu = dst_mtu(ndst) - VXLAN6_HEADROOM;
-
-			skb_dst_update_pmtu(skb, mtu);
-		}
+		skb_dst_update_pmtu(skb, dst_mtu(ndst) - VXLAN6_HEADROOM);
 
 		tos = ip_tunnel_ecn_encap(tos, old_iph, skb);
 		ttl = ttl ? : ip6_dst_hoplimit(ndst);

-- 
2.19.1

We shouldn't abuse exceptions: if the destination MTU is already higher
than what we're transmitting, no exception should be created.

Fixes: 52a589d51f10 ("geneve: update skb dst pmtu on tx path")
Fixes: a93bf0ff4490 ("vxlan: update skb dst pmtu on tx path")
Signed-off-by: Stefano Brivio <redacted>
Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/geneve.c |  7 +++----
 drivers/net/vxlan.c  |  4 ++--
 include/net/dst.h    | 10 ++++++++++
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 61c4bfbeb41c..493cd382b8aa 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c

@@ -830,8 +830,8 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	if (IS_ERR(rt))
 		return PTR_ERR(rt);
 
-	skb_dst_update_pmtu(skb, dst_mtu(&rt->dst) -
-				 GENEVE_IPV4_HLEN - info->options_len);
+	skb_tunnel_check_pmtu(skb, &rt->dst,
+			      GENEVE_IPV4_HLEN + info->options_len);
 
 	sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true);
 	if (geneve->collect_md) {

@@ -872,8 +872,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	if (IS_ERR(dst))
 		return PTR_ERR(dst);
 
-	skb_dst_update_pmtu(skb, dst_mtu(dst) -
-				 GENEVE_IPV6_HLEN - info->options_len);
+	skb_tunnel_check_pmtu(skb, dst, GENEVE_IPV6_HLEN + info->options_len);
 
 	sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true);
 	if (geneve->collect_md) {

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 22e0ce592e07..27bd586b94b0 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c

@@ -2194,7 +2194,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		}
 
 		ndst = &rt->dst;
-		skb_dst_update_pmtu(skb, dst_mtu(ndst) - VXLAN_HEADROOM);
+		skb_tunnel_check_pmtu(skb, ndst, VXLAN_HEADROOM);
 
 		tos = ip_tunnel_ecn_encap(tos, old_iph, skb);
 		ttl = ttl ? : ip4_dst_hoplimit(&rt->dst);

@@ -2231,7 +2231,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 				goto out_unlock;
 		}
 
-		skb_dst_update_pmtu(skb, dst_mtu(ndst) - VXLAN6_HEADROOM);
+		skb_tunnel_check_pmtu(skb, ndst, VXLAN6_HEADROOM);
 
 		tos = ip_tunnel_ecn_encap(tos, old_iph, skb);
 		ttl = ttl ? : ip6_dst_hoplimit(ndst);

diff --git a/include/net/dst.h b/include/net/dst.h
index 7f735e76ca73..6cf0870414c7 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h

@@ -527,4 +527,14 @@ static inline void skb_dst_update_pmtu(struct sk_buff *skb, u32 mtu)
 		dst->ops->update_pmtu(dst, NULL, skb, mtu);
 }
 
+static inline void skb_tunnel_check_pmtu(struct sk_buff *skb,
+					 struct dst_entry *encap_dst,
+					 int headroom)
+{
+	u32 encap_mtu = dst_mtu(encap_dst);
+
+	if (skb->len > encap_mtu - headroom)
+		skb_dst_update_pmtu(skb, encap_mtu - headroom);
+}
+
 #endif /* _NET_DST_H */

-- 
2.19.1

On Sat, Oct 13, 2018 at 6:54 AM Stefano Brivio [off-list ref] wrote:

We shouldn't abuse exceptions: if the destination MTU is already higher
than what we're transmitting, no exception should be created.

makes sense, shouldn't ip(6) tunnels also do this?

On Mon, 15 Oct 2018 15:01:31 +0900
Xin Long [off-list ref] wrote:

On Sat, Oct 13, 2018 at 6:54 AM Stefano Brivio [off-list ref] wrote:

quoted

We shouldn't abuse exceptions: if the destination MTU is already higher
than what we're transmitting, no exception should be created.  

makes sense, shouldn't ip(6) tunnels also do this?

I should probably have mentioned this in the cover letter: in theory
yes, but I'm doing this as preparation for ICMP handling in UDP
tunnels, and those will get selftests soon (once I'm done).

Writing extensive selftests for IP tunnels will take significantly
longer, so I'm not too confident to change this right now. I'd prefer
to address that at a later time.

-- 
Stefano

On Sat, Oct 13, 2018 at 6:54 AM Stefano Brivio [off-list ref] wrote:

This series fixes the exception abuse described in 2/2, and 1/2
is just a preparatory change to make 2/2 less ugly.

Stefano Brivio (2):
  geneve, vxlan: Don't check skb_dst() twice
  geneve, vxlan: Don't set exceptions if skb->len < mtu

 drivers/net/geneve.c | 14 +++-----------
 drivers/net/vxlan.c  | 12 ++----------
 include/net/dst.h    | 10 ++++++++++
 3 files changed, 15 insertions(+), 21 deletions(-)

--
2.19.1

Series Reviewed-by: Xin Long [off-list ref]

Le 12/10/2018 à 23:53, Stefano Brivio a écrit :

Commit f15ca723c1eb ("net: don't call update_pmtu unconditionally") avoids
that we try updating PMTU for a non-existent destination, but didn't clean
up cases where the check was already explicit. Drop those redundant checks.

Yes, I leave them to avoid calculating the new mtu value when not needed. We are
in the xmit path.
As skb_dst_update_pmtu() is inlined, we probably don't care, but gcc could still
decide to not inline it.


Regards,
Nicolas

On Mon, 15 Oct 2018 12:19:41 +0200
Nicolas Dichtel [off-list ref] wrote:

Le 12/10/2018 à 23:53, Stefano Brivio a écrit :

quoted

Commit f15ca723c1eb ("net: don't call update_pmtu unconditionally") avoids
that we try updating PMTU for a non-existent destination, but didn't clean
up cases where the check was already explicit. Drop those redundant checks.  

Yes, I leave them to avoid calculating the new mtu value when not needed. We are
in the xmit path.

Before 2/2 of this series, though, we call skb_dst_update_pmtu() (and
in turn dst->ops->update_pmtu()) for *every* packet with a dst, which
I'd dare saying is by far the most common case. Besides, 2/2 needs
anyway to calculate the MTU to fix a bug.

So I think this is a vast improvement overall.

If we want to improve this further and avoid any indirect calls in the
most common path, we would need to cache the MTU in the dst -- it's
probably doable, but I would fix the specific issue addressed by 2/2
first.

-- 
Stefano

Le 15/10/2018 à 13:08, Stefano Brivio a écrit :

On Mon, 15 Oct 2018 12:19:41 +0200
Nicolas Dichtel [off-list ref] wrote:

quoted

Le 12/10/2018 à 23:53, Stefano Brivio a écrit :

quoted

Commit f15ca723c1eb ("net: don't call update_pmtu unconditionally") avoids
that we try updating PMTU for a non-existent destination, but didn't clean
up cases where the check was already explicit. Drop those redundant checks.  

Yes, I leave them to avoid calculating the new mtu value when not needed. We are
in the xmit path.

Before 2/2 of this series, though, we call skb_dst_update_pmtu() (and
in turn dst->ops->update_pmtu()) for *every* packet with a dst, which

Not if dst is of type md_dst_ops.

I'd dare saying is by far the most common case. Besides, 2/2 needs
anyway to calculate the MTU to fix a bug.

So I think this is a vast improvement overall.

Fair point.

From: Stefano Brivio <redacted>
Date: Fri, 12 Oct 2018 23:53:57 +0200

This series fixes the exception abuse described in 2/2, and 1/2
is just a preparatory change to make 2/2 less ugly.

Series applied.