[Intel-wired-lan] [PATCH v2] igb: Fix watchdog_task race with shutdown

[Intel-wired-lan] [PATCH v2] igb: Fix watchdog_task race with shutdown

[Ian Ray]

A rare [1] race condition is observed between the igb_watchdog_task and
shutdown on a dual-core i.MX6 based system with two I210 controllers.

Using printk, the igb_watchdog_task is hung in igb_read_phy_reg because
__igb_shutdown has already called __igb_close.

The fix is to delete timer and cancel the work after settting IGB_DOWN.
This approach mirrors igb_up.

reboot             kworker

__igb_shutdown
  rtnl_lock
  __igb_close
  :                igb_watchdog_task
  :                :
  :                igb_read_phy_reg (hung)
  rtnl_unlock

[1] Note that this is easier to reproduce with 'initcall_debug' logging
and additional and printk logging in igb_main.

Signed-off-by: Ian Ray <ian.ray@gehealthcare.com>
---
Changes in v2:
- Change strategy to avoid taking RTNL.
- Link to v1: https://lore.kernel.org/all/20250428115450.639-1-ian.ray@gehealthcare.com/
---
 drivers/net/ethernet/intel/igb/igb_main.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 9e9a5900e6e5..a65ae7925ae8 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2175,10 +2175,14 @@ void igb_down(struct igb_adapter *adapter)
 	u32 tctl, rctl;
 	int i;

-	/* signal that we're down so the interrupt handler does not
-	 * reschedule our watchdog timer
+	/* The watchdog timer may be rescheduled, so explicitly
+	 * disable watchdog from being rescheduled.
 	 */
 	set_bit(__IGB_DOWN, &adapter->state);
+	timer_delete_sync(&adapter->watchdog_timer);
+	timer_delete_sync(&adapter->phy_info_timer);
+
+	cancel_work_sync(&adapter->watchdog_task);

 	/* disable receives in the hardware */
 	rctl = rd32(E1000_RCTL);
@@ -2210,9 +2214,6 @@ void igb_down(struct igb_adapter *adapter)
 		}
 	}

-	timer_delete_sync(&adapter->watchdog_timer);
-	timer_delete_sync(&adapter->phy_info_timer);
-
 	/* record the stats before reset*/
 	spin_lock(&adapter->stats64_lock);
 	igb_update_stats(adapter);
--
2.49.0

Re: [Intel-wired-lan] [PATCH v2] igb: Fix watchdog_task race with shutdown

[Jakub Kicinski]

On Tue,  3 Jun 2025 11:09:49 +0300 Ian Ray wrote:
>  	set_bit(__IGB_DOWN, &adapter->state);
> +	timer_delete_sync(&adapter->watchdog_timer);
> +	timer_delete_sync(&adapter->phy_info_timer);
> +
> +	cancel_work_sync(&adapter->watchdog_task);

This doesn't look very race-proof as watchdog_task
can schedule the timer as its last operation?

Re: [Intel-wired-lan] [PATCH v2] igb: Fix watchdog_task race with shutdown

[Ian Ray]

On Thu, Jun 05, 2025 at 06:43:39PM -0700, Jakub Kicinski wrote:
> On Tue,  3 Jun 2025 11:09:49 +0300 Ian Ray wrote:
> >       set_bit(__IGB_DOWN, &adapter->state);
> > +     timer_delete_sync(&adapter->watchdog_timer);
> > +     timer_delete_sync(&adapter->phy_info_timer);
> > +
> > +     cancel_work_sync(&adapter->watchdog_task);
> 
> This doesn't look very race-proof as watchdog_task
> can schedule the timer as its last operation?

Thanks for the reply.  __IGB_DOWN is the key to this design.

If watchdog_task runs *before* __IGB_DOWN is set, then the
timer is stopped (by this patch) as required.

However, if watchdog_task runs *after* __IGB_DOWN is set,
then the timer will not even be started (by watchdog_task).

Regards,
Ian

Re: [Intel-wired-lan] [PATCH v2] igb: Fix watchdog_task race with shutdown

[Jakub Kicinski]

