diff mbox

netpoll: cleanup sparse warnings

Message ID 1360248961-30721-1-git-send-email-nhorman@tuxdriver.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Neil Horman Feb. 7, 2013, 2:56 p.m. UTC 
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(-)

Comments

Eric Dumazet Feb. 7, 2013, 3:52 p.m. UTC | #1 
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) ?


--
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 Feb. 7, 2013, 6:37 p.m. UTC | #2 
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?
> 
> 
--
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
Eric Dumazet Feb. 8, 2013, 3:25 a.m. UTC | #3 
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)


--
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 Feb. 8, 2013, 3:47 p.m. UTC | #4 
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

> 
> 
--
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
Eric Dumazet Feb. 8, 2013, 4:44 p.m. UTC | #5 
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 ;)


--
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 Feb. 8, 2013, 6:14 p.m. UTC | #6 
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

> 
> 
--
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 Feb. 11, 2013, 8:25 p.m. UTC | #7 
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>

--
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
David Miller Feb. 11, 2013, 9:11 p.m. UTC | #8 
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.
--
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 Feb. 11, 2013, 9:51 p.m. UTC | #9 
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

--
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 mbox

Patch

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;