Looking for a lost patch

PATCH Alexander Duyck 4 participants 11 messages May 13, 2015 – May 28, 2015

Subsystems (4)

Networking Network drivers IPv4/IPv6 Networking IPsec

lore

Share & tools

lore

Alexander Duyck

author

2015-05-13 17:47 UTC #0 lore raw

So I am in the process of trying to do some work on VTI6 and in the process of doing so I am trying to setup an IPv4 VTI tunnel and I have come across what appears to be a lost patch.

So in commit a32452366b72 ("vti4: Don't count header length twice.") the following change was made:

net/ipv4/ip_vti.c +0 -1

index 687ddef..cd62596 100644
@@ -349,7 +349,6 @@ static int vti_tunnel_init(struct net_device *dev)
 	memcpy(dev->broadcast, &iph->daddr, 4);
 
 	dev->type		= ARPHRD_TUNNEL;
-	dev->hard_header_len	= LL_MAX_HEADER + sizeof(struct iphdr);
 	dev->mtu		= ETH_DATA_LEN;
 	dev->flags		= IFF_NOARP;
 	dev->iflink		= 0;

However in commit f895f0cfbb77 ("Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec") the change appears to have been undone as a result of a merge commit.

I'm just wondering which is correct. Should the hard_header_len be set or unset in vti_tunnel_init? I ask because I have two kernels and one has the patch and one does not and I am seeing an MTU of 1332 for a VTI tunnel without, and 1480 for a VTI tunnel with.

- Alex

Steffen Klassert

maintainer

2015-05-18 07:38 UTC #1 lore raw

Quoted 20 lines · Alexander Duyck

So I am in the process of trying to do some work on VTI6 and in the process of doing so I am trying to setup an IPv4 VTI tunnel and I have come across what appears to be a lost patch.
So in commit a32452366b72 ("vti4: Don't count header length twice.") the following change was made:
net/ipv4/ip_vti.c +0 -1
index 687ddef..cd62596 100644
@@ -349,7 +349,6 @@ static int vti_tunnel_init(struct net_device *dev)
 	memcpy(dev->broadcast, &iph->daddr, 4);
 
 	dev->type		= ARPHRD_TUNNEL;
-	dev->hard_header_len	= LL_MAX_HEADER + sizeof(struct iphdr);
 	dev->mtu		= ETH_DATA_LEN;
 	dev->flags		= IFF_NOARP;
 	dev->iflink		= 0;

Quoted 8 lines