On Mon, 9 Jun 2025 09:32:58 +0300 Ian Ray wrote:
> On Thu, Jun 05, 2025 at 06:43:39PM -0700, Jakub Kicinski wrote:
> > On Tue,  3 Jun 2025 11:09:49 +0300 Ian Ray wrote:  
> > >       set_bit(__IGB_DOWN, &adapter->state);
> > > +     timer_delete_sync(&adapter->watchdog_timer);
> > > +     timer_delete_sync(&adapter->phy_info_timer);
> > > +
> > > +     cancel_work_sync(&adapter->watchdog_task);  
> > 
> > This doesn't look very race-proof as watchdog_task
> > can schedule the timer as its last operation?  
> 
> Thanks for the reply.  __IGB_DOWN is the key to this design.
> 
> If watchdog_task runs *before* __IGB_DOWN is set, then the
> timer is stopped (by this patch) as required.
> 
> However, if watchdog_task runs *after* __IGB_DOWN is set,
> then the timer will not even be started (by watchdog_task).

Well, yes, but what if the two functions run *simultaneously* 
There is no mutual exclusion between these two pieces of code AFAICT

Re: [Intel-wired-lan] [PATCH v2] igb: Fix watchdog_task race with shutdown

[Ian Ray]

On Mon, Jun 09, 2025 at 04:10:39PM -0700, Jakub Kicinski wrote:
> On Mon, 9 Jun 2025 09:32:58 +0300 Ian Ray wrote:
> > On Thu, Jun 05, 2025 at 06:43:39PM -0700, Jakub Kicinski wrote:
> > > On Tue,  3 Jun 2025 11:09:49 +0300 Ian Ray wrote:
> > > >       set_bit(__IGB_DOWN, &adapter->state);
> > > > +     timer_delete_sync(&adapter->watchdog_timer);
> > > > +     timer_delete_sync(&adapter->phy_info_timer);
> > > > +
> > > > +     cancel_work_sync(&adapter->watchdog_task);
> > >
> > > This doesn't look very race-proof as watchdog_task
> > > can schedule the timer as its last operation?
> >
> > Thanks for the reply.  __IGB_DOWN is the key to this design.
> >
> > If watchdog_task runs *before* __IGB_DOWN is set, then the
> > timer is stopped (by this patch) as required.
> >
> > However, if watchdog_task runs *after* __IGB_DOWN is set,
> > then the timer will not even be started (by watchdog_task).
> 
> Well, yes, but what if the two functions run *simultaneously*
> There is no mutual exclusion between these two pieces of code AFAICT

Thank you for clarifying.

IIUC set_bit() is an atomic operation (via bitops.h), and so
my previous comment still stands.

(Sorry if I have misunderstood your question.)

Either watchdog_task runs just before __IGB_DOWN is set (and
the timer is stopped by this patch) -- or watchdog_task runs
just after __IGB_DOWN is set (and thus the timer will not be
restarted).

In both cases, the final cancel_work_sync ensures that the
watchdog_task completes before igb_down() continues.

Regards,
Ian

Re: [Intel-wired-lan] [PATCH v2] igb: Fix watchdog_task race with shutdown

[Jacob Keller]

On 6/10/2025 5:44 AM, Ian Ray wrote:
> On Mon, Jun 09, 2025 at 04:10:39PM -0700, Jakub Kicinski wrote:
>> On Mon, 9 Jun 2025 09:32:58 +0300 Ian Ray wrote:
>>> On Thu, Jun 05, 2025 at 06:43:39PM -0700, Jakub Kicinski wrote:
>>>> On Tue,  3 Jun 2025 11:09:49 +0300 Ian Ray wrote:
>>>>>       set_bit(__IGB_DOWN, &adapter->state);
>>>>> +     timer_delete_sync(&adapter->watchdog_timer);
>>>>> +     timer_delete_sync(&adapter->phy_info_timer);
>>>>> +
>>>>> +     cancel_work_sync(&adapter->watchdog_task);
>>>>
>>>> This doesn't look very race-proof as watchdog_task
>>>> can schedule the timer as its last operation?
>>>
>>> Thanks for the reply.  __IGB_DOWN is the key to this design.
>>>
>>> If watchdog_task runs *before* __IGB_DOWN is set, then the
>>> timer is stopped (by this patch) as required.
>>>
>>> However, if watchdog_task runs *after* __IGB_DOWN is set,
>>> then the timer will not even be started (by watchdog_task).
>>
>> Well, yes, but what if the two functions run *simultaneously*
>> There is no mutual exclusion between these two pieces of code AFAICT
> 
> Thank you for clarifying.
> 
> IIUC set_bit() is an atomic operation (via bitops.h), and so
> my previous comment still stands.
> 
> (Sorry if I have misunderstood your question.)
> 
> Either watchdog_task runs just before __IGB_DOWN is set (and
> the timer is stopped by this patch) -- or watchdog_task runs
> just after __IGB_DOWN is set (and thus the timer will not be
> restarted).
> 
> In both cases, the final cancel_work_sync ensures that the
> watchdog_task completes before igb_down() continues.
> 
> Regards,
> Ian

