On Mon, 2017-01-30 at 19:08 +0000, Anoob Soman wrote: > > On 30/01/17 17:26, Eric Dumazet wrote: > > On Thu, 2016-10-06 at 20:50 -0400, David Miller wrote: > >> From: Anoob Soman <[email protected]> > >> Date: Wed, 5 Oct 2016 15:12:54 +0100 > >> > >>> If a socket has FANOUT sockopt set, a new proto_hook is registered > >>> as part of fanout_add(). When processing a NETDEV_UNREGISTER event in > >>> af_packet, __fanout_unlink is called for all sockets, but prot_hook which > >>> was > >>> registered as part of fanout_add is not removed. Call fanout_release, on a > >>> NETDEV_UNREGISTER, which removes prot_hook and removes fanout from the > >>> fanout_list. > >>> > >>> This fixes BUG_ON(!list_empty(&dev->ptype_specific)) in netdev_run_todo() > >>> > >>> Signed-off-by: Anoob Soman <[email protected]> > >> Applied and queued up for -stable, thanks. > > This commit (6664498280cf "packet: call fanout_release, while > > UNREGISTERING a netdev") > > looks buggy : > > > > We end up calling fanout_release() while holding a spinlock > > ( spin_lock(&po->bind_lock); ) > > > > But fanout_release() grabs a mutex ( mutex_lock(&fanout_mutex) ), and > > this is absolutely not valid while holding a spinlock. > > Yes, that is wrong. > > > > > Anoob, can you cook a fix, I guess you have a way to reproduce the thing > > that wanted a kernel patch ? > > > > (Please build your test kernel with CONFIG_LOCKDEP=y) > > Sure, I am planning to move fanout_release(sk) after > spin_unlock(bind_lock). Something like this. > } > if (msg == NETDEV_UNREGISTER) { > packet_cached_dev_reset(po); > - fanout_release(sk); > po->ifindex = -1; > if (po->prot_hook.dev) > dev_put(po->prot_hook.dev); > po->prot_hook.dev = NULL; > } > spin_unlock(&po->bind_lock); > + if (msg == NETDEV_UNREGISTER) { > + fanout_release(sk); > + } > } > break; > > I will quickly test it out.
It wont be enough. You need to also fix a race if two cpus call fanout_release(sk) at the same time. Thanks.
