On Tue, 2017-01-31 at 17:03 +0000, Anoob Soman wrote: > > On 30/01/17 19:44, Eric Dumazet wrote: > > 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. > > > > > > > Hi Eric, > > I have ran into some problem trying to enable CONFIG_LOCKDEP. I think > this particular scenario, taking mutex_lock() while holding a spin_lock > debugging, requires CONFIG_DEBUG_ATOMIC_SLEEP to be enabled. > CONFIG_DEBUG_ATOMIC_SLEEP, selects CONFIG_PREEMPT_COUNT and my kernel > doesn't behave well if PREEMPTION is enabled. I am trying to reproduce > this issue in a way that I might be able to use debug_atomic_sleep. > > Meanwhile, I have modified patch fix the race.
So you can definitely have in a .config all these at the same time (LOCKDEP, non PREEMPT, and DEBUG_ATOMIC_SLEEP) $ egrep "DEBUG_ATOMIC_SLEEP|PREEMPT|LOCKDEP" .config CONFIG_LOCKDEP_SUPPORT=y CONFIG_PREEMPT_NOTIFIERS=y CONFIG_PREEMPT_NONE=y # CONFIG_PREEMPT_VOLUNTARY is not set # CONFIG_PREEMPT is not set CONFIG_PREEMPT_COUNT=y CONFIG_LOCKDEP=y # CONFIG_DEBUG_LOCKDEP is not set CONFIG_DEBUG_ATOMIC_SLEEP=y
