Patchwork ipv6: update Destination Cache entries when gateway turn into host

login
register
mail settings
Submitter Duan Jiong
Date May 9, 2014, 3:24 a.m.
Message ID <536C4A74.2050103@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/347292/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Duan Jiong - May 9, 2014, 3:24 a.m.
RFC 4861 states in 7.2.5:

	The IsRouter flag in the cache entry MUST be set based on the
         Router flag in the received advertisement.  In those cases
         where the IsRouter flag changes from TRUE to FALSE as a result
         of this update, the node MUST remove that router from the
         Default Router List and update the Destination Cache entries
         for all destinations using that neighbor as a router as
         specified in Section 7.3.3.  This is needed to detect when a
         node that is used as a router stops forwarding packets due to
         being configured as a host.

Currently, when dealing with NA Message which IsRouter flag changes from
TRUE to FALSE, the kernel only removes router from the Default Router List,
and don't update the Destination Cache entries.

Now in order to update those Destination Cache entries, i introduce
function rt6_clean_tohost().

Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
---
 include/net/ip6_route.h |  1 +
 net/ipv6/ndisc.c        |  7 ++-----
 net/ipv6/route.c        | 23 +++++++++++++++++++++++
 3 files changed, 26 insertions(+), 5 deletions(-)
Hannes Frederic Sowa - May 12, 2014, 12:54 a.m.
On Thu, May 8, 2014, at 20:24, Duan Jiong wrote:
> 
> RFC 4861 states in 7.2.5:
> 
> 	The IsRouter flag in the cache entry MUST be set based on the
>          Router flag in the received advertisement.  In those cases
>          where the IsRouter flag changes from TRUE to FALSE as a result
>          of this update, the node MUST remove that router from the
>          Default Router List and update the Destination Cache entries
>          for all destinations using that neighbor as a router as
>          specified in Section 7.3.3.  This is needed to detect when a
>          node that is used as a router stops forwarding packets due to
>          being configured as a host.
> 
> Currently, when dealing with NA Message which IsRouter flag changes from
> TRUE to FALSE, the kernel only removes router from the Default Router List,
> and don't update the Destination Cache entries.
> 
> Now in order to update those Destination Cache entries, i introduce
> function rt6_clean_tohost().
>
> [...]
>
> +/*remove routers and update dst entries when gateway turn into host.*/
> +static int fib6_clean_tohost(struct rt6_info *rt, void *arg)
> +{
> +	struct in6_addr *gateway = (struct in6_addr *)arg;
> +
> +	if (((rt->rt6i_flags & (RTF_ADDRCONF | RTF_DEFAULT | RTF_GATEWAY))
> +	    == (RTF_ADDRCONF | RTF_DEFAULT | RTF_GATEWAY))
> +	    && ipv6_addr_equal(gateway, &rt->rt6i_gateway)) {
> +		return -1;
> +	} else if (((rt->rt6i_flags & (RTF_GATEWAY | RTF_CACHE))
> +		      == (RTF_GATEWAY | RTF_CACHE))
> +		    && ipv6_addr_equal(gateway, &rt->rt6i_gateway)) {
> +		rt->rt6i_flags |= RTF_REJECT;
> +		rt->dst.error = -ENETUNREACH;
> +	}
> +	return 0;
> +}

I am not so happy with that but have not tried that.

The Destination Cache you quote from the RFC (if you follow 7.3.3.) actually refers to the neighbouring
subsystem, where we would need to generate subsequent errors in case we try to forward a packet
through a this particular router.

The reason why I am not that happy is, that the semantics when neighbour nodes are cleared is well
defined but we don't have that semantics when those rt6_nodes get cleared up. E.g. consider a router which just temporarily switches forwarding off and on.

I guess we need to inspect NTF_ROUTER flag in the output path somehow. :/

Greetings,

  Hannes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Duan Jiong - May 12, 2014, 3:07 a.m.
