[PATCH] netpoll: cleanup sparse warnings

[PATCH] netpoll: cleanup sparse warnings

[Neil Horman]

With my recent commit I introduced two sparse warnings.  Looking closer there
were a few more in the same file, so I fixed them all up.  Basic rcu pointer
dereferencing suff

Signed-offy-by: Neil Horman <nhorman@tuxdriver.com>
CC: fengguang.wu@intel.com
CC: David Miller <davem@davemloft.net>
---
 net/core/netpoll.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index edcd9ad..1c829c0 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -205,7 +205,7 @@ static void netpoll_poll_dev(struct net_device *dev)
 	 * the dev_open/close paths use this to block netpoll activity
 	 * while changing device state
 	 */
-	if (!mutex_trylock(&dev->npinfo->dev_lock))
+	if (!mutex_trylock(&ni->dev_lock))
 		return;
 
 	if (!dev || !netif_running(dev))
@@ -220,7 +220,7 @@ static void netpoll_poll_dev(struct net_device *dev)
 
 	poll_napi(dev);
 
-	mutex_unlock(&dev->npinfo->dev_lock);
+	mutex_unlock(&ni->dev_lock);
 
 	if (dev->flags & IFF_SLAVE) {
 		if (ni) {
@@ -1054,7 +1054,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev, gfp_t gfp)
 				goto free_npinfo;
 		}
 	} else {
-		npinfo = ndev->npinfo;
+		npinfo = rtnl_dereference(ndev->npinfo);
 		atomic_inc(&npinfo->refcnt);
 	}
 
@@ -1234,7 +1234,11 @@ void __netpoll_cleanup(struct netpoll *np)
 	struct netpoll_info *npinfo;
 	unsigned long flags;
 
-	npinfo = np->dev->npinfo;
+	/* rtnl_dereference would be preferable here but
+	 * rcu_cleanup_netpoll path can put us in here safely without
+	 * holding the rtnl, so plain rcu_dereference it is
+	 */
+	npinfo = rcu_dereference(np->dev->npinfo);
 	if (!npinfo)
 		return;
 
-- 
1.7.11.7

Re: [PATCH] netpoll: cleanup sparse warnings

[Eric Dumazet]

On Thu, 2013-02-07 at 09:56 -0500, Neil Horman wrote:
> With my recent commit I introduced two sparse warnings.  Looking closer there
> were a few more in the same file, so I fixed them all up.  Basic rcu pointer
> dereferencing suff

> -	npinfo = np->dev->npinfo;
> +	/* rtnl_dereference would be preferable here but
> +	 * rcu_cleanup_netpoll path can put us in here safely without
> +	 * holding the rtnl, so plain rcu_dereference it is
> +	 */
> +	npinfo = rcu_dereference(np->dev->npinfo);
>  	if (!npinfo)
>  		return;
>  

Are you sure it wont trigger a LOCKDEP complain (CONFIG_PROVE_RCU=y) ?

Re: [PATCH] netpoll: cleanup sparse warnings

[Neil Horman]

On Thu, Feb 07, 2013 at 07:52:56AM -0800, Eric Dumazet wrote:
> On Thu, 2013-02-07 at 09:56 -0500, Neil Horman wrote:
> > With my recent commit I introduced two sparse warnings.  Looking closer there
> > were a few more in the same file, so I fixed them all up.  Basic rcu pointer
> > dereferencing suff
> 
> > -	npinfo = np->dev->npinfo;
> > +	/* rtnl_dereference would be preferable here but
> > +	 * rcu_cleanup_netpoll path can put us in here safely without
> > +	 * holding the rtnl, so plain rcu_dereference it is
> > +	 */
> > +	npinfo = rcu_dereference(np->dev->npinfo);
> >  	if (!npinfo)
> >  		return;
> >  
> 
> Are you sure it wont trigger a LOCKDEP complain (CONFIG_PROVE_RCU=y) ?
> 
Hm, looking at it, you're probably right.  We're not holding the rcu_read_lock,
and I'd forgotten that rcu_dereference implicitly checks that rcu_read_lock is
held.  I guess, since the only paths that we get here on are in a bh rcu
quiescence point or with the rtnl held we should probably make this:

rcu_dereference_protected(np->dev->npinfo, rtnl_locked() || in_interrupt());

