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

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

[Stefano Brivio]

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

[PATCH net 1/2] geneve, vxlan: Don't check skb_dst() twice

[Stefano Brivio]

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 <sbrivio@redhat.com>
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

Re: [PATCH net 1/2] geneve, vxlan: Don't check skb_dst() twice

[Nicolas Dichtel]

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

Re: [PATCH net 1/2] geneve, vxlan: Don't check skb_dst() twice

[Stefano Brivio]

On Mon, 15 Oct 2018 12:19:41 +0200
Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:

> 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.

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

Re: [PATCH net 1/2] geneve, vxlan: Don't check skb_dst() twice

[Nicolas Dichtel]

Le 15/10/2018 à 13:08, Stefano Brivio a écrit :
> On Mon, 15 Oct 2018 12:19:41 +0200
> Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
> 
>> 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.
> 
> 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.

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

[Stefano Brivio]

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 <sbrivio@redhat.com>
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

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

[Xin Long]

On Sat, Oct 13, 2018 at 6:54 AM Stefano Brivio <sbrivio@redhat.com> 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?

>
> 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 <sbrivio@redhat.com>
> 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
>

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

[Stefano Brivio]

On Mon, 15 Oct 2018 15:01:31 +0900
Xin Long <lucien.xin@gmail.com> wrote:

> On Sat, Oct 13, 2018 at 6:54 AM Stefano Brivio <sbrivio@redhat.com> 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?

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

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

[Xin Long]

On Sat, Oct 13, 2018 at 6:54 AM Stefano Brivio <sbrivio@redhat.com> 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 <lucien.xin@gmail.com>

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

[David Miller]

From: Stefano Brivio <sbrivio@redhat.com>
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.