[PATCH linux] net: fix deadlock while clearing neighbor proxy table

[PATCH linux] net: fix deadlock while clearing neighbor proxy table

[Wolfgang Bumiller]

When coming from ndisc_netdev_event() in net/ipv6/ndisc.c,
neigh_ifdown() is called with &nd_tbl, locking this while
clearing the proxy neighbor entries when eg. deleting an
interface. Calling the table's pndisc_destructor() with the
lock still held, however, can cause a deadlock: When a
multicast listener is available an IGMP packet of type
ICMPV6_MGM_REDUCTION may be sent out. When reaching
ip6_finish_output2(), if no neighbor entry for the target
address is found, __neigh_create() is called with &nd_tbl,
which it'll want to lock.

Move the elements into their own list, then unlock the table
and perform the destruction.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=199289
Fixes: 6fd6ce2056de ("ipv6: Do not depend on rt->n in ip6_finish_output2().")
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
I do feel bad about moving the unlock call. Perhaps returning the
freelist and dealing with it in neigh_ifdown() would be better?

 net/core/neighbour.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 7b7a14abba28..601df647588c 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -292,7 +292,6 @@ int neigh_ifdown(struct neigh_table *tbl, struct net_device *dev)
 	write_lock_bh(&tbl->lock);
 	neigh_flush_dev(tbl, dev);
 	pneigh_ifdown(tbl, dev);
-	write_unlock_bh(&tbl->lock);
 
 	del_timer_sync(&tbl->proxy_timer);
 	pneigh_queue_purge(&tbl->proxy_queue);
@@ -683,7 +682,7 @@ int pneigh_delete(struct neigh_table *tbl, struct net *net, const void *pkey,
 
 static int pneigh_ifdown(struct neigh_table *tbl, struct net_device *dev)
 {
-	struct pneigh_entry *n, **np;
+	struct pneigh_entry *n, **np, *freelist = NULL;
 	u32 h;
 
 	for (h = 0; h <= PNEIGH_HASHMASK; h++) {
@@ -691,16 +690,23 @@ static int pneigh_ifdown(struct neigh_table *tbl, struct net_device *dev)
 		while ((n = *np) != NULL) {
 			if (!dev || n->dev == dev) {
 				*np = n->next;
-				if (tbl->pdestructor)
-					tbl->pdestructor(n);
-				if (n->dev)
-					dev_put(n->dev);
-				kfree(n);
+				n->next = freelist;
+				freelist = n;
 				continue;
 			}
 			np = &n->next;
 		}
 	}
+	write_unlock_bh(&tbl->lock);
+	while ((n = freelist)) {
+		freelist = n->next;
+		n->next = NULL;
+		if (tbl->pdestructor)
+			tbl->pdestructor(n);
+		if (n->dev)
+			dev_put(n->dev);
+		kfree(n);
+	}
 	return -ENOENT;
 }
 
-- 
2.11.0

Re: [PATCH linux] net: fix deadlock while clearing neighbor proxy table

[David Miller]

From: Wolfgang Bumiller <w.bumiller@proxmox.com>
Date: Tue, 10 Apr 2018 11:15:14 +0200

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 7b7a14abba28..601df647588c 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -292,7 +292,6 @@ int neigh_ifdown(struct neigh_table *tbl, struct net_device *dev)
>  	write_lock_bh(&tbl->lock);
>  	neigh_flush_dev(tbl, dev);
>  	pneigh_ifdown(tbl, dev);
> -	write_unlock_bh(&tbl->lock);

If we are going to fix it this way, we need to annotate the code here in some
way so that future readers understand why the tbl->lock is not being released
here.

One way is to add a comment.

Another way is to rename pneigh_ifdown() to "pneigh_ifdown_and_unlock()".

Re: [PATCH linux] net: fix deadlock while clearing neighbor proxy table

[Wolfgang Bumiller]

David Miller wrote:
> From: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Date: Tue, 10 Apr 2018 11:15:14 +0200
> 
> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> > index 7b7a14abba28..601df647588c 100644
> > --- a/net/core/neighbour.c
> > +++ b/net/core/neighbour.c
> > @@ -292,7 +292,6 @@ int neigh_ifdown(struct neigh_table *tbl, struct net_device *dev)
> >  	write_lock_bh(&tbl->lock);
> >  	neigh_flush_dev(tbl, dev);
> >  	pneigh_ifdown(tbl, dev);
> > -	write_unlock_bh(&tbl->lock);
> 
> If we are going to fix it this way, we need to annotate the code here in some
> way so that future readers understand why the tbl->lock is not being released
> here.

A better way would of course be nice, too, but I find it hard to find
one given how "far away" the IGMP and then output code are from this
point.

> One way is to add a comment.
> 
> Another way is to rename pneigh_ifdown() to "pneigh_ifdown_and_unlock()".

Sure, I can send a v2 with whichever is preferred - personally I prefer
the rename as it'll be visible at both the calling & implementation
side.

Re: [PATCH linux] net: fix deadlock while clearing neighbor proxy table

[David Miller]

From: Wolfgang Bumiller <w.bumiller@proxmox.com>
Date: Wed, 11 Apr 2018 14:17:01 +0200

> David Miller wrote:
>> Another way is to rename pneigh_ifdown() to "pneigh_ifdown_and_unlock()".
> 
> Sure, I can send a v2 with whichever is preferred - personally I prefer
> the rename as it'll be visible at both the calling & implementation
> side.

Yeah the rename is probably best.