Although, thinking about this further somewhat begs the question as to how we
prevent one context from calling __netpoll_cleanup in a path holding rtnl, while
in parallel calling __netpoll_cleanup from the rcu callback.  That might not be
a huge deal as __netpoll_cleanup uses spinlocks to do list modification, and an
atomic_dec_and_test to gate the free, but it still seems ugly.

What do you think?
> 
> 

Re: [PATCH] netpoll: cleanup sparse warnings

[Eric Dumazet]

On Thu, 2013-02-07 at 13:37 -0500, Neil Horman wrote:
> On Thu, Feb 07, 2013 at 07:52:56AM -0800, Eric Dumazet wrote:
> > On Thu, 2013-02-07 at 09:56 -0500, Neil Horman wrote:
> > > With my recent commit I introduced two sparse warnings.  Looking closer there
> > > were a few more in the same file, so I fixed them all up.  Basic rcu pointer
> > > dereferencing suff
> > 
> > > -	npinfo = np->dev->npinfo;
> > > +	/* rtnl_dereference would be preferable here but
> > > +	 * rcu_cleanup_netpoll path can put us in here safely without
> > > +	 * holding the rtnl, so plain rcu_dereference it is
> > > +	 */
> > > +	npinfo = rcu_dereference(np->dev->npinfo);
> > >  	if (!npinfo)
> > >  		return;
> > >  
> > 
> > Are you sure it wont trigger a LOCKDEP complain (CONFIG_PROVE_RCU=y) ?
> > 
> Hm, looking at it, you're probably right.  We're not holding the rcu_read_lock,
> and I'd forgotten that rcu_dereference implicitly checks that rcu_read_lock is
> held.  I guess, since the only paths that we get here on are in a bh rcu
> quiescence point or with the rtnl held we should probably make this:
> 
> rcu_dereference_protected(np->dev->npinfo, rtnl_locked() || in_interrupt());
> 
> Although, thinking about this further somewhat begs the question as to how we
> prevent one context from calling __netpoll_cleanup in a path holding rtnl, while
> in parallel calling __netpoll_cleanup from the rcu callback.  That might not be
> a huge deal as __netpoll_cleanup uses spinlocks to do list modification, and an
> atomic_dec_and_test to gate the free, but it still seems ugly.
> 
> What do you think?

I think you could use 

 rcu_dereference_check(p, lockdep_rtnl_is_held() || something)

Re: [PATCH] netpoll: cleanup sparse warnings

[Neil Horman]

On Thu, Feb 07, 2013 at 07:25:33PM -0800, Eric Dumazet wrote:
> On Thu, 2013-02-07 at 13:37 -0500, Neil Horman wrote:
> > On Thu, Feb 07, 2013 at 07:52:56AM -0800, Eric Dumazet wrote:
> > > On Thu, 2013-02-07 at 09:56 -0500, Neil Horman wrote:
> > > > With my recent commit I introduced two sparse warnings.  Looking closer there
> > > > were a few more in the same file, so I fixed them all up.  Basic rcu pointer
> > > > dereferencing suff
> > > 
> > > > -	npinfo = np->dev->npinfo;
> > > > +	/* rtnl_dereference would be preferable here but
> > > > +	 * rcu_cleanup_netpoll path can put us in here safely without
> > > > +	 * holding the rtnl, so plain rcu_dereference it is
> > > > +	 */
> > > > +	npinfo = rcu_dereference(np->dev->npinfo);
> > > >  	if (!npinfo)
> > > >  		return;
> > > >  
> > > 
> > > Are you sure it wont trigger a LOCKDEP complain (CONFIG_PROVE_RCU=y) ?
> > > 
> > Hm, looking at it, you're probably right.  We're not holding the rcu_read_lock,
> > and I'd forgotten that rcu_dereference implicitly checks that rcu_read_lock is
> > held.  I guess, since the only paths that we get here on are in a bh rcu
> > quiescence point or with the rtnl held we should probably make this:
> > 
> > rcu_dereference_protected(np->dev->npinfo, rtnl_locked() || in_interrupt());
> > 
> > Although, thinking about this further somewhat begs the question as to how we
> > prevent one context from calling __netpoll_cleanup in a path holding rtnl, while
> > in parallel calling __netpoll_cleanup from the rcu callback.  That might not be
> > a huge deal as __netpoll_cleanup uses spinlocks to do list modification, and an
> > atomic_dec_and_test to gate the free, but it still seems ugly.
> > 
> > What do you think?
> 
> I think you could use 
> 
>  rcu_dereference_check(p, lockdep_rtnl_is_held() || something)
> 
Actually, I think all we need to do is take the rcu_read_lock and use
rcu_dereference directly.  __netpoll_cleanup doesn't actually change the
dev->npinfo pointer itself, thats handled by the caller, always under protection
of the rtnl, so here we just need to ensure that no one changes npifo while
we're using it
Neil

