xfrm6: Do not use xfrm_local_error for path MTU issues in tunnels

This change makes it so that we use icmpv6_send to report PMTU issues back into tunnels in the case that the resulting packet is larger than the MTU of the outgoing interface. Previously xfrm_local_error was being used in this case, however this was resulting in no changes, I suspect due to the fact that the tunnel itself was being kept out of the loop.

This patch fixes PMTU problems seen on ip6_vti tunnels and is based on the behavior seen if the socket was orphaned. Instead of requiring the socket to be orphaned this patch simply defaults to using icmpv6_send in the case that the frame came though a tunnel.

Signed-off-by Alexander Duyck <[email protected]>

Quoted 11 lines · Alexander Duyck

This change makes it so that we use icmpv6_send to report PMTU issues back into tunnels in the case that the resulting packet is larger than the MTU of the outgoing interface. Previously xfrm_local_error was being used in this case, however this was resulting in no changes, I suspect due to the fact that the tunnel itself was being kept out of the loop.

This patch fixes PMTU problems seen on ip6_vti tunnels and is based on the behavior seen if the socket was orphaned. Instead of requiring the socket to be orphaned this patch simply defaults to using icmpv6_send in the case that the frame came though a tunnel.

Quoted 1 line

Signed-off-by Alexander Duyck <[email protected]>

Does this still work with normal tunnel mode and identical inner and outer addresses? I recall we used to have a bug where in that situation the kernel would interpret the ICMP message as a reduction in outer MTU and thus resulting in a loop where the MTU keeps getting smaller.

This change makes it so that we use icmpv6_send to report PMTU issues back into tunnels in the case that the resulting packet is larger than the MTU of the outgoing interface. Previously xfrm_local_error was being used in this case, however this was resulting in no changes, I suspect due to the fact that the tunnel itself was being kept out of the loop.

This patch fixes PMTU problems seen on ip6_vti tunnels and is based on the behavior seen if the socket was orphaned. Instead of requiring the socket to be orphaned this patch simply defaults to using icmpv6_send in the case that the frame came though a tunnel.

Quoted 10 lines · Alexander Duyck

This change makes it so that we use icmpv6_send to report PMTU issues back into tunnels in the case that the resulting packet is larger than the MTU of the outgoing interface. Previously xfrm_local_error was being used in this case, however this was resulting in no changes, I suspect due to the fact that the tunnel itself was being kept out of the loop.

This patch fixes PMTU problems seen on ip6_vti tunnels and is based on the behavior seen if the socket was orphaned. Instead of requiring the socket to be orphaned this patch simply defaults to using icmpv6_send in the case that the frame came though a tunnel.

We can use icmpv6_send() just in the case that the packet was already transmitted by a tunnel device, otherwise we get the bug back that I mentioned in my other mail.

Not sure if we have something to know that the packet traversed a tunnel device. That's what I asked in the thread 'Looking for a lost patch'.

Quoted 11 lines · Steffen Klassert

Quoted 10 lines · Alexander Duyck

This change makes it so that we use icmpv6_send to report PMTU issues back into tunnels in the case that the resulting packet is larger than the MTU of the outgoing interface. Previously xfrm_local_error was being used in this case, however this was resulting in no changes, I suspect due to the fact that the tunnel itself was being kept out of the loop.

This patch fixes PMTU problems seen on ip6_vti tunnels and is based on the behavior seen if the socket was orphaned. Instead of requiring the socket to be orphaned this patch simply defaults to using icmpv6_send in the case that the frame came though a tunnel.

Quoted 7 lines

We can use icmpv6_send() just in the case that the packet was already transmitted by a tunnel device, otherwise we get the bug back that I mentioned in my other mail.

Not sure if we have something to know that the packet traversed a tunnel device. That's what I asked in the thread 'Looking for a lost patch'.

Okay I will try to do some more digging. From what I can tell right now it looks like my ping attempts are getting hung up on the xfrm_local_error in __xfrm6_output. I wonder if we couldn't somehow make use of the skb->cb to store a pointer to the tunnel that could be checked to determine if we are going through a VTI or not.

Quoted 16 lines · Alexander Duyck

Quoted 15 lines · Steffen Klassert

Quoted 10 lines · Alexander Duyck

