[PATCH net 0/3] ipv6: use rt6i_gateway as nexthop

[PATCH net 0/3] ipv6: use rt6i_gateway as nexthop

[Julian Anastasov]

	The following patchset makes sure that rt6i_gateway
contains valid nexthop information in all cases, so that
we can use different nexthop for sending.

	The first patch is a simple fix that makes IPVS, TEE,
RAW(hdrincl) and RTF_DYNAMIC(without RTF_GATEWAY) work as
before 3.9. There is a single corner case not solved by
this patch: RAW(hdrincl) or TEE using local address for
nexthop, a silly feature, I guess. In this case we
see zeroes in rt6i_gateway because we get route that is not
cloned. This is solved only with patch 2.

	The second patch is an optimization that makes sure
all resulting routes have rt6i_gateway filled, so that we
can avoid the complex ipv6_addr_any() call added to rt6_nexthop()
by patch 1. And it sets rt6i_gateway for local routes, a case
not handled by patch 1.

	The third patch uses the new rt6_nexthop() function to fix
the matching of gateways in the same way as commit bbb5823cf742a7
("netfilter: nf_conntrack: fix rt_gateway checks for H.323 helper")
fixes nf_conntrack_h323_main.c for IPv4. Currently, it depends on
the new definition of rt6_nexthop() in patch 2. Actually, if
patch 2 is applied, patch 3 becomes a cosmetic change.

	I see the following two alternatives for applying these
patches:

1. Linger patch 2 in net-next to avoid surprises in the upcoming
release. In this case patch 3 can be reworked not to depend on
the new rt6_nexthop() definition in patch 2. I guess this is a
better option, so that patch 2 can be reviewed and tested for
longer time.

2. Include all 3 patches in net tree - more risky because this
is my first attempt to change IPv6.

	Here is the situation as handled by patch 2:

	In IPv6 the resolved routes are always host routes (/128
with DST_HOST), mostly cloned ones. We allow routes in FIB
to contain rt6i_gateway with zeroes (eg. for local subnets) but
on cloning we can fill the rt6i_gateway field in result.
This works even without this patchset.

	There is a single special case where dst is provided as
skb_dst directly without a routing call: icmp6_dst_alloc(). It is a
private dst allocated just for the particular ICMP packet. Patch 2
fills rt6i_gateway in this case, needed for the new rt6_nexthop()
simplification.

	The last case is addrconf_dst_alloc(), it can put in
FIB local/anycast routes when addresses are added. Patch 2
needs to fill rt6i_gateway in this case because such routes
are returned without cloning.

Julian Anastasov (3):
  ipv6: always prefer rt6i_gateway if present
  ipv6: fill rt6i_gateway with nexthop address
  netfilter: nf_conntrack: fix rt6i_gateway checks for H.323 helper

 include/net/ip6_route.h                | 6 ++----
 net/ipv6/ip6_output.c                  | 4 ++--
 net/ipv6/route.c                       | 8 ++++++--
 net/netfilter/nf_conntrack_h323_main.c | 4 ++--
 4 files changed, 12 insertions(+), 10 deletions(-)

-- 
1.8.3.1

[PATCH net 1/3] ipv6: always prefer rt6i_gateway if present

[Julian Anastasov]