> 
> 

Re: [PATCH] netpoll: cleanup sparse warnings

[Eric Dumazet]

On Fri, 2013-02-08 at 10:47 -0500, Neil Horman wrote:

> Actually, I think all we need to do is take the rcu_read_lock and use
> rcu_dereference directly.  __netpoll_cleanup doesn't actually change the
> dev->npinfo pointer itself, thats handled by the caller, always under protection
> of the rtnl, so here we just need to ensure that no one changes npifo while
> we're using it

SGTM ;)

Re: [PATCH] netpoll: cleanup sparse warnings

[Neil Horman]

On Fri, Feb 08, 2013 at 08:44:43AM -0800, Eric Dumazet wrote:
> On Fri, 2013-02-08 at 10:47 -0500, Neil Horman wrote:
> 
> > Actually, I think all we need to do is take the rcu_read_lock and use
> > rcu_dereference directly.  __netpoll_cleanup doesn't actually change the
> > dev->npinfo pointer itself, thats handled by the caller, always under protection
> > of the rtnl, so here we just need to ensure that no one changes npifo while
> > we're using it
> 
> SGTM ;)
> 
Testing it out now.  The more I look at this the more I'm concerned that there
are other problems with netpoll in the release path.  Shocking...I know :)
Neil

> 
> 

[PATCH v2 0/2] netpoll: Cleanup netpoll locking and sparse warnings

[Neil Horman]

I was fixing some sparse warnings in netpoll from a previous commit, and it was
pointed out to me that one of the rcu_dereferences that I fixed up in 
__netpoll_cleanup might not have been correct.  On investigation I found that 
rtnl_dereference should have been the correct macro to use, but we had a path in
which we weren't properly holding the rtnl lock.  This patch series fixes up the
locking in the __netpoll_free_rcu path, and then properly corrects the sparse 
warnings in the netpoll.c file that I previously introduced.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: David Miller <davem@davemloft.net>
CC: fengguang.wu@intel.com
CC: eric.dumazet@gmail.com
CC: Cong Wang <amwang@redhat.com>

[PATCH v2 1/2] netpoll: Fix __netpoll_rcu_free so that it can hold the rtnl lock

[Neil Horman]

__netpoll_rcu_free is used to free netpoll structures when the rtnl_lock is
already held.  The mechanism is used to asynchronously call __netpoll_cleanup
outside of the holding of the rtnl_lock, so as to avoid deadlock.
Unfortunately, __netpoll_cleanup modifies pointers (dev->np), which means the
rtnl_lock must be held while calling it.  Further, it cannot be held, because
rcu callbacks may be issued in softirq contexts, which cannot sleep.

Fix this by converting the rcu callback to a work queue that is guaranteed to
get scheduled in process context, so that we can hold the rtnl properly while
calling __netpoll_cleanup

Tested successfully by myself.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: Cong Wang <amwang@redhat.com>
CC: Eric Dumazet <eric.dumazet@gmail.com>
---
 drivers/net/bonding/bond_main.c |  2 +-
 include/linux/netpoll.h         |  4 ++--
 net/8021q/vlan_dev.c            |  2 +-
 net/bridge/br_device.c          |  2 +-
 net/core/netpoll.c              | 16 ++++++++++------
 5 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 2239937..94c1534 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1249,7 +1249,7 @@ static inline void slave_disable_netpoll(struct slave *slave)
 		return;
 
 	slave->np = NULL;
