Question: should local address be expired when updating PMTU?
Subsystems (4)
Hi, David Miller
Since commit 81aded246 (ipv6: Handle PMTU in ICMP error handlers), the entries in neigh table may get expired. But in the situation:
Host only
PC <------------> Virtual Machinea packet is sent from PC to VM, and the packet looks like:
-----------------------------------
| IPv6 (src=PC-addr, dst=VM-addr) | |---------------------------------| | ICMPv6 (Packet Too Big) | |---------------------------------| | IPv6 (src=VM-addr, dst=VM-addr) | |---------------------------------| | ICMPv6 (Neighbor Advertisement) |
-----------------------------------
Then the local addr on VM will be updated with an expire value. After the lifetime of the local addr is expired, the VM is unreachable from PC.
# ip -6 route list table local local fe80::1 dev lo metric 0 *expire 596*
I find that the current code seems not check whether the entry is a local one when doing PMTU update. And if the following code is added, the situation could be avoided.
net/ipv6/route.c +3 -0
@@ -1136,6 +1136,9 @@ static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk, {struct rt6_info *rt6 = (struct rt6_info *)dst; +if (rt6->rt6i_flags &RTF_LOCAL ) +return ; +dst_confirm (dst);if (mtu <dst_mtu (dst) && rt6->rt6i_dst.plen ==128 ) {struct net *net =dev_net (dst->dev);
So is this modification correct? Or how can we avoid such expiring?
thx & BR,
Sheng
David Miller
maintainerFrom: shengyong <[email protected]>
Date: Mon, 2 Feb 2015 16:20:24 +0800
Quoted 1 line
Hi, David Miller
There are other people on this list more skilled than I am at answering this question, just FYI...
Hello Shengyong,
Quoted 15 lines
net/ipv6/route.c +3
-0@@ -1136,6 +1136,9 @@ static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk, {struct rt6_info *rt6 = (struct rt6_info *)dst; +if (rt6->rt6i_flags &RTF_LOCAL ) +return ; +dst_confirm (dst);if (mtu <dst_mtu (dst) && rt6->rt6i_dst.plen ==128 ) {struct net *net =dev_net (dst->dev);
Quoted 1 line
So is this modification correct? Or how can we avoid such expiring?
FWIW, we encountered this problem with IPVS tunneling. Here's a patch done by Calvin (cc'ed) that fixes my attempted fix for this. We're not particularly proud of this...
At a high level, I don't think the RTF_LOCAL check was sufficient, but I didn't investigate deeply enough and hopefully Calvin can say why.
net/ipv6/route.c +11 -11
@@ -1159,18 +1159,18 @@ static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk, }dst_metric_set (dst,RTAX_MTU , mtu); -/* FACEBOOK HACK: We need to not expire local non-expiring -* routes so that we don't accidentally start blackholing -* ipvs traffic when we happen to use it locally for -* healthchecking (see ip_vs_xmit.c -- -* __ip_vs_get_out_rt_v6 invokes update_pmtu if the rt is -* associated with a socket) -* Alex Gartrell <[email protected]> +/* +* FACEBOOK HACK: Only expire routes that aren't destined for +* the loopback interface. +* +* This prevents the strange route coalescing that happens when +* you add an address to the loopback that had a route that had +* been used when the address didn't exist from getting expired +* and causing packet loss in shiv. */ -if (!(rt6->rt6i_flags &RTF_LOCAL ) || - (rt6->rt6i_flags & (RTF_EXPIRES |RTF_CACHE ))) -rt6_update_expires ( - rt6, net->ipv6.sysctl.ip6_rt_mtu_expires); +if (!(dst->dev->flags &IFF_LOOPBACK )) +rt6_update_expires (rt6, + net->ipv6.sysctl.ip6_rt_mtu_expires); } }
Cheers,
Signature · 1 line
Alex Gartrell <[email protected]>
Quoted 47 lines · Alex Gartrell
Hello Shengyong,
Quoted 15 lines
net/ipv6/route.c +3
-0@@ -1136,6 +1136,9 @@ static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk, {struct rt6_info *rt6 = (struct rt6_info *)dst; +if (rt6->rt6i_flags &RTF_LOCAL ) +return ; +dst_confirm (dst);if (mtu <dst_mtu (dst) && rt6->rt6i_dst.plen ==128 ) {struct net *net =dev_net (dst->dev);Quoted 1 line
So is this modification correct? Or how can we avoid such expiring?
FWIW, we encountered this problem with IPVS tunneling. Here's a patch done by Calvin (cc'ed) that fixes my attempted fix for this. We're not particularly proud of this...
At a high level, I don't think the
RTF_LOCALcheck was sufficient, but I didn't investigate deeply enough and hopefully Calvin can say why.net/ipv6/route.c +8
-7@@ -1159,18 +1159,18 @@ static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk, }dst_metric_set (dst,RTAX_MTU , mtu); -/* FACEBOOK HACK: We need to not expire local non-expiring -* routes so that we don't accidentally start blackholing -* ipvs traffic when we happen to use it locally for -* healthchecking (see ip_vs_xmit.c -- -* __ip_vs_get_out_rt_v6 invokes update_pmtu if the rt is -* associated with a socket) -* Alex Gartrell <[email protected]> +/* +* FACEBOOK HACK: Only expire routes that aren't destined for +* the loopback interface. +* +* This prevents the strange route coalescing that happens when +* you add an address to the loopback that had a route that had +* been used when the address didn't exist from getting expired +* and causing packet loss in shiv.
net/ipv6/route.c static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk, · line 1175
Quoted 10 lines
net/ipv6/route.c +3
-4*/ -if (!(rt6->rt6i_flags &RTF_LOCAL ) || - (rt6->rt6i_flags & (RTF_EXPIRES |RTF_CACHE ))) -rt6_update_expires ( - rt6, net->ipv6.sysctl.ip6_rt_mtu_expires); +if (!(dst->dev->flags &IFF_LOOPBACK )) +rt6_update_expires (rt6, + net->ipv6.sysctl.ip6_rt_mtu_expires); } }
Thanks, your approach can also solve the problem I met. I just a bit confuse that is this kind of packets (like I sent in the first mail) normal? and if they are abnormal, I think we'd better drop them before update rt6i_flags.
thx,
Sheng
Quoted 3 lines
net/ipv6/route.c +0
-0Cheers,
Quoted 20 lines · Alex Gartrell
Hello Shengyong,
net/ipv6/route.c static void ip6_rt_update_pmtu(struct
Quoted 5 lines
net/ipv6/route.c +0
-0@@ -1136,6 +1136,9 @@ static void ip6_rt_update_pmtu(struct
dst_entry*dst, struct sock *sk,Quoted 11 lines
net/ipv6/route.c +3
-0{struct rt6_info *rt6 = (struct rt6_info *)dst; +if (rt6->rt6i_flags &RTF_LOCAL ) +return ; +dst_confirm (dst);if (mtu <dst_mtu (dst) && rt6->rt6i_dst.plen ==128 ) {struct net *net =dev_net (dst->dev);So is this modification correct? Or how can we avoid such expiring?
Quoted 7 lines
FWIW, we encountered this problem with IPVS tunneling. Here's a patch done by Calvin (cc'ed) that fixes my attempted fix for this. We're not particularly proud of this...
At a high level, I don't think the
RTF_LOCALcheck was sufficient, but I didn't investigate deeply enough and hopefully Calvin can say why.
I honestly didn't spend much time at all finding the underlying cause because it appeared to be fixed upstream: on 3.19-rc5 you get all 3 expected routes after the last step of my repro below. I just really needed to get this working at the time, and the gross disgusting horrible ugly awful [more negative adjectives] patch included below made it work.
FWIW, the explanation I wrote down in my notes was:
"The absence of RTF_NONEXTHOP is causing COWs to happen, which are always marked as RTF_CACHE. Somehow that's screwing things up in rt6_do_redirect()"
That could be BS though, I don't at all remember how I came to that conclusion.
(/me resolves to write better notes in the future...)
Here's how to get the weird behavior on 3.10 (+stable):
$ sudo ip addr add local 4444::1 dev lo ### Now I have 2 routes in /proc/net/ipv6_route, a local and a non-local ### Both have the RTF_NONEXTHOP flag set (0x00200000) $ sudo ip route add local 4444::1 dev lo ### Now I have 3 routes in /proc/net/ipv6_route to 4444::1 ### Notice the new route does NOT have the RTF_NONEXTHOP flag set $ sudo ip addr del local 4444::1 dev lo ### Now I just have the one route I created before $ sudo ip addr add local 4444::1 dev lo ### And now I have 3 routes again $ sudo ping6 4444::1 [blah blah blah successful ping] $ sudo ip addr del local 4444::1 dev lo $ sudo ip addr add local 4444::1 dev lo ### Still have 3 routes $ sudo ip addr del local 4444::1 dev lo ### Now I just have my one route yet again ### Now, *without the address on lo*, talk to it (it works), then re-add it $ ping6 4444::1 [blah blah blah successful ping] $ sudo ip addr add local 4444::1 dev lo ### Now I only have 2 routes... WAT!? ### Notice the LOCAL (0x80000000) route doesn't have the RTF_NONEXTHOP flag set
Thanks,
Calvin
Quoted 39 lines
net/ipv6/route.c +11
-11@@ -1159,18 +1159,18 @@ static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk, }dst_metric_set (dst ,RTAX_MTU ,mtu ); -/* FACEBOOK HACK: We need to not expire local non-expiring -* routes so that we don't accidentally start blackholing -* ipvs traffic when we happen to use it locally for -* healthchecking (see ip_vs_xmit.c -- -* __ip_vs_get_out_rt_v6 invokes update_pmtu if the rt is -* associated with a socket) -* Alex Gartrell <[email protected]> +/* +* FACEBOOK HACK: Only expire routes that aren't destined for +* the loopback interface. +* +* This prevents the strange route coalescing that happens when +* you add an address to the loopback that had a route that had +* been used when the address didn't exist from getting expired +* and causing packet loss in shiv. */ -if (!(rt6->rt6i_flags &RTF_LOCAL ) || - (rt6->rt6i_flags & (RTF_EXPIRES |RTF_CACHE ))) -rt6_update_expires ( - rt6, net->ipv6.sysctl.ip6_rt_mtu_expires); +if (!(dst->dev->flags &IFF_LOOPBACK )) +rt6_update_expires (rt6, + net->ipv6.sysctl.ip6_rt_mtu_expires); } }Cheers,
Signature · 1 line
Alex Gartrell <[email protected]>
Quoted 25 lines · Calvin Owens
Quoted 24 lines · Alex Gartrell
Hello Shengyong,
net/ipv6/route.c static void ip6_rt_update_pmtu(struct
Quoted 5 lines
net/ipv6/route.c +0
-0@@ -1136,6 +1136,9 @@ static void ip6_rt_update_pmtu(struct
dst_entry*dst, struct sock *sk,Quoted 11 lines
net/ipv6/route.c +3
-0{struct rt6_info *rt6 = (struct rt6_info *)dst; +if (rt6->rt6i_flags &RTF_LOCAL ) +return ; +dst_confirm (dst);if (mtu <dst_mtu (dst) && rt6->rt6i_dst.plen ==128 ) {struct net *net =dev_net (dst->dev);So is this modification correct? Or how can we avoid such expiring?
FWIW, we encountered this problem with IPVS tunneling. Here's a patch done by Calvin (cc'ed) that fixes my attempted fix for this. We're not particularly proud of this...
Quoted 7 lines
Quoted 3 lines
At a high level, I don't think the
RTF_LOCALcheck was sufficient, but I didn't investigate deeply enough and hopefully Calvin can say why.I honestly didn't spend much time at all finding the underlying cause because it appeared to be fixed upstream: on 3.19-rc5 you get all 3 expected routes after the last step of my repro below.
Hi,
I do my test on 3.19.0-rc7 just now, it seems it still doesn't solve the local-addr-expired problem.
I just really
Quoted 32 lines
needed to get this working at the time, and the gross disgusting horrible ugly awful [more negative adjectives] patch included below made it work.
FWIW, the explanation I wrote down in my notes was:
"The absence of
RTF_NONEXTHOPis causing COWs to happen, which are always marked asRTF_CACHE. Somehow that's screwing things up inrt6_do_redirect()"That could be BS though, I don't at all remember how I came to that conclusion.
(/me resolves to write better notes in the future...)
Here's how to get the weird behavior on 3.10 (+stable):
$ sudo ip addr add local 4444::1 dev lo ### Now I have 2 routes in /proc/net/ipv6_route, a local and a non-local ### Both have the RTF_NONEXTHOP flag set (0x00200000) $ sudo ip route add local 4444::1 dev lo ### Now I have 3 routes in /proc/net/ipv6_route to 4444::1 ### Notice the new route does NOT have the RTF_NONEXTHOP flag set $ sudo ip addr del local 4444::1 dev lo ### Now I just have the one route I created before $ sudo ip addr add local 4444::1 dev lo ### And now I have 3 routes again $ sudo ping6 4444::1 [blah blah blah successful ping] $ sudo ip addr del local 4444::1 dev lo $ sudo ip addr add local 4444::1 dev lo ### Still have 3 routes
Quoted 8 lines
$ sudo ip addr del local 4444::1 dev lo ### Now I just have my one route yet again ### Now, *without the address on lo*, talk to it (it works), then re-add it $ ping6 4444::1 [blah blah blah successful ping] $ sudo ip addr add local 4444::1 dev lo ### Now I only have 2 routes... WAT!? ### Notice the LOCAL (0x80000000) route doesn't have the RTF_NONEXTHOP flag set
Looks like we meet different problems. Here is how I do my test (as well as on 3.10 +stable):
Host only
PC <------------> Virtual Machine
create and send a packet using scapy:
-----------------------------------
| IPv6 (src=PC-addr, dst=VM-addr) | |---------------------------------| | ICMPv6 (Packet Too Big) | |---------------------------------| | IPv6 (src=VM-addr, dst=VM-addr) | |---------------------------------| | ICMPv6 (Neighbor Advertisement) |
-----------------------------------
Then the local-addr is set to expire. After expired, the VM is unreachable from PC side.
thanks,
Sheng
Quoted 46 lines
Thanks,
CalvinQuoted 36 lines
net/ipv6/route.c +11
-11@@ -1159,18 +1159,18 @@ static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk, }dst_metric_set (dst ,RTAX_MTU ,mtu ); -/* FACEBOOK HACK: We need to not expire local non-expiring -* routes so that we don't accidentally start blackholing -* ipvs traffic when we happen to use it locally for -* healthchecking (see ip_vs_xmit.c -- -* __ip_vs_get_out_rt_v6 invokes update_pmtu if the rt is -* associated with a socket) -* Alex Gartrell <[email protected]> +/* +* FACEBOOK HACK: Only expire routes that aren't destined for +* the loopback interface. +* +* This prevents the strange route coalescing that happens when +* you add an address to the loopback that had a route that had +* been used when the address didn't exist from getting expired +* and causing packet loss in shiv. */ -if (!(rt6->rt6i_flags &RTF_LOCAL ) || - (rt6->rt6i_flags & (RTF_EXPIRES |RTF_CACHE ))) -rt6_update_expires ( - rt6, net->ipv6.sysctl.ip6_rt_mtu_expires); +if (!(dst->dev->flags &IFF_LOOPBACK )) +rt6_update_expires (rt6, + net->ipv6.sysctl.ip6_rt_mtu_expires); } }Quoted 3 lines
Cheers,
Signature · 1 line
Alex Gartrell <[email protected]>.
Quoted 16 lines · shengyong
Hi, David Miller
Since commit81aded246(ipv6: Handle PMTU in ICMP error handlers), the entries in neigh table may get expired. But in the situation:Host only PC <------------> Virtual Machinea packet is sent from PC to VM, and the packet looks like:
-----------------------------------| IPv6 (src=PC-addr, dst=VM-addr) | |---------------------------------| | ICMPv6 (Packet Too Big) | |---------------------------------| | IPv6 (src=VM-addr, dst=VM-addr) | |---------------------------------| | ICMPv6 (Neighbor Advertisement) |
Quoted 7 lines
-----------------------------------
Then the local addr on VM will be updated with an expire value. After the lifetime of the local addr is expired, the VM is unreachable from PC.
# ip -6 route list table local local fe80::1 dev lo metric 0 *expire 596*
We first need to find out why you receive this Packet Too Big message, can you capture this packet somehow? Then we have to see why this loopback route gets a pmtu update from that packet. Is the destination address of the Packet Too Big message really fe80::1?
Quoted 19 lines · Steffen Klassert
Quoted 18 lines · shengyong
Hi, David Miller
Since commit81aded246(ipv6: Handle PMTU in ICMP error handlers), the entries in neigh table may get expired. But in the situation:Host only PC <------------> Virtual Machinea packet is sent from PC to VM, and the packet looks like:
-----------------------------------| IPv6 (src=PC-addr, dst=VM-addr) | |---------------------------------| | ICMPv6 (Packet Too Big) | |---------------------------------| | IPv6 (src=VM-addr, dst=VM-addr) | |---------------------------------| | ICMPv6 (Neighbor Advertisement) |-----------------------------------
Quoted 7 lines
Quoted 5 lines
Then the local addr on VM will be updated with an expire value. After the lifetime of the local addr is expired, the VM is unreachable from PC.
# ip -6 route list table local local fe80::1 dev lo metric 0 *expire 596*We first need to find out why you receive this Packet Too Big message,
The packet is sent by a commercial-off-the-shelf testcase, and I can reproduce the situation by using scapy and creating a packet as the following:
$ cat packet-too-big.py #!/usr/bin/python
from scapy.all import * # fe80::800:27ff:fe00:0 is linklocal addr of PC # fe80::a00:27ff:fe1a:e2a0 is linklocal addr of VM base=IPv6(src='fe80::800:27ff:fe00:0',dst='fe80::a00:27ff:fe1a:e2a0') pkt_too_big=ICMPv6PacketTooBig(mtu=1024) ext_base=IPv6(src='fe80::a00:27ff:fe1a:e2a0',dst='fe80::a00:27ff:fe1a:e2a0',plen=24) ext_nd_na=ICMPv6ND_NA() packet=base/pkt_too_big/ext_base/ext_nd_na send(packet)
Quoted 1 line
can you capture this packet somehow?
I captured the packet in wireshark, it is exact the packet created by the script.
Quoted 2 lines
Then we have to see why this loopback
route gets a pmtu update from that packet.
I tried to print info when the VM receives pkt-too-big packet. The calling stack is icmpv6_rcv->icmpv6_notify->icmpv6_err->ip6_update_pmtu->ip6_rt_update_pmtu->rt6_update_expires. In ip6_update_pmtu, ip6_route_output looks up the route table, and returns the dst_entry of the linklocal addr. Then it is set to expire.
Quoted 2 lines
Is the destination address
of the Packet Too Big message really fe80::1?
In fact, if the dst of the above `ext_base' is the local addr of the VM, the local addr on VM will be expired.
thx,
Sheng
Quoted 7 lines
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 12 lines · shengyong
Quoted 3 lines · Steffen Klassert
On Mon, Feb 02, 2015 at 04:20:24PM +0800, shengyong wrote:
We first need to find out why you receive this Packet Too Big message,
The packet is sent by a commercial-off-the-shelf testcase, and I can reproduce the situation by using scapy and creating a packet as the following:
$ cat packet-too-big.py #!/usr/bin/python
Quoted 11 lines
from scapy.all import * # fe80::800:27ff:fe00:0 is linklocal addr of PC # fe80::a00:27ff:fe1a:e2a0 is linklocal addr of VM base=IPv6(src='fe80::800:27ff:fe00:0',dst='fe80::a00:27ff:fe1a:e2a0') pkt_too_big=ICMPv6PacketTooBig(mtu=1024) ext_base=IPv6(src='fe80::a00:27ff:fe1a:e2a0',dst='fe80::a00:27ff:fe1a:e2a0',plen=24) ext_nd_na=ICMPv6ND_NA() packet=base/pkt_too_big/ext_base/ext_nd_na send(packet)
So it is not a valid pmtu update, this make life easier.
Can you please test the patch below (compile tested only)?
This should fix your problem, and in combination with the two patches I sent out last week, it should cure the whole 'expiring of uncached routes' problem.
net/ipv6/route.c +2 -1
@@ -1156,7 +1156,8 @@ static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,struct rt6_info *rt6 = (struct rt6_info *)dst;dst_confirm (dst); -if (mtu <dst_mtu (dst) && rt6->rt6i_dst.plen ==128 ) { +if (mtu <dst_mtu (dst) && rt6->rt6i_dst.plen ==128 && + (rt6->rt6i_flags &RTF_CACHE )) {struct net *net =dev_net (dst->dev); rt6->rt6i_flags |=RTF_MODIFIED ;
Quoted 22 lines · Steffen Klassert
Quoted 21 lines · shengyong
Quoted 3 lines · Steffen Klassert
On Mon, Feb 02, 2015 at 04:20:24PM +0800, shengyong wrote:
We first need to find out why you receive this Packet Too Big message,
The packet is sent by a commercial-off-the-shelf testcase, and I can reproduce the situation by using scapy and creating a packet as the following:
$ cat packet-too-big.py #!/usr/bin/pythonfrom scapy.all import * # fe80::800:27ff:fe00:0 is linklocal addr of PC # fe80::a00:27ff:fe1a:e2a0 is linklocal addr of VM base=IPv6(src='fe80::800:27ff:fe00:0',dst='fe80::a00:27ff:fe1a:e2a0') pkt_too_big=ICMPv6PacketTooBig(mtu=1024) ext_base=IPv6(src='fe80::a00:27ff:fe1a:e2a0',dst='fe80::a00:27ff:fe1a:e2a0',plen=24) ext_nd_na=ICMPv6ND_NA()
Quoted 6 lines
Quoted 2 lines
packet=base/pkt_too_big/ext_base/ext_nd_na send(packet)
So it is not a valid pmtu update, this make life easier.
Can you please test the patch below (compile tested only)?
Sorry, the later. I test it on 3.10-stable. It can fix this problem. So maybe this is a bug? And the 3 approaches (different flags are used: RTF_LOCAL, IFF_LOOPBACK and RTF_CACHE) in these mails can fix the expire of local address. I'm confused about these flags:
* RTF_LOCAL: the entries of local address, like address binded to the native NIC
* RTF_CACHE: all cached entries
* IFF_LOOPBACK: this is a device-related flag, which has the same meaning as RTF_LOCAL
Am I right? If so, I think RTF_LOCAL is appropriate, because we just want entries of local addresses to be not expired and we don't care other entries (I think if they get expired, a neigh discovery could find them back).
thx,
Sheng
Quoted 25 lines
This should fix your problem, and in combination with the two patches I sent out last week, it should cure the whole 'expiring of uncached routes' problem.
net/ipv6/route.c +2
-2@@ -1156,7 +1156,8 @@ static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,struct rt6_info *rt6 = (struct rt6_info *)dst;dst_confirm (dst); -if (mtu <dst_mtu (dst) && rt6->rt6i_dst.plen ==128 ) { +if (mtu <dst_mtu (dst) && rt6->rt6i_dst.plen ==128 && + (rt6->rt6i_flags &RTF_CACHE )) {struct net *net =dev_net (dst->dev); rt6->rt6i_flags |=RTF_MODIFIED ; --To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html.
Quoted 2 lines · shengyong
Sorry, the later. I test it on 3.10-stable. It can fix this problem. So maybe this is a bug?
Yes, it's a bug.
Quoted 9 lines
And the 3 approaches (different flags are used:
RTF_LOCAL,IFF_LOOPBACKandRTF_CACHE) in these mails can fix the expire of local address. I'm confused about these flags:
*RTF_LOCAL: the entries of local address, like address binded to the native NIC
*RTF_CACHE: all cached entries
*IFF_LOOPBACK: this is a device-related flag, which has the same meaning asRTF_LOCALAm I right? If so, I think
RTF_LOCALis appropriate, because we just want entries of local addresses to be not expired and we don't care other entries (I think if they get expired, a neigh discovery could find them back).
It is not the address that expires, it is the learned PMTU value that expires. If we delete an uncached route, nothing will bring it back unless you readd it manually.
We need to ensure that all routes that can learn something what expires are cached. This means that we clone the inserted route when it is used for the first time. The learned values are stored at this cloned route. If the learned value expires, the clone is deleted. The original route remains in the fib tree and can be still looked up.
The problem is, that we currently don't cache/clone host routes. So if a host route learns something that expires, the original route is removed from the fib tree and we loose connectivity to that host. We don't cache host routes because some of them (the local ones) are automatically added with metric 0. If we try to cache such a route, the clone will be identical to the original route and we fail to insert it to the fib tree.
So we need to adjust the caching to all routes that actually can learn something and leave out only those that can not.
I'll send a patchset that should fix this at the beginning of the next week.
Quoted 35 lines · Steffen Klassert
Quoted 2 lines · shengyong
Sorry, the later. I test it on 3.10-stable. It can fix this problem. So maybe this is a bug?
Yes, it's a bug.
Quoted 9 lines
And the 3 approaches (different flags are used:
RTF_LOCAL,IFF_LOOPBACKandRTF_CACHE) in these mails can fix the expire of local address. I'm confused about these flags:
*RTF_LOCAL: the entries of local address, like address binded to the native NIC
*RTF_CACHE: all cached entries
*IFF_LOOPBACK: this is a device-related flag, which has the same meaning asRTF_LOCALAm I right? If so, I think
RTF_LOCALis appropriate, because we just want entries of local addresses to be not expired and we don't care other entries (I think if they get expired, a neigh discovery could find them back).It is not the address that expires, it is the learned PMTU value that expires. If we delete an uncached route, nothing will bring it back unless you readd it manually.
We need to ensure that all routes that can learn something what expires are cached. This means that we clone the inserted route when it is used for the first time. The learned values are stored at this cloned route. If the learned value expires, the clone is deleted. The original route remains in the fib tree and can be still looked up.
The problem is, that we currently don't cache/clone host routes. So if a host route learns something that expires, the original route is removed from the fib tree and we loose connectivity to that host. We don't cache host routes because some of them (the local ones) are automatically added with metric 0. If we try to cache such a route, the clone will be identical to the original route and we fail to insert it to the fib tree.
Quoted 5 lines
So we need to adjust the caching to all routes that actually can learn something and leave out only those that can not.
I'll send a patchset that should fix this at the beginning of the next week.
Hi, Steffen
Is this patchset ready? It seems that I didn't find it in the mainling list. If it is ready, I can test it to see if it solves the problem I met :)
many thanks,
Sheng
Quoted 3 lines
.
Quoted 4 lines · shengyong
Hi, Steffen
Is this patchset ready? It seems that I didn't find it in the mainling list. If it is ready, I can test it to see if it solves the problem I met :)
Martin Lau pointed me to a problem when cached host routes are deleted. We may delete the clone but not the original route. This has to be resolved first. Unfortunately I had no time to look into this so far. I hope to get to it next week.
Quoted 41 lines · shengyong
Quoted 38 lines · Steffen Klassert
Quoted 2 lines · shengyong
Sorry, the later. I test it on 3.10-stable. It can fix this problem. So maybe this is a bug?
Yes, it's a bug.
Quoted 9 lines
And the 3 approaches (different flags are used:
RTF_LOCAL,IFF_LOOPBACKandRTF_CACHE) in these mails can fix the expire of local address. I'm confused about these flags:
*RTF_LOCAL: the entries of local address, like address binded to the native NIC
*RTF_CACHE: all cached entries
*IFF_LOOPBACK: this is a device-related flag, which has the same meaning asRTF_LOCALAm I right? If so, I think
RTF_LOCALis appropriate, because we just want entries of local addresses to be not expired and we don't care other entries (I think if they get expired, a neigh discovery could find them back).It is not the address that expires, it is the learned PMTU value that expires. If we delete an uncached route, nothing will bring it back unless you readd it manually.
We need to ensure that all routes that can learn something what expires are cached. This means that we clone the inserted route when it is used for the first time. The learned values are stored at this cloned route. If the learned value expires, the clone is deleted. The original route remains in the fib tree and can be still looked up.
The problem is, that we currently don't cache/clone host routes. So if a host route learns something that expires, the original route is removed from the fib tree and we loose connectivity to that host. We don't cache host routes because some of them (the local ones) are automatically added with metric 0. If we try to cache such a route, the clone will be identical to the original route and we fail to insert it to the fib tree.
So we need to adjust the caching to all routes that actually can learn something and leave out only those that can not.
Quoted 6 lines
Quoted 2 lines
I'll send a patchset that should fix this at the beginning of the next week.
Hi, Steffen
Is this patchset ready? It seems that I didn't find it in the mainling list. If it is ready, I can test it to see if it solves the problem I met :)
I finally end up with a patchset that passed all your testcases. I'll send it in reply to this mail, please test it.
Thanks!
RFC PATCH 1/3 ipv6: Fix after pmtu events dissapearing host routes
We currently don't clone host routes before we use them. If a pmtu event is received on such a route, it gets an expires value. As soon as the expiration time is elapsed, the route is deleted. As a result, the host es not reachable any more.
We fix this by cloning host routes if they are not local, i.e. if pmtu events can happen. Also adjust the route deletion because the cached and the original host route are on the same fib node.
--- net/ipv6/route.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
net/ipv6/route.c +6 -1
@@ -961,7 +961,7 @@ redo_rt6_select:if (!(rt->rt6i_flags & (RTF_NONEXTHOP |RTF_GATEWAY ))) nrt =rt6_alloc_cow (rt, &fl6->daddr, &fl6->saddr); -else if (!(rt->dst.flags &DST_HOST )) +else if (!(rt->dst.flags &DST_HOST ) || !(rt->rt6i_flags &RTF_LOCAL )) nrt =rt6_alloc_clone (rt, &fl6->daddr);else goto out2;
@@ -1796,6 +1796,8 @@ static int ip6_route_del(struct fib6_config *cfg)continue ;if (cfg->fc_metric && cfg->fc_metric != rt->rt6i_metric)continue ; +if (!(cfg->fc_flags &RTF_CACHE ) && (rt->rt6i_flags &RTF_CACHE )) +continue ;dst_hold (&rt->dst);read_unlock_bh (&table->tb6_lock);
@@ -2433,6 +2435,9 @@ static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh,if (rtm->rtm_type ==RTN_LOCAL ) cfg->fc_flags |=RTF_LOCAL ; +if (rtm->rtm_flags &RTM_F_CLONED ) + cfg->fc_flags |=RTF_CACHE ; + cfg->fc_nlinfo.portid =NETLINK_CB (skb).portid; cfg->fc_nlinfo.nlh = nlh; cfg->fc_nlinfo.nl_net =sock_net (skb->sk);
Signature · 1 line
1.9.1
Re: [PATCH RFC 1/3] ipv6: Fix after pmtu events dissapearing host routes
Quoted 30 lines · Steffen Klassert
We currently don't clone host routes before we use them. If a pmtu event is received on such a route, it gets an expires value. As soon as the expiration time is elapsed, the route is deleted. As a result, the host es not reachable any more.
We fix this by cloning host routes if they are not local, i.e. if pmtu events can happen. Also adjust the route deletion because the cached and the original host route are on the same fib node.
Signed-off-by Steffen Klassert <[email protected]>--- net/ipv6/route.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)net/ipv6/route.c +1
-1@@ -961,7 +961,7 @@ redo_rt6_select:if (!(rt->rt6i_flags & (RTF_NONEXTHOP |RTF_GATEWAY ))) nrt =rt6_alloc_cow (rt, &fl6->daddr, &fl6->saddr); -else if (!(rt->dst.flags &DST_HOST )) +else if (!(rt->dst.flags &DST_HOST ) || !(rt->rt6i_flags &RTF_LOCAL )) nrt =rt6_alloc_clone (rt, &fl6->daddr);else goto out2;
net/ipv6/route.c static int ip6_route_del(struct fib6_config *cfg) · line 1799
Quoted 4 lines
net/ipv6/route.c +1
-0@@ -1796,6 +1796,8 @@ static int ip6_route_del(struct fib6_config *cfg)continue ;if (cfg->fc_metric && cfg->fc_metric != rt->rt6i_metric)continue ; +if (!(cfg->fc_flags &RTF_CACHE ) && (rt->rt6i_flags &RTF_CACHE ))
There is a trivial coding style problem, which makes checkpatch.pl unhappy. The line is over 80 characters.
Quoted 15 lines
net/ipv6/route.c +4
-0+continue ;dst_hold (&rt->dst);read_unlock_bh (&table->tb6_lock);@@ -2433,6 +2435,9 @@ static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh,if (rtm->rtm_type ==RTN_LOCAL ) cfg->fc_flags |=RTF_LOCAL ; +if (rtm->rtm_flags &RTM_F_CLONED ) + cfg->fc_flags |=RTF_CACHE ; + cfg->fc_nlinfo.portid =NETLINK_CB (skb).portid; cfg->fc_nlinfo.nlh = nlh; cfg->fc_nlinfo.nl_net =sock_net (skb->sk);
Re: [PATCH RFC 1/3] ipv6: Fix after pmtu events dissapearing host routes
In reply to Steffen KlassertQuoted 5 lines · Steffen Klassert
We currently don't clone host routes before we use them. If a pmtu event is received on such a route, it gets an expires value. As soon as the expiration time is elapsed, the route is deleted. As a result, the host es not reachable any more.
s/es/is
Quoted 15 lines
We fix this by cloning host routes if they are not local, i.e. if pmtu events can happen. Also adjust the route deletion because the cached and the original host route are on the same fib node.
Signed-off-by Steffen Klassert <[email protected]>--- net/ipv6/route.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)net/ipv6/route.c +0
-0
net/ipv6/route.c redo_rt6_select: · line 965
Quoted 6 lines
net/ipv6/route.c +1
-1@@ -961,7 +961,7 @@ redo_rt6_select:if (!(rt->rt6i_flags & (RTF_NONEXTHOP |RTF_GATEWAY ))) nrt =rt6_alloc_cow (rt, &fl6->daddr, &fl6->saddr); -else if (!(rt->dst.flags &DST_HOST )) +else if (!(rt->dst.flags &DST_HOST ) || !(rt->rt6i_flags &RTF_LOCAL )) nrt =rt6_alloc_clone (rt, &fl6->daddr);
I suspect the nrt and rt will share the same inetpeer. Hence, after the nrt is removed, the MTU (obtained from PMTU update) will stay.
Quoted 10 lines
net/ipv6/route.c +2
-0@@ -1796,6 +1796,8 @@ static int ip6_route_del(struct fib6_config *cfg)continue ;if (cfg->fc_metric && cfg->fc_metric != rt->rt6i_metric)continue ; +if (!(cfg->fc_flags &RTF_CACHE ) && (rt->rt6i_flags &RTF_CACHE )) +continue ;dst_hold (&rt->dst);read_unlock_bh (&table->tb6_lock);
net/ipv6/route.c static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh, · line 2443
Quoted 9 lines
net/ipv6/route.c +3
-0@@ -2433,6 +2435,9 @@ static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh,if (rtm->rtm_type ==RTN_LOCAL ) cfg->fc_flags |=RTF_LOCAL ; +if (rtm->rtm_flags &RTM_F_CLONED ) + cfg->fc_flags |=RTF_CACHE ; + cfg->fc_nlinfo.portid =NETLINK_CB (skb).portid; cfg->fc_nlinfo.nlh = nlh; cfg->fc_nlinfo.nl_net =sock_net (skb->sk);
I had pulled your last patch to my local branch that I am cooking. I also added similar changes to ip6_route_del() and rtm_to_fib_config(), and confirmed it can keep 'ip -6 r del' and 'ip -6 r flush table cache' to keep working properly.
Thanks,
--Martin
Re: [PATCH RFC 1/3] ipv6: Fix after pmtu events dissapearing host routes
Quoted 11 lines · Martin Lau
Quoted 10 lines · Steffen Klassert
--- net/ipv6/route.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)net/ipv6/route.c +0
-0@@ -961,7 +961,7 @@ redo_rt6_select:
Quoted 7 lines
net/ipv6/route.c redo_rt6_select: · line 965
Quoted 5 lines
net/ipv6/route.c +1
-1if (!(rt->rt6i_flags & (RTF_NONEXTHOP |RTF_GATEWAY ))) nrt =rt6_alloc_cow (rt, &fl6->daddr, &fl6->saddr); -else if (!(rt->dst.flags &DST_HOST )) +else if (!(rt->dst.flags &DST_HOST ) || !(rt->rt6i_flags &RTF_LOCAL )) nrt =rt6_alloc_clone (rt, &fl6->daddr);I suspect the nrt and rt will share the same inetpeer. Hence, after the nrt is removed, the MTU (obtained from PMTU update) will stay.
I used a route with default metrics in my tests, therefore the merics were copied for nrt. But if the route has a mtu set, the metrics are not copied. So we overwrite the route mtu with the pmtu value. Maybe we should make sure to always copy the metrics if we do a pmtu update.
Something like this:
net/ipv6/route.c +1 -0
@@ -1182,6 +1182,7 @@ static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,if (mtu <IPV6_MIN_MTU ) mtu =IPV6_MIN_MTU ; +dst_cow_metrics_generic (dst, dst->_metrics);dst_metric_set (dst,RTAX_MTU , mtu);rt6_update_expires (rt6, net->ipv6.sysctl.ip6_rt_mtu_expires); }
This could fix the pmtu case, but what are we going to do if we have multiple routes with different mtu values to the same host? Adding a second hostroute will overwrite the mtu value of the first hostroute.
This reminds me at the times when we removed the ipv4 routing cache. The first approach to handle pmtu updates was to cache the pmtu values at the inetpeer, but we never got it to work. We swiched then to use exceptional routes in this case.
RFC PATCH 2/3 ipv6: Extend the route lookups to low priority metrics.
In reply to Steffen KlassertWe search only for routes with highest prioriy metric in find_rr_leaf(). However if one of these routes is marked as invalid, we may fail to find a route even if there is a appropriate route with lower priority. Then we loose connectivity until the garbage collector deletes the invalid route. This typically happens if a host route expires afer a pmtu event. Fix this by searching also for routes with a lower priority metric.
--- net/ipv6/route.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)
net/ipv6/route.c +23 -5
@@ -654,15 +654,33 @@ static struct rt6_info *find_rr_leaf(struct fib6_node *fn, u32 metric, int oif, int strict, bool *do_rr) { - struct rt6_info *rt, *match; + struct rt6_info *rt, *match, *cont;int mpri =-1 ; match =NULL ; -for (rt = rr_head; rt && rt->rt6i_metric == metric; - rt = rt->dst.rt6_next) + cont =NULL ; +for (rt = rr_head; rt; rt = rt->dst.rt6_next) { +if (rt->rt6i_metric != metric) { + cont = rt; +break ; + } + + match =find_match (rt, oif, strict, &mpri, match, do_rr); + } + +for (rt = fn->leaf; rt && rt != rr_head; rt = rt->dst.rt6_next) { +if (rt->rt6i_metric != metric) { + cont = rt; +break ; + } + match =find_match (rt, oif, strict, &mpri, match, do_rr); -for (rt = fn->leaf; rt && rt != rr_head && rt->rt6i_metric == metric; - rt = rt->dst.rt6_next) + } + +if (match || !cont) +return match; + +for (rt = cont; rt; rt = rt->dst.rt6_next) match =find_match (rt, oif, strict, &mpri, match, do_rr);return match;
Signature · 1 line
1.9.1
RFC PATCH 3/3 ipv6: Don't update pmtu on uncached routes.
In reply to Steffen KlassertA pmtu update on an uncached route will delete the original route after expiration time is elapsed. As a result, the host is not reachable any more.
All routes where pmtu discovery can happen are cached now, so it is ok to refuse pmtu updates on uncached routes.
--- net/ipv6/route.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
net/ipv6/route.c +2 -1
@@ -1174,7 +1174,8 @@ static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,struct rt6_info *rt6 = (struct rt6_info *)dst;dst_confirm (dst); -if (mtu <dst_mtu (dst) && rt6->rt6i_dst.plen ==128 ) { +if (mtu <dst_mtu (dst) && rt6->rt6i_dst.plen ==128 && + (rt6->rt6i_flags &RTF_CACHE )) {struct net *net =dev_net (dst->dev); rt6->rt6i_flags |=RTF_MODIFIED ;
Signature · 1 line
1.9.1
Quoted 44 lines · Steffen Klassert
Quoted 43 lines · shengyong
Quoted 40 lines · Steffen Klassert
Quoted 2 lines · shengyong
Sorry, the later. I test it on 3.10-stable. It can fix this problem. So maybe this is a bug?
Yes, it's a bug.
Quoted 9 lines
And the 3 approaches (different flags are used:
RTF_LOCAL,IFF_LOOPBACKandRTF_CACHE) in these mails can fix the expire of local address. I'm confused about these flags:
*RTF_LOCAL: the entries of local address, like address binded to the native NIC
*RTF_CACHE: all cached entries
*IFF_LOOPBACK: this is a device-related flag, which has the same meaning asRTF_LOCALAm I right? If so, I think
RTF_LOCALis appropriate, because we just want entries of local addresses to be not expired and we don't care other entries (I think if they get expired, a neigh discovery could find them back).It is not the address that expires, it is the learned PMTU value that expires. If we delete an uncached route, nothing will bring it back unless you readd it manually.
We need to ensure that all routes that can learn something what expires are cached. This means that we clone the inserted route when it is used for the first time. The learned values are stored at this cloned route. If the learned value expires, the clone is deleted. The original route remains in the fib tree and can be still looked up.
The problem is, that we currently don't cache/clone host routes. So if a host route learns something that expires, the original route is removed from the fib tree and we loose connectivity to that host. We don't cache host routes because some of them (the local ones) are automatically added with metric 0. If we try to cache such a route, the clone will be identical to the original route and we fail to insert it to the fib tree.
So we need to adjust the caching to all routes that actually can learn something and leave out only those that can not.
I'll send a patchset that should fix this at the beginning of the next week.
Quoted 8 lines
Quoted 4 lines
Hi, Steffen
Is this patchset ready? It seems that I didn't find it in the mainling list. If it is ready, I can test it to see if it solves the problem I met :)I finally end up with a patchset that passed all your testcases. I'll send it in reply to this mail, please test it.
I just test the patchset, and it can solve the problem I met. Thanks very much,
Sheng
Quoted 4 lines
Thanks!
.