于 2014年05月12日 08:54, Hannes Frederic Sowa 写道:
> On Thu, May 8, 2014, at 20:24, Duan Jiong wrote:
>>
>> RFC 4861 states in 7.2.5:
>>
>> 	The IsRouter flag in the cache entry MUST be set based on the
>>          Router flag in the received advertisement.  In those cases
>>          where the IsRouter flag changes from TRUE to FALSE as a result
>>          of this update, the node MUST remove that router from the
>>          Default Router List and update the Destination Cache entries
>>          for all destinations using that neighbor as a router as
>>          specified in Section 7.3.3.  This is needed to detect when a
>>          node that is used as a router stops forwarding packets due to
>>          being configured as a host.
>>
>> Currently, when dealing with NA Message which IsRouter flag changes from
>> TRUE to FALSE, the kernel only removes router from the Default Router List,
>> and don't update the Destination Cache entries.
>>
>> Now in order to update those Destination Cache entries, i introduce
>> function rt6_clean_tohost().
>>
>> [...]
>>
>> +/*remove routers and update dst entries when gateway turn into host.*/
>> +static int fib6_clean_tohost(struct rt6_info *rt, void *arg)
>> +{
>> +	struct in6_addr *gateway = (struct in6_addr *)arg;
>> +
>> +	if (((rt->rt6i_flags & (RTF_ADDRCONF | RTF_DEFAULT | RTF_GATEWAY))
>> +	    == (RTF_ADDRCONF | RTF_DEFAULT | RTF_GATEWAY))
>> +	    && ipv6_addr_equal(gateway, &rt->rt6i_gateway)) {
>> +		return -1;
>> +	} else if (((rt->rt6i_flags & (RTF_GATEWAY | RTF_CACHE))
>> +		      == (RTF_GATEWAY | RTF_CACHE))
>> +		    && ipv6_addr_equal(gateway, &rt->rt6i_gateway)) {
>> +		rt->rt6i_flags |= RTF_REJECT;
>> +		rt->dst.error = -ENETUNREACH;
>> +	}
>> +	return 0;
>> +}
> 
> I am not so happy with that but have not tried that.
> 
> The Destination Cache you quote from the RFC (if you follow 7.3.3.) actually refers to the neighbouring
> subsystem, where we would need to generate subsequent errors in case we try to forward a packet
> through a this particular router.
> 
> The reason why I am not that happy is, that the semantics when neighbour nodes are cleared is well
> defined but we don't have that semantics when those rt6_nodes get cleared up. E.g. consider a router which just temporarily switches forwarding off and on.
> 
> I guess we need to inspect NTF_ROUTER flag in the output path somehow. :/

Why we need to inspect NTF_ROUTER flag?
In my opinion, the problem is that we can't use the neighbour node as next hop.

Actually, after a node switches from being a router to being a host, we should 
perform next-hop determination rather than continue sending traffic to the former
router, so we could just delete those Destination Cache entries.

You can refer to the below:
RFC 4861 states in 6.3.5: 
   Whenever the Lifetime of an entry in the Default Router List expires,
   that entry is discarded.  When removing a router from the Default
   Router list, the node MUST update the Destination Cache in such a way
   that all entries using the router perform next-hop determination
   again rather than continue sending traffic to the (deleted) router.

Thanks,
  Duan
> 
> Greetings,
> 
>   Hannes
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - May 12, 2014, 5:01 a.m.
From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
Date: Fri, 9 May 2014 11:24:36 +0800

> +/*remove routers and update dst entries when gateway turn into host.*/

This comment is poorly formatted.

Please put a space after "/*", capitalize "Remove" and place
a space before "*/" at the end of the comment.