-	__netpoll_free_rcu(np);
+	__netpoll_free_async(np);
 }
 static inline bool slave_dev_support_netpoll(struct net_device *slave_dev)
 {
diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index ab856d5..9d7d8c6 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -32,7 +32,7 @@ struct netpoll {
 	u8 remote_mac[ETH_ALEN];
 
 	struct list_head rx; /* rx_np list element */
-	struct rcu_head rcu;
+	struct work_struct cleanup_work;
 };
 
 struct netpoll_info {
@@ -68,7 +68,7 @@ int netpoll_setup(struct netpoll *np);
 int netpoll_trap(void);
 void netpoll_set_trap(int trap);
 void __netpoll_cleanup(struct netpoll *np);
-void __netpoll_free_rcu(struct netpoll *np);
+void __netpoll_free_async(struct netpoll *np);
 void netpoll_cleanup(struct netpoll *np);
 int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo);
 void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 09f9108..b29fba0 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -723,7 +723,7 @@ static void vlan_dev_netpoll_cleanup(struct net_device *dev)
 
 	vlan->netpoll = NULL;
 
-	__netpoll_free_rcu(netpoll);
+	__netpoll_free_async(netpoll);
 }
 #endif /* CONFIG_NET_POLL_CONTROLLER */
 
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index e1bc090..bd03084 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -266,7 +266,7 @@ void br_netpoll_disable(struct net_bridge_port *p)
 
 	p->np = NULL;
 
-	__netpoll_free_rcu(np);
+	__netpoll_free_async(np);
 }
 
 #endif
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index edcd9ad..c536474 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -61,6 +61,7 @@ static struct srcu_struct netpoll_srcu;
 
 static void zap_completion_queue(void);
 static void netpoll_neigh_reply(struct sk_buff *skb, struct netpoll_info *npinfo);
+static void netpoll_async_cleanup(struct work_struct *work);
 
 static unsigned int carrier_timeout = 4;
 module_param(carrier_timeout, uint, 0644);
@@ -1020,6 +1021,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev, gfp_t gfp)
 
 	np->dev = ndev;
 	strlcpy(np->dev_name, ndev->name, IFNAMSIZ);
+	INIT_WORK(&np->cleanup_work, netpoll_async_cleanup);
 
 	if ((ndev->priv_flags & IFF_DISABLE_NETPOLL) ||
 	    !ndev->netdev_ops->ndo_poll_controller) {
@@ -1255,25 +1257,27 @@ void __netpoll_cleanup(struct netpoll *np)
 		if (ops->ndo_netpoll_cleanup)
 			ops->ndo_netpoll_cleanup(np->dev);
 
-		RCU_INIT_POINTER(np->dev->npinfo, NULL);
+		rcu_assign_pointer(np->dev->npinfo, NULL);
 		call_rcu_bh(&npinfo->rcu, rcu_cleanup_netpoll_info);
 	}
 }
 EXPORT_SYMBOL_GPL(__netpoll_cleanup);
 
-static void rcu_cleanup_netpoll(struct rcu_head *rcu_head)
+static void netpoll_async_cleanup(struct work_struct *work)
 {
-	struct netpoll *np = container_of(rcu_head, struct netpoll, rcu);
+	struct netpoll *np = container_of(work, struct netpoll, cleanup_work);
 
+	rtnl_lock();
 	__netpoll_cleanup(np);
+	rtnl_unlock();
 	kfree(np);
 }
 
-void __netpoll_free_rcu(struct netpoll *np)
+void __netpoll_free_async(struct netpoll *np)
 {
-	call_rcu_bh(&np->rcu, rcu_cleanup_netpoll);
+	schedule_work(&np->cleanup_work);
 }
-EXPORT_SYMBOL_GPL(__netpoll_free_rcu);
+EXPORT_SYMBOL_GPL(__netpoll_free_async);
 
 void netpoll_cleanup(struct netpoll *np)
 {
-- 
1.7.11.7

[PATCH v2 2/2] netpoll: cleanup sparse warnings

[Neil Horman]

With my recent commit I introduced two sparse warnings.  Looking closer there
were a few more in the same file, so I fixed them all up.  Basic rcu pointer
dereferencing suff.

I've validated these changes using CONFIG_PROVE_RCU while starting and stopping
netconsole repeatedly in bonded and non-bonded configurations

Signed-offy-by: Neil Horman <nhorman@tuxdriver.com>
CC: fengguang.wu@intel.com
CC: David Miller <davem@davemloft.net>
CC: eric.dumazet@gmail.com
---
 net/core/netpoll.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index c536474..bcfd4f4 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -206,7 +206,7 @@ static void netpoll_poll_dev(struct net_device *dev)
 	 * the dev_open/close paths use this to block netpoll activity
 	 * while changing device state
 	 */
-	if (!mutex_trylock(&dev->npinfo->dev_lock))
+	if (!mutex_trylock(&ni->dev_lock))
 		return;
 
 	if (!dev || !netif_running(dev))
@@ -221,7 +221,7 @@ static void netpoll_poll_dev(struct net_device *dev)
 
 	poll_napi(dev);
 
-	mutex_unlock(&dev->npinfo->dev_lock);
+	mutex_unlock(&ni->dev_lock);
 
 	if (dev->flags & IFF_SLAVE) {
 		if (ni) {
@@ -1056,7 +1056,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev, gfp_t gfp)
 				goto free_npinfo;
 		}
 	} else {
-		npinfo = ndev->npinfo;
+		npinfo = rtnl_dereference(ndev->npinfo);
 		atomic_inc(&npinfo->refcnt);
 	}
 
