| Message ID | 20180410091514.28704-1-w.bumiller@proxmox.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [linux] net: fix deadlock while clearing neighbor proxy table | expand |
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()".
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.
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.
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; }
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(-)