Skip to contentSkip to conversation

net/ipv6: couple of fixes for rcu change to from

PATCH In review David Ahern 3 participants 6 messages Apr 23, 2018 – Apr 24, 2018

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

David Ahern

author
#0 lore raw

cover net/ipv6: couple of fixes for rcu change to from

So many details... I am thankful for all the robots running the permutations and tools.

Two bug fixes from the rcu change to rt->from:
1. missing rcu lock in ip6_negative_advice
2. rcu dereferences in 2 sites

David Ahern (2):
net/ipv6: add rcu locking to ip6_negative_advice net/ipv6: Fix missing rcu dereferences on from

 net/ipv6/route.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
Signature · 1 line
2.11.0
author
#1 lore raw

PATCH 1/2 net/ipv6: add rcu locking to ip6_negative_advice

syzbot reported a suspicious rcu_dereference_check:

  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x1b9/0x294 lib/dump_stack.c:113
  lockdep_rcu_suspicious+0x14a/0x153 kernel/locking/lockdep.c:4592
  rt6_check_expired+0x38b/0x3e0 net/ipv6/route.c:410
  ip6_negative_advice+0x67/0xc0 net/ipv6/route.c:2204
  dst_negative_advice include/net/sock.h:1786 [inline]
  sock_setsockopt+0x138f/0x1fe0 net/core/sock.c:1051
  __sys_setsockopt+0x2df/0x390 net/socket.c:1899
  SYSC_setsockopt net/socket.c:1914 [inline]
  SyS_setsockopt+0x34/0x50 net/socket.c:1911

Add rcu locking around call to rt6_check_expired in ip6_negative_advice.

Fixes a68886a69180 ("net/ipv6: Make from in rt6_info rcu protected")
Reported-by [email protected]
Signed-off-by David Ahern <[email protected]>
---
 net/ipv6/route.c | 2 ++
 1 file changed, 2 insertions(+)
