linux-kernel - Re: [Intel-wired-lan] [PATCH v2] igb: Fix watchdog

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3504878c-6b3f-4d5f-bcfd-2e7e4a912570@intel.com>
Date: Mon, 16 Jun 2025 14:47:29 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Ian Ray <ian.ray@...ealthcare.com>, Jakub Kicinski <kuba@...nel.org>
CC: <horms@...nel.org>, Tony Nguyen <anthony.l.nguyen@...el.com>, "Przemek
 Kitszel" <przemyslaw.kitszel@...el.com>, Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
	Paolo Abeni <pabeni@...hat.com>, <brian.ruley@...ealthcare.com>,
	<intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [Intel-wired-lan] [PATCH v2] igb: Fix watchdog_task race with
 shutdown



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