[PATCH] e1000: do not allow watchdog to reenable transmits on shutdown

[PATCH] e1000: do not allow watchdog to reenable transmits on shutdown

[Marcelo Tosatti]

There is a race on the shutdown path of the e1000 driver
that allows the card to DMA into free'd memory.

The symptoms are similar to those described at 
commit d5bc77a223b0e9b9dfb002048d2b34a79e7d0b48,
"e1000: don't enable dma receives until after dma address has been
setup", where memory corruption is visible due to E1000_RXD_STAT_DD
being written to the DMA transfer descriptor.

Fix by not allowing the watchdog and tx fifo stall detector
to re-enable E1000_TCTL_EN.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/drivers/net/ethernet/intel/e1000/e1000.h b/drivers/net/ethernet/intel/e1000/e1000.h
index 10a0f22..bb5dc1a 100644
--- a/drivers/net/ethernet/intel/e1000/e1000.h
+++ b/drivers/net/ethernet/intel/e1000/e1000.h
@@ -321,7 +321,8 @@ struct e1000_adapter {
 enum e1000_state_t {
 	__E1000_TESTING,
 	__E1000_RESETTING,
-	__E1000_DOWN
+	__E1000_DOWN,
+	__E1000_NOTX = 4,
 };
 
 #undef pr_fmt
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 46e6544..b20ce98 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -414,6 +414,7 @@ int e1000_up(struct e1000_adapter *adapter)
 	e1000_configure(adapter);
 
 	clear_bit(__E1000_DOWN, &adapter->flags);
+	clear_bit(__E1000_NOTX, &adapter->flags);
 
 	napi_enable(&adapter->napi);
 
@@ -524,6 +525,10 @@ void e1000_down(struct e1000_adapter *adapter)
 
 	netif_tx_disable(netdev);
 
+	/* do not allow watchdog to reenable transmits between 
+           clearing E1000_TCTL_EN below and setting E1000_DOWN */
+	set_bit(__E1000_NOTX, &adapter->flags);
+
 	/* disable transmits in the hardware */
 	tctl = er32(TCTL);
 	tctl &= ~E1000_TCTL_EN;
@@ -2339,6 +2344,9 @@ static void e1000_82547_tx_fifo_stall_task(struct work_struct *work)
 	struct net_device *netdev = adapter->netdev;
 	u32 tctl;
 
+	if (test_bit(__E1000_NOTX, &adapter->flags))
+		return;
+
 	if (atomic_read(&adapter->tx_fifo_stall)) {
 		if ((er32(TDT) == er32(TDH)) &&
 		   (er32(TDFT) == er32(TDFH)) &&
@@ -2412,6 +2420,9 @@ static void e1000_watchdog(struct work_struct *work)
 	struct e1000_tx_ring *txdr = adapter->tx_ring;
 	u32 link, tctl;
 
+	if (test_bit(__E1000_NOTX, &adapter->flags))
+		return;
+
 	link = e1000_has_link(adapter);
 	if ((netif_carrier_ok(netdev)) && link)
 		goto link_up;




------------------------------------------------------------------------------
Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce.
With Perforce, you get hassle-free workflows. Merge that actually works. 
Faster operations. Version large binaries.  Built-in WAN optimization and the
freedom to use Git, Perforce or both. Make the move to Perforce.
http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH] e1000: do not allow watchdog to reenable transmits on shutdown

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 839 bytes --]

On Thu, 2014-03-06 at 23:04 -0300, Marcelo Tosatti wrote:
> 
> There is a race on the shutdown path of the e1000 driver
> that allows the card to DMA into free'd memory.
> 
> The symptoms are similar to those described at 
> commit d5bc77a223b0e9b9dfb002048d2b34a79e7d0b48,
> "e1000: don't enable dma receives until after dma address has been
> setup", where memory corruption is visible due to E1000_RXD_STAT_DD
> being written to the DMA transfer descriptor.
> 
> Fix by not allowing the watchdog and tx fifo stall detector
> to re-enable E1000_TCTL_EN.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Thanks I will add this to the queue.

Note- there are trailing whitespace issues and 2 comments are not
formatted correctly.  I can fix these up if there are no other issues
with the patch, if you want.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] e1000: do not allow watchdog to reenable transmits on shutdown

[Marcelo Tosatti]

On Thu, Mar 06, 2014 at 07:24:31PM -0800, Jeff Kirsher wrote:
> On Thu, 2014-03-06 at 23:04 -0300, Marcelo Tosatti wrote:
> > 
> > There is a race on the shutdown path of the e1000 driver
> > that allows the card to DMA into free'd memory.
> > 
> > The symptoms are similar to those described at 
> > commit d5bc77a223b0e9b9dfb002048d2b34a79e7d0b48,
> > "e1000: don't enable dma receives until after dma address has been
> > setup", where memory corruption is visible due to E1000_RXD_STAT_DD
> > being written to the DMA transfer descriptor.
> > 
> > Fix by not allowing the watchdog and tx fifo stall detector
> > to re-enable E1000_TCTL_EN.
> > 
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Thanks I will add this to the queue.
> 
> Note- there are trailing whitespace issues and 2 comments are not
> formatted correctly.  I can fix these up if there are no other issues
> with the patch, if you want.

