[RFC] ipv6: rt6_update_expires() seems racy

[RFC] ipv6: rt6_update_expires() seems racy

[Eric Dumazet]

Some strange crashes happen in rt6_check_expired(), with access
to random addresses.

At first glance, it looks like the RTF_EXPIRES and
stuff added in commit 1716a96101c49186b
(ipv6: fix problem with expired dst cache)
are racy : same dst could be manipulated at the same time
on different cpus.

At some point, our stack believes rt->dst.from contains a dst pointer,
while its really a jiffie value (as rt->dst.expires shares the same area
of memory)

rt6_update_expires() should be fixed, or am I missing something ?

CC Neil because of https://bugzilla.redhat.com/show_bug.cgi?id=892060

[RFC PATCH] ipv6: Split from and expires field in dst_entry out of union [net-next]

[Neil Horman]

I've been looking into some random ipv6 crashes lately that occur around
ip6_link_failure.  This and simmilar partial stack traces:
ip6_link_failure+0xbe/0xd0
ipip6_tunnel_xmit+0x7e7/0x860 [sit]
dev_hard_start_xmit+0x3e3/0x690
dev_queue_xmit+0x38f/0x610
neigh_direct_output+0x11/0x20
ip6_finish_output2+0x90/0x340
? ac6_proc_exit+0x20/0x20
ip6_finish_output+0x98/0xc0
ip6_output
? __ip6_local_out
ip6_local_out
ip6_push_pending
? ip6_append_data
udp_v6_push_pending_frames
udpv6_sendmsg

were all I had.  Eric D. Made this click with me this morning however, noting a
possible/likely race condition in the ipv6 code.  It appears that, if a dst
entry is accessed by multiple cpus (and it appears that certainly can happen, as
routes created via ip6_rt_copy are hashed back into the fib, for future
lookups), then the use of the rt6i_flags field can race, leading to multiple
conflicting uses of the aforementioned union (jiffies being interpreted as the
from pointer and vice versa).

Eric suggested that this be fixed in rt6_update_expires, but looking at the
other uses of this union I don't think thats a complete fix.  All the accessors
for the expired|from union in the dst entry seem to rely on the RTF_EXPIRES flag
being accessed and updated atomically, and thats just not the case.

I think the only fix here, that doesn't involve additional locking, is to
separate the expires and from fields into their own storage, so as not to
trample one another.

This is currently untested, but I've given it to several people who can
reproduce this problem and are testing it now.  I'll post again when I have
results from them.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: eric.dumazet@gmail.com
CC: David Miller <davem@davemloft.net>
CC: Gao feng <gaofeng@cn.fujitsu.com>
CC: Jiri Bohac <jbohac@suse.cz>
---
 include/net/dst.h     |  9 +++------
 include/net/ip6_fib.h | 13 ++++++++-----
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/net/dst.h b/include/net/dst.h
index 3da47e0..6b7ebcf 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -36,13 +36,10 @@ struct dst_entry {
 	struct net_device       *dev;
 	struct  dst_ops	        *ops;
 	unsigned long		_metrics;
-	union {
-		unsigned long           expires;
-		/* point to where the dst_entry copied from */
-		struct dst_entry        *from;
-	};
+	unsigned long           expires;
+	/* point to where the dst_entry copied from */
+	struct dst_entry        *from;
 	struct dst_entry	*path;
-	void			*__pad0;
 #ifdef CONFIG_XFRM
 	struct xfrm_state	*xfrm;
 #else
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 6919a50..a285e37 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -164,31 +164,33 @@ static inline struct inet6_dev *ip6_dst_idev(struct dst_entry *dst)
 
 static inline void rt6_clean_expires(struct rt6_info *rt)
 {
-	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from)
+	if (!rt->rt6i_flags & RTF_EXPIRES)
 		dst_release(rt->dst.from);
 
 	rt->rt6i_flags &= ~RTF_EXPIRES;
 	rt->dst.from = NULL;
+	rt->dst.expires = 0;
 }
 
 static inline void rt6_set_expires(struct rt6_info *rt, unsigned long expires)
 {
-	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from)
+	if (!rt->rt6i_flags & RTF_EXPIRES)
 		dst_release(rt->dst.from);
 
 	rt->rt6i_flags |= RTF_EXPIRES;