Hmm. Well set_bit is atomic, but I don't think it has ordering
guarantees on its own. Wouldn't we need to be using a barrier here to
guarantee ordering here?

Perhaps cancel_work_sync has barriers implied and that makes this work
properly?

> ORDERING
> --------
> 
> Like with atomic_t, the rule of thumb is:
> 
>  - non-RMW operations are unordered;
> 
>  - RMW operations that have no return value are unordered;
> 
>  - RMW operations that have a return value are fully ordered.
> 
>  - RMW operations that are conditional are fully ordered.
> 
> Except for a successful test_and_set_bit_lock() which has ACQUIRE semantics,
> clear_bit_unlock() which has RELEASE semantics and test_bit_acquire which has
> ACQUIRE semantics.
> 

set_bit is listed as a RMW without a return value, so its unordered.
That makes me think we'd want clear_bit_unlock() if the cancel_work_sync
itself doesn't provide the barriers we need.

Thanks,
Jake

Re: [Intel-wired-lan] [PATCH v2] igb: Fix watchdog_task race with shutdown

[Ian Ray]

On Mon, Jun 16, 2025 at 02:47:29PM -0700, Jacob Keller wrote:
> On 6/10/2025 5:44 AM, Ian Ray wrote:
> > On Mon, Jun 09, 2025 at 04:10:39PM -0700, Jakub Kicinski wrote:
:
> > IIUC set_bit() is an atomic operation (via bitops.h), and so
> > my previous comment still stands.
> >
> > (Sorry if I have misunderstood your question.)
> >
> > Either watchdog_task runs just before __IGB_DOWN is set (and
> > the timer is stopped by this patch) -- or watchdog_task runs
> > just after __IGB_DOWN is set (and thus the timer will not be
> > restarted).
> >
> > In both cases, the final cancel_work_sync ensures that the
> > watchdog_task completes before igb_down() continues.
> >
> > Regards,
> > Ian
> 
> Hmm. Well set_bit is atomic, but I don't think it has ordering
> guarantees on its own. Wouldn't we need to be using a barrier here to
> guarantee ordering here?
> 
> Perhaps cancel_work_sync has barriers implied and that makes this work
> properly?

Ah, I see.  I checked the cancel_work_documentation and implementation
and I am not sure we can make any assumptions about barriers.

Would two additional calls to smp_mb__after_atomic() be acceptable?
Something like this (on top of this series v2).

-- >8 --
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index a65ae7925ae8..9b63dc594454 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2179,6 +2179,7 @@ void igb_down(struct igb_adapter *adapter)
         * disable watchdog from being rescheduled.
         */
        set_bit(__IGB_DOWN, &adapter->state);
+       smp_mb__after_atomic();
        timer_delete_sync(&adapter->watchdog_timer);
        timer_delete_sync(&adapter->phy_info_timer);

@@ -3886,6 +3887,7 @@ static void igb_remove(struct pci_dev *pdev)
         * disable watchdog from being rescheduled.
         */
        set_bit(__IGB_DOWN, &adapter->state);
+       smp_mb__after_atomic();
        timer_delete_sync(&adapter->watchdog_timer);
        timer_delete_sync(&adapter->phy_info_timer);
-- >8 --

Thanks,
Ian

> 
> > ORDERING
> > --------
> >
> > Like with atomic_t, the rule of thumb is:
> >
> >  - non-RMW operations are unordered;
> >
> >  - RMW operations that have no return value are unordered;
> >
> >  - RMW operations that have a return value are fully ordered.
> >
> >  - RMW operations that are conditional are fully ordered.
> >
> > Except for a successful test_and_set_bit_lock() which has ACQUIRE semantics,
> > clear_bit_unlock() which has RELEASE semantics and test_bit_acquire which has
> > ACQUIRE semantics.
> >
> 
> set_bit is listed as a RMW without a return value, so its unordered.
> That makes me think we'd want clear_bit_unlock() if the cancel_work_sync
> itself doesn't provide the barriers we need.
> 
> Thanks,
> Jake