checkpatch.pl did not complain about those - feel free to fix them,
thanks.



------------------------------------------------------------------------------
Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce.
With Perforce, you get hassle-free workflows. Merge that actually works. 
Faster operations. Version large binaries.  Built-in WAN optimization and the
freedom to use Git, Perforce or both. Make the move to Perforce.
http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH] e1000: do not allow watchdog to reenable transmits on shutdown

[David Miller]

From: Marcelo Tosatti <mtosatti@redhat.com>
Date: Fri, 7 Mar 2014 12:14:35 -0300

> checkpatch.pl did not complain about those - feel free to fix them,
> thanks.

The problem is that GIT does when the maintainer applies your
patch.

You can sanity check your patch using GIT without commiting it
into the tree you are using by saying something like:

	git apply --check --whitespace=error-all $1

------------------------------------------------------------------------------
Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce.
With Perforce, you get hassle-free workflows. Merge that actually works. 
Faster operations. Version large binaries.  Built-in WAN optimization and the
freedom to use Git, Perforce or both. Make the move to Perforce.
http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH] e1000: do not allow watchdog to reenable transmits on shutdown

[Joe Perches]

On Fri, 2014-03-07 at 12:14 -0300, Marcelo Tosatti wrote:
> On Thu, Mar 06, 2014 at 07:24:31PM -0800, Jeff Kirsher wrote:
> > On Thu, 2014-03-06 at 23:04 -0300, Marcelo Tosatti wrote:
> > > There is a race on the shutdown path of the e1000 driver
> > > that allows the card to DMA into free'd memory.
[]
> > Note- there are trailing whitespace issues and 2 comments are not
> > formatted correctly.  I can fix these up if there are no other issues
> > with the patch, if you want.
> 
> checkpatch.pl did not complain about those - feel free to fix them,
> thanks.

Maybe you need a newer version of checkpatch?

$ ./scripts/checkpatch.pl e1000-do-not-allow-watchdog-to-reenable-transmits-on-shutdown.patch 
ERROR: trailing whitespace
#63: FILE: drivers/net/ethernet/intel/e1000/e1000_main.c:528:
+^I/* do not allow watchdog to reenable transmits between $

ERROR: code indent should use tabs where possible
#64: FILE: drivers/net/ethernet/intel/e1000/e1000_main.c:529:
+           clearing E1000_TCTL_EN below and setting E1000_DOWN */$

WARNING: networking block comments start with * on subsequent lines
#64: FILE: drivers/net/ethernet/intel/e1000/e1000_main.c:529:
+	/* do not allow watchdog to reenable transmits between 
+           clearing E1000_TCTL_EN below and setting E1000_DOWN */

WARNING: networking block comments put the trailing */ on a separate line
#64: FILE: drivers/net/ethernet/intel/e1000/e1000_main.c:529:
+           clearing E1000_TCTL_EN below and setting E1000_DOWN */

total: 2 errors, 2 warnings, 44 lines checked

NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
      scripts/cleanfile

e1000-do-not-allow-watchdog-to-reenable-transmits-on-shutdown.patch has style problems, please review.

If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.

[PATCH] e1000: do not allow watchdog to reenable transmits on shutdown (v2)

[Marcelo Tosatti]

There is a race on the shutdown path of the e1000 driver
that allows the card to DMA into free'd memory.

The symptoms are similar to those described at
commit d5bc77a223b0e9b9dfb002048d2b34a79e7d0b48,
"e1000: don't enable dma receives until after dma address has been
setup", where memory corruption is visible due to E1000_RXD_STAT_DD
being written to the DMA transfer descriptor.

