Patchwork netconsole: release the spinlock before __netpoll_cleanup()

login
register
mail settings
Submitter Veaceslav Falico
Date March 6, 2013, 2:46 p.m.
Message ID <1362581203-8994-1-git-send-email-vfalico@redhat.com>
Download mbox | patch
Permalink /patch/225511/
State Superseded
Delegated to: David Miller
Headers show

Comments

Veaceslav Falico - March 6, 2013, 2:46 p.m.
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(-)
Amerigo Wang - March 6, 2013, 2:51 p.m.
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
Neil Horman - March 7, 2013, 12:08 a.m.
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

Patch

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;
 				}