geneve, vxlan: Don't set exceptions if skb->len < mtu

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 <[email protected]>

Reviewed-by Sabrina Dubroca <[email protected]>

---
 drivers/net/geneve.c |  7 +++----
 drivers/net/vxlan.c  |  4 ++--
 include/net/dst.h    | 10 ++++++++++
 3 files changed, 15 insertions(+), 6 deletions(-)

drivers/net/geneve.c +3 -4

index 61c4bfbeb41c..493cd382b8aa 100644
@@ -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) {

drivers/net/vxlan.c +2 -2

index 22e0ce592e07..27bd586b94b0 100644
@@ -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);

include/net/dst.h +10 -1

index 7f735e76ca73..6cf0870414c7 100644
@@ -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

Quoted 3 lines · Stefano Brivio

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?

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.

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

Quoted 3 lines

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.

Quoted 1 line · On Mon, 15 Oct 2018 12:19:41 +0200 Nicolas Dichtel <[email protected]>…

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

Quoted 5 lines

Quoted 3 lines

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.

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