However in commit f895f0cfbb77 ("Merge branch 'master' of
git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec") the change appears to have been undone as a result of a merge commit.
I'm just wondering which is correct. Should the hard_header_len be set or unset in vti_tunnel_init? I ask because I have two kernels and one has the patch and one does not and I am seeing an MTU of 1332 for a VTI tunnel without, and 1480 for a VTI tunnel with.

A MTU of 1332 is definitively wrong. Actually I think a vti tunnel can have a MTU of 1500 because xfrm cares to calculate a PMTU based on the used states. The MTU of 1480 is because the generic ip_tunnel_bind_dev() assumes that an ip tunnel has always the overhead of an additional ip header. On IPsec this header is included in the PMTU calculation.

Alexander Duyck

2015-05-18 16:02 UTC #2 lore raw

Quoted 25 lines · Steffen Klassert

Quoted 24 lines · Alexander Duyck
So I am in the process of trying to do some work on VTI6 and in the process of doing so I am trying to setup an IPv4 VTI tunnel and I have come across what appears to be a lost patch.
So in commit a32452366b72 ("vti4: Don't count header length twice.") the following change was made:
net/ipv4/ip_vti.c +0 -1
index 687ddef..cd62596 100644
@@ -349,7 +349,6 @@ static int vti_tunnel_init(struct net_device *dev)
  	memcpy(dev->broadcast, &iph->daddr, 4);
 
  	dev->type		= ARPHRD_TUNNEL;
-	dev->hard_header_len	= LL_MAX_HEADER + sizeof(struct iphdr);
  	dev->mtu		= ETH_DATA_LEN;
  	dev->flags		= IFF_NOARP;
  	dev->iflink		= 0;
However in commit f895f0cfbb77 ("Merge branch 'master' of
git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec") the change appears to have been undone as a result of a merge commit.

Quoted 10 lines

Quoted 4 lines
I'm just wondering which is correct. Should the hard_header_len be set or unset in vti_tunnel_init? I ask because I have two kernels and one has the patch and one does not and I am seeing an MTU of 1332 for a VTI tunnel without, and 1480 for a VTI tunnel with.
A MTU of 1332 is definitively wrong. Actually I think a vti tunnel can have a MTU of 1500 because xfrm cares to calculate a PMTU based on the used states. The MTU of 1480 is because the generic ip_tunnel_bind_dev() assumes that an ip tunnel has always the overhead of an additional ip header. On IPsec this header is included in the PMTU calculation.

So if I understand correctly then is 1480 the correct MTU or should I be looking for some other value?

My initial though was to try and find the maximum overhead that can be generated for an IPv4/IPSec tunnel. However it seems like there isn't any solid documentation anywhere on what the upper limit is. I notice a number of references use either 1400 or 1412, however these tunnels appear to be using either an arbitrary value or a value that seems to also account for PPP and GRE overhead.

- Alex

Steffen Klassert

maintainer

2015-05-19 07:57 UTC #3 lore raw

Quoted 37 lines · Alexander Duyck

Quoted 25 lines · Steffen Klassert
Quoted 24 lines · Alexander Duyck
So I am in the process of trying to do some work on VTI6 and in the process of doing so I am trying to setup an IPv4 VTI tunnel and I have come across what appears to be a lost patch.
So in commit a32452366b72 ("vti4: Don't count header length twice.") the following change was made:
net/ipv4/ip_vti.c +0 -1
index 687ddef..cd62596 100644
@@ -349,7 +349,6 @@ static int vti_tunnel_init(struct net_device *dev)
 	memcpy(dev->broadcast, &iph->daddr, 4);
 
 	dev->type		= ARPHRD_TUNNEL;
-	dev->hard_header_len	= LL_MAX_HEADER + sizeof(struct iphdr);
 	dev->mtu		= ETH_DATA_LEN;
 	dev->flags		= IFF_NOARP;
 	dev->iflink		= 0;
However in commit f895f0cfbb77 ("Merge branch 'master' of
git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec") the change appears to have been undone as a result of a merge commit.
Quoted 10 lines
Quoted 4 lines
I'm just wondering which is correct. Should the hard_header_len be set or unset in vti_tunnel_init? I ask because I have two kernels and one has the patch and one does not and I am seeing an MTU of 1332 for a VTI tunnel without, and 1480 for a VTI tunnel with.
A MTU of 1332 is definitively wrong. Actually I think a vti tunnel can have a MTU of 1500 because xfrm cares to calculate a PMTU based on the used states. The MTU of 1480 is because the generic ip_tunnel_bind_dev() assumes that an ip tunnel has always the overhead of an additional ip header. On IPsec this header is included in the PMTU calculation.

Quoted 2 lines

So if I understand correctly then is 1480 the correct MTU or should I be looking for some other value?

The MTU should be 1500. All the IPsec overhead is handled by PMTU discovery, just like in the case we use IPsec without vti tunnels. The IPv6 side of vti does it like that.

Quoted 4 lines

My initial though was to try and find the maximum overhead that can be generated for an IPv4/IPSec tunnel. However it seems like there isn't any solid documentation anywhere on what the upper limit is.

There is no fixed upper limit on the overhead. The overhead also depends on the used crypto algorithm (IV size, chiper block size, ICV size etc.). That's why we handle this whith PMTU discovery. With this, each path can have it's own MTU based on the configured xfrm_state.

Alexander Duyck

2015-05-19 18:32 UTC #4 lore raw

Quoted 37 lines · Steffen Klassert

Quoted 36 lines · Alexander Duyck
Quoted 35 lines · Steffen Klassert
Quoted 20 lines · Alexander Duyck
So I am in the process of trying to do some work on VTI6 and in the process of doing so I am trying to setup an IPv4 VTI tunnel and I have come across what appears to be a lost patch.
So in commit a32452366b72 ("vti4: Don't count header length twice.") the following change was made:
net/ipv4/ip_vti.c +0 -1
index 687ddef..cd62596 100644
@@ -349,7 +349,6 @@ static int vti_tunnel_init(struct net_device *dev)
  	memcpy(dev->broadcast, &iph->daddr, 4);
 
  	dev->type		= ARPHRD_TUNNEL;
-	dev->hard_header_len	= LL_MAX_HEADER + sizeof(struct iphdr);
  	dev->mtu		= ETH_DATA_LEN;
  	dev->flags		= IFF_NOARP;
  	dev->iflink		= 0;
Quoted 8 lines
However in commit f895f0cfbb77 ("Merge branch 'master' of
git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec") the change appears to have been undone as a result of a merge commit.
I'm just wondering which is correct. Should the hard_header_len be set or unset in vti_tunnel_init? I ask because I have two kernels and one has the patch and one does not and I am seeing an MTU of 1332 for a VTI tunnel without, and 1480 for a VTI tunnel with.
A MTU of 1332 is definitively wrong. Actually I think a vti tunnel can have a MTU of 1500 because xfrm cares to calculate a PMTU based on the used states. The MTU of 1480 is because the generic ip_tunnel_bind_dev() assumes that an ip tunnel has always the overhead of an additional ip header. On IPsec this header is included in the PMTU calculation.

Quoted 5 lines

Quoted 2 lines
So if I understand correctly then is 1480 the correct MTU or should I be looking for some other value?
The MTU should be 1500. All the IPsec overhead is handled by PMTU discovery, just like in the case we use IPsec without vti tunnels. The IPv6 side of vti does it like that.

The problem is the PMTU isn't communicated to things that make use of the tunnel. For example if I do a "ping -s 2000 x.x.x.x" across an IPv6 VTI interface it will fail currently as it assumes the MTU is 1500 and so it is fragmenting the ping packet at sizes that won't be communicated across the underlying interface.

Quoted 7 lines

Quoted 3 lines
My initial though was to try and find the maximum overhead that can be generated for an IPv4/IPSec tunnel. However it seems like there isn't any solid documentation anywhere on what the upper limit is.
There is no fixed upper limit on the overhead. The overhead also depends on the used crypto algorithm (IV size, chiper block size, ICV size etc.). That's why we handle this whith PMTU discovery. With this, each path can have it's own MTU based on the configured xfrm_state.

My concern with all of this is that I plan to resubmit your original patch as the 1332 is a far smaller MTU than the tunnel actually needs, however I suspect we will then start receiving bugzilla's about the fragmentation being screwed up for things like UDP over the tunnel since the packets will be fragmented before they are handed off to the VTI, not after. That is why I was thinking it might be safer to determine what the maximum overhead could be for an IPSec tunnel and then I would use that to determine the MTU. I realize the value would be conservative, however I suspect it would still be a larger MTU than the interface is working with now.

As an example most of the Cisco descriptions for this end up suggesting an MTU of 1400 for the tunnels since that provides more than enough overhead for most tunnel combinations. What I would like to try and find is a similar value that would be a good fit for almost all configuration to avoid any fragmentation issues on the tunnel.

- Alex

Steffen Klassert

maintainer

2015-05-20 06:32 UTC #5 lore raw

Quoted 10 lines · Alexander Duyck

Quoted 3 lines · Steffen Klassert
The MTU should be 1500. All the IPsec overhead is handled by PMTU discovery, just like in the case we use IPsec without vti tunnels. The IPv6 side of vti does it like that.
The problem is the PMTU isn't communicated to things that make use of the tunnel. For example if I do a "ping -s 2000 x.x.x.x" across an IPv6 VTI interface it will fail currently as it assumes the MTU is 1500 and so it is fragmenting the ping packet at sizes that won't be communicated across the underlying interface.

Well, the problem is that the local socket is still attached on the skb. The socket gets an error notification if the packet is too big, but ping does not care much about these error notifications.

One option to get such applications to work is to orphan the skb in the vti xmit function. Then the packet is not assumed to be local, so PMTU discovery is triggered on that route.

Something like this should work for IPv6:

net/ipv6/ip6_vti.c +1 -0

index ed9d681..cd50bc1 100644
@@ -459,6 +459,7 @@ vti6_xmit(struct sk_buff *skb, struct net_device *dev, struct flowi *fl)
 		goto tx_err_dst_release;
 	}
 
+	skb_orphan(skb);
 	skb_scrub_packet(skb, !net_eq(t->net, dev_net(dev)));
 	skb_dst_set(skb, dst);
 	skb->dev = skb_dst(skb)->dev;

Alexander Duyck

author

2015-05-21 02:06 UTC #6 lore raw

Quoted 26 lines · Steffen Klassert

Quoted 9 lines · Alexander Duyck
Quoted 3 lines · Steffen Klassert
The MTU should be 1500. All the IPsec overhead is handled by PMTU discovery, just like in the case we use IPsec without vti tunnels. The IPv6 side of vti does it like that.
The problem is the PMTU isn't communicated to things that make use of the tunnel. For example if I do a "ping -s 2000 x.x.x.x" across an IPv6 VTI interface it will fail currently as it assumes the MTU is 1500 and so it is fragmenting the ping packet at sizes that won't be communicated across the underlying interface.
Well, the problem is that the local socket is still attached on the skb. The socket gets an error notification if the packet is too big, but ping does not care much about these error notifications.
One option to get such applications to work is to orphan the skb in the vti xmit function. Then the packet is not assumed to be local, so PMTU discovery is triggered on that route.
Something like this should work for IPv6:
net/ipv6/ip6_vti.c +0 -0
index ed9d681..cd50bc1 100644

net/ipv6/ip6_vti.c vti6_xmit(struct sk_buff *skb, struct net_device *dev, struct flowi *fl) · line 465

Quoted 7 lines

net/ipv6/ip6_vti.c +1 -0

@@ -459,6 +459,7 @@ vti6_xmit(struct sk_buff *skb, struct net_device *dev, struct flowi *fl)
  		goto tx_err_dst_release;
  	}
 
+	skb_orphan(skb);
  	skb_scrub_packet(skb, !net_eq(t->net, dev_net(dev)));
  	skb_dst_set(skb, dst);
  	skb->dev = skb_dst(skb)->dev;

This works, sort-of. I have to ping from both ends before I can start to send traffic.

I am going to do some digging. It seems like there should be some way to get the PMTU fed back into the PMTU for the VTI interface. If we can do that it would save a ton of pain.

- Alex

David Miller

maintainer

2015-05-21 21:25 UTC #7 lore raw

In reply to Steffen Klassert

From: Steffen Klassert <[email protected]>
Date: Wed, 20 May 2015 08:32:23 +0200

Quoted 16 lines · Alexander Duyck

Quoted 10 lines · Steffen Klassert
Quoted 3 lines
The MTU should be 1500. All the IPsec overhead is handled by PMTU discovery, just like in the case we use IPsec without vti tunnels. The IPv6 side of vti does it like that.
The problem is the PMTU isn't communicated to things that make use of the tunnel. For example if I do a "ping -s 2000 x.x.x.x" across an IPv6 VTI interface it will fail currently as it assumes the MTU is 1500 and so it is fragmenting the ping packet at sizes that won't be communicated across the underlying interface.
Well, the problem is that the local socket is still attached on the skb. The socket gets an error notification if the packet is too big, but ping does not care much about these error notifications.

Quoted 5 lines

One option to get such applications to work is to orphan the skb in the vti xmit function. Then the packet is not assumed to be local, so PMTU discovery is triggered on that route.
Something like this should work for IPv6:

When a packet traverses software layered devices, we should not orphan the socket.

In fact, we have taken great pains to make sure this works so that the socket memory accounting is done correctly on the original top-level socket.

Steffen Klassert

maintainer

2015-05-27 08:35 UTC #8 lore raw

Quoted 23 lines · David Miller

From: Steffen Klassert <[email protected]>
Date: Wed, 20 May 2015 08:32:23 +0200
Quoted 20 lines · Alexander Duyck
Quoted 10 lines · Steffen Klassert
Quoted 3 lines
The MTU should be 1500. All the IPsec overhead is handled by PMTU discovery, just like in the case we use IPsec without vti tunnels. The IPv6 side of vti does it like that.
The problem is the PMTU isn't communicated to things that make use of the tunnel. For example if I do a "ping -s 2000 x.x.x.x" across an IPv6 VTI interface it will fail currently as it assumes the MTU is 1500 and so it is fragmenting the ping packet at sizes that won't be communicated across the underlying interface.
Well, the problem is that the local socket is still attached on the skb. The socket gets an error notification if the packet is too big, but ping does not care much about these error notifications.
One option to get such applications to work is to orphan the skb in the vti xmit function. Then the packet is not assumed to be local, so PMTU discovery is triggered on that route.

Quoted 8 lines

Quoted 1 line
Something like this should work for IPv6:
When a packet traverses software layered devices, we should not orphan the socket.
In fact, we have taken great pains to make sure this works so that the socket memory accounting is done correctly on the original top-level socket.

I have not considered this as an official patch :)
It was more to demonstrate that PMTU discovery with IPsec tunnels can work, so we don't need to reduce the MTU of the tunnel device.

We currently check if a socket is attached to a skb and do socket error notification in this case, otherwise we do PMTU discovery if the packet is too big. Looks like this socket check is not sufficient if the packet is already transmitted through a tunnel device.

I wonder if we have something to know that a packet was already transmitted through a tunnel device. We could switch from socket notification to PMTU discovery in this case.

David Miller

maintainer

2015-05-27 15:46 UTC #9 lore raw

From: Steffen Klassert <[email protected]>
Date: Wed, 27 May 2015 10:35:16 +0200

Quoted 8 lines

We currently check if a socket is attached to a skb and do socket error notification in this case, otherwise we do PMTU discovery if the packet is too big. Looks like this socket check is not sufficient if the packet is already transmitted through a tunnel device.
I wonder if we have something to know that a packet was already transmitted through a tunnel device. We could switch from socket notification to PMTU discovery in this case.

Generally speaking, we should not be orphaning the socket as it traverses through tunnels.

In fact we have taken great pains to avoid doing this.

See, for example, commits:

7026b1ddb6b8d4e6ee33dc2bd06c0ca8746fa7ab
aad88724c9d54acb1a9737cb6069d8470fa85f74
b0270e91014dabfceaf37f5b40ad51bbf21a1302

Therefore what we always should do is retain the original socket ownership on the SKB, and layers that implement tunneling using sockets should pass the socket pointer through their output path(s) and never use skb->sk for this.

Steffen Klassert

maintainer

2015-05-28 05:52 UTC #10 lore raw

Quoted 8 lines · David Miller

From: Steffen Klassert <[email protected]>
Date: Wed, 27 May 2015 10:35:16 +0200
Quoted 5 lines
We currently check if a socket is attached to a skb and do socket error notification in this case, otherwise we do PMTU discovery if the packet is too big. Looks like this socket check is not sufficient if the packet is already transmitted through a tunnel device.

Quoted 8 lines

Quoted 3 lines
I wonder if we have something to know that a packet was already transmitted through a tunnel device. We could switch from socket notification to PMTU discovery in this case.
Generally speaking, we should not be orphaning the socket as it traverses through tunnels.
In fact we have taken great pains to avoid doing this.

Yes, I'm aware of this. I don't want to orphan the socket, all I wanted to do is to change the way we notify about MTU changes. I.e. use icmpv6_send() instead of xfrm_local_error() if the packet traversed a tunnel, that's why I wondered whether we can know this.