[PATCH 1/2] ipv6: do not hold route table lock when send ndisc probe

In rt6_probe(), we call ndisc_send_ns() with root->rwlock,
but this is not necessary, so we can drop it before calling
ndisc_send_ns().

This could probably fix the deadlock reported by Debabrata:
https://lkml.org/lkml/2012/8/16/432

Reported-by: "Banerjee, Debabrata" <redacted>
Cc: "Banerjee, Debabrata" <redacted>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Hideaki YOSHIFUJI <redacted>
Cc: Patrick McHardy <redacted>
Signed-off-by: Cong Wang <redacted>
---
 net/ipv6/route.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 0ddf2d1..7a36df2 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c

@@ -460,13 +460,18 @@ static void rt6_probe(struct rt6_info *rt)
 	    time_after(jiffies, neigh->updated + rt->rt6i_idev->cnf.rtr_probe_interval)) {
 		struct in6_addr mcaddr;
 		struct in6_addr *target;
+		struct net_device *dev = rt->dst.dev;
+		struct fib6_table *table = rt->rt6i_table;
 
 		neigh->updated = jiffies;
 		read_unlock_bh(&neigh->lock);
+		read_unlock_bh(&table->tb6_lock);
 
 		target = (struct in6_addr *)&neigh->primary_key;
 		addrconf_addr_solict_mult(target, &mcaddr);
-		ndisc_send_ns(rt->dst.dev, NULL, target, &mcaddr, NULL);
+		ndisc_send_ns(dev, NULL, target, &mcaddr, NULL);
+
+		read_lock_bh(&table->tb6_lock);
 	} else {
 		read_unlock_bh(&neigh->lock);
 	}

-- 
1.7.7.6

After this commit:
	commit 97cac0821af4474ec4ba3a9e7a36b98ed9b6db88
	Author: David S. Miller [off-list ref]
	Date:   Mon Jul 2 22:43:47 2012 -0700

	    ipv6: Store route neighbour in rt6_info struct.

we no longer use RCU to protect route neighbour.

Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <redacted>
---
 net/ipv6/ip6_output.c |   13 ++-----------
 net/ipv6/route.c      |   15 ++-------------
 2 files changed, 4 insertions(+), 24 deletions(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 5b2d63e..9f67746 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c

@@ -123,16 +123,11 @@ static int ip6_finish_output2(struct sk_buff *skb)
 				skb->len);
 	}
 
-	rcu_read_lock();
 	rt = (struct rt6_info *) dst;
 	neigh = rt->n;
-	if (neigh) {
-		int res = dst_neigh_output(dst, neigh, skb);
+	if (neigh)
+		return dst_neigh_output(dst, neigh, skb);
 
-		rcu_read_unlock();
-		return res;
-	}
-	rcu_read_unlock();
 	IP6_INC_STATS_BH(dev_net(dst->dev),
 			 ip6_dst_idev(dst), IPSTATS_MIB_OUTNOROUTES);
 	kfree_skb(skb);