+	rt->dst.from = NULL;
 	rt->dst.expires = expires;
 }
 
 static inline void rt6_update_expires(struct rt6_info *rt, int timeout)
 {
 	if (!(rt->rt6i_flags & RTF_EXPIRES)) {
-		if (rt->dst.from)
-			dst_release(rt->dst.from);
+		dst_release(rt->dst.from);
 		/* dst_set_expires relies on expires == 0 
 		 * if it has not been set previously.
 		 */
 		rt->dst.expires = 0;
+		rt6->dst.from = NULL;
 	}
 
 	dst_set_expires(&rt->dst, timeout);
@@ -199,7 +201,7 @@ static inline void rt6_set_from(struct rt6_info *rt, struct rt6_info *from)
 {
 	struct dst_entry *new = (struct dst_entry *) from;
 
-	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from) {
+	if (!rt->rt6i_flags & RTF_EXPIRES) {
 		if (new == rt->dst.from)
 			return;
 		dst_release(rt->dst.from);
@@ -207,6 +209,7 @@ static inline void rt6_set_from(struct rt6_info *rt, struct rt6_info *from)
 
 	rt->rt6i_flags &= ~RTF_EXPIRES;
 	rt->dst.from = new;
+	rt->dst.expires = 0;
 	dst_hold(new);
 }
 
-- 
1.7.11.7

Re: [RFC PATCH] ipv6: Split from and expires field in dst_entry out of union [net-next]

[Eric Dumazet]

On Tue, 2013-02-19 at 15:28 -0500, Neil Horman wrote:

>  static inline void rt6_update_expires(struct rt6_info *rt, int timeout)
>  {
>  	if (!(rt->rt6i_flags & RTF_EXPIRES)) {
> -		if (rt->dst.from)
> -			dst_release(rt->dst.from);
> +		dst_release(rt->dst.from);
>  		/* dst_set_expires relies on expires == 0 
>  		 * if it has not been set previously.
>  		 */
>  		rt->dst.expires = 0;
> +		rt6->dst.from = NULL;
>  	}
>  

Sorry you didnt really address the problem, only reduce the race window.

Re: [RFC PATCH] ipv6: Split from and expires field in dst_entry out of union [net-next]

[Neil Horman]

On Tue, Feb 19, 2013 at 01:17:45PM -0800, Eric Dumazet wrote:
> On Tue, 2013-02-19 at 15:28 -0500, Neil Horman wrote:
> 
> >  static inline void rt6_update_expires(struct rt6_info *rt, int timeout)
> >  {
> >  	if (!(rt->rt6i_flags & RTF_EXPIRES)) {
> > -		if (rt->dst.from)
> > -			dst_release(rt->dst.from);
> > +		dst_release(rt->dst.from);
> >  		/* dst_set_expires relies on expires == 0 
> >  		 * if it has not been set previously.
> >  		 */
> >  		rt->dst.expires = 0;
> > +		rt6->dst.from = NULL;
> >  	}
> >  
> 
> Sorry you didnt really address the problem, only reduce the race window.
> 
I kinda had a feeling you would say that, but the only other solution I see here
is to either introduce some locking to protect the from pointer, or two revert
the patch that introduced the from pointer alltogether, neither of which sounds
appealing to me.  I suppose we could use an xchng to atomically update the from
pointer, so there was only ever one context that was able to free it in
rt6_update_path.  Does that seem reasonable to you?

Neil

Re: [RFC PATCH] ipv6: Split from and expires field in dst_entry out of union [net-next]

[Eric Dumazet]

On Tue, 2013-02-19 at 16:49 -0500, Neil Horman wrote:
> On Tue, Feb 19, 2013 at 01:17:45PM -0800, Eric Dumazet wrote:
> > On Tue, 2013-02-19 at 15:28 -0500, Neil Horman wrote:
> > 
> > >  static inline void rt6_update_expires(struct rt6_info *rt, int timeout)
> > >  {
> > >  	if (!(rt->rt6i_flags & RTF_EXPIRES)) {
> > > -		if (rt->dst.from)
> > > -			dst_release(rt->dst.from);
> > > +		dst_release(rt->dst.from);
> > >  		/* dst_set_expires relies on expires == 0 
> > >  		 * if it has not been set previously.
> > >  		 */
> > >  		rt->dst.expires = 0;
> > > +		rt6->dst.from = NULL;
> > >  	}
> > >  
> > 
> > Sorry you didnt really address the problem, only reduce the race window.
> > 
> I kinda had a feeling you would say that, but the only other solution I see here
> is to either introduce some locking to protect the from pointer, or two revert
> the patch that introduced the from pointer alltogether, neither of which sounds
> appealing to me.  I suppose we could use an xchng to atomically update the from
> pointer, so there was only ever one context that was able to free it in
> rt6_update_path.  Does that seem reasonable to you?

I believe the setting of rt6->dst.from is safe :
It should be done at :
- dst creation time, when we are the only user.
- dst destry time, when we are the only user.

We only have to do the dst_release() at the right place, when we are the
last user of the dst.

So rt6_update_expires() should not mess with rt6->dst.from at all.

Re: [RFC PATCH] ipv6: Split from and expires field in dst_entry out of union [net-next]

[Gao feng]

Hi Eric & Neil,
On 2013/02/20 05:55, Eric Dumazet wrote:
> On Tue, 2013-02-19 at 16:49 -0500, Neil Horman wrote:
>> On Tue, Feb 19, 2013 at 01:17:45PM -0800, Eric Dumazet wrote:
>>> On Tue, 2013-02-19 at 15:28 -0500, Neil Horman wrote:
>>>
>>>>  static inline void rt6_update_expires(struct rt6_info *rt, int timeout)
>>>>  {
>>>>  	if (!(rt->rt6i_flags & RTF_EXPIRES)) {
>>>> -		if (rt->dst.from)
>>>> -			dst_release(rt->dst.from);
>>>> +		dst_release(rt->dst.from);
>>>>  		/* dst_set_expires relies on expires == 0 
>>>>  		 * if it has not been set previously.
>>>>  		 */
>>>>  		rt->dst.expires = 0;
>>>> +		rt6->dst.from = NULL;
>>>>  	}
>>>>  
>>>
>>> Sorry you didnt really address the problem, only reduce the race window.
>>>
>> I kinda had a feeling you would say that, but the only other solution I see here
>> is to either introduce some locking to protect the from pointer, or two revert
>> the patch that introduced the from pointer alltogether, neither of which sounds
>> appealing to me.  I suppose we could use an xchng to atomically update the from
>> pointer, so there was only ever one context that was able to free it in
>> rt6_update_path.  Does that seem reasonable to you?
> 

Thanks for your research on this problem.

> I believe the setting of rt6->dst.from is safe :
> It should be done at :
> - dst creation time, when we are the only user.
> - dst destry time, when we are the only user.
> 
> We only have to do the dst_release() at the right place, when we are the
> last user of the dst.
> 
> So rt6_update_expires() should not mess with rt6->dst.from at all.
> 

How can we?
one usage of rt6_update_expires and rt6_set_expires
is changing rt6->dst.from to rt6->dst.expires, we should release the
already holded reference of rt6->dst.from.

I think maybe we should rework the commit 1716a96101c49186b
(ipv6: fix problem with expired dst cache). just force setting flag
RTF_EXPIRES and expires for the rt6 which copied from the rt6(which
is generated from RA). The only problem of this situation is this
copied rt6's expire time may be imprecise, we may receive a new
RA with a new expire time.

Re: [RFC PATCH] ipv6: Split from and expires field in dst_entry out of union [net-next]

[Eric Dumazet]

On Wed, 2013-02-20 at 14:33 +0800, Gao feng wrote:

> How can we?
> one usage of rt6_update_expires and rt6_set_expires
> is changing rt6->dst.from to rt6->dst.expires, we should release the
> already holded reference of rt6->dst.from.
> 

Just don't union the two fields, as Neil did.

Setting the dst.expires value should not change dst.from at all.

Something like the (untested, because its too late here) patch ?

 include/net/dst.h     |   11 +++++------
 include/net/ip6_fib.h |   21 +++------------------
 net/ipv6/route.c      |    3 +--
 3 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/include/net/dst.h b/include/net/dst.h
index 3da47e0..3f31a48 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -36,13 +36,12 @@ struct dst_entry {
 	struct net_device       *dev;
 	struct  dst_ops	        *ops;
 	unsigned long		_metrics;
-	union {
-		unsigned long           expires;
-		/* point to where the dst_entry copied from */
-		struct dst_entry        *from;
-	};
+	unsigned long           expires;
+
+	/* point to where the dst_entry copied from */
+	struct dst_entry        *from;
+
 	struct dst_entry	*path;
-	void			*__pad0;
 #ifdef CONFIG_XFRM
 	struct xfrm_state	*xfrm;
 #else
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 6919a50..731b35c 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -164,33 +164,19 @@ static inline struct inet6_dev *ip6_dst_idev(struct dst_entry *dst)
 
 static inline void rt6_clean_expires(struct rt6_info *rt)
 {
-	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from)
-		dst_release(rt->dst.from);
-
 	rt->rt6i_flags &= ~RTF_EXPIRES;
-	rt->dst.from = NULL;
+	rt->dst.expires = 0;
 }
 
 static inline void rt6_set_expires(struct rt6_info *rt, unsigned long expires)
 {
-	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from)
-		dst_release(rt->dst.from);
-
 	rt->rt6i_flags |= RTF_EXPIRES;
 	rt->dst.expires = expires;
 }
 
 static inline void rt6_update_expires(struct rt6_info *rt, int timeout)
 {
-	if (!(rt->rt6i_flags & RTF_EXPIRES)) {
-		if (rt->dst.from)
-			dst_release(rt->dst.from);
-		/* dst_set_expires relies on expires == 0 
-		 * if it has not been set previously.
-		 */
-		rt->dst.expires = 0;
-	}
-
+	rt->dst.expires = 0;
 	dst_set_expires(&rt->dst, timeout);
 	rt->rt6i_flags |= RTF_EXPIRES;
 }
@@ -199,13 +185,12 @@ static inline void rt6_set_from(struct rt6_info *rt, struct rt6_info *from)
 {
 	struct dst_entry *new = (struct dst_entry *) from;
 
-	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from) {
+	if (rt->dst.from) {
 		if (new == rt->dst.from)
 			return;
 		dst_release(rt->dst.from);
 	}
 
-	rt->rt6i_flags &= ~RTF_EXPIRES;
 	rt->dst.from = new;
 	dst_hold(new);
 }
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 515bb51..6874b6e 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -296,8 +296,7 @@ static void ip6_dst_destroy(struct dst_entry *dst)
 		in6_dev_put(idev);
 	}
 
-	if (!(rt->rt6i_flags & RTF_EXPIRES) && dst->from)
-		dst_release(dst->from);
+	dst_release(dst->from);
 
 	if (rt6_has_peer(rt)) {
 		struct inet_peer *peer = rt6_peer_ptr(rt);

Re: [RFC PATCH] ipv6: Split from and expires field in dst_entry out of union [net-next]

[Gao feng]

On 2013/02/20 15:00, Eric Dumazet wrote:
> On Wed, 2013-02-20 at 14:33 +0800, Gao feng wrote:
> 
>> How can we?
>> one usage of rt6_update_expires and rt6_set_expires
>> is changing rt6->dst.from to rt6->dst.expires, we should release the
>> already holded reference of rt6->dst.from.
>>
> 
> Just don't union the two fields, as Neil did.
> 
> Setting the dst.expires value should not change dst.from at all.
> 

Get it,it looks good to me.we can make sure dst->from doesn't
being operated at same time.

Thanks!

Re: [RFC PATCH] ipv6: Split from and expires field in dst_entry out of union [net-next]

[YOSHIFUJI Hideaki]

Eric Dumazet wrote:
> On Wed, 2013-02-20 at 14:33 +0800, Gao feng wrote:
> 
>> How can we?
>> one usage of rt6_update_expires and rt6_set_expires
>> is changing rt6->dst.from to rt6->dst.expires, we should release the
>> already holded reference of rt6->dst.from.
>>
> 
> Just don't union the two fields, as Neil did.
> 
> Setting the dst.expires value should not change dst.from at all.
> 
> Something like the (untested, because its too late here) patch ?
> 
>  include/net/dst.h     |   11 +++++------
>  include/net/ip6_fib.h |   21 +++------------------
>  net/ipv6/route.c      |    3 +--
>  3 files changed, 9 insertions(+), 26 deletions(-)
> 
> diff --git a/include/net/dst.h b/include/net/dst.h
> index 3da47e0..3f31a48 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -36,13 +36,12 @@ struct dst_entry {
>  	struct net_device       *dev;
>  	struct  dst_ops	        *ops;
>  	unsigned long		_metrics;
> -	union {
> -		unsigned long           expires;
> -		/* point to where the dst_entry copied from */
> -		struct dst_entry        *from;
> -	};
> +	unsigned long           expires;
> +
> +	/* point to where the dst_entry copied from */
> +	struct dst_entry        *from;
> +
>  	struct dst_entry	*path;
> -	void			*__pad0;
>  #ifdef CONFIG_XFRM
>  	struct xfrm_state	*xfrm;
>  #else

Initialize "from" in dst_alloc().

> @@ -199,13 +185,12 @@ static inline void rt6_set_from(struct rt6_info *rt, struct rt6_info *from)
>  {
>  	struct dst_entry *new = (struct dst_entry *) from;
>  
> -	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from) {
> +	if (rt->dst.from) {
>  		if (new == rt->dst.from)
>  			return;
>  		dst_release(rt->dst.from);
>  	}
>  
> -	rt->rt6i_flags &= ~RTF_EXPIRES;
>  	rt->dst.from = new;
>  	dst_hold(new);
>  }

rt6_set_from can work only for fresh entries.

In fact, I have very similar patch.

--yoshfuji

[PATCH] ipv6: fix race condition regarding dst->expires and dst->from.

[YOSHIFUJI Hideaki]

Eric Dumazet wrote:
| Some strange crashes happen in rt6_check_expired(), with access
| to random addresses.
|
| At first glance, it looks like the RTF_EXPIRES and
| stuff added in commit 1716a96101c49186b
| (ipv6: fix problem with expired dst cache)
| are racy : same dst could be manipulated at the same time
| on different cpus.
|
| At some point, our stack believes rt->dst.from contains a dst pointer,
| while its really a jiffie value (as rt->dst.expires shares the same area
| of memory)
|
| rt6_update_expires() should be fixed, or am I missing something ?
|
| CC Neil because of https://bugzilla.redhat.com/show_bug.cgi?id=892060

Because we do not have any locks for dst_entry, we cannot change
essential structure in the entry; e.g., we cannot change reference
to other entities.

To fix this issue, split 'from' and 'expires' field in dst_entry
out of union.  Once it is 'from' is assigned in the constructor,
keep the reference until the very last stage of the life time of
the object.

Of course, it is unsafe to change 'from', so make rt6_set_from simple
just for fresh entries.

Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Reported-by: Neil Horman <nhorman@tuxdriver.com>
CC: Gao Feng <gaofeng@cn.fujitsu.com>
Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
---
 include/net/dst.h     |    8 ++------
 include/net/ip6_fib.h |   38 +++++++++++---------------------------
 net/core/dst.c        |    1 +
 net/ipv6/route.c      |    8 +++-----
 4 files changed, 17 insertions(+), 38 deletions(-)

diff --git a/include/net/dst.h b/include/net/dst.h
index 3da47e0..853cda1 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -36,13 +36,9 @@ struct dst_entry {
 	struct net_device       *dev;
 	struct  dst_ops	        *ops;
 	unsigned long		_metrics;
-	union {
-		unsigned long           expires;
-		/* point to where the dst_entry copied from */
-		struct dst_entry        *from;
-	};
+	unsigned long           expires;
 	struct dst_entry	*path;
-	void			*__pad0;
+	struct dst_entry	*from;
 #ifdef CONFIG_XFRM
 	struct xfrm_state	*xfrm;
 #else
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 6919a50..6a3c8ab 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -164,50 +164,34 @@ static inline struct inet6_dev *ip6_dst_idev(struct dst_entry *dst)
 
 static inline void rt6_clean_expires(struct rt6_info *rt)
 {
-	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from)
-		dst_release(rt->dst.from);
-
 	rt->rt6i_flags &= ~RTF_EXPIRES;
-	rt->dst.from = NULL;
 }
 
 static inline void rt6_set_expires(struct rt6_info *rt, unsigned long expires)
 {
-	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from)
-		dst_release(rt->dst.from);
-
-	rt->rt6i_flags |= RTF_EXPIRES;
 	rt->dst.expires = expires;
+	rt->rt6i_flags |= RTF_EXPIRES;
 }
 
-static inline void rt6_update_expires(struct rt6_info *rt, int timeout)
+static inline void rt6_update_expires(struct rt6_info *rt0, int timeout)
 {
-	if (!(rt->rt6i_flags & RTF_EXPIRES)) {
-		if (rt->dst.from)
-			dst_release(rt->dst.from);
-		/* dst_set_expires relies on expires == 0 
-		 * if it has not been set previously.
-		 */
-		rt->dst.expires = 0;
-	}
-
-	dst_set_expires(&rt->dst, timeout);
-	rt->rt6i_flags |= RTF_EXPIRES;
+	struct rt6_info *rt;
+
+	for (rt = rt0; rt && !(rt->rt6i_flags & RTF_EXPIRES); rt = rt->dst.from);
+	if (rt && rt != rt0)
+		rt0->dst.expires = rt->dst.expires;
+
+	dst_set_expires(&rt0->dst, timeout);
+	rt0->rt6i_flags |= RTF_EXPIRES;
 }
 
 static inline void rt6_set_from(struct rt6_info *rt, struct rt6_info *from)
 {
 	struct dst_entry *new = (struct dst_entry *) from;
 
-	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from) {
-		if (new == rt->dst.from)
-			return;
-		dst_release(rt->dst.from);
-	}
-
 	rt->rt6i_flags &= ~RTF_EXPIRES;
-	rt->dst.from = new;
 	dst_hold(new);
+	rt->dst.from = new;
 }
 
 static inline void ip6_rt_put(struct rt6_info *rt)
diff --git a/net/core/dst.c b/net/core/dst.c
index ee6153e..35fd12f 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -179,6 +179,7 @@ void *dst_alloc(struct dst_ops *ops, struct net_device *dev,
 	dst_init_metrics(dst, dst_default_metrics, true);
 	dst->expires = 0UL;
 	dst->path = dst;
+	dst->from = NULL;
 #ifdef CONFIG_XFRM
 	dst->xfrm = NULL;
 #endif
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 515bb51..9db8622 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -287,6 +287,7 @@ static void ip6_dst_destroy(struct dst_entry *dst)
 {
 	struct rt6_info *rt = (struct rt6_info *)dst;
 	struct inet6_dev *idev = rt->rt6i_idev;
+	struct dst_entry *from = dst->from;
 
 	if (!(rt->dst.flags & DST_HOST))
 		dst_destroy_metrics_generic(dst);
@@ -296,8 +297,8 @@ static void ip6_dst_destroy(struct dst_entry *dst)
 		in6_dev_put(idev);
 	}
 
-	if (!(rt->rt6i_flags & RTF_EXPIRES) && dst->from)
-		dst_release(dst->from);
+	dst->from = NULL;
+	dst_release(dst->from);
 
 	if (rt6_has_peer(rt)) {
 		struct inet_peer *peer = rt6_peer_ptr(rt);
@@ -1010,7 +1011,6 @@ struct dst_entry *ip6_blackhole_route(struct net *net, struct dst_entry *dst_ori
 
 		rt->rt6i_gateway = ort->rt6i_gateway;
 		rt->rt6i_flags = ort->rt6i_flags;
-		rt6_clean_expires(rt);
 		rt->rt6i_metric = 0;
 
 		memcpy(&rt->rt6i_dst, &ort->rt6i_dst, sizeof(struct rt6key));
@@ -1784,8 +1784,6 @@ static struct rt6_info *ip6_rt_copy(struct rt6_info *ort,
 		if ((ort->rt6i_flags & (RTF_DEFAULT | RTF_ADDRCONF)) ==
 		    (RTF_DEFAULT | RTF_ADDRCONF))
 			rt6_set_from(rt, ort);
-		else
-			rt6_clean_expires(rt);
 		rt->rt6i_metric = 0;
 
 #ifdef CONFIG_IPV6_SUBTREES
-- 
1.7.9.5

Re: [PATCH] ipv6: fix race condition regarding dst->expires and dst->from.

[YOSHIFUJI Hideaki]

YOSHIFUJI Hideaki wrote:

> -static inline void rt6_update_expires(struct rt6_info *rt, int timeout)
> +static inline void rt6_update_expires(struct rt6_info *rt0, int timeout)
>  {
> -	if (!(rt->rt6i_flags & RTF_EXPIRES)) {
> -		if (rt->dst.from)
> -			dst_release(rt->dst.from);
> -		/* dst_set_expires relies on expires == 0 
> -		 * if it has not been set previously.
> -		 */
> -		rt->dst.expires = 0;
> -	}
> -
> -	dst_set_expires(&rt->dst, timeout);
> -	rt->rt6i_flags |= RTF_EXPIRES;
> +	struct rt6_info *rt;
> +
> +	for (rt = rt0; rt && !(rt->rt6i_flags & RTF_EXPIRES); rt = rt->dst.from);
> +	if (rt && rt != rt0)
> +		rt0->dst.expires = rt->dst.expires;
> +
> +	dst_set_expires(&rt0->dst, timeout);
> +	rt0->rt6i_flags |= RTF_EXPIRES;
>  }
>  

Oops, I sent wrong version, will resend.

--yoshfuji

[PATCH V2] ipv6: fix race condition regarding dst->expires and dst->from.

[YOSHIFUJI Hideaki]

Eric Dumazet wrote:
| Some strange crashes happen in rt6_check_expired(), with access
| to random addresses.
|
| At first glance, it looks like the RTF_EXPIRES and
| stuff added in commit 1716a96101c49186b
| (ipv6: fix problem with expired dst cache)
| are racy : same dst could be manipulated at the same time
| on different cpus.
|
| At some point, our stack believes rt->dst.from contains a dst pointer,
| while its really a jiffie value (as rt->dst.expires shares the same area
| of memory)
|
| rt6_update_expires() should be fixed, or am I missing something ?
|
| CC Neil because of https://bugzilla.redhat.com/show_bug.cgi?id=892060

Because we do not have any locks for dst_entry, we cannot change
essential structure in the entry; e.g., we cannot change reference
to other entity.

To fix this issue, split 'from' and 'expires' field in dst_entry
out of union.  Once it is 'from' is assigned in the constructor,
keep the reference until the very last stage of the life time of
the object.

Of course, it is unsafe to change 'from', so make rt6_set_from simple
just for fresh entries.

Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Reported-by: Neil Horman <nhorman@tuxdriver.com>
CC: Gao Feng <gaofeng@cn.fujitsu.com>
Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
---
 include/net/dst.h     |    8 ++------
 include/net/ip6_fib.h |   39 ++++++++++++---------------------------
 net/core/dst.c        |    1 +
 net/ipv6/route.c      |    8 +++-----
 4 files changed, 18 insertions(+), 38 deletions(-)

diff --git a/include/net/dst.h b/include/net/dst.h
index 3da47e0..853cda1 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -36,13 +36,9 @@ struct dst_entry {
 	struct net_device       *dev;
 	struct  dst_ops	        *ops;
 	unsigned long		_metrics;
-	union {
-		unsigned long           expires;
-		/* point to where the dst_entry copied from */
-		struct dst_entry        *from;
-	};
+	unsigned long           expires;
 	struct dst_entry	*path;
-	void			*__pad0;
+	struct dst_entry	*from;
 #ifdef CONFIG_XFRM
 	struct xfrm_state	*xfrm;
 #else
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 6919a50..2a601e7 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -164,50 +164,35 @@ static inline struct inet6_dev *ip6_dst_idev(struct dst_entry *dst)
 
 static inline void rt6_clean_expires(struct rt6_info *rt)
 {
-	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from)
-		dst_release(rt->dst.from);
-
 	rt->rt6i_flags &= ~RTF_EXPIRES;
-	rt->dst.from = NULL;
 }
 
 static inline void rt6_set_expires(struct rt6_info *rt, unsigned long expires)
 {
-	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from)
-		dst_release(rt->dst.from);
-
-	rt->rt6i_flags |= RTF_EXPIRES;
 	rt->dst.expires = expires;
+	rt->rt6i_flags |= RTF_EXPIRES;
 }
 
-static inline void rt6_update_expires(struct rt6_info *rt, int timeout)
+static inline void rt6_update_expires(struct rt6_info *rt0, int timeout)
 {
-	if (!(rt->rt6i_flags & RTF_EXPIRES)) {
-		if (rt->dst.from)
-			dst_release(rt->dst.from);
-		/* dst_set_expires relies on expires == 0 
-		 * if it has not been set previously.
-		 */
-		rt->dst.expires = 0;
-	}
-
-	dst_set_expires(&rt->dst, timeout);
-	rt->rt6i_flags |= RTF_EXPIRES;
+	struct rt6_info *rt;
+
+	for (rt = rt0; rt && !(rt->rt6i_flags & RTF_EXPIRES);
+	     rt = (struct rt6_info *)rt->dst.from);
+	if (rt && rt != rt0)
+		rt0->dst.expires = rt->dst.expires;
+
+	dst_set_expires(&rt0->dst, timeout);
+	rt0->rt6i_flags |= RTF_EXPIRES;
 }
 
 static inline void rt6_set_from(struct rt6_info *rt, struct rt6_info *from)
 {
 	struct dst_entry *new = (struct dst_entry *) from;
 
-	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from) {
-		if (new == rt->dst.from)
-			return;
-		dst_release(rt->dst.from);
-	}
-
 	rt->rt6i_flags &= ~RTF_EXPIRES;
-	rt->dst.from = new;
 	dst_hold(new);
+	rt->dst.from = new;
 }
 
 static inline void ip6_rt_put(struct rt6_info *rt)
diff --git a/net/core/dst.c b/net/core/dst.c
index ee6153e..35fd12f 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -179,6 +179,7 @@ void *dst_alloc(struct dst_ops *ops, struct net_device *dev,
 	dst_init_metrics(dst, dst_default_metrics, true);
 	dst->expires = 0UL;
 	dst->path = dst;
+	dst->from = NULL;
 #ifdef CONFIG_XFRM
 	dst->xfrm = NULL;
 #endif
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 515bb51..9282665 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -287,6 +287,7 @@ static void ip6_dst_destroy(struct dst_entry *dst)
 {
 	struct rt6_info *rt = (struct rt6_info *)dst;
 	struct inet6_dev *idev = rt->rt6i_idev;
+	struct dst_entry *from = dst->from;
 
 	if (!(rt->dst.flags & DST_HOST))
 		dst_destroy_metrics_generic(dst);
@@ -296,8 +297,8 @@ static void ip6_dst_destroy(struct dst_entry *dst)
 		in6_dev_put(idev);
 	}
 
-	if (!(rt->rt6i_flags & RTF_EXPIRES) && dst->from)
-		dst_release(dst->from);
+	dst->from = NULL;
+	dst_release(from);
 
 	if (rt6_has_peer(rt)) {
 		struct inet_peer *peer = rt6_peer_ptr(rt);
@@ -1010,7 +1011,6 @@ struct dst_entry *ip6_blackhole_route(struct net *net, struct dst_entry *dst_ori
 
 		rt->rt6i_gateway = ort->rt6i_gateway;
 		rt->rt6i_flags = ort->rt6i_flags;
-		rt6_clean_expires(rt);
 		rt->rt6i_metric = 0;
 
 		memcpy(&rt->rt6i_dst, &ort->rt6i_dst, sizeof(struct rt6key));
@@ -1784,8 +1784,6 @@ static struct rt6_info *ip6_rt_copy(struct rt6_info *ort,
 		if ((ort->rt6i_flags & (RTF_DEFAULT | RTF_ADDRCONF)) ==
 		    (RTF_DEFAULT | RTF_ADDRCONF))
 			rt6_set_from(rt, ort);
-		else
-			rt6_clean_expires(rt);
 		rt->rt6i_metric = 0;
 
 #ifdef CONFIG_IPV6_SUBTREES
-- 
1.7.9.5

Re: [PATCH V2] ipv6: fix race condition regarding dst->expires and dst->from.

[Eric Dumazet]

On Wed, 2013-02-20 at 19:29 +0900, YOSHIFUJI Hideaki wrote:
> Eric Dumazet wrote:
> | Some strange crashes happen in rt6_check_expired(), with access
> | to random addresses.
> |
> | At first glance, it looks like the RTF_EXPIRES and
> | stuff added in commit 1716a96101c49186b
> | (ipv6: fix problem with expired dst cache)
> | are racy : same dst could be manipulated at the same time
> | on different cpus.
> |
> | At some point, our stack believes rt->dst.from contains a dst pointer,
> | while its really a jiffie value (as rt->dst.expires shares the same area
> | of memory)
> |
> | rt6_update_expires() should be fixed, or am I missing something ?
> |
> | CC Neil because of https://bugzilla.redhat.com/show_bug.cgi?id=892060
> 
> Because we do not have any locks for dst_entry, we cannot change
> essential structure in the entry; e.g., we cannot change reference
> to other entity.
> 
> To fix this issue, split 'from' and 'expires' field in dst_entry
> out of union.  Once it is 'from' is assigned in the constructor,
> keep the reference until the very last stage of the life time of
> the object.
> 
> Of course, it is unsafe to change 'from', so make rt6_set_from simple
> just for fresh entries.
> 
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Reported-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Gao Feng <gaofeng@cn.fujitsu.com>
> Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
> ---

This seems good to me, but I cant test it at this moment.

I CC Steinar as he reported one crash to me.

Thanks Yoshifuji !

Reviewed-by: Eric Dumazet <edumazet@google.com>

Reported-by: Steinar H. Gunderson <sesse@google.com>

Re: [PATCH V2] ipv6: fix race condition regarding dst->expires and dst->from.

[Neil Horman]

On Wed, Feb 20, 2013 at 08:12:40AM -0800, Eric Dumazet wrote:
> On Wed, 2013-02-20 at 19:29 +0900, YOSHIFUJI Hideaki wrote:
> > Eric Dumazet wrote:
> > | Some strange crashes happen in rt6_check_expired(), with access
> > | to random addresses.
> > |
> > | At first glance, it looks like the RTF_EXPIRES and
> > | stuff added in commit 1716a96101c49186b
> > | (ipv6: fix problem with expired dst cache)
> > | are racy : same dst could be manipulated at the same time
> > | on different cpus.
> > |
> > | At some point, our stack believes rt->dst.from contains a dst pointer,
> > | while its really a jiffie value (as rt->dst.expires shares the same area
> > | of memory)
> > |
> > | rt6_update_expires() should be fixed, or am I missing something ?
> > |
> > | CC Neil because of https://bugzilla.redhat.com/show_bug.cgi?id=892060
> > 
> > Because we do not have any locks for dst_entry, we cannot change
> > essential structure in the entry; e.g., we cannot change reference
> > to other entity.
> > 
> > To fix this issue, split 'from' and 'expires' field in dst_entry
> > out of union.  Once it is 'from' is assigned in the constructor,
> > keep the reference until the very last stage of the life time of
> > the object.
> > 
> > Of course, it is unsafe to change 'from', so make rt6_set_from simple
> > just for fresh entries.
> > 
> > Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> > Reported-by: Neil Horman <nhorman@tuxdriver.com>
> > CC: Gao Feng <gaofeng@cn.fujitsu.com>
> > Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
> > ---
> 
> This seems good to me, but I cant test it at this moment.
> 
> I CC Steinar as he reported one crash to me.
> 
> Thanks Yoshifuji !
> 
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> 
> Reported-by: Steinar H. Gunderson <sesse@google.com>
> 
I've also got requests in to test, and a recent fedora build running here:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5036210

If anyone else wants to test it.  Although, looking at it, I think this is a
good fix:
Reviewed-by: Neil Horman <nhorman@tuxdriver.com>

> 
> 
> 

Re: [PATCH V2] ipv6: fix race condition regarding dst->expires and dst->from.

[David Miller]

From: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Date: Wed, 20 Feb 2013 19:29:08 +0900

> Eric Dumazet wrote:
> | Some strange crashes happen in rt6_check_expired(), with access
> | to random addresses.
> |
> | At first glance, it looks like the RTF_EXPIRES and
> | stuff added in commit 1716a96101c49186b
> | (ipv6: fix problem with expired dst cache)
> | are racy : same dst could be manipulated at the same time
> | on different cpus.
> |
> | At some point, our stack believes rt->dst.from contains a dst pointer,
> | while its really a jiffie value (as rt->dst.expires shares the same area
> | of memory)
> |
> | rt6_update_expires() should be fixed, or am I missing something ?
> |
> | CC Neil because of https://bugzilla.redhat.com/show_bug.cgi?id=892060
> 
> Because we do not have any locks for dst_entry, we cannot change
> essential structure in the entry; e.g., we cannot change reference
> to other entity.
> 
> To fix this issue, split 'from' and 'expires' field in dst_entry
> out of union.  Once it is 'from' is assigned in the constructor,
> keep the reference until the very last stage of the life time of
> the object.
> 
> Of course, it is unsafe to change 'from', so make rt6_set_from simple
> just for fresh entries.
> 
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Reported-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Gao Feng <gaofeng@cn.fujitsu.com>
> Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>

Applied and queued up for -stable, thanks.

RE: [RFC PATCH] ipv6: Split from and expires field in dst_entry out of union [net-next]

[David Laight]

>  static inline void rt6_clean_expires(struct rt6_info *rt)
>  {
> -	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from)
> +	if (!rt->rt6i_flags & RTF_EXPIRES)
>  		dst_release(rt->dst.from);
> 
>  	rt->rt6i_flags &= ~RTF_EXPIRES;
>  	rt->dst.from = NULL;
> +	rt->dst.expires = 0;
>  }
> 
>  static inline void rt6_set_expires(struct rt6_info *rt, unsigned long expires)
>  {
> -	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from)
> +	if (!rt->rt6i_flags & RTF_EXPIRES)
>  		dst_release(rt->dst.from);
> 
>  	rt->rt6i_flags |= RTF_EXPIRES;
> +	rt->dst.from = NULL;
>  	rt->dst.expires = expires;
>  }
> 
>  static inline void rt6_update_expires(struct rt6_info *rt, int timeout)
>  {
>  	if (!(rt->rt6i_flags & RTF_EXPIRES)) {
> -		if (rt->dst.from)
> -			dst_release(rt->dst.from);
> +		dst_release(rt->dst.from);
>  		/* dst_set_expires relies on expires == 0
>  		 * if it has not been set previously.
>  		 */
>  		rt->dst.expires = 0;
> +		rt6->dst.from = NULL;
>  	}

Aren't there also problems with setting and clearing RTF_EXPIRES?
Since that flag looks as though it was the descriminant for the union
it probably isn't needed - provided dst.expires is never 0 when valid.

	David

Re: [RFC PATCH] ipv6: Split from and expires field in dst_entry out of union [net-next]

[Neil Horman]

On Wed, Feb 20, 2013 at 10:55:35AM -0000, David Laight wrote:
> >  static inline void rt6_clean_expires(struct rt6_info *rt)
> >  {
> > -	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from)
> > +	if (!rt->rt6i_flags & RTF_EXPIRES)
> >  		dst_release(rt->dst.from);
> > 
> >  	rt->rt6i_flags &= ~RTF_EXPIRES;
> >  	rt->dst.from = NULL;
> > +	rt->dst.expires = 0;
> >  }
> > 
> >  static inline void rt6_set_expires(struct rt6_info *rt, unsigned long expires)
> >  {
> > -	if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from)
> > +	if (!rt->rt6i_flags & RTF_EXPIRES)
> >  		dst_release(rt->dst.from);
> > 
> >  	rt->rt6i_flags |= RTF_EXPIRES;
> > +	rt->dst.from = NULL;
> >  	rt->dst.expires = expires;
> >  }
> > 
> >  static inline void rt6_update_expires(struct rt6_info *rt, int timeout)
> >  {
> >  	if (!(rt->rt6i_flags & RTF_EXPIRES)) {
> > -		if (rt->dst.from)
> > -			dst_release(rt->dst.from);
> > +		dst_release(rt->dst.from);
> >  		/* dst_set_expires relies on expires == 0
> >  		 * if it has not been set previously.
> >  		 */
> >  		rt->dst.expires = 0;
> > +		rt6->dst.from = NULL;
> >  	}
> 
> Aren't there also problems with setting and clearing RTF_EXPIRES?
> Since that flag looks as though it was the descriminant for the union
> it probably isn't needed - provided dst.expires is never 0 when valid.
> 
> 	David
The use of RTF_EXPIRES is weak at this point, if there are multiple accessors,
but I think the point is moot, in that the only thing we ever do when we change
the flag is release dst.from, which is safe.
Neil

RE: [RFC PATCH] ipv6: Split from and expires field in dst_entry out of union [net-next]

[David Laight]

> > Aren't there also problems with setting and clearing RTF_EXPIRES?
> > Since that flag looks as though it was the descriminant for the union
> > it probably isn't needed - provided dst.expires is never 0 when valid.
> >
> > 	David
>
> The use of RTF_EXPIRES is weak at this point, if there are multiple accessors,
> but I think the point is moot, in that the only thing we ever do when we change
> the flag is release dst.from, which is safe.
>
> Neil

I was also worried about the RMW cycles of rt6i_flags itself.

	David