netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [BUG] ipv6: all routes share same inetpeer
@ 2011-07-19 17:23 Eric Dumazet
  2011-07-19 17:37 ` David Miller
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2011-07-19 17:23 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

Hi David

While polishing a patch and testing it, I found that all ipv6 routes
shared the same inetpeer ! Oh well...


Apparently we call rt6_bind_peer() at wrong time, providing NULL
addresses.

Maybe you can find the bug before me ?

With following quick/dirty/debugging patch :

diff --git a/include/net/inetpeer.h b/include/net/inetpeer.h
index 39d1230..f24391c 100644
--- a/include/net/inetpeer.h
+++ b/include/net/inetpeer.h
@@ -88,6 +88,7 @@ static inline struct inet_peer *inet_getpeer_v6(const struct in6_addr *v6daddr,
 
        ipv6_addr_copy((struct in6_addr *)daddr.addr.a6, v6daddr);
        daddr.family = AF_INET6;
+       WARN_ON(daddr.addr.a6[0] == 0 && daddr.addr.a6[1] == 0 && daddr.addr.a6[2] == 0 && daddr.addr.a6[3] == 0);
        return inet_getpeer(&daddr, create);
 }
 

I get for example :

[  299.024117] ------------[ cut here ]------------
[  299.024176] WARNING: at include/net/inetpeer.h:91 rt6_bind_peer+0x84/0xb0()
[  299.024234] Hardware name: ProLiant BL460c G1
[  299.024287] Modules linked in: xt_hashlimit ipmi_devintf ipmi_si ipmi_msghandler tg3 bonding
[  299.024583] Pid: 7119, comm: ping6 Tainted: G        W   3.0.0-rc7-03555-ge798b6e-dirty #1048
[  299.024657] Call Trace:
[  299.024709]  [<c1042b4d>] warn_slowpath_common+0x6d/0xa0
[  299.024765]  [<c1373104>] ? rt6_bind_peer+0x84/0xb0
[  299.024820]  [<c1373104>] ? rt6_bind_peer+0x84/0xb0
[  299.024875]  [<c1042b9d>] warn_slowpath_null+0x1d/0x20
[  299.024931]  [<c1373104>] rt6_bind_peer+0x84/0xb0
[  299.024985]  [<c13731ec>] ipv6_cow_metrics+0xbc/0xe0
[  299.025046]  [<c13722a8>] ip6_rt_copy+0x1e8/0x210
[  299.025101]  [<c1372a70>] rt6_alloc_cow.isra.32+0x10/0x1d0
[  299.025158]  [<c1048fb9>] ? local_bh_enable_ip+0x59/0xc0
[  299.025213]  [<c137356b>] ip6_pol_route.isra.37+0x29b/0x2a0
[  299.025270]  [<c13735a1>] ip6_pol_route_output+0x31/0x40
[  299.025325]  [<c1376277>] fib6_rule_lookup+0x17/0x20
[  299.025380]  [<c137238c>] ip6_route_output+0x5c/0xa0
[  299.025436]  [<c1373570>] ? ip6_pol_route.isra.37+0x2a0/0x2a0
[  299.025492]  [<c1365004>] ip6_dst_lookup_tail+0xd4/0xe0
[  299.025548]  [<c136519f>] ip6_dst_lookup_flow+0x2f/0x90
[  299.025604]  [<c1048fb9>] ? local_bh_enable_ip+0x59/0xc0
[  299.025660]  [<c1390ef4>] ip6_datagram_connect+0x174/0x490
[  299.025717]  [<c12bdd42>] ? release_sock+0xf2/0x150
[  299.025772]  [<c137d9a7>] ? udp_v6_get_port+0x47/0x60
[  299.025829]  [<c132f838>] inet_dgram_connect+0x28/0x70
[  299.025884]  [<c12bc5c0>] sys_connect+0x60/0xa0
[  299.025939]  [<c10d239e>] ? might_fault+0x2e/0x80
[  299.026001]  [<c13c438d>] ? _raw_spin_unlock+0x1d/0x20
[  299.026057]  [<c10d239e>] ? might_fault+0x2e/0x80
[  299.026117]  [<c10d23e4>] ? might_fault+0x74/0x80
[  299.026172]  [<c12bcfeb>] sys_socketcall+0xbb/0x2e0
[  299.026227]  [<c13c4fc3>] ? sysenter_exit+0xf/0x18
[  299.026282]  [<c11a3ec0>] ? trace_hardirqs_on_thunk+0xc/0x10
[  299.026338]  [<c13c4f90>] sysenter_do_call+0x12/0x36
[  299.026393] ---[ end trace 53d11c892332cf99 ]---


or :

[  299.032017] ------------[ cut here ]------------
[  299.032072] WARNING: at include/net/inetpeer.h:91 rt6_bind_peer+0x84/0xb0()
[  299.032130] Hardware name: ProLiant BL460c G1
[  299.032183] Modules linked in: xt_hashlimit ipmi_devintf ipmi_si ipmi_msghandler tg3 bonding
[  299.032482] Pid: 0, comm: kworker/0:1 Tainted: G        W   3.0.0-rc7-03555-ge798b6e-dirty #1048
[  299.032557] Call Trace:
[  299.032614]  [<c1042b4d>] warn_slowpath_common+0x6d/0xa0
[  299.032671]  [<c1373104>] ? rt6_bind_peer+0x84/0xb0
[  299.032725]  [<c1373104>] ? rt6_bind_peer+0x84/0xb0
[  299.032780]  [<c1042b9d>] warn_slowpath_null+0x1d/0x20
[  299.032835]  [<c1373104>] rt6_bind_peer+0x84/0xb0
[  299.032890]  [<c13731ec>] ipv6_cow_metrics+0xbc/0xe0
[  299.032945]  [<c1373a90>] icmp6_dst_alloc+0x1a0/0x2a0
[  299.033001]  [<c13738f0>] ? ip6_blackhole_route+0x240/0x240
[  299.033058]  [<c137a0bf>] ndisc_send_skb+0x4f/0x310
[  299.033113]  [<c137957b>] ? ndisc_fill_addr_option+0x5b/0x90
[  299.033169]  [<c137a3d2>] __ndisc_send+0x52/0x60
[  299.033224]  [<c137ad5d>] ndisc_send_ns+0x5d/0x90
[  299.033279]  [<c136b559>] ? ipv6_chk_addr+0x119/0x130
[  299.033335]  [<c137ae2f>] ndisc_solicit+0x9f/0x130
[  299.033391]  [<c12d8b8e>] neigh_timer_handler+0x10e/0x2a0
[  299.033447]  [<c105168a>] run_timer_softirq+0x13a/0x370
[  299.033503]  [<c1051608>] ? run_timer_softirq+0xb8/0x370
[  299.033558]  [<c12d8a80>] ? neigh_update+0x4c0/0x4c0
[  299.033614]  [<c1049577>] __do_softirq+0x97/0x1f0
[  299.033674]  [<c10494e0>] ? remote_softirq_receive+0x60/0x60




^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [BUG] ipv6: all routes share same inetpeer
  2011-07-19 17:23 [BUG] ipv6: all routes share same inetpeer Eric Dumazet
@ 2011-07-19 17:37 ` David Miller
  2011-07-19 18:20   ` Eric Dumazet
  0 siblings, 1 reply; 9+ messages in thread
From: David Miller @ 2011-07-19 17:37 UTC (permalink / raw)
  To: eric.dumazet; +Cc: netdev

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 19 Jul 2011 19:23:49 +0200

> Maybe you can find the bug before me ?

I think when we add the route we cow the metrics almost immediately.
The daddr is, unfortunately, fully prefixed at that point.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [BUG] ipv6: all routes share same inetpeer
  2011-07-19 17:37 ` David Miller