This change makes it so that we use icmpv6_send to report PMTU issues back into tunnels in the case that the resulting packet is larger than the MTU of the outgoing interface. Previously xfrm_local_error was being used in this case, however this was resulting in no changes, I suspect due to the fact that the tunnel itself was being kept out of the loop.

This patch fixes PMTU problems seen on ip6_vti tunnels and is based on the behavior seen if the socket was orphaned. Instead of requiring the socket to be orphaned this patch simply defaults to using icmpv6_send in the case that the frame came though a tunnel.

We can use icmpv6_send() just in the case that the packet was already transmitted by a tunnel device, otherwise we get the bug back that I mentioned in my other mail.

Quoted 8 lines

Quoted 3 lines

Not sure if we have something to know that the packet traversed a tunnel device. That's what I asked in the thread 'Looking for a lost patch'.

Okay I will try to do some more digging. From what I can tell right now it looks like my ping attempts are getting hung up on the xfrm_local_error in __xfrm6_output. I wonder if we couldn't somehow make use of the skb->cb to store a pointer to the tunnel that could be checked to determine if we are going through a VTI or not.

Maybe it is as easy as the patch below, could you please test it?

Subject: [PATCH RFC] vti6: Add pmtu handling to vti6_xmit.

We currently rely on the PMTU discovery of xfrm. However if a packet is localy sent, the PMTU mechanism of xfrm tries to to local socket notification what might not work for applications like ping that don't check for this. So add pmtu handling to vti6_xmit to report MTU changes immediately.

Signed-off-by Steffen Klassert <[email protected]>

---
  net/ipv6/ip6_vti.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

net/ipv6/ip6_vti.c +3 -0

index ff3bd86..13cb771 100644
@@ -434,6 +434,7 @@ vti6_xmit(struct sk_buff *skb, struct net_device *dev, struct flowi *fl)
  	struct dst_entry *dst = skb_dst(skb);
  	struct net_device *tdev;
  	struct xfrm_state *x;
+	int mtu;
  	int err = -1;
 
  	if (!dst)

@@ -468,6 +469,15 @@ vti6_xmit(struct sk_buff *skb, struct net_device *dev, struct flowi *fl)
  	skb_dst_set(skb, dst);
  	skb->dev = skb_dst(skb)->dev;
 