In v3.9 6fd6ce2056de2709 ("ipv6: Do not depend on rt->n in
ip6_finish_output2()." changed the behaviour of ip6_finish_output2()
such that the recently introduced rt6_nexthop() is used
instead of an assigned neighbor.

As rt6_nexthop() prefers rt6i_gateway only for gatewayed
routes this causes a problem for users like IPVS, xt_TEE and
RAW(hdrincl) if they want to use different address for routing
compared to the destination address.

Another case is when redirect can create RTF_DYNAMIC
route without RTF_GATEWAY flag, we ignore the rt6i_gateway
in rt6_nexthop().

Fix the above problems by considering the rt6i_gateway if
present, so that traffic routed to address on local subnet is
not wrongly diverted to the destination address.

Thanks to Simon Horman and Phil Oester for spotting the
problematic commit.

Thanks to Hannes Frederic Sowa for his review and help in testing.

Reported-by: Phil Oester <kernel@linuxace.com>
Reported-by: Mark Brooks <mark@loadbalancer.org>
Signed-off-by: Julian Anastasov <ja@ssi.bg>
---
 include/net/ip6_route.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
index f525e70..481404a 100644
--- a/include/net/ip6_route.h
+++ b/include/net/ip6_route.h
@@ -196,7 +196,7 @@ static inline int ip6_skb_dst_mtu(struct sk_buff *skb)
 
 static inline struct in6_addr *rt6_nexthop(struct rt6_info *rt, struct in6_addr *dest)
 {
-	if (rt->rt6i_flags & RTF_GATEWAY)
+	if (rt->rt6i_flags & RTF_GATEWAY || !ipv6_addr_any(&rt->rt6i_gateway))
 		return &rt->rt6i_gateway;
 	return dest;
 }
-- 
1.8.3.1

[PATCH net 2/3] ipv6: fill rt6i_gateway with nexthop address

[Julian Anastasov]

Make sure rt6i_gateway contains nexthop information in
all routes returned from lookup or when routes are directly
attached to skb for generated ICMP packets.

The effect of this patch should be a faster version of
rt6_nexthop() and the consideration of local addresses as
nexthop.

Signed-off-by: Julian Anastasov <ja@ssi.bg>
---
 include/net/ip6_route.h | 6 ++----
 net/ipv6/ip6_output.c   | 4 ++--
 net/ipv6/route.c        | 8 ++++++--
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
index 481404a..2b786b7 100644
--- a/include/net/ip6_route.h
+++ b/include/net/ip6_route.h
@@ -194,11 +194,9 @@ static inline int ip6_skb_dst_mtu(struct sk_buff *skb)
 	       skb_dst(skb)->dev->mtu : dst_mtu(skb_dst(skb));
 }
 
-static inline struct in6_addr *rt6_nexthop(struct rt6_info *rt, struct in6_addr *dest)
+static inline struct in6_addr *rt6_nexthop(struct rt6_info *rt)
 {
-	if (rt->rt6i_flags & RTF_GATEWAY || !ipv6_addr_any(&rt->rt6i_gateway))
-		return &rt->rt6i_gateway;
-	return dest;
+	return &rt->rt6i_gateway;
 }
 
 #endif
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index a54c45c..2b404df 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -105,7 +105,7 @@ static int ip6_finish_output2(struct sk_buff *skb)
 	}
 
 	rcu_read_lock_bh();
-	nexthop = rt6_nexthop((struct rt6_info *)dst, &ipv6_hdr(skb)->daddr);
+	nexthop = rt6_nexthop((struct rt6_info *)dst);
 	neigh = __ipv6_neigh_lookup_noref(dst->dev, nexthop);
 	if (unlikely(!neigh))
 		neigh = __neigh_create(&nd_tbl, nexthop, dst->dev, false);