@ 2011-07-19 18:20   ` Eric Dumazet
  2011-07-19 18:57     ` Eric Dumazet
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2011-07-19 18:20 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

Le mardi 19 juillet 2011 à 10:37 -0700, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 19 Jul 2011 19:23:49 +0200
> 
> > Maybe you can find the bug before me ?
> 
> I think when we add the route we cow the metrics almost immediately.
> The daddr is, unfortunately, fully prefixed at that point.

Yes, we shall provide a second ip6_rt_copy() argument, with the
destination address.

I am testing :

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ddef80f..2a6d70a 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -72,7 +72,7 @@
 #define RT6_TRACE(x...) do { ; } while (0)
 #endif
 
-static struct rt6_info * ip6_rt_copy(struct rt6_info *ort);
+static struct rt6_info * ip6_rt_copy(struct rt6_info *ort, const struct in6_addr *dest);
 static struct dst_entry	*ip6_dst_check(struct dst_entry *dst, u32 cookie);
 static unsigned int	 ip6_default_advmss(const struct dst_entry *dst);
 static unsigned int	 ip6_default_mtu(const struct dst_entry *dst);
@@ -699,7 +699,7 @@ static struct rt6_info *rt6_alloc_cow(struct rt6_info *ort, const struct in6_add
 	 *	Clone the route.
 	 */
 
-	rt = ip6_rt_copy(ort);
+	rt = ip6_rt_copy(ort, daddr);
 
 	if (rt) {
 		struct neighbour *neigh;
@@ -712,7 +712,6 @@ static struct rt6_info *rt6_alloc_cow(struct rt6_info *ort, const struct in6_add
 			ipv6_addr_copy(&rt->rt6i_gateway, daddr);
 		}
 
-		ipv6_addr_copy(&rt->rt6i_dst.addr, daddr);
 		rt->rt6i_dst.plen = 128;
 		rt->rt6i_flags |= RTF_CACHE;
 		rt->dst.flags |= DST_HOST;
@@ -761,9 +760,9 @@ static struct rt6_info *rt6_alloc_cow(struct rt6_info *ort, const struct in6_add
 
 static struct rt6_info *rt6_alloc_clone(struct rt6_info *ort, const struct in6_addr *daddr)
 {
-	struct rt6_info *rt = ip6_rt_copy(ort);
+	struct rt6_info *rt = ip6_rt_copy(ort, daddr);
+
 	if (rt) {
-		ipv6_addr_copy(&rt->rt6i_dst.addr, daddr);
 		rt->rt6i_dst.plen = 128;
 		rt->rt6i_flags |= RTF_CACHE;
 		rt->dst.flags |= DST_HOST;
@@ -1584,7 +1583,7 @@ void rt6_redirect(const struct in6_addr *dest, const struct in6_addr *src,
 	if (neigh == dst_get_neighbour(&rt->dst))
 		goto out;
 
-	nrt = ip6_rt_copy(rt);
+	nrt = ip6_rt_copy(rt, dest);
 	if (nrt == NULL)
 		goto out;
 
@@ -1592,7 +1591,6 @@ void rt6_redirect(const struct in6_addr *dest, const struct in6_addr *src,
 	if (on_link)
 		nrt->rt6i_flags &= ~RTF_GATEWAY;
 
-	ipv6_addr_copy(&nrt->rt6i_dst.addr, dest);
 	nrt->rt6i_dst.plen = 128;
 	nrt->dst.flags |= DST_HOST;
 
@@ -1730,7 +1728,7 @@ void rt6_pmtu_discovery(const struct in6_addr *daddr, const struct in6_addr *sad
  *	Misc support functions
  */
 
-static struct rt6_info * ip6_rt_copy(struct rt6_info *ort)
+static struct rt6_info * ip6_rt_copy(struct rt6_info *ort, const struct in6_addr *dest)
 {
 	struct net *net = dev_net(ort->rt6i_dev);
 	struct rt6_info *rt = ip6_dst_alloc(&net->ipv6.ip6_dst_ops,
@@ -1740,6 +1738,8 @@ static struct rt6_info * ip6_rt_copy(struct rt6_info *ort)
 		rt->dst.input = ort->dst.input;
 		rt->dst.output = ort->dst.output;
 
+		ipv6_addr_copy(&rt->rt6i_dst.addr, dest);
+		rt->rt6i_dst.plen = ort->rt6i_dst.plen;
 		dst_copy_metrics(&rt->dst, &ort->dst);
 		rt->dst.error = ort->dst.error;
 		rt->rt6i_idev = ort->rt6i_idev;
@@ -1752,7 +1752,6 @@ static struct rt6_info * ip6_rt_copy(struct rt6_info *ort)
 		rt->rt6i_flags = ort->rt6i_flags & ~RTF_EXPIRES;
 		rt->rt6i_metric = 0;
 
-		memcpy(&rt->rt6i_dst, &ort->rt6i_dst, sizeof(struct rt6key));
 #ifdef CONFIG_IPV6_SUBTREES
 		memcpy(&rt->rt6i_src, &ort->rt6i_src, sizeof(struct rt6key));
 #endif



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [BUG] ipv6: all routes share same inetpeer
  2011-07-19 18:20   ` Eric Dumazet
@ 2011-07-19 18:57     ` Eric Dumazet
  2011-07-19 18:59       ` David Miller
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2011-07-19 18:57 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

Le mardi 19 juillet 2011 à 20:20 +0200, Eric Dumazet a écrit :
> Le mardi 19 juillet 2011 à 10:37 -0700, David Miller a écrit :
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Tue, 19 Jul 2011 19:23:49 +0200
> > 
> > > Maybe you can find the bug before me ?
> > 
> > I think when we add the route we cow the metrics almost immediately.
> > The daddr is, unfortunately, fully prefixed at that point.
> 
> Yes, we shall provide a second ip6_rt_copy() argument, with the
> destination address.
> 

Hmm, or maybe just change the dst_copy_metrics(&rt->dst, &ort->dst);
call done from ip6_rt_copy(), to avoid doing the COW if not really
needed ?

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ddef80f..5403cea 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1740,7 +1740,7 @@ static struct rt6_info * ip6_rt_copy(struct rt6_info *ort)
 		rt->dst.input = ort->dst.input;
 		rt->dst.output = ort->dst.output;
 
-		dst_copy_metrics(&rt->dst, &ort->dst);
+		rt->dst._metrics = ort->dst._metrics;
 		rt->dst.error = ort->dst.error;
 		rt->rt6i_idev = ort->rt6i_idev;
 		if (rt->rt6i_idev)




^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [BUG] ipv6: all routes share same inetpeer
  2011-07-19 18:57     ` Eric Dumazet
@ 2011-07-19 18:59       ` David Miller
  2011-07-20  5:29         ` Eric Dumazet
  0 siblings, 1 reply; 9+ messages in thread
From: David Miller @ 2011-07-19 18:59 UTC (permalink / raw)
  To: eric.dumazet; +Cc: netdev

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 19 Jul 2011 20:57:50 +0200

> Le mardi 19 juillet 2011 à 20:20 +0200, Eric Dumazet a écrit :
>> Le mardi 19 juillet 2011 à 10:37 -0700, David Miller a écrit :
>> > From: Eric Dumazet <eric.dumazet@gmail.com>
>> > Date: Tue, 19 Jul 2011 19:23:49 +0200
>> > 
>> > > Maybe you can find the bug before me ?
>> > 
>> > I think when we add the route we cow the metrics almost immediately.
>> > The daddr is, unfortunately, fully prefixed at that point.
>> 
>> Yes, we shall provide a second ip6_rt_copy() argument, with the
>> destination address.
>> 
> 
> Hmm, or maybe just change the dst_copy_metrics(&rt->dst, &ort->dst);
> call done from ip6_rt_copy(), to avoid doing the COW if not really
> needed ?

This is ok if it handles the case where ort's metrics point to
writable inetpeer memory.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [BUG] ipv6: all routes share same inetpeer
  2011-07-19 18:59       ` David Miller
@ 2011-07-20  5:29         ` Eric Dumazet
  2011-07-20  6:18           ` Eric Dumazet
  2011-07-22  0:18           ` David Miller
  0 siblings, 2 replies; 9+ messages in thread
From: Eric Dumazet @ 2011-07-20  5:29 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

Le mardi 19 juillet 2011 à 11:59 -0700, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 19 Jul 2011 20:57:50 +0200
> 
> > Le mardi 19 juillet 2011 à 20:20 +0200, Eric Dumazet a écrit :
> >> Le mardi 19 juillet 2011 à 10:37 -0700, David Miller a écrit :
> >> > From: Eric Dumazet <eric.dumazet@gmail.com>
> >> > Date: Tue, 19 Jul 2011 19:23:49 +0200
> >> > 
> >> > > Maybe you can find the bug before me ?
> >> > 
> >> > I think when we add the route we cow the metrics almost immediately.
> >> > The daddr is, unfortunately, fully prefixed at that point.
> >> 
> >> Yes, we shall provide a second ip6_rt_copy() argument, with the
> >> destination address.
> >> 
> > 
> > Hmm, or maybe just change the dst_copy_metrics(&rt->dst, &ort->dst);
> > call done from ip6_rt_copy(), to avoid doing the COW if not really
> > needed ?
> 
> This is ok if it handles the case where ort's metrics point to
> writable inetpeer memory.

OK but if ort's metrics are writeable we must perform the dst_copy_metrics()
and therefore fill rt6i_dst before ?

My first patch had an issue in rt6_alloc_cow(), line 710, where 
ipv6_addr_equal(&rt->rt6i_dst.addr, daddr) becomes always true.

I guess I can replace it by ipv6_addr_equal(&ort->rt6i_dst.addr, daddr)




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [BUG] ipv6: all routes share same inetpeer
  2011-07-20  5:29         ` Eric Dumazet
@ 2011-07-20  6:18           ` Eric Dumazet
  2011-07-22  4:25             ` David Miller
  2011-07-22  0:18           ` David Miller
  1 sibling, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2011-07-20  6:18 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

Le mercredi 20 juillet 2011 à 07:29 +0200, Eric Dumazet a écrit :

> My first patch had an issue in rt6_alloc_cow(), line 710, where 
> ipv6_addr_equal(&rt->rt6i_dst.addr, daddr) becomes always true.
> 
> I guess I can replace it by ipv6_addr_equal(&ort->rt6i_dst.addr, daddr)
> 
> 

Here the combo patch I tested :

I also had to solve the icmp6_dst_alloc() problem
[it uses dst_metric_set(&rt->dst, RTAX_HOPLIMIT, 255);]

Note : this is based on net-2.6, but I really tested it on net-next-2.6
(with the frag ident patch applied too)

[PATCH] ipv6: unshare inetpeers

We currently cow metrics a bit too soon in IPv6 case : All routes are
tied to a single inetpeer entry.

Change ip6_rt_copy() to get destination address as second argument, so
that we fill rt6i_dst before the dst_copy_metrics() call.

icmp6_dst_alloc() must set rt6i_dst before calling dst_metric_set(), or
else the cow is done while rt6i_dst is still NULL.

If orig route points to readonly metrics, we can share the pointer
instead of performing the memory allocation and copy.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/ipv6/route.c |   33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 0ef1f08..5b5a32d 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -72,7 +72,8 @@
 #define RT6_TRACE(x...) do { ; } while (0)
 #endif
 
-static struct rt6_info * ip6_rt_copy(struct rt6_info *ort);
+static struct rt6_info *ip6_rt_copy(const struct rt6_info *ort,
+				    const struct in6_addr *dest);
 static struct dst_entry	*ip6_dst_check(struct dst_entry *dst, u32 cookie);
 static unsigned int	 ip6_default_advmss(const struct dst_entry *dst);
 static unsigned int	 ip6_default_mtu(const struct dst_entry *dst);
@@ -683,7 +684,8 @@ int ip6_ins_rt(struct rt6_info *rt)
 	return __ip6_ins_rt(rt, &info);
 }
 
-static struct rt6_info *rt6_alloc_cow(struct rt6_info *ort, const struct in6_addr *daddr,
+static struct rt6_info *rt6_alloc_cow(const struct rt6_info *ort,
+				      const struct in6_addr *daddr,
 				      const struct in6_addr *saddr)
 {
 	struct rt6_info *rt;
@@ -692,7 +694,7 @@ static struct rt6_info *rt6_alloc_cow(struct rt6_info *ort, const struct in6_add
 	 *	Clone the route.
 	 */
 
-	rt = ip6_rt_copy(ort);
+	rt = ip6_rt_copy(ort, daddr);
 
 	if (rt) {
 		struct neighbour *neigh;
@@ -700,12 +702,11 @@ static struct rt6_info *rt6_alloc_cow(struct rt6_info *ort, const struct in6_add
 
 		if (!(rt->rt6i_flags&RTF_GATEWAY)) {
 			if (rt->rt6i_dst.plen != 128 &&
-			    ipv6_addr_equal(&rt->rt6i_dst.addr, daddr))
+			    ipv6_addr_equal(&ort->rt6i_dst.addr, daddr))
 				rt->rt6i_flags |= RTF_ANYCAST;
 			ipv6_addr_copy(&rt->rt6i_gateway, daddr);
 		}
 
-		ipv6_addr_copy(&rt->rt6i_dst.addr, daddr);
 		rt->rt6i_dst.plen = 128;
 		rt->rt6i_flags |= RTF_CACHE;
 		rt->dst.flags |= DST_HOST;
@@ -752,11 +753,12 @@ static struct rt6_info *rt6_alloc_cow(struct rt6_info *ort, const struct in6_add
 	return rt;
 }
 
-static struct rt6_info *rt6_alloc_clone(struct rt6_info *ort, const struct in6_addr *daddr)
+static struct rt6_info *rt6_alloc_clone(struct rt6_info *ort,
+					const struct in6_addr *daddr)
 {
-	struct rt6_info *rt = ip6_rt_copy(ort);
+	struct rt6_info *rt = ip6_rt_copy(ort, daddr);
+
 	if (rt) {
-		ipv6_addr_copy(&rt->rt6i_dst.addr, daddr);
 		rt->rt6i_dst.plen = 128;
 		rt->rt6i_flags |= RTF_CACHE;
 		rt->dst.flags |= DST_HOST;
@@ -900,7 +902,10 @@ struct dst_entry *ip6_blackhole_route(struct net *net, struct dst_entry *dst_ori
 		new->input = dst_discard;
 		new->output = dst_discard;
 
-		dst_copy_metrics(new, &ort->dst);
+		if (dst_metrics_read_only(&ort->dst))
+			new->_metrics = ort->dst._metrics;
+		else
+			dst_copy_metrics(new, &ort->dst);
 		rt->rt6i_idev = ort->rt6i_idev;
 		if (rt->rt6i_idev)
 			in6_dev_hold(rt->rt6i_idev);
@@ -1060,6 +1065,7 @@ struct dst_entry *icmp6_dst_alloc(struct net_device *dev,
 	rt->rt6i_idev     = idev;
 	rt->rt6i_nexthop  = neigh;
 	atomic_set(&rt->dst.__refcnt, 1);
+	ipv6_addr_copy(&rt->rt6i_dst.addr, addr);
 	dst_metric_set(&rt->dst, RTAX_HOPLIMIT, 255);
 	rt->dst.output  = ip6_output;
 
@@ -1577,7 +1583,7 @@ void rt6_redirect(const struct in6_addr *dest, const struct in6_addr *src,
 	if (neigh == rt->dst.neighbour)
 		goto out;
 
-	nrt = ip6_rt_copy(rt);
+	nrt = ip6_rt_copy(rt, dest);
 	if (nrt == NULL)
 		goto out;
 
@@ -1585,7 +1591,6 @@ void rt6_redirect(const struct in6_addr *dest, const struct in6_addr *src,
 	if (on_link)
 		nrt->rt6i_flags &= ~RTF_GATEWAY;
 
-	ipv6_addr_copy(&nrt->rt6i_dst.addr, dest);
 	nrt->rt6i_dst.plen = 128;
 	nrt->dst.flags |= DST_HOST;
 
@@ -1723,7 +1728,8 @@ void rt6_pmtu_discovery(const struct in6_addr *daddr, const struct in6_addr *sad
  *	Misc support functions
  */
 
-static struct rt6_info * ip6_rt_copy(struct rt6_info *ort)
+static struct rt6_info *ip6_rt_copy(const struct rt6_info *ort,
+				    const struct in6_addr *dest)
 {
 	struct net *net = dev_net(ort->rt6i_dev);
 	struct rt6_info *rt = ip6_dst_alloc(&net->ipv6.ip6_dst_ops,
@@ -1733,6 +1739,8 @@ static struct rt6_info * ip6_rt_copy(struct rt6_info *ort)
 		rt->dst.input = ort->dst.input;
 		rt->dst.output = ort->dst.output;
 
+		ipv6_addr_copy(&rt->rt6i_dst.addr, dest);
+		rt->rt6i_dst.plen = ort->rt6i_dst.plen;
 		dst_copy_metrics(&rt->dst, &ort->dst);
 		rt->dst.error = ort->dst.error;
 		rt->rt6i_idev = ort->rt6i_idev;
@@ -1745,7 +1753,6 @@ static struct rt6_info * ip6_rt_copy(struct rt6_info *ort)
 		rt->rt6i_flags = ort->rt6i_flags & ~RTF_EXPIRES;
 		rt->rt6i_metric = 0;
 
-		memcpy(&rt->rt6i_dst, &ort->rt6i_dst, sizeof(struct rt6key));
 #ifdef CONFIG_IPV6_SUBTREES
 		memcpy(&rt->rt6i_src, &ort->rt6i_src, sizeof(struct rt6key));
 #endif
	


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [BUG] ipv6: all routes share same inetpeer
  2011-07-20  5:29         ` Eric Dumazet
  2011-07-20  6:18           ` Eric Dumazet
@ 2011-07-22  0:18           ` David Miller
  1 sibling, 0 replies; 9+ messages in thread
From: David Miller @ 2011-07-22  0:18 UTC (permalink / raw)
  To: eric.dumazet; +Cc: netdev

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 20 Jul 2011 07:29:34 +0200

> Le mardi 19 juillet 2011 à 11:59 -0700, David Miller a écrit :
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Tue, 19 Jul 2011 20:57:50 +0200
>> 
>> > Le mardi 19 juillet 2011 à 20:20 +0200, Eric Dumazet a écrit :
>> >> Le mardi 19 juillet 2011 à 10:37 -0700, David Miller a écrit :
>> >> > From: Eric Dumazet <eric.dumazet@gmail.com>
>> >> > Date: Tue, 19 Jul 2011 19:23:49 +0200
>> >> > 
>> >> > > Maybe you can find the bug before me ?
>> >> > 
>> >> > I think when we add the route we cow the metrics almost immediately.
>> >> > The daddr is, unfortunately, fully prefixed at that point.
>> >> 
>> >> Yes, we shall provide a second ip6_rt_copy() argument, with the
>> >> destination address.
>> >> 
>> > 
>> > Hmm, or maybe just change the dst_copy_metrics(&rt->dst, &ort->dst);
>> > call done from ip6_rt_copy(), to avoid doing the COW if not really
>> > needed ?
>> 
>> This is ok if it handles the case where ort's metrics point to
>> writable inetpeer memory.
> 
> OK but if ort's metrics are writeable we must perform the dst_copy_metrics()
> and therefore fill rt6i_dst before ?

If it's writable, then there are no problems.

Oh actually, I see what you mean.  This is a mess.

I'll try to think about this some more and look at your most
recent patches.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [BUG] ipv6: all routes share same inetpeer
  2011-07-20  6:18           ` Eric Dumazet
@ 2011-07-22  4:25             ` David Miller
  0 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2011-07-22  4:25 UTC (permalink / raw)
  To: eric.dumazet; +Cc: netdev

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 20 Jul 2011 08:18:36 +0200

> Le mercredi 20 juillet 2011 à 07:29 +0200, Eric Dumazet a écrit :
> 
>> My first patch had an issue in rt6_alloc_cow(), line 710, where 
>> ipv6_addr_equal(&rt->rt6i_dst.addr, daddr) becomes always true.
>> 
>> I guess I can replace it by ipv6_addr_equal(&ort->rt6i_dst.addr, daddr)
>> 
>> 
> 
> Here the combo patch I tested :
> 
> I also had to solve the icmp6_dst_alloc() problem
> [it uses dst_metric_set(&rt->dst, RTAX_HOPLIMIT, 255);]
> 
> Note : this is based on net-2.6, but I really tested it on net-next-2.6
> (with the frag ident patch applied too)
> 
> [PATCH] ipv6: unshare inetpeers

I applied this to net-next and queued it up for -stable, thanks!

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2011-07-22  4:25 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-19 17:23 [BUG] ipv6: all routes share same inetpeer Eric Dumazet
2011-07-19 17:37 ` David Miller
2011-07-19 18:20   ` Eric Dumazet
2011-07-19 18:57     ` Eric Dumazet
2011-07-19 18:59       ` David Miller
2011-07-20  5:29         ` Eric Dumazet
2011-07-20  6:18           ` Eric Dumazet
2011-07-22  4:25             ` David Miller
2011-07-22  0:18           ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).