@@ -1236,7 +1236,11 @@ void __netpoll_cleanup(struct netpoll *np)
 	struct netpoll_info *npinfo;
 	unsigned long flags;
 
-	npinfo = np->dev->npinfo;
+	/* rtnl_dereference would be preferable here but
+	 * rcu_cleanup_netpoll path can put us in here safely without
+	 * holding the rtnl, so plain rcu_dereference it is
+	 */
+	npinfo = rtnl_dereference(np->dev->npinfo);
 	if (!npinfo)
 		return;
 
-- 
1.7.11.7

Re: [PATCH v2 0/2] netpoll: Cleanup netpoll locking and sparse warnings

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Mon, 11 Feb 2013 15:25:29 -0500

> I was fixing some sparse warnings in netpoll from a previous commit, and it was
> pointed out to me that one of the rcu_dereferences that I fixed up in 
> __netpoll_cleanup might not have been correct.  On investigation I found that 
> rtnl_dereference should have been the correct macro to use, but we had a path in
> which we weren't properly holding the rtnl lock.  This patch series fixes up the
> locking in the __netpoll_free_rcu path, and then properly corrects the sparse 
> warnings in the netpoll.c file that I previously introduced.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

I happen to know that this is meant for net-next, but you really need
to state that clearly in your patch submissions.

Re: [PATCH v2 0/2] netpoll: Cleanup netpoll locking and sparse warnings

[Neil Horman]

On Mon, Feb 11, 2013 at 04:11:35PM -0500, David Miller wrote:
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Mon, 11 Feb 2013 15:25:29 -0500
> 
> > I was fixing some sparse warnings in netpoll from a previous commit, and it was
> > pointed out to me that one of the rcu_dereferences that I fixed up in 
> > __netpoll_cleanup might not have been correct.  On investigation I found that 
> > rtnl_dereference should have been the correct macro to use, but we had a path in
> > which we weren't properly holding the rtnl lock.  This patch series fixes up the
> > locking in the __netpoll_free_rcu path, and then properly corrects the sparse 
> > warnings in the netpoll.c file that I previously introduced.
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> I happen to know that this is meant for net-next, but you really need
> to state that clearly in your patch submissions.
> 
Understood, sorry Dave.
Neil

Re: [PATCH v2 1/2] netpoll: Fix __netpoll_rcu_free so that it can hold the rtnl lock

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Mon, 11 Feb 2013 15:25:30 -0500

> __netpoll_rcu_free is used to free netpoll structures when the rtnl_lock is
> already held.  The mechanism is used to asynchronously call __netpoll_cleanup
> outside of the holding of the rtnl_lock, so as to avoid deadlock.
> Unfortunately, __netpoll_cleanup modifies pointers (dev->np), which means the
> rtnl_lock must be held while calling it.  Further, it cannot be held, because
> rcu callbacks may be issued in softirq contexts, which cannot sleep.
> 
> Fix this by converting the rcu callback to a work queue that is guaranteed to
> get scheduled in process context, so that we can hold the rtnl properly while
> calling __netpoll_cleanup
> 
> Tested successfully by myself.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

Applied.

Re: [PATCH v2 2/2] netpoll: cleanup sparse warnings

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Mon, 11 Feb 2013 15:25:31 -0500

> With my recent commit I introduced two sparse warnings.  Looking closer there
> were a few more in the same file, so I fixed them all up.  Basic rcu pointer
> dereferencing suff.
> 
> I've validated these changes using CONFIG_PROVE_RCU while starting and stopping
> netconsole repeatedly in bonded and non-bonded configurations
> 
> Signed-offy-by: Neil Horman <nhorman@tuxdriver.com>

Applied.