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

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

[Cong Wang]

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" <dbanerje@akamai.com>
Cc: "Banerjee, Debabrata" <dbanerje@akamai.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 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

[PATCH 2/2] ipv6: remove some useless RCU read lock

[Cong Wang]

After this commit:
	commit 97cac0821af4474ec4ba3a9e7a36b98ed9b6db88
	Author: David S. Miller <davem@davemloft.net>
	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 <amwang@redhat.com>
---
 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

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

[Debabrata Banerjee]

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 <amwang@redhat.com> wrote:
> 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" <dbanerje@akamai.com>
> Cc: "Banerjee, Debabrata" <dbanerje@akamai.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
> Cc: Patrick McHardy <kaber@trash.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>
> ---
>  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
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Cong Wang]

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!

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

[Cong Wang]

[-- Attachment #1: Type: text/plain, Size: 75 bytes --]

Hi, Debabrata,

Could you help to test the attached patch below?

Thanks!


[-- Attachment #2: ipv6-deadlock.diff --]
[-- Type: text/x-patch, Size: 1563 bytes --]

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index eb3f1e4..cd141a5 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -440,9 +440,26 @@ out:
 }
 
 #ifdef CONFIG_IPV6_ROUTER_PREF
+struct ndisc_work {
+	struct work_struct work;
+	struct in6_addr mcaddr;
+	struct in6_addr target;
+	struct net_device *dev;
+};
+
+static void queue_ndisc(struct work_struct *work)
+{
+	struct ndisc_work *nw =
+		container_of(work, struct ndisc_work, work);
+	ndisc_send_ns(nw->dev, NULL, &nw->target, &nw->mcaddr, NULL);
+	kfree(nw);
+}
+
 static void rt6_probe(struct rt6_info *rt)
 {
 	struct neighbour *neigh;
+	struct ndisc_work *nw;
+
 	/*
 	 * Okay, this does not seem to be appropriate
 	 * for now, however, we need to check if it
@@ -457,15 +474,18 @@ static void rt6_probe(struct rt6_info *rt)
 	read_lock_bh(&neigh->lock);
 	if (!(neigh->nud_state & NUD_VALID) &&
 	    time_after(jiffies, neigh->updated + rt->rt6i_idev->cnf.rtr_probe_interval)) {
-		struct in6_addr mcaddr;
-		struct in6_addr *target;
 
 		neigh->updated = jiffies;
 		read_unlock_bh(&neigh->lock);
 
-		target = (struct in6_addr *)&neigh->primary_key;
-		addrconf_addr_solict_mult(target, &mcaddr);
-		ndisc_send_ns(rt->dst.dev, NULL, target, &mcaddr, NULL);
+		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);
+		}
 	} else {
 		read_unlock_bh(&neigh->lock);
 	}

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

[Banerjee, Debabrata]

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" <amwang@redhat.com> wrote:

>Hi, Debabrata,
>
>Could you help to test the attached patch below?
>
>Thanks!
>

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

[Cong Wang]

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.

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

[Eric Dumazet]

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.

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

[Cong Wang]

On Thu, 2012-08-23 at 18:10 +0200, Eric Dumazet wrote:
> 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.
> 

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.

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

[Eric Dumazet]

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.

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

[Cong Wang]

On Fri, 2012-08-24 at 11:44 +0200, Eric Dumazet wrote:
> 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.
> 
> 

Agreed. I will make a new patch.

Thanks!