| Message ID | 20180412084655.12607-1-w.bumiller@proxmox.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v2,net] net: fix deadlock while clearing neighbor proxy table | expand |
From: Wolfgang Bumiller <w.bumiller@proxmox.com> Date: Thu, 12 Apr 2018 10:46:55 +0200 > 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> > --- > Changes to v1: > * Renamed 'pneigh_ifdown' to 'pneigh_ifdown_and_unlock'. Applied and queued up for -stable. > Btw. I'm not actually sure how much sense the Fixes tag makes as the > commit itself isn't wrong, it just happens to be the most easily > triggerable code path there (and I can't definitively rule out others, > given that the "sending something over the network with the lock held > will deadlock" comment at the top of the file is also from the initial > commit I'd expect other backtraces to be possible from out of that > function) and the other affected lines are mostly the initial git > commit... Understood.
diff --git a/net/core/neighbour.c b/net/core/neighbour.c index 7b7a14abba28..e22d2aefbd78 100644 --- a/net/core/neighbour.c +++ b/net/core/neighbour.c @@ -55,7 +55,8 @@ static void neigh_timer_handler(struct timer_list *t); static void __neigh_notify(struct neighbour *n, int type, int flags, u32 pid); static void neigh_update_notify(struct neighbour *neigh, u32 nlmsg_pid); -static int pneigh_ifdown(struct neigh_table *tbl, struct net_device *dev); +static int pneigh_ifdown_and_unlock(struct neigh_table *tbl, + struct net_device *dev); #ifdef CONFIG_PROC_FS static const struct file_operations neigh_stat_seq_fops; @@ -291,8 +292,7 @@ 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); + pneigh_ifdown_and_unlock(tbl, dev); del_timer_sync(&tbl->proxy_timer); pneigh_queue_purge(&tbl->proxy_queue); @@ -681,9 +681,10 @@ int pneigh_delete(struct neigh_table *tbl, struct net *net, const void *pkey, return -ENOENT; } -static int pneigh_ifdown(struct neigh_table *tbl, struct net_device *dev) +static int pneigh_ifdown_and_unlock(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 +692,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> --- Changes to v1: * Renamed 'pneigh_ifdown' to 'pneigh_ifdown_and_unlock'. Btw. I'm not actually sure how much sense the Fixes tag makes as the commit itself isn't wrong, it just happens to be the most easily triggerable code path there (and I can't definitively rule out others, given that the "sending something over the network with the lock held will deadlock" comment at the top of the file is also from the initial commit I'd expect other backtraces to be possible from out of that function) and the other affected lines are mostly the initial git commit... net/core/neighbour.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)