linux-kernel - Re: [PATCH] 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: <20250429152021.GP3339421@horms.kernel.org>
Date: Tue, 29 Apr 2025 16:20:21 +0100
From: Simon Horman <horms@...nel.org>
To: Ian Ray <ian.ray@...ealthcare.com>
Cc: 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>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	brian.ruley@...ealthcare.com, intel-wired-lan@...ts.osuosl.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Toke Høiland-Jørgensen <toke@...hat.com>
Subject: Re: [PATCH] igb: Fix watchdog_task race with shutdown

+ Toke

On Mon, Apr 28, 2025 at 02:54:49PM +0300, Ian Ray wrote:
> 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.
> 
> Fix this by locking in igb_watchdog_task (in the same way as is done in
> igb_reset_task).
> 
> 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@...ealthcare.com>

Hi Ian,

Thanks for your patch.

While I think that the simplicity of this approach may well be appropriate
as a fix for the problem described I do have a concern.

I am worried that taking RTNL each time the watchdog tasks will create
unnecessary lock contention. That may manifest in weird and wonderful ways
in future.  Maybe this patch doesn't make things materially worse in that
regard.  But it would be nice to have a plan to move away from using RTNL,
as is happening elsewhere.

...