+	mtu = dst_mtu(dst);
+	if (!skb->ignore_df && skb->len > mtu) {

Quoted 59 lines · Steffen Klassert

Quoted 16 lines · Alexander Duyck

Quoted 15 lines · Steffen Klassert

Quoted 10 lines · Alexander Duyck

This change makes it so that we use icmpv6_send to report PMTU issues back into tunnels in the case that the resulting packet is larger than the MTU of the outgoing interface. Previously xfrm_local_error was being used in this case, however this was resulting in no changes, I suspect due to the fact that the tunnel itself was being kept out of the loop.

This patch fixes PMTU problems seen on ip6_vti tunnels and is based on the behavior seen if the socket was orphaned. Instead of requiring the socket to be orphaned this patch simply defaults to using icmpv6_send in the case that the frame came though a tunnel.

We can use icmpv6_send() just in the case that the packet was already transmitted by a tunnel device, otherwise we get the bug back that I mentioned in my other mail.

Quoted 8 lines

Quoted 3 lines

Not sure if we have something to know that the packet traversed a tunnel device. That's what I asked in the thread 'Looking for a lost patch'.

Okay I will try to do some more digging. From what I can tell right now it looks like my ping attempts are getting hung up on the xfrm_local_error in __xfrm6_output. I wonder if we couldn't somehow make use of the skb->cb to store a pointer to the tunnel that could be checked to determine if we are going through a VTI or not.

Maybe it is as easy as the patch below, could you please test it?

Subject: [PATCH RFC] vti6: Add pmtu handling to vti6_xmit.

We currently rely on the PMTU discovery of xfrm. However if a packet is localy sent, the PMTU mechanism of xfrm tries to to local socket notification what might not work for applications like ping that don't check for this. So add pmtu handling to vti6_xmit to report MTU changes immediately.

Signed-off-by Steffen Klassert <[email protected]>

---
  net/ipv6/ip6_vti.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

net/ipv6/ip6_vti.c +2 -0

index ff3bd86..13cb771 100644
@@ -434,6 +434,7 @@ vti6_xmit(struct sk_buff *skb, struct net_device 
 *dev, struct flowi *fl)
      struct dst_entry *dst = skb_dst(skb);
      struct net_device *tdev;
      struct xfrm_state *x;
+    int mtu;
      int err = -1;
        if (!dst)

@@ -468,6 +469,15 @@ vti6_xmit(struct sk_buff *skb, struct net_device 
 *dev, struct flowi *fl)
      skb_dst_set(skb, dst);
      skb->dev = skb_dst(skb)->dev;
  +    mtu = dst_mtu(dst);
+    if (!skb->ignore_df && skb->len > mtu) {

net/ipv6/ip6_vti.c vti6_xmit(struct sk_buff *skb, struct net_device · line 483

Quoted 10 lines

net/ipv6/ip6_vti.c +7 -0

+        skb_dst(skb)->ops->update_pmtu(dst, NULL, skb, mtu);
+
+        icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
+
+        return -EMSGSIZE;
+    }
+
      err = dst_output(skb);
      if (net_xmit_eval(err) == 0) {
          struct pcpu_sw_netstats *tstats = this_cpu_ptr(dev->tstats);

That seems to be working for me. I'm able to ping and while the first packet fails the second one and all that follow make it through correctly after the ptmu update.

- Alex

From: Steffen Klassert <[email protected]>

We currently rely on the PMTU discovery of xfrm. However if a packet is localy sent, the PMTU mechanism of xfrm tries to to local socket notification what might not work for applications like ping that don't check for this. So add pmtu handling to vti6_xmit to report MTU changes immediately.

Signed-off-by Steffen Klassert <[email protected]>

Signed-off-by Alexander Duyck <[email protected]>

From: Steffen Klassert <steffen.klassert@xxxxxxxxxxx>

We currently rely on the PMTU discovery of xfrm. However if a packet is localy sent, the PMTU mechanism of xfrm tries to to local socket notification what might not work for applications like ping that don't check for this. So add pmtu handling to vti6_xmit to report MTU changes immediately.

Signed-off-by Steffen Klassert <steffen.klassert@xxxxxxxxxxx>

Signed-off-by Alexander Duyck <alexander.h.duyck@xxxxxxxxxx>

---

So this version is slightly modified to cover the IPv4 case in addition to the IPv6 case. With this patch I was able to run netperf over either an IPv4 or IPv6 address routed over the ip6_vti tunnel.

Quoted 4 lines

So this version is slightly modified to cover the IPv4 case in addition to the IPv6 case. With this patch I was able to run netperf over either an IPv4 or IPv6 address routed over the ip6_vti tunnel.

We have the same issue. When we do a local ping to a remote device over a v4 vti tunnel and an intermediate device has a low mtu, pmtu discovery reduces the route's pmtu, and ping fails because it does not handle the local error message generated by xfrm4_tunnel_check_size(). Your patch fixes our issue for v6 vti tunnels, but the issue still exists for v4 tunnels. Is there any particular reason this patch was not delivered for v4 tunnels too - i.e. in vti_xmit()?

Quoted 10 lines · Mark McKinstry

Quoted 3 lines

So this version is slightly modified to cover the IPv4 case in addition to the IPv6 case. With this patch I was able to run netperf over either an IPv4 or IPv6 address routed over the ip6_vti tunnel.

We have the same issue. When we do a local ping to a remote device over a v4 vti tunnel and an intermediate device has a low mtu, pmtu discovery reduces the route's pmtu, and ping fails because it does not handle the local error message generated by xfrm4_tunnel_check_size(). Your patch fixes our issue for v6 vti tunnels, but the issue still exists for v4 tunnels. Is there any particular reason this patch was not delivered for v4 tunnels too - i.e. in vti_xmit()?

I don't remember why we fixed it just for ipv6, we probably need a similar patch for ipv4.

Does the patch below help (compile tested only)?

Subject: [PATCH] vti: Add pmtu handling to vti_xmit.

We currently rely on the PMTU discovery of xfrm. However if a packet is localy sent, the PMTU mechanism of xfrm tries to to local socket notification what might not work for applications like ping that don't check for this. So add pmtu handling to vti_xmit to report MTU changes immediately.

Signed-off-by Steffen Klassert <[email protected]>

---
  net/ipv4/ip_vti.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)

net/ipv4/ip_vti.c +6 -0

index 5cf10b7..6862305 100644
@@ -156,6 +156,7 @@ static netdev_tx_t vti_xmit(struct sk_buff *skb, struct net_device *dev,
  	struct dst_entry *dst = skb_dst(skb);
  	struct net_device *tdev;	/* Device to other host */
  	int err;
+	int mtu;
 
  	if (!dst) {
  		dev->stats.tx_carrier_errors++;

@@ -196,6 +197,18 @@ static netdev_tx_t vti_xmit(struct sk_buff *skb, struct net_device *dev,
  	skb_dst_set(skb, dst);
  	skb->dev = skb_dst(skb)->dev;
 
+	mtu = dst_mtu(dst);
+	if (!skb->ignore_df && skb->len > mtu) {
+		skb_dst(skb)->ops->update_pmtu(dst, NULL, skb, mtu);
+		if (skb->protocol == htons(ETH_P_IP))
+			icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,

This patch fixes our issue, thanks. In our scenario the tunnel path MTU now gets updated so that subsequent large packets sent over the tunnel get fragmented correctly.

Quoted 3 lines · Mark McKinstry

This patch fixes our issue, thanks. In our scenario the tunnel path MTU now gets updated so that subsequent large packets sent over the tunnel get fragmented correctly.

I've applied this patch to the ipsec tree now.
Thanks for testing!

Quoted 6 lines · Steffen Klassert

Quoted 3 lines · Mark McKinstry

This patch fixes our issue, thanks. In our scenario the tunnel path MTU now gets updated so that subsequent large packets sent over the tunnel get fragmented correctly.

I've applied this patch to the ipsec tree now.
Thanks for testing!

I spoke too soon. Upon further testing with this patch we have found it causes
a skt buffer leak. This is problematic for us and can cause memory exhaustion in
one of our test scenarios that has an IPv4 IPsec tunnel over a PPP link. Also
the patch's -EMSGSIZE return value appears to be invalid because vti_xmit() should be returning a type netdev_tx_t (NETDEV_TX_OK etc). It looks to me that
this patch should really be doing a goto tx_error rather than doing an early return with -EMSGSIZE. This would result in the skt buffer being freed, NETDEV_TX_OK being returned (thus indicating vti_xmit() "took care of packet"),
and the tx_errors counter being incremented (which seems like a reasonable thing to do).

I think the original IPv6 patch probably has the same issues, and could be causing a DOS attack vulnerability in recent Linux releases.

Quoted 4 lines · Steffen Klassert

Quoted 3 lines · Mark McKinstry

This patch fixes our issue, thanks. In our scenario the tunnel path MTU now gets updated so that subsequent large packets sent over the tunnel get fragmented correctly.

Quoted 2 lines

I've applied this patch to the ipsec tree now.
Thanks for testing!

I spoke too soon. Upon further testing with this patch we have found it causes
a skt buffer leak. This is problematic for us and can cause memory exhaustion in
one of our test scenarios that has an IPv4 IPsec tunnel over a PPP link.

Quoted 5 lines · Mark McKinstry

Quoted 4 lines · Steffen Klassert

Quoted 3 lines · Mark McKinstry

This patch fixes our issue, thanks. In our scenario the tunnel path MTU now gets updated so that subsequent large packets sent over the tunnel get fragmented correctly.

Quoted 7 lines

Quoted 2 lines

I've applied this patch to the ipsec tree now.
Thanks for testing!

I spoke too soon. Upon further testing with this patch we have found it causes
a skt buffer leak. This is problematic for us and can cause memory exhaustion in
one of our test scenarios that has an IPv4 IPsec tunnel over a PPP link.

The patch below is what I plan to apply on top of the original patch.

Subject: [PATCH] vti: Fix recource leeks on pmtu discovery

A recent patch introduced pmtu handling directly in the vti transmit routine. Unfortunately we now return without releasing the dst_entry and freeing the sk_buff. This patch fixes the issue.

Fixes 325b71fe0f57 ("vti: Add pmtu handling to vti_xmit.")

Reported-by Mark McKinstry <[email protected]>

Signed-off-by Steffen Klassert <[email protected]>

---
  net/ipv4/ip_vti.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

net/ipv4/ip_vti.c +0 -0

index 6862305..2ea2b6e 100644

refdst_drop() > dst_release(). In our scenario your patch results in

Your patch adds a dst_release() call to my suggested fix, but this is problematic because the kfree_skb() call at tx_error already takes care of releasing dst - via kfree_skb() > __kfree_skb() > skb_release_all() > skb_release_head_state() > skb_dst_drop()

Quoted 1 line

refdst_drop() > dst_release(). In our scenario your patch results in

a negative refcount kernel warning being generated in dst_release() for every packet that is too big to go over the vti.

Quoted 7 lines · Mark McKinstry

Your patch adds a dst_release() call to my suggested fix, but this is problematic because the kfree_skb() call at tx_error already takes care of releasing dst - via kfree_skb() > __kfree_skb() > skb_release_all() > skb_release_head_state() > skb_dst_drop()

Quoted 1 line

refdst_drop() > dst_release(). In our scenario your patch results in

a negative refcount kernel warning being generated in dst_release() for every packet that is too big to go over the vti.

Hm. I've just noticed that my pmtu test does not trigger this codepath, so I did not see the warning.

Seems like we do the pmtu handling too late, it should happen before we do skb_dst_set(). Also skb_scrub_packet() resets skb->ignore_df, so checking ignore_df after skb_scrub_packet() does not make much sense.

I'll send an updated version after some more testing.

Quoted 8 lines · Steffen Klassert

Quoted 7 lines · Mark McKinstry

Your patch adds a dst_release() call to my suggested fix, but this is problematic because the kfree_skb() call at tx_error already takes care of releasing dst - via kfree_skb() > __kfree_skb() > skb_release_all() > skb_release_head_state() > skb_dst_drop()

Quoted 1 line

refdst_drop() > dst_release(). In our scenario your patch results in

a negative refcount kernel warning being generated in dst_release() for every packet that is too big to go over the vti.

Quoted 9 lines

Hm. I've just noticed that my pmtu test does not trigger this codepath, so I did not see the warning.

Seems like we do the pmtu handling too late, it should happen before we do skb_dst_set(). Also skb_scrub_packet() resets skb->ignore_df, so checking ignore_df after skb_scrub_packet() does not make much sense.

I'll send an updated version after some more testing.

I've added a testcase that triggers this codepath to my testing environment. The patch below works for me, could you please test if it fixes your problems?

Subject: [PATCH] vti: Add pmtu handling to vti_xmit.

We currently rely on the PMTU discovery of xfrm. However if a packet is locally sent, the PMTU mechanism of xfrm tries to do local socket notification what might not work for applications like ping that don't check for this. So add pmtu handling to vti_xmit to report MTU changes immediately.

Signed-off-by Steffen Klassert <[email protected]>

---
  net/ipv4/ip_vti.c | 18 ++++++++++++++++++
  1 file changed, 18 insertions(+)

net/ipv4/ip_vti.c +18 -0

index 5cf10b7..a917903 100644
@@ -156,6 +156,7 @@ static netdev_tx_t vti_xmit(struct sk_buff *skb, struct net_device *dev,
  	struct dst_entry *dst = skb_dst(skb);
  	struct net_device *tdev;	/* Device to other host */
  	int err;
+	int mtu;
 
  	if (!dst) {
  		dev->stats.tx_carrier_errors++;

@@ -192,6 +193,23 @@ static netdev_tx_t vti_xmit(struct sk_buff *skb, struct net_device *dev,
  			tunnel->err_count = 0;
  	}
 
+	mtu = dst_mtu(dst);
+	if (skb->len > mtu) {
+		skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
+		if (skb->protocol == htons(ETH_P_IP)) {
+			icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
+				  htonl(mtu));
+		} else {
+			if (mtu < IPV6_MIN_MTU)
+				mtu = IPV6_MIN_MTU;
+
+			icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
+		}
+
+		dst_release(dst);
+		goto tx_error;
+	}
+
  	skb_scrub_packet(skb, !net_eq(tunnel->net, dev_net(dev)));
  	skb_dst_set(skb, dst);
  	skb->dev = skb_dst(skb)->dev;

I've tested this patch in our scenario and I can confirm that it still fixes all of our issues.