| Message ID | 1362581203-8994-1-git-send-email-vfalico@redhat.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2013-03-06 at 15:46 +0100, Veaceslav Falico wrote: > Commit 3335f0ca130c201f8680e97f63612053fbc16e22 removed spinlock unlocking > before __netpoll_cleanup() in netconsole_netdev_event(), however we still > might sleep in __netpoll_cleanup() - via synchronize_srcu(). Revert it and > add a comment. > synchronize_srcu() was actually introduced by Neil: commit ca99ca14c95ae49fb4c9cd3abf5f84d11a7e8a61 Author: Neil Horman <nhorman@tuxdriver.com> Date: Tue Feb 5 08:05:43 2013 +0000 netpoll: protect napi_poll and poll_controller during dev_[open| close] Cc'ing him. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 06, 2013 at 03:46:43PM +0100, Veaceslav Falico wrote: > Commit 3335f0ca130c201f8680e97f63612053fbc16e22 removed spinlock unlocking > before __netpoll_cleanup() in netconsole_netdev_event(), however we still > might sleep in __netpoll_cleanup() - via synchronize_srcu(). Revert it and > add a comment. > > Signed-off-by: Veaceslav Falico <vfalico@redhat.com> > --- > drivers/net/netconsole.c | 10 ++++++++++ > 1 files changed, 10 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c > index 37add21..dd62b4c 100644 > --- a/drivers/net/netconsole.c > +++ b/drivers/net/netconsole.c > @@ -680,7 +680,17 @@ static int netconsole_netdev_event(struct notifier_block *this, > * rtnl_lock already held > */ > if (nt->np.dev) { > + /* > + * we still might sleep in > + * __netpoll_cleanup(), so release > + * the lock > + */ > + spin_unlock_irqrestore( > + &target_list_lock, > + flags); > __netpoll_cleanup(&nt->np); > + spin_lock_irqsave(&target_list_lock, > + flags); > dev_put(nt->np.dev); > nt->np.dev = NULL; > } > -- > 1.7.1 > Thanks for noticing this Vaeceslav, but you can't just drop and re-acquire the lock like this, as it protect the list_for_each_entry loop that you're in. You can drop the lock in the above if clause, but then, after the nt->np.dev = NULL, go back an re-aquire the lock, and start the for loop. I thought we had already done this for some other purpose in this code using a label and a goto, but I suppose I was mistaken Best Neil > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 37add21..dd62b4c 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -680,7 +680,17 @@ static int netconsole_netdev_event(struct notifier_block *this, * rtnl_lock already held */ if (nt->np.dev) { + /* + * we still might sleep in + * __netpoll_cleanup(), so release + * the lock + */ + spin_unlock_irqrestore( + &target_list_lock, + flags); __netpoll_cleanup(&nt->np); + spin_lock_irqsave(&target_list_lock, + flags); dev_put(nt->np.dev); nt->np.dev = NULL; }
Commit 3335f0ca130c201f8680e97f63612053fbc16e22 removed spinlock unlocking before __netpoll_cleanup() in netconsole_netdev_event(), however we still might sleep in __netpoll_cleanup() - via synchronize_srcu(). Revert it and add a comment. Signed-off-by: Veaceslav Falico <vfalico@redhat.com> --- drivers/net/netconsole.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)