Re: [PATCH 2/2] e1000: fix possible reset_task running after adapter down

[Jeff Kirsher]

On Fri, 2013-11-22 at 12:20 +0400, Vladimir Davydov wrote:
> On e1000_down(), we should ensure every asynchronous work is canceled
> before proceeding. Since the watchdog_task can schedule other works
> apart from itself, it should be stopped first, but currently it is
> stopped after the reset_task. This can result in the following race
> leading to the reset_task running after the module unload:
> 
> e1000_down_and_stop():  e1000_watchdog():
> --  -
> 
> cancel_work_sync(reset_task)
> schedule_work(reset_task)
> cancel_delayed_work_sync(watchdog_task)
> 
> The patch moves cancel_delayed_work_sync(watchdog_task) at the
> beginning
> of e1000_down_and_stop() thus ensuring the race is impossible.
> 
> Signed-off-by: Vladimir Davydov 
> Cc: Tushar Dave 
> Cc: Patrick McHardy 
> Cc: David S. Miller 
> ---
>  drivers/net/ethernet/intel/e1000/e1000_main.c |   15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)

I have applied your patch to my queue, thanks!


signature.asc
Description: This is a digitally signed message part

[PATCH 2/2] e1000: fix possible reset_task running after adapter down

[Vladimir Davydov]

On e1000_down(), we should ensure every asynchronous work is canceled
before proceeding. Since the watchdog_task can schedule other works
apart from itself, it should be stopped first, but currently it is
stopped after the reset_task. This can result in the following race
leading to the reset_task running after the module unload:

e1000_down_and_stop():  e1000_watchdog():
--  -

cancel_work_sync(reset_task)
schedule_work(reset_task)
cancel_delayed_work_sync(watchdog_task)

The patch moves cancel_delayed_work_sync(watchdog_task) at the beginning
of e1000_down_and_stop() thus ensuring the race is impossible.

Signed-off-by: Vladimir Davydov 
Cc: Tushar Dave 
Cc: Patrick McHardy 
Cc: David S. Miller 
---
 drivers/net/ethernet/intel/e1000/e1000_main.c |   15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c 
b/drivers/net/ethernet/intel/e1000/e1000_main.c
index ca0723f..2d914de 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -494,13 +494,20 @@ static void e1000_down_and_stop(struct e1000_adapter 
*adapter)
 {
set_bit(__E1000_DOWN, &adapter->flags);
 
-   /* Only kill reset task if adapter is not resetting */
-   if (!test_bit(__E1000_RESETTING, &adapter->flags))
-   cancel_work_sync(&adapter->reset_task);
-
cancel_delayed_work_sync(&adapter->watchdog_task);
+
+   /*
+* Since the watchdog task can reschedule other tasks, we should cancel
+* it first, otherwise we can run into the situation when a work is
+* still running after the adapter has been turned down.
+*/
+
cancel_delayed_work_sync(&adapter->phy_info_task);
cancel_delayed_work_sync(&adapter->fifo_stall_task);
+
+   /* Only kill reset task if adapter is not resetting */
+   if (!test_bit(__E1000_RESETTING, &adapter->flags))
+   cancel_work_sync(&adapter->reset_task);
 }
 
 void e1000_down(struct e1000_adapter *adapter)
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/