| Message ID | 20140307020420.GA10797@amt.cnet |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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. -- 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
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 -- 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
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. -- 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
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. -- 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
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.
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;
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> -- 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