[PATCH] net/mv643xx: don't disable the mib timer too early

[PATCH] net/mv643xx: don't disable the mib timer too early

[Sebastian Andrzej Siewior]

because mib_counters_update() also restarts the timer.
So the timer is dequeued, the stats are read and then the timer is
enqueued again. This is okay, unless someone unloads the module.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 drivers/net/mv643xx_eth.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
index 6977abe..3c6847a 100644
--- a/drivers/net/mv643xx_eth.c
+++ b/drivers/net/mv643xx_eth.c
@@ -2213,8 +2213,6 @@ static int mv643xx_eth_stop(struct net_device *dev)
 	wrlp(mp, INT_MASK, 0x00000000);
 	rdlp(mp, INT_MASK);
 
-	del_timer_sync(&mp->mib_counters_timer);
-
 	napi_disable(&mp->napi);
 
 	del_timer_sync(&mp->rx_oom);
@@ -2226,6 +2224,7 @@ static int mv643xx_eth_stop(struct net_device *dev)
 	port_reset(mp);
 	mv643xx_eth_get_stats(dev);
 	mib_counters_update(mp);
+	del_timer_sync(&mp->mib_counters_timer);
 
 	skb_queue_purge(&mp->rx_recycle);
 
-- 
1.6.0.6

Re: [PATCH] net/mv643xx: don't disable the mib timer too early

[Sebastian Andrzej Siewior]

* Sebastian Andrzej Siewior | 2009-02-15 10:27:19 [+0100]:

>because mib_counters_update() also restarts the timer.
>So the timer is dequeued, the stats are read and then the timer is
>enqueued again. This is okay, unless someone unloads the module.
I got to NAK that one myself. I make a new one which does not break sth.
else .)

Sebastian

[PATCH v2] net/mv643xx: don't disable the mib timer too early and lock properly

[Sebastian Andrzej Siewior]

mib_counters_update() also restarts the timer.
So the timer is dequeued, the stats are read and then the timer is
enqueued again. This is "okay" unless someone unloads the module.
The locking here is also broken:
mib_counters_update() grabs just a simple spinlock. The only thing the
lock is good for is to protect the timer func against other callers
namely mv643xx_eth_stop() && mv643xx_eth_get_ethtool_stats(). That means
if the spinlock is taken via the ethtool path and than the timer kicks
in then the box will lock up.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 drivers/net/mv643xx_eth.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
index 6977abe..582b346 100644
--- a/drivers/net/mv643xx_eth.c
+++ b/drivers/net/mv643xx_eth.c
@@ -1173,7 +1173,7 @@ static void mib_counters_update(struct mv643xx_eth_private *mp)
 {
 	struct mib_counters *p = &mp->mib_counters;
 
-	spin_lock(&mp->mib_counters_lock);
+	spin_lock_bh(&mp->mib_counters_lock);
 	p->good_octets_received += mib_read(mp, 0x00);
 	p->good_octets_received += (u64)mib_read(mp, 0x04) << 32;
 	p->bad_octets_received += mib_read(mp, 0x08);
@@ -1206,7 +1206,7 @@ static void mib_counters_update(struct mv643xx_eth_private *mp)
 	p->bad_crc_event += mib_read(mp, 0x74);
 	p->collision += mib_read(mp, 0x78);
 	p->late_collision += mib_read(mp, 0x7c);
-	spin_unlock(&mp->mib_counters_lock);
+	spin_unlock_bh(&mp->mib_counters_lock);
 
 	mod_timer(&mp->mib_counters_timer, jiffies + 30 * HZ);
 }
@@ -2213,8 +2213,6 @@ static int mv643xx_eth_stop(struct net_device *dev)
 	wrlp(mp, INT_MASK, 0x00000000);
 	rdlp(mp, INT_MASK);
 
-	del_timer_sync(&mp->mib_counters_timer);
-
 	napi_disable(&mp->napi);
 
 	del_timer_sync(&mp->rx_oom);
@@ -2226,6 +2224,7 @@ static int mv643xx_eth_stop(struct net_device *dev)
 	port_reset(mp);
 	mv643xx_eth_get_stats(dev);
 	mib_counters_update(mp);
+	del_timer_sync(&mp->mib_counters_timer);
 
 	skb_queue_purge(&mp->rx_recycle);
 
-- 
1.6.0.6

Re: [PATCH v2] net/mv643xx: don't disable the mib timer too early and lock properly

[Lennert Buytenhek]

On Mon, Feb 16, 2009 at 10:28:15PM +0100, Sebastian Andrzej Siewior wrote:

> mib_counters_update() also restarts the timer.
> So the timer is dequeued, the stats are read and then the timer is
> enqueued again. This is "okay" unless someone unloads the module.
> The locking here is also broken:
> mib_counters_update() grabs just a simple spinlock. The only thing the
> lock is good for is to protect the timer func against other callers
> namely mv643xx_eth_stop() && mv643xx_eth_get_ethtool_stats(). That means
> if the spinlock is taken via the ethtool path and than the timer kicks
> in then the box will lock up.
> 
> Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>

Acked-by: Lennert Buytenhek <buytenh@marvell.com>

Re: [PATCH v2] net/mv643xx: don't disable the mib timer too early and lock properly

[David Miller]

From: Lennert Buytenhek <buytenh@wantstofly.org>
Date: Thu, 19 Feb 2009 02:05:29 +0100

> On Mon, Feb 16, 2009 at 10:28:15PM +0100, Sebastian Andrzej Siewior wrote:
> 
> > mib_counters_update() also restarts the timer.
> > So the timer is dequeued, the stats are read and then the timer is
> > enqueued again. This is "okay" unless someone unloads the module.
> > The locking here is also broken:
> > mib_counters_update() grabs just a simple spinlock. The only thing the
> > lock is good for is to protect the timer func against other callers
> > namely mv643xx_eth_stop() && mv643xx_eth_get_ethtool_stats(). That means
> > if the spinlock is taken via the ethtool path and than the timer kicks
> > in then the box will lock up.
> > 
> > Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> 
> Acked-by: Lennert Buytenhek <buytenh@marvell.com>

Also applied, thanks!