@@ -980,7 +975,6 @@ static int ip6_dst_lookup_tail(struct sock *sk,
 	 * dst entry and replace it instead with the
 	 * dst entry of the nexthop router
 	 */
-	rcu_read_lock();
 	rt = (struct rt6_info *) *dst;
 	n = rt->n;
 	if (n && !(n->nud_state & NUD_VALID)) {

@@ -988,7 +982,6 @@ static int ip6_dst_lookup_tail(struct sock *sk,
 		struct flowi6 fl_gw6;
 		int redirect;
 
-		rcu_read_unlock();
 		ifp = ipv6_get_ifaddr(net, &fl6->saddr,
 				      (*dst)->dev, 1);
 

@@ -1008,8 +1001,6 @@ static int ip6_dst_lookup_tail(struct sock *sk,
 			if ((err = (*dst)->error))
 				goto out_err_release;
 		}
-	} else {
-		rcu_read_unlock();
 	}
 #endif
 

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 7a36df2..0aeeb98 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c

@@ -451,10 +451,9 @@ static void rt6_probe(struct rt6_info *rt)
 	 * Router Reachability Probe MUST be rate-limited
 	 * to no more than one per minute.
 	 */
-	rcu_read_lock();
 	neigh = rt ? rt->n : NULL;
 	if (!neigh || (neigh->nud_state & NUD_VALID))
-		goto out;
+		return;
 	read_lock_bh(&neigh->lock);
 	if (!(neigh->nud_state & NUD_VALID) &&
 	    time_after(jiffies, neigh->updated + rt->rt6i_idev->cnf.rtr_probe_interval)) {

@@ -475,8 +474,6 @@ static void rt6_probe(struct rt6_info *rt)
 	} else {
 		read_unlock_bh(&neigh->lock);
 	}
-out:
-	rcu_read_unlock();
 }
 #else
 static inline void rt6_probe(struct rt6_info *rt)

@@ -503,7 +500,6 @@ static inline int rt6_check_neigh(struct rt6_info *rt)
 	struct neighbour *neigh;
 	int m;
 
-	rcu_read_lock();
 	neigh = rt->n;
 	if (rt->rt6i_flags & RTF_NONEXTHOP ||
 	    !(rt->rt6i_flags & RTF_GATEWAY))

@@ -521,7 +517,6 @@ static inline int rt6_check_neigh(struct rt6_info *rt)
 		read_unlock_bh(&neigh->lock);
 	} else
 		m = 0;
-	rcu_read_unlock();
 	return m;
 }
 

