* [PATCH RFC 1/2] ipv6: Fix after pmtu events dissapearing host routes
2015-01-28 12:10 ` Steffen Klassert
@ 2015-01-28 12:11 ` Steffen Klassert
2015-01-29 10:26 ` Hannes Frederic Sowa
2015-02-05 23:56 ` Martin Lau
2015-01-28 12:12 ` [PATCH RFC 2/2] ipv6: Extend the route lookups to low priority metrics Steffen Klassert
2015-01-29 3:16 ` A problem about ICMP packet-too-big message handler Yang Yingliang
2 siblings, 2 replies; 16+ messages in thread
From: Steffen Klassert @ 2015-01-28 12:11 UTC (permalink / raw)
To: Yang Yingliang; +Cc: netdev, David S. Miller
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
is not reachable any more.
We fix this by cloning host routes if they are gatewayed,
i.e. if pmtu events can happen.
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
net/ipv6/route.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index c910831..3e864e7 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -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_GATEWAY))
nrt = rt6_alloc_clone(rt, &fl6->daddr);
else
goto out2;
--
1.9.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH RFC 1/2] ipv6: Fix after pmtu events dissapearing host routes
2015-01-28 12:11 ` [PATCH RFC 1/2] ipv6: Fix after pmtu events dissapearing host routes Steffen Klassert
@ 2015-01-29 10:26 ` Hannes Frederic Sowa
2015-01-29 10:44 ` Steffen Klassert
2015-02-05 23:56 ` Martin Lau
1 sibling, 1 reply; 16+ messages in thread
From: Hannes Frederic Sowa @ 2015-01-29 10:26 UTC (permalink / raw)
To: Steffen Klassert; +Cc: Yang Yingliang, netdev, David S. Miller
On Mi, 2015-01-28 at 13:11 +0100, Steffen Klassert wrote:
> 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
> is not reachable any more.
>
> We fix this by cloning host routes if they are gatewayed,
> i.e. if pmtu events can happen.
>
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> ---
> net/ipv6/route.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index c910831..3e864e7 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -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_GATEWAY))
> nrt = rt6_alloc_clone(rt, &fl6->daddr);
> else
> goto out2;
My approach was to suppress mtu updates on the loopback interface.
Hmm...
Bye,
Hannes
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RFC 1/2] ipv6: Fix after pmtu events dissapearing host routes
2015-01-29 10:26 ` Hannes Frederic Sowa
@ 2015-01-29 10:44 ` Steffen Klassert
0 siblings, 0 replies; 16+ messages in thread
From: Steffen Klassert @ 2015-01-29 10:44 UTC (permalink / raw)
To: Hannes Frederic Sowa; +Cc: Yang Yingliang, netdev, David S. Miller
On Thu, Jan 29, 2015 at 11:26:51AM +0100, Hannes Frederic Sowa wrote:
> On Mi, 2015-01-28 at 13:11 +0100, Steffen Klassert wrote:
> > 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
> > is not reachable any more.
> >
> > We fix this by cloning host routes if they are gatewayed,
> > i.e. if pmtu events can happen.
> >
> > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> > ---
> > net/ipv6/route.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> > index c910831..3e864e7 100644
> > --- a/net/ipv6/route.c
> > +++ b/net/ipv6/route.c
> > @@ -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_GATEWAY))
> > nrt = rt6_alloc_clone(rt, &fl6->daddr);
> > else
> > goto out2;
>
> My approach was to suppress mtu updates on the loopback interface.
> Hmm...
Maybe we need to do this too. My patch fixes just the case
where we get a valid pmtu update from a remote host.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RFC 1/2] ipv6: Fix after pmtu events dissapearing host routes
2015-01-28 12:11 ` [PATCH RFC 1/2] ipv6: Fix after pmtu events dissapearing host routes Steffen Klassert
2015-01-29 10:26 ` Hannes Frederic Sowa
@ 2015-02-05 23:56 ` Martin Lau
2015-02-09 10:26 ` Steffen Klassert
1 sibling, 1 reply; 16+ messages in thread
From: Martin Lau @ 2015-02-05 23:56 UTC (permalink / raw)
To: Steffen Klassert
Cc: Yang Yingliang, netdev, Hannes Frederic Sowa, David S. Miller
On Wed, Jan 28, 2015 at 01:11:51PM +0100, Steffen Klassert wrote:
> 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
> is not reachable any more.
>
> We fix this by cloning host routes if they are gatewayed,
> i.e. if pmtu events can happen.
>
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> ---
> net/ipv6/route.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index c910831..3e864e7 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -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_GATEWAY))
> nrt = rt6_alloc_clone(rt, &fl6->daddr);
The del path may also require changes. I am thinking:
1. Create a /128 via gateway route
2. Send some traffic and RTF_CACHE rt is created
3. Delete the /128 route by ip route del. I suspect the RTF_CACHE route may be
deleted and the route added in (1) stays.
Thanks
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RFC 1/2] ipv6: Fix after pmtu events dissapearing host routes
2015-02-05 23:56 ` Martin Lau
@ 2015-02-09 10:26 ` Steffen Klassert
2015-03-09 9:00 ` Steffen Klassert
0 siblings, 1 reply; 16+ messages in thread
From: Steffen Klassert @ 2015-02-09 10:26 UTC (permalink / raw)
To: Martin Lau; +Cc: Yang Yingliang, netdev, Hannes Frederic Sowa, David S. Miller
On Thu, Feb 05, 2015 at 03:56:29PM -0800, Martin Lau wrote:
> On Wed, Jan 28, 2015 at 01:11:51PM +0100, Steffen Klassert wrote:
> > 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
> > is not reachable any more.
> >
> > We fix this by cloning host routes if they are gatewayed,
> > i.e. if pmtu events can happen.
> >
> > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> > ---
> > net/ipv6/route.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> > index c910831..3e864e7 100644
> > --- a/net/ipv6/route.c
> > +++ b/net/ipv6/route.c
> > @@ -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_GATEWAY))
> > nrt = rt6_alloc_clone(rt, &fl6->daddr);
> The del path may also require changes. I am thinking:
> 1. Create a /128 via gateway route
> 2. Send some traffic and RTF_CACHE rt is created
> 3. Delete the /128 route by ip route del. I suspect the RTF_CACHE route may be
> deleted and the route added in (1) stays.
Good point.
Both routes are on the same fib node. The cached one has the better
metric, so I guess this one will be found and deleted.
I'll check this. Thanks for the hint!
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RFC 1/2] ipv6: Fix after pmtu events dissapearing host routes
2015-02-09 10:26 ` Steffen Klassert
@ 2015-03-09 9:00 ` Steffen Klassert
0 siblings, 0 replies; 16+ messages in thread
From: Steffen Klassert @ 2015-03-09 9:00 UTC (permalink / raw)
To: Martin Lau; +Cc: Yang Yingliang, netdev, Hannes Frederic Sowa, David S. Miller
On Mon, Feb 09, 2015 at 11:26:20AM +0100, Steffen Klassert wrote:
> On Thu, Feb 05, 2015 at 03:56:29PM -0800, Martin Lau wrote:
> > On Wed, Jan 28, 2015 at 01:11:51PM +0100, Steffen Klassert wrote:
> > > 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
> > > is not reachable any more.
> > >
> > > We fix this by cloning host routes if they are gatewayed,
> > > i.e. if pmtu events can happen.
> > >
> > > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> > > ---
> > > net/ipv6/route.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> > > index c910831..3e864e7 100644
> > > --- a/net/ipv6/route.c
> > > +++ b/net/ipv6/route.c
> > > @@ -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_GATEWAY))
> > > nrt = rt6_alloc_clone(rt, &fl6->daddr);
> > The del path may also require changes. I am thinking:
> > 1. Create a /128 via gateway route
> > 2. Send some traffic and RTF_CACHE rt is created
> > 3. Delete the /128 route by ip route del. I suspect the RTF_CACHE route may be
> > deleted and the route added in (1) stays.
>
> Good point.
>
> Both routes are on the same fib node. The cached one has the better
> metric, so I guess this one will be found and deleted.
>
> I'll check this. Thanks for the hint!
I finally got around to test this. It is as you say, we delete
the clone but not the original route.
I was able to fix this by adding the following:
---
net/ipv6/route.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index c20186f..e4f6a56 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1814,6 +1814,8 @@ static int ip6_route_del(struct fib6_config *cfg)
continue;
if (cfg->fc_metric && cfg->fc_metric != rt->rt6i_metric)
continue;
+ if (rt->rt6i_flags & RTF_CACHE)
+ continue;
dst_hold(&rt->dst);
read_unlock_bh(&table->tb6_lock);
It should be ok to ignore cached routes here because they are
removed with the original route. With this patch applied both
routes, the cached and the original go away when deleting.
I'll send an updated version of my patchset if this survives
some advanced testing.
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH RFC 2/2] ipv6: Extend the route lookups to low priority metrics.
2015-01-28 12:10 ` Steffen Klassert
2015-01-28 12:11 ` [PATCH RFC 1/2] ipv6: Fix after pmtu events dissapearing host routes Steffen Klassert
@ 2015-01-28 12:12 ` Steffen Klassert
2015-01-29 3:16 ` A problem about ICMP packet-too-big message handler Yang Yingliang
2 siblings, 0 replies; 16+ messages in thread
From: Steffen Klassert @ 2015-01-28 12:12 UTC (permalink / raw)
To: Yang Yingliang; +Cc: netdev, David S. Miller
We 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.
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
net/ipv6/route.c | 28 +++++++++++++++++++++++-----
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 3e864e7..ce354c9 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -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;
--
1.9.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: A problem about ICMP packet-too-big message handler
2015-01-28 12:10 ` Steffen Klassert
2015-01-28 12:11 ` [PATCH RFC 1/2] ipv6: Fix after pmtu events dissapearing host routes Steffen Klassert
2015-01-28 12:12 ` [PATCH RFC 2/2] ipv6: Extend the route lookups to low priority metrics Steffen Klassert
@ 2015-01-29 3:16 ` Yang Yingliang
2 siblings, 0 replies; 16+ messages in thread
From: Yang Yingliang @ 2015-01-29 3:16 UTC (permalink / raw)
To: Steffen Klassert; +Cc: netdev, David S. Miller
On 2015/1/28 20:10, Steffen Klassert wrote:
> On Tue, Jan 27, 2015 at 08:58:53PM +0800, Yang Yingliang wrote:
>> Hi,
>>
>> My kernel is 3.10 LTS.
>>
>> I got a problem here about handling ICMP packet-too-big message.
>>
>> Before sending a packet-too-big packet :
>>
>> # ip -6 route list table local
>> local ::1 dev lo metric 0
>> local fe80:: dev lo metric 0
>> local fe80:: dev lo metric 0
>> local fe80:: dev lo metric 0
>> local fe80:: dev lo metric 0
>> local fe80:: dev lo metric 0
>> local fe80:: dev lo metric 0
>> local fe80::1 dev lo metric 0 //It does not have expire value
>> local fe80::200:ff:fe00:0 dev lo metric 0
>> local fe80::200:ff:fe00:0 dev lo metric 0
>> local fe80::200:ff:fe00:0 dev lo metric 0
>> local fe80::200:ff:fe00:0 dev lo metric 0
>> local fe80::200:ff:fe00:2 dev lo metric 0
>> local fe80::5054:ff:fe12:3456 dev lo metric 0
>>
>>
>> After sending a packet-too-big packet
>>
>> ip -6 route list table local
>> local ::1 dev lo metric 0
>> local fe80:: dev lo metric 0
>> local fe80:: dev lo metric 0
>> local fe80:: dev lo metric 0
>> local fe80:: dev lo metric 0
>> local fe80:: dev lo metric 0
>> local fe80:: dev lo metric 0
>> local fe80::1 dev lo metric 0 expires _597_ //It has expire value
>
> Is this route still present after the expiration time is elapsed?
>
>> local fe80::200:ff:fe00:0 dev lo metric 0
>> local fe80::200:ff:fe00:0 dev lo metric 0
>> local fe80::200:ff:fe00:0 dev lo metric 0
>> local fe80::200:ff:fe00:0 dev lo metric 0
>> local fe80::200:ff:fe00:2 dev lo metric 0
>> local fe80::5054:ff:fe12:3456 dev lo metric 0
>>
>> When time is up, I can't ping fe80::1 .
>> Is it ok or a bug ?
>
> This looks pretty similar to a bug I discovered recently.
> In my case, a ipv6 host route dissapeared 10 minutes after
> a PMTU event. As a result, this host was not reachable
> anymore.
>
> This happens because we don't clone host routes before
> we use them. If a PMTU event happens, the original route
> is marked with an expire value. After the expiration time
> is elapsed, the original route is deleted and we loose
> conectivity to the host.
>
> I'm currently testing patches to fix this. With these
> patches the ipv6 host routes are cloned if they are
> gateway routes, i.e. if PMTU events can happen.
>
> I fear it will not fix your case because PMTU events are
> not expected to happen at local fe80 routes.
I found current kernel will do ip6_update_pmtu() anyway after
receiving an ICMPV6_PKT_TOOBIG type packet in icmpv6_err().
Does it need some more condition ? Like this:
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index a5e95199585e..7c0a28add109 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -89,8 +89,10 @@ static void icmpv6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
/* icmpv6_notify checks 8 bytes can be pulled, icmp6hdr is 8 bytes */
struct icmp6hdr *icmp6 = (struct icmp6hdr *) (skb->data + offset);
struct net *net = dev_net(skb->dev);
+ const struct ipv6hdr *iph = (struct ipv6hdr *) skb->data;
- if (type == ICMPV6_PKT_TOOBIG)
+ if (type == ICMPV6_PKT_TOOBIG &&
+ !(ipv6_addr_type(&iph->daddr) & IPV6_ADDR_LINKLOCAL))
ip6_update_pmtu(skb, net, info, 0, 0);
else if (type == NDISC_REDIRECT)
ip6_redirect(skb, net, skb->dev->ifindex, 0);
Regards,
Yang
^ permalink raw reply related [flat|nested] 16+ messages in thread