net/ipv6/route.c +2 -0
index 0407bbc5a028..354a5b8d016f 100644
@@ -2201,10 +2201,12 @@ static struct dst_entry *ip6_negative_advice(struct dst_entry *dst)
 
 	if (rt) {
 		if (rt->rt6i_flags & RTF_CACHE) {
+			rcu_read_lock();
 			if (rt6_check_expired(rt)) {
 				rt6_remove_exception_rt(rt);
 				dst = NULL;
 			}
+			rcu_read_unlock();
 		} else {
 			dst_release(dst);
 			dst = NULL;
Signature · 1 line
2.11.0
author
#2 lore raw

PATCH 2/2 net/ipv6: Fix missing rcu dereferences on from

In reply to David Ahern

kbuild test robot reported 2 uses of rt->from not properly accessed using rcu_dereference:
1. add rcu_dereference_protected to rt6_remove_exception_rt and make sure it is always called with rcu lock held.

2. change rt6_do_redirect to take a reference on 'from' when accessed the first time so it can be used the sceond time outside of the lock

Fixes a68886a69180 ("net/ipv6: Make from in rt6_info rcu protected")
Reported-by kbuild test robot <[email protected]>
Signed-off-by David Ahern <[email protected]>
---
 net/ipv6/route.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
net/ipv6/route.c +10 -5
index 354a5b8d016f..ac3e51631c65 100644
@@ -1541,11 +1541,13 @@ static struct rt6_info *rt6_find_cached_rt(struct fib6_info *rt,
 static int rt6_remove_exception_rt(struct rt6_info *rt)
 {
 	struct rt6_exception_bucket *bucket;
-	struct fib6_info *from = rt->from;
 	struct in6_addr *src_key = NULL;
 	struct rt6_exception *rt6_ex;
+	struct fib6_info *from;
 	int err;
 
+	from = rcu_dereference_protected(rt->from,
+					 lockdep_is_held(&rt6_exception_lock));
 	if (!from ||
 	    !(rt->rt6i_flags & RTF_CACHE))
 		return -EINVAL;

@@ -2223,6 +2225,7 @@ static void ip6_link_failure(struct sk_buff *skb)
 
 	rt = (struct rt6_info *) skb_dst(skb);
 	if (rt) {
+		rcu_read_lock();
 		if (rt->rt6i_flags & RTF_CACHE) {
 			if (dst_hold_safe(&rt->dst))
 				rt6_remove_exception_rt(rt);

@@ -2230,15 +2233,14 @@ static void ip6_link_failure(struct sk_buff *skb)
 			struct fib6_info *from;
 			struct fib6_node *fn;
 
-			rcu_read_lock();
 			from = rcu_dereference(rt->from);
 			if (from) {
 				fn = rcu_dereference(from->fib6_node);
 				if (fn && (rt->rt6i_flags & RTF_DEFAULT))
 					fn->fn_sernum = -1;
 			}
-			rcu_read_unlock();
 		}
+		rcu_read_unlock();
 	}
 }
 

@@ -3340,8 +3342,10 @@ static void rt6_do_redirect(struct dst_entry *dst, struct sock *sk, struct sk_bu
 
 	rcu_read_lock();
 	from = rcu_dereference(rt->from);
-	nrt = ip6_rt_cache_alloc(from, &msg->dest, NULL);
+	fib6_info_hold(from);
 	rcu_read_unlock();
+
+	nrt = ip6_rt_cache_alloc(from, &msg->dest, NULL);
 	if (!nrt)
 		goto out;
 

@@ -3355,7 +3359,7 @@ static void rt6_do_redirect(struct dst_entry *dst, struct sock *sk, struct sk_bu
 	 * a cached route because rt6_insert_exception() will
 	 * takes care of it
 	 */
-	if (rt6_insert_exception(nrt, rt->from)) {
+	if (rt6_insert_exception(nrt, from)) {
 		dst_release_immediate(&nrt->dst);
 		goto out;
 	}

@@ -3367,6 +3371,7 @@ static void rt6_do_redirect(struct dst_entry *dst, struct sock *sk, struct sk_bu
 	call_netevent_notifiers(NETEVENT_REDIRECT, &netevent);
 
 out:
+	fib6_info_release(from);
 	neigh_release(neigh);
 }
Signature · 1 line
2.11.0
#5 lore raw
In reply to David Ahern

From: David Ahern <[email protected]>
Date: Mon, 23 Apr 2018 11:32:05 -0700

Quoted 2 lines

So many details... I am thankful for all the robots running the permutations and tools.

This is why I am not afraid of the robots taking over. :-)

Quoted 3 lines

Two bug fixes from the rcu change to rt->from:
1. missing rcu lock in ip6_negative_advice
2. rcu dereferences in 2 sites

Series applied, thanks David.

#3 lore raw

Re: [PATCH net-next 2/2] net/ipv6: Fix missing rcu dereferences on from

In reply to David Ahern
Quoted 21 lines · David Ahern

kbuild test robot reported 2 uses of rt->from not properly accessed using rcu_dereference:
1. add rcu_dereference_protected to rt6_remove_exception_rt and make sure it is always called with rcu lock held.

2. change rt6_do_redirect to take a reference on 'from' when accessed the first time so it can be used the sceond time outside of the lock

Fixes a68886a69180 ("net/ipv6: Make from in rt6_info rcu protected")
Reported-by kbuild test robot <[email protected]>
Signed-off-by David Ahern <[email protected]>
---
 net/ipv6/route.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
net/ipv6/route.c +0 -0
index 354a5b8d016f..ac3e51631c65 100644
@@ -1541,11 +1541,13 @@ static struct rt6_info *rt6_find_cached_rt(struct fib6_info *rt,
 static int rt6_remove_exception_rt(struct rt6_info *rt)

net/ipv6/route.c static struct rt6_info *rt6_find_cached_rt(struct fib6_info *rt, · line 1550

Quoted 10 lines
net/ipv6/route.c +3 -1
 {
 	struct rt6_exception_bucket *bucket;
-	struct fib6_info *from = rt->from;
 	struct in6_addr *src_key = NULL;
 	struct rt6_exception *rt6_ex;
+	struct fib6_info *from;
 	int err;
 
+	from = rcu_dereference_protected(rt->from,
+					 lockdep_is_held(&rt6_exception_lock));

This does not make any sense.

We lock rt6_exception_lock a bit later in this function (line 1558)

If we really were holding rt6_exception_lock here we would dead lock.

Quoted 4 lines
net/ipv6/route.c +0 -0
 	if (!from ||
 	    !(rt->rt6i_flags & RTF_CACHE))
 		return -EINVAL;

@@ -2223,6 +2225,7 @@ static void ip6_link_failure(struct sk_buff *skb)
#4 lore raw

Re: [PATCH net-next 2/2] net/ipv6: Fix missing rcu dereferences on from

Quoted 32 lines · Eric Dumazet
Quoted 29 lines · David Ahern

kbuild test robot reported 2 uses of rt->from not properly accessed using rcu_dereference:
1. add rcu_dereference_protected to rt6_remove_exception_rt and make sure it is always called with rcu lock held.

2. change rt6_do_redirect to take a reference on 'from' when accessed the first time so it can be used the sceond time outside of the lock

Fixes a68886a69180 ("net/ipv6: Make from in rt6_info rcu protected")
Reported-by kbuild test robot <[email protected]>
Signed-off-by David Ahern <[email protected]>
---
 net/ipv6/route.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
net/ipv6/route.c +1 -1
index 354a5b8d016f..ac3e51631c65 100644
@@ -1541,11 +1541,13 @@ static struct rt6_info *rt6_find_cached_rt(struct fib6_info *rt,
 static int rt6_remove_exception_rt(struct rt6_info *rt)
 {
 	struct rt6_exception_bucket *bucket;
-	struct fib6_info *from = rt->from;
 	struct in6_addr *src_key = NULL;
 	struct rt6_exception *rt6_ex;
+	struct fib6_info *from;
 	int err;
Quoted 8 lines

net/ipv6/route.c static struct rt6_info *rt6_find_cached_rt(struct fib6_info *rt, · line 1550

Quoted 2 lines
net/ipv6/route.c +2 -0
+	from = rcu_dereference_protected(rt->from,
+					 lockdep_is_held(&rt6_exception_lock));

This does not make any sense.

We lock rt6_exception_lock a bit later in this function (line 1558)

If we really were holding rt6_exception_lock here we would dead lock.

I will send this fix :

net/ipv6/route.c +1 -2
index ac3e51631c659b5c5c8a93c17011cb7f3ad266e2..432c4bcc1111085671f32987e4673e47898085a3 100644
@@ -1546,8 +1546,7 @@ static int rt6_remove_exception_rt(struct rt6_info *rt)
        struct fib6_info *from;
        int err;
 
-       from = rcu_dereference_protected(rt->from,
-                                        lockdep_is_held(&rt6_exception_lock));
+       from = rcu_dereference(rt->from);
        if (!from ||
            !(rt->rt6i_flags & RTF_CACHE))
                return -EINVAL;