@@ -2470,15 +2465,11 @@ static int rt6_fill_node(struct net *net,
 	if (rtnetlink_put_metrics(skb, dst_metrics_ptr(&rt->dst)) < 0)
 		goto nla_put_failure;
 
-	rcu_read_lock();
 	n = rt->n;
 	if (n) {
-		if (nla_put(skb, RTA_GATEWAY, 16, &n->primary_key) < 0) {
-			rcu_read_unlock();
+		if (nla_put(skb, RTA_GATEWAY, 16, &n->primary_key) < 0)
 			goto nla_put_failure;
-		}
 	}
-	rcu_read_unlock();
 
 	if (rt->dst.dev &&
 	    nla_put_u32(skb, RTA_OIF, rt->dst.dev->ifindex))

@@ -2680,14 +2671,12 @@ static int rt6_info_route(struct rt6_info *rt, void *p_arg)
 #else
 	seq_puts(m, "00000000000000000000000000000000 00 ");
 #endif
-	rcu_read_lock();
 	n = rt->n;
 	if (n) {
 		seq_printf(m, "%pi6", n->primary_key);
 	} else {
 		seq_puts(m, "00000000000000000000000000000000");
 	}
-	rcu_read_unlock();
 	seq_printf(m, " %08x %08x %08x %08x %8s\n",
 		   rt->rt6i_metric, atomic_read(&rt->dst.__refcnt),
 		   rt->dst.__use, rt->rt6i_flags,

-- 
1.7.7.6

Well it get rids of the deadlock for sure, but I am not sure it
doesn't break something else, one would have to know all of this code
much better to tell. You'll notice read_unlock_bh(&table->tb6_lock)
for the first lock in ip6_pol_route() has more in the critical section
after the rt6_select() call, especially that rather scary BACKTRACK()
macro.

-Debabrata

On Fri, Aug 17, 2012 at 3:11 AM, Cong Wang [off-list ref] wrote:

On Fri, 2012-08-17 at 14:54 -0400, Debabrata Banerjee wrote:

Well it get rids of the deadlock for sure, but I am not sure it
doesn't break something else, one would have to know all of this code
much better to tell. You'll notice read_unlock_bh(&table->tb6_lock)
for the first lock in ip6_pol_route() has more in the critical section
after the rt6_select() call, especially that rather scary BACKTRACK()
macro.

Yeah, I noticed that, ->tb6_lock seems to protect lookup of ->tb6_root,
not sure if 'rt' may still valid after retaking this lock... Hmm,
probably calling ndisc_send_ns() inside a work is a better approach.

I will update the patch.

Thanks!

Hi, Debabrata,

Could you help to test the attached patch below?

Thanks!

Thanks for the patch. We're discussing how to reach to this code properly
in the lab now. Although we will probably have to modify so it's compliant
with the RFC, by checking if a ndisc_send_ns() has already been queued
within rt->rt6i_idev->cnf.rtr_probe_interval, otherwise it could flood the
network with neighbor discoveries.

-Debabrata

On 8/20/12 11:44 PM, "Cong Wang" [off-list ref] wrote:

Hi, Debabrata,

Could you help to test the attached patch below?

Thanks!

On Wed, 2012-08-22 at 12:04 -0400, Banerjee, Debabrata wrote:

Thanks for the patch. We're discussing how to reach to this code properly
in the lab now. Although we will probably have to modify so it's compliant
with the RFC, by checking if a ndisc_send_ns() has already been queued
within rt->rt6i_idev->cnf.rtr_probe_interval, otherwise it could flood the
network with neighbor discoveries.

Thanks for testing, I can't reproduce the deadlock you reported here.

Note that the original code didn't check rtr_probe_interval either.

On Tue, 2012-08-21 at 11:44 +0800, Cong Wang wrote:

Hi, Debabrata,

Could you help to test the attached patch below?

Thanks!

Hard to comment on your patch since its not inlined.

+               nw = kmalloc(sizeof(*nw), GFP_ATOMIC);
+               if (nw) {
+                       memcpy(&nw->target, &neigh->primary_key, sizeof(struct in6_addr));
+                       addrconf_addr_solict_mult(&nw->target, &nw->mcaddr);
+                       nw->dev = rt->dst.dev;
+                       INIT_WORK(&nw->work, queue_ndisc);
+                       schedule_work(&nw->work);
+               }

You cant do that without taking extra reference on dev,
and release it in queue_ndisc()

This also will add interesting side effects at device dismantle.

On Thu, 2012-08-23 at 18:10 +0200, Eric Dumazet wrote:

On Tue, 2012-08-21 at 11:44 +0800, Cong Wang wrote:

quoted

Hi, Debabrata,

Could you help to test the attached patch below?

Thanks!

Hard to comment on your patch since its not inlined.

+               nw = kmalloc(sizeof(*nw), GFP_ATOMIC);
+               if (nw) {
+                       memcpy(&nw->target, &neigh->primary_key, sizeof(struct in6_addr));
+                       addrconf_addr_solict_mult(&nw->target, &nw->mcaddr);
+                       nw->dev = rt->dst.dev;
+                       INIT_WORK(&nw->work, queue_ndisc);
+                       schedule_work(&nw->work);
+               }

You cant do that without taking extra reference on dev,
and release it in queue_ndisc()

This also will add interesting side effects at device dismantle.

Right... If we call dev_hold() in the work, it is possible that the work
is still not scheduled to running when we unregister the device. What's
more, we can't flush work here as we are holding a read lock.

Hmm.

On Fri, 2012-08-24 at 17:15 +0800, Cong Wang wrote:

Right... If we call dev_hold() in the work, it is possible that the work
is still not scheduled to running when we unregister the device. What's
more, we can't flush work here as we are holding a read lock.

You need a global list (and a single work, not one per req), so that a
notifier can flush it at demand.

On Fri, 2012-08-24 at 11:44 +0200, Eric Dumazet wrote:

On Fri, 2012-08-24 at 17:15 +0800, Cong Wang wrote:

quoted

Right... If we call dev_hold() in the work, it is possible that the work
is still not scheduled to running when we unregister the device. What's
more, we can't flush work here as we are holding a read lock.

You need a global list (and a single work, not one per req), so that a
notifier can flush it at demand.

Agreed. I will make a new patch.

Thanks!