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

Marcelo Tosatti

author

2014-03-07 02:04 UTC #0 lore raw

PATCH 1/1 e1000: do not allow watchdog to reenable transmits on shutdown

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 <[email protected]>

drivers/net/ethernet/intel/e1000/e1000.h +2 -1

index 10a0f22..bb5dc1a 100644
@@ -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

drivers/net/ethernet/intel/e1000/e1000_main.c +11 -0

index 46e6544..b20ce98 100644
@@ -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
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

Jeff Kirsher

2014-03-07 03:24 UTC #1 lore raw

Quoted 10 lines · 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.

Quoted 4 lines

Fix by not allowing the watchdog and tx fifo stall detector to re-enable E1000_TCTL_EN.
Signed-off-by Marcelo Tosatti <[email protected]>

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.

Marcelo Tosatti

author

2014-03-07 15:14 UTC #2 lore raw

Quoted 14 lines · Jeff Kirsher

Quoted 13 lines · 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.

Quoted 7 lines

Quoted 1 line
Signed-off-by Marcelo Tosatti <[email protected]>
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

David Miller

maintainer

2014-03-07 18:54 UTC #3 lore raw

From: Marcelo Tosatti <[email protected]>
Date: Fri, 7 Mar 2014 12:14:35 -0300

Quoted 2 lines

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
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

Joe Perches

2014-03-07 19:41 UTC #4 lore raw

In reply to Marcelo Tosatti

Quoted 4 lines · Marcelo Tosatti

Quoted 3 lines · Jeff Kirsher
Quoted 2 lines · Marcelo Tosatti
There is a race on the shutdown path of the e1000 driver that allows the card to DMA into free'd memory.

[]

Quoted 6 lines

Quoted 3 lines
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.

Marcelo Tosatti

author

2014-03-07 20:59 UTC #5 lore raw

PATCH 1/1 e1000: do not allow watchdog to reenable transmits on shutdown (v2)

In reply to Jeff Kirsher

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 <[email protected]>

drivers/net/ethernet/intel/e1000/e1000.h +2 -1

index 10a0f22..bb5dc1a 100644
@@ -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

drivers/net/ethernet/intel/e1000/e1000_main.c +12 -0

index 46e6544..a39484f 100644
@@ -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
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

Jeff Kirsher

2014-03-08 00:47 UTC #6 lore raw

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

Quoted 9 lines · 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.

Quoted 4 lines

Fix by not allowing the watchdog and tx fifo stall detector to re-enable E1000_TCTL_EN.
Signed-off-by Marcelo Tosatti <[email protected]>

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

Marcelo Tosatti

author

2014-03-07 21:00 UTC #7 lore raw

In reply to Jeff Kirsher

Quoted 14 lines · Jeff Kirsher

Quoted 13 lines · 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.

Quoted 7 lines

Quoted 1 line
Signed-off-by Marcelo Tosatti <[email protected]>
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

Jeff Kirsher

2014-03-07 21:32 UTC #8 lore raw

Quoted 17 lines · Marcelo Tosatti

Quoted 16 lines · Jeff Kirsher
Quoted 14 lines · 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 <[email protected]>

Quoted 9 lines

Quoted 5 lines
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.