Fix by not allowing the watchdog and tx fifo stall detector
to re-enable E1000_TCTL_EN.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/drivers/net/ethernet/intel/e1000/e1000.h b/drivers/net/ethernet/intel/e1000/e1000.h
index 10a0f22..bb5dc1a 100644
--- a/drivers/net/ethernet/intel/e1000/e1000.h
+++ b/drivers/net/ethernet/intel/e1000/e1000.h
@@ -321,7 +321,8 @@ struct e1000_adapter {
 enum e1000_state_t {
 	__E1000_TESTING,
 	__E1000_RESETTING,
-	__E1000_DOWN
+	__E1000_DOWN,
+	__E1000_NOTX = 4,
 };
 
 #undef pr_fmt
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 46e6544..a39484f 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -414,6 +414,7 @@ int e1000_up(struct e1000_adapter *adapter)
 	e1000_configure(adapter);
 
 	clear_bit(__E1000_DOWN, &adapter->flags);
+	clear_bit(__E1000_NOTX, &adapter->flags);
 
 	napi_enable(&adapter->napi);
 
@@ -524,6 +525,11 @@ void e1000_down(struct e1000_adapter *adapter)
 
 	netif_tx_disable(netdev);
 
+	/* do not allow watchdog to reenable transmits between
+	 * clearing E1000_TCTL_EN below and setting E1000_DOWN
+	 */
+	set_bit(__E1000_NOTX, &adapter->flags);
+
 	/* disable transmits in the hardware */
 	tctl = er32(TCTL);
 	tctl &= ~E1000_TCTL_EN;
@@ -2339,6 +2345,9 @@ static void e1000_82547_tx_fifo_stall_task(struct work_struct *work)
 	struct net_device *netdev = adapter->netdev;
 	u32 tctl;
 
+	if (test_bit(__E1000_NOTX, &adapter->flags))
+		return;
+
 	if (atomic_read(&adapter->tx_fifo_stall)) {
 		if ((er32(TDT) == er32(TDH)) &&
 		   (er32(TDFT) == er32(TDFH)) &&
@@ -2412,6 +2421,9 @@ static void e1000_watchdog(struct work_struct *work)
 	struct e1000_tx_ring *txdr = adapter->tx_ring;
 	u32 link, tctl;
 
+	if (test_bit(__E1000_NOTX, &adapter->flags))
+		return;
+
 	link = e1000_has_link(adapter);
 	if ((netif_carrier_ok(netdev)) && link)
 		goto link_up;

------------------------------------------------------------------------------
Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce.
With Perforce, you get hassle-free workflows. Merge that actually works. 
Faster operations. Version large binaries.  Built-in WAN optimization and the
freedom to use Git, Perforce or both. Make the move to Perforce.
http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH] e1000: do not allow watchdog to reenable transmits on shutdown (v2)

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 681 bytes --]

On Fri, 2014-03-07 at 17:59 -0300, Marcelo Tosatti wrote:
> There is a race on the shutdown path of the e1000 driver
> that allows the card to DMA into free'd memory.
> 
> The symptoms are similar to those described at
> commit d5bc77a223b0e9b9dfb002048d2b34a79e7d0b48,
> "e1000: don't enable dma receives until after dma address has been
> setup", where memory corruption is visible due to E1000_RXD_STAT_DD
> being written to the DMA transfer descriptor.
> 
> Fix by not allowing the watchdog and tx fifo stall detector
> to re-enable E1000_TCTL_EN.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Thanks for the updated patch, I will update my queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] e1000: do not allow watchdog to reenable transmits on shutdown

[Marcelo Tosatti]

On Thu, Mar 06, 2014 at 07:24:31PM -0800, Jeff Kirsher wrote:
> On Thu, 2014-03-06 at 23:04 -0300, Marcelo Tosatti wrote:
> > 
> > There is a race on the shutdown path of the e1000 driver
> > that allows the card to DMA into free'd memory.
> > 
> > The symptoms are similar to those described at 
> > commit d5bc77a223b0e9b9dfb002048d2b34a79e7d0b48,
> > "e1000: don't enable dma receives until after dma address has been
> > setup", where memory corruption is visible due to E1000_RXD_STAT_DD
> > being written to the DMA transfer descriptor.
> > 
> > Fix by not allowing the watchdog and tx fifo stall detector
> > to re-enable E1000_TCTL_EN.
> > 
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Thanks I will add this to the queue.
> 
> Note- there are trailing whitespace issues and 2 comments are not
> formatted correctly.  I can fix these up if there are no other issues
> with the patch, if you want.

Ok, resent v2 with those fixed, must have inlined an older patch
in the message, sorry.


------------------------------------------------------------------------------
Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce.
With Perforce, you get hassle-free workflows. Merge that actually works. 
Faster operations. Version large binaries.  Built-in WAN optimization and the
freedom to use Git, Perforce or both. Make the move to Perforce.
http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH] e1000: do not allow watchdog to reenable transmits on shutdown

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1223 bytes --]

On Fri, 2014-03-07 at 18:00 -0300, Marcelo Tosatti wrote:
> On Thu, Mar 06, 2014 at 07:24:31PM -0800, Jeff Kirsher wrote:
> > On Thu, 2014-03-06 at 23:04 -0300, Marcelo Tosatti wrote:
> > > 
> > > There is a race on the shutdown path of the e1000 driver
> > > that allows the card to DMA into free'd memory.
> > > 
> > > The symptoms are similar to those described at 
> > > commit d5bc77a223b0e9b9dfb002048d2b34a79e7d0b48,
> > > "e1000: don't enable dma receives until after dma address has been
> > > setup", where memory corruption is visible due to E1000_RXD_STAT_DD
> > > being written to the DMA transfer descriptor.
> > > 
> > > Fix by not allowing the watchdog and tx fifo stall detector
> > > to re-enable E1000_TCTL_EN.
> > > 
> > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > Thanks I will add this to the queue.
> > 
> > Note- there are trailing whitespace issues and 2 comments are not
> > formatted correctly.  I can fix these up if there are no other issues
> > with the patch, if you want.
> 
> Ok, resent v2 with those fixed, must have inlined an older patch
> in the message, sorry.
> 

Thanks Marcelo, I will update the patch in my queue when I receive it.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]