@@ -874,7 +874,7 @@ static int ip6_dst_lookup_tail(struct sock *sk,
 	 */
 	rt = (struct rt6_info *) *dst;
 	rcu_read_lock_bh();
-	n = __ipv6_neigh_lookup_noref(rt->dst.dev, rt6_nexthop(rt, &fl6->daddr));
+	n = __ipv6_neigh_lookup_noref(rt->dst.dev, rt6_nexthop(rt));
 	err = n && !(n->nud_state & NUD_VALID) ? -EINVAL : 0;
 	rcu_read_unlock_bh();
 
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index c979dd9..c1ee381 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -851,7 +851,6 @@ static struct rt6_info *rt6_alloc_cow(struct rt6_info *ort,
 			if (ort->rt6i_dst.plen != 128 &&
 			    ipv6_addr_equal(&ort->rt6i_dst.addr, daddr))
 				rt->rt6i_flags |= RTF_ANYCAST;
-			rt->rt6i_gateway = *daddr;
 		}
 
 		rt->rt6i_flags |= RTF_CACHE;
@@ -1338,6 +1337,7 @@ struct dst_entry *icmp6_dst_alloc(struct net_device *dev,
 	rt->dst.flags |= DST_HOST;
 	rt->dst.output  = ip6_output;
 	atomic_set(&rt->dst.__refcnt, 1);
+	rt->rt6i_gateway  = fl6->daddr;
 	rt->rt6i_dst.addr = fl6->daddr;
 	rt->rt6i_dst.plen = 128;
 	rt->rt6i_idev     = idev;
@@ -1873,7 +1873,10 @@ static struct rt6_info *ip6_rt_copy(struct rt6_info *ort,
 			in6_dev_hold(rt->rt6i_idev);
 		rt->dst.lastuse = jiffies;
 
-		rt->rt6i_gateway = ort->rt6i_gateway;
+		if (ort->rt6i_flags & RTF_GATEWAY)
+			rt->rt6i_gateway = ort->rt6i_gateway;
+		else
+			rt->rt6i_gateway = *dest;
 		rt->rt6i_flags = ort->rt6i_flags;
 		if ((ort->rt6i_flags & (RTF_DEFAULT | RTF_ADDRCONF)) ==
 		    (RTF_DEFAULT | RTF_ADDRCONF))
@@ -2160,6 +2163,7 @@ struct rt6_info *addrconf_dst_alloc(struct inet6_dev *idev,
 	else
 		rt->rt6i_flags |= RTF_LOCAL;
 
+	rt->rt6i_gateway  = *addr;
 	rt->rt6i_dst.addr = *addr;
 	rt->rt6i_dst.plen = 128;
 	rt->rt6i_table = fib6_get_table(net, RT6_TABLE_LOCAL);
-- 
1.8.3.1

[PATCH net 3/3] netfilter: nf_conntrack: fix rt6i_gateway checks for H.323 helper

[Julian Anastasov]

Now when rt6_nexthop() can return nexthop address we can use it
for proper nexthop comparison of directly connected destinations.
For more information refer to commit bbb5823cf742a7
("netfilter: nf_conntrack: fix rt_gateway checks for H.323 helper").

Signed-off-by: Julian Anastasov <ja@ssi.bg>
---
 net/netfilter/nf_conntrack_h323_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index bdebd03..70866d1 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -778,8 +778,8 @@ static int callforward_do_filter(const union nf_inet_addr *src,
 				   flowi6_to_flowi(&fl1), false)) {
 			if (!afinfo->route(&init_net, (struct dst_entry **)&rt2,
 					   flowi6_to_flowi(&fl2), false)) {
-				if (!memcmp(&rt1->rt6i_gateway, &rt2->rt6i_gateway,
-					    sizeof(rt1->rt6i_gateway)) &&
+				if (ipv6_addr_equal(rt6_nexthop(rt1),
+						    rt6_nexthop(rt2)) &&
 				    rt1->dst.dev == rt2->dst.dev)
 					ret = 1;
 				dst_release(&rt2->dst);
-- 
1.8.3.1

Re: [PATCH net 2/3] ipv6: fill rt6i_gateway with nexthop address

[Hannes Frederic Sowa]

On Sun, Oct 20, 2013 at 03:43:04PM +0300, Julian Anastasov wrote:
> Make sure rt6i_gateway contains nexthop information in
> all routes returned from lookup or when routes are directly
> attached to skb for generated ICMP packets.
> 
> The effect of this patch should be a faster version of
> rt6_nexthop() and the consideration of local addresses as
> nexthop.
> 
> Signed-off-by: Julian Anastasov <ja@ssi.bg>
> ---
>  include/net/ip6_route.h | 6 ++----
>  net/ipv6/ip6_output.c   | 4 ++--
>  net/ipv6/route.c        | 8 ++++++--
>  3 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
> index 481404a..2b786b7 100644
> --- a/include/net/ip6_route.h
> +++ b/include/net/ip6_route.h
> @@ -194,11 +194,9 @@ static inline int ip6_skb_dst_mtu(struct sk_buff *skb)
>  	       skb_dst(skb)->dev->mtu : dst_mtu(skb_dst(skb));
>  }
>  
> -static inline struct in6_addr *rt6_nexthop(struct rt6_info *rt, struct in6_addr *dest)
> +static inline struct in6_addr *rt6_nexthop(struct rt6_info *rt)
>  {
> -	if (rt->rt6i_flags & RTF_GATEWAY || !ipv6_addr_any(&rt->rt6i_gateway))
> -		return &rt->rt6i_gateway;
> -	return dest;
> +	return &rt->rt6i_gateway;
>  }
>  

Hm, looks good. Will test is soon.

One small remark:

Maybe we can kill rt6_nexthop completly and use rt6i_gateway directly so a
reader does not have to lookup this function.

Thanks,

  Hannes

Re: [PATCH net 1/3] ipv6: always prefer rt6i_gateway if present

[Hannes Frederic Sowa]

On Sun, Oct 20, 2013 at 03:43:03PM +0300, Julian Anastasov wrote:
> In v3.9 6fd6ce2056de2709 ("ipv6: Do not depend on rt->n in
> ip6_finish_output2()." changed the behaviour of ip6_finish_output2()
> such that the recently introduced rt6_nexthop() is used
> instead of an assigned neighbor.
> 
> As rt6_nexthop() prefers rt6i_gateway only for gatewayed
> routes this causes a problem for users like IPVS, xt_TEE and
> RAW(hdrincl) if they want to use different address for routing
> compared to the destination address.
> 
> Another case is when redirect can create RTF_DYNAMIC
> route without RTF_GATEWAY flag, we ignore the rt6i_gateway
> in rt6_nexthop().
> 
> Fix the above problems by considering the rt6i_gateway if
> present, so that traffic routed to address on local subnet is
> not wrongly diverted to the destination address.
> 
> Thanks to Simon Horman and Phil Oester for spotting the
> problematic commit.
> 
> Thanks to Hannes Frederic Sowa for his review and help in testing.
> 
> Reported-by: Phil Oester <kernel@linuxace.com>
> Reported-by: Mark Brooks <mark@loadbalancer.org>
> Signed-off-by: Julian Anastasov <ja@ssi.bg>

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Re: [PATCH net 2/3] ipv6: fill rt6i_gateway with nexthop address

[Hannes Frederic Sowa]

On Sun, Oct 20, 2013 at 03:43:04PM +0300, Julian Anastasov wrote:
> Make sure rt6i_gateway contains nexthop information in
> all routes returned from lookup or when routes are directly
> attached to skb for generated ICMP packets.
> 
> The effect of this patch should be a faster version of
> rt6_nexthop() and the consideration of local addresses as
> nexthop.
> 
> Signed-off-by: Julian Anastasov <ja@ssi.bg>

The patch is fine. I don't mind if we leave it as is or remove rt6_nexthop,
so:

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Thanks,

  Hannes

Re: [PATCH net 3/3] netfilter: nf_conntrack: fix rt6i_gateway checks for H.323 helper

[Hannes Frederic Sowa]

On Sun, Oct 20, 2013 at 03:43:05PM +0300, Julian Anastasov wrote:
> Now when rt6_nexthop() can return nexthop address we can use it
> for proper nexthop comparison of directly connected destinations.
> For more information refer to commit bbb5823cf742a7
> ("netfilter: nf_conntrack: fix rt_gateway checks for H.323 helper").
> 
> Signed-off-by: Julian Anastasov <ja@ssi.bg>

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Cool, that the bug is fixed and the code is a bit more simple. :)

Thanks,

  Hannes

Re: [PATCH net 2/3] ipv6: fill rt6i_gateway with nexthop address

[Julian Anastasov]

	Hello,

On Mon, 21 Oct 2013, Hannes Frederic Sowa wrote:

> On Sun, Oct 20, 2013 at 03:43:04PM +0300, Julian Anastasov wrote:
> > Make sure rt6i_gateway contains nexthop information in
> > all routes returned from lookup or when routes are directly
> > attached to skb for generated ICMP packets.
> > 
> > The effect of this patch should be a faster version of
> > rt6_nexthop() and the consideration of local addresses as
> > nexthop.
> > 
> > Signed-off-by: Julian Anastasov <ja@ssi.bg>
> 
> The patch is fine. I don't mind if we leave it as is or remove rt6_nexthop,
> so:
> 
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

	Thanks for the review! I don't mind too about
removing rt6_nexthop. For me it is 51% against 49% to keep it
as it denotes the places that use nexthop and not gateway.
May be more opinions will help to decide because I don't know
if there are any plans to use similar techniques as done for IPv4.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH net 0/3] ipv6: use rt6i_gateway as nexthop

[Hannes Frederic Sowa]

On Sun, Oct 20, 2013 at 03:43:02PM +0300, Julian Anastasov wrote:
> 	The second patch is an optimization that makes sure
> all resulting routes have rt6i_gateway filled, so that we
> can avoid the complex ipv6_addr_any() call added to rt6_nexthop()
> by patch 1. And it sets rt6i_gateway for local routes, a case
> not handled by patch 1.

Not related to the patch:

That reminds me that Yoshifuji had the idea to cache the results for
ipv6_addr_type in IP6CB to avoid calling this function over and over again.
Maybe we can do the same for rt6_infos to save some cycles here and there.


Also, what do you think about this site:

net/ipv6/ip6_output.c:
    411 
    412                 rt = (struct rt6_info *) dst;
    413                 if (rt->rt6i_flags & RTF_GATEWAY)
    414                         target = &rt->rt6i_gateway;
    415                 else
    416                         target = &hdr->daddr;
    417 

Our provided skb_dst should come from ip6_route_input, thus ip6_pol_route. So
I assume we have rt6i_gateway == hdr->daddr there, too. It is a bit more
complicated because of possible routing extension headers. Maybe you already
looked at this already?

I just found it while searching which other code paths do emit packets
while xt_TEE is processing (generation of redirects) and could also lead
to stack exhaustion. But the path in ip6_forward seems fine.

Greetings,

  Hannes

Re: [PATCH net 0/3] ipv6: use rt6i_gateway as nexthop

[Julian Anastasov]

	Hello,

On Mon, 21 Oct 2013, Hannes Frederic Sowa wrote:

> Not related to the patch:
> 
> That reminds me that Yoshifuji had the idea to cache the results for
> ipv6_addr_type in IP6CB to avoid calling this function over and over again.
> Maybe we can do the same for rt6_infos to save some cycles here and there.

	Yes, ipv6_addr_type has little price. May be only
DNAT and IPSec can complicate such caching.

> Also, what do you think about this site:
> 
> net/ipv6/ip6_output.c:
>     411 
>     412                 rt = (struct rt6_info *) dst;
>     413                 if (rt->rt6i_flags & RTF_GATEWAY)
>     414                         target = &rt->rt6i_gateway;
>     415                 else
>     416                         target = &hdr->daddr;
>     417 
> 
> Our provided skb_dst should come from ip6_route_input, thus ip6_pol_route. So
> I assume we have rt6i_gateway == hdr->daddr there, too. It is a bit more
> complicated because of possible routing extension headers. Maybe you already
> looked at this already?

	Yes, I checked every site that uses rt6i_gateway but
not with the perspective to use rt6_nexthop(). It seems
this is a rt6_nexthop() candidate.

> I just found it while searching which other code paths do emit packets
> while xt_TEE is processing (generation of redirects) and could also lead
> to stack exhaustion. But the path in ip6_forward seems fine.

	Yes. I found only one place more dangerous:
fib6_add_rt2node(). But I think its checks are still valid.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH net 0/3] ipv6: use rt6i_gateway as nexthop

[David Miller]

From: Julian Anastasov <ja@ssi.bg>
Date: Sun, 20 Oct 2013 15:43:02 +0300

> 	I see the following two alternatives for applying these
> patches:
> 
> 1. Linger patch 2 in net-next to avoid surprises in the upcoming
> release. In this case patch 3 can be reworked not to depend on
> the new rt6_nexthop() definition in patch 2. I guess this is a
> better option, so that patch 2 can be reviewed and tested for
> longer time.
> 
> 2. Include all 3 patches in net tree - more risky because this
> is my first attempt to change IPv6.

I have decided to merge all three patches into -net right now.
I've reviewed these patches several times and they look good
to me.

I'll let them cook upstream for at least a week before submitting them
to -stable to let any last minute errors show themselves and
subsequently get resolved.

Thanks!

Re: [PATCH net 2/3] ipv6: fill rt6i_gateway with nexthop address

[David Miller]

From: Julian Anastasov <ja@ssi.bg>
Date: Mon, 21 Oct 2013 10:31:06 +0300 (EEST)

> 	Thanks for the review! I don't mind too about
> removing rt6_nexthop. For me it is 51% against 49% to keep it
> as it denotes the places that use nexthop and not gateway.
> May be more opinions will help to decide because I don't know
> if there are any plans to use similar techniques as done for IPv4.

I have no strong opinion about removing rt6_nexthop.

If it is of low cost today and makes the code easier to
undersand and grep, just leave it alone.

Re: [PATCH net 0/3] ipv6: use rt6i_gateway as nexthop

[Simon Horman]

On Mon, Oct 21, 2013 at 06:40:57PM -0400, David Miller wrote:
> From: Julian Anastasov <ja@ssi.bg>
> Date: Sun, 20 Oct 2013 15:43:02 +0300
> 
> > 	I see the following two alternatives for applying these
> > patches:
> > 
> > 1. Linger patch 2 in net-next to avoid surprises in the upcoming
> > release. In this case patch 3 can be reworked not to depend on
> > the new rt6_nexthop() definition in patch 2. I guess this is a
> > better option, so that patch 2 can be reviewed and tested for
> > longer time.
> > 
> > 2. Include all 3 patches in net tree - more risky because this
> > is my first attempt to change IPv6.
> 
> I have decided to merge all three patches into -net right now.
> I've reviewed these patches several times and they look good
> to me.
> 
> I'll let them cook upstream for at least a week before submitting them
> to -stable to let any last minute errors show themselves and
> subsequently get resolved.

Thanks Dave,

FWIW, I have verified that these changes resolve the problem
that I reported with IPVS that I believe prompted Julian to write
these changes. That is IPv6 IPVS-DR once again works with these
changes in place.