> +static int fib6_clean_tohost(struct rt6_info *rt, void *arg)
> +{
> +	struct in6_addr *gateway = (struct in6_addr *)arg;
> +
> +	if (((rt->rt6i_flags & (RTF_ADDRCONF | RTF_DEFAULT | RTF_GATEWAY))
> +	    == (RTF_ADDRCONF | RTF_DEFAULT | RTF_GATEWAY))
> +	    && ipv6_addr_equal(gateway, &rt->rt6i_gateway)) {

This conditional is poorly formatted:

> +	} else if (((rt->rt6i_flags & (RTF_GATEWAY | RTF_CACHE))
> +		      == (RTF_GATEWAY | RTF_CACHE))
> +		    && ipv6_addr_equal(gateway, &rt->rt6i_gateway)) {

As is this one.

Never put operators at the beginning of a continuation line, rather
put them at the end of the previous line.  This includes "&&", "||",
and "=="
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hannes Frederic Sowa - May 12, 2014, 10:13 a.m.
On Sun, May 11, 2014, at 20:07, Duan Jiong wrote:
> 于 2014年05月12日 08:54, Hannes Frederic Sowa 写道:
> > On Thu, May 8, 2014, at 20:24, Duan Jiong wrote:
> >>
> >> RFC 4861 states in 7.2.5:
> >>
> >> 	The IsRouter flag in the cache entry MUST be set based on the
> >>          Router flag in the received advertisement.  In those cases
> >>          where the IsRouter flag changes from TRUE to FALSE as a result
> >>          of this update, the node MUST remove that router from the
> >>          Default Router List and update the Destination Cache entries
> >>          for all destinations using that neighbor as a router as
> >>          specified in Section 7.3.3.  This is needed to detect when a
> >>          node that is used as a router stops forwarding packets due to
> >>          being configured as a host.
> >>
> >> Currently, when dealing with NA Message which IsRouter flag changes from
> >> TRUE to FALSE, the kernel only removes router from the Default Router List,
> >> and don't update the Destination Cache entries.
> >>
> >> Now in order to update those Destination Cache entries, i introduce
> >> function rt6_clean_tohost().
> >>
> >> [...]
> >>
> >> +/*remove routers and update dst entries when gateway turn into host.*/
> >> +static int fib6_clean_tohost(struct rt6_info *rt, void *arg)
> >> +{
> >> +	struct in6_addr *gateway = (struct in6_addr *)arg;
> >> +
> >> +	if (((rt->rt6i_flags & (RTF_ADDRCONF | RTF_DEFAULT | RTF_GATEWAY))
> >> +	    == (RTF_ADDRCONF | RTF_DEFAULT | RTF_GATEWAY))
> >> +	    && ipv6_addr_equal(gateway, &rt->rt6i_gateway)) {
> >> +		return -1;
> >> +	} else if (((rt->rt6i_flags & (RTF_GATEWAY | RTF_CACHE))
> >> +		      == (RTF_GATEWAY | RTF_CACHE))
> >> +		    && ipv6_addr_equal(gateway, &rt->rt6i_gateway)) {
> >> +		rt->rt6i_flags |= RTF_REJECT;
> >> +		rt->dst.error = -ENETUNREACH;
> >> +	}
> >> +	return 0;
> >> +}
> > 
> > I am not so happy with that but have not tried that.
> > 
> > The Destination Cache you quote from the RFC (if you follow 7.3.3.) actually refers to the neighbouring
> > subsystem, where we would need to generate subsequent errors in case we try to forward a packet
> > through a this particular router.
> > 
> > The reason why I am not that happy is, that the semantics when neighbour nodes are cleared is well
> > defined but we don't have that semantics when those rt6_nodes get cleared up. E.g. consider a router which just temporarily switches forwarding off and on.
> > 
> > I guess we need to inspect NTF_ROUTER flag in the output path somehow. :/
> 
> Why we need to inspect NTF_ROUTER flag?
> In my opinion, the problem is that we can't use the neighbour node as next hop.

I agree. I don't see a problem with returning -1 from the function but with the case
where you originate errors from the routing table by setting rt->dst.error. These
entries have a lifetime governed by the gc.

Greetings,

  Hannes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
index 6c4f5ea..216cecc 100644
--- a/include/net/ip6_route.h
+++ b/include/net/ip6_route.h
@@ -127,6 +127,7 @@  int rt6_dump_route(struct rt6_info *rt, void *p_arg);
 void rt6_ifdown(struct net *net, struct net_device *dev);
 void rt6_mtu_change(struct net_device *dev, unsigned int mtu);
 void rt6_remove_prefsrc(struct inet6_ifaddr *ifp);
+void rt6_clean_tohost(struct net *net, struct in6_addr *gateway);
 
 
 /*
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 09a22f4..ca8d4ea 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -851,7 +851,7 @@  out:
 static void ndisc_recv_na(struct sk_buff *skb)
 {
 	struct nd_msg *msg = (struct nd_msg *)skb_transport_header(skb);
-	const struct in6_addr *saddr = &ipv6_hdr(skb)->saddr;
+	struct in6_addr *saddr = &ipv6_hdr(skb)->saddr;
 	const struct in6_addr *daddr = &ipv6_hdr(skb)->daddr;
 	u8 *lladdr = NULL;
 	u32 ndoptlen = skb_tail_pointer(skb) - (skb_transport_header(skb) +
@@ -944,10 +944,7 @@  static void ndisc_recv_na(struct sk_buff *skb)
 			/*
 			 * Change: router to host
 			 */
-			struct rt6_info *rt;
-			rt = rt6_get_dflt_router(saddr, dev);
-			if (rt)
-				ip6_del_rt(rt);
+			rt6_clean_tohost(dev_net(dev),  saddr);
 		}
 
 out:
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 004fffb..ebde127 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2234,6 +2234,29 @@  void rt6_remove_prefsrc(struct inet6_ifaddr *ifp)
 	fib6_clean_all(net, fib6_remove_prefsrc, &adni);
 }
 
+/*remove routers and update dst entries when gateway turn into host.*/
+static int fib6_clean_tohost(struct rt6_info *rt, void *arg)
+{
+	struct in6_addr *gateway = (struct in6_addr *)arg;
+
+	if (((rt->rt6i_flags & (RTF_ADDRCONF | RTF_DEFAULT | RTF_GATEWAY))
+	    == (RTF_ADDRCONF | RTF_DEFAULT | RTF_GATEWAY))
+	    && ipv6_addr_equal(gateway, &rt->rt6i_gateway)) {
+		return -1;
+	} else if (((rt->rt6i_flags & (RTF_GATEWAY | RTF_CACHE))
+		      == (RTF_GATEWAY | RTF_CACHE))
+		    && ipv6_addr_equal(gateway, &rt->rt6i_gateway)) {
+		rt->rt6i_flags |= RTF_REJECT;
+		rt->dst.error = -ENETUNREACH;
+	}
+	return 0;
+}
+
+void rt6_clean_tohost(struct net *net, struct in6_addr *gateway)
+{
+	fib6_clean_all(net, fib6_clean_tohost, gateway);
+}
+
 struct arg_dev_net {
 	struct net_device *dev;
 	struct net *net;