netdev - RE: [PATCH] net: fec: ethtool: fix unbalanced IRQ wake disable

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <DB8PR04MB6795F51CEFF0DE50E57C9FFBE63C9@DB8PR04MB6795.eurprd04.prod.outlook.com>
Date:   Wed, 23 Feb 2022 10:28:34 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Sascha Hauer <s.hauer@...gutronix.de>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "David S . Miller" <davem@...emloft.net>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        Ahmad Fatoum <a.fatoum@...gutronix.de>
Subject: RE: [PATCH] net: fec: ethtool: fix unbalanced IRQ wake disable


Hi Sascha,

> -----Original Message-----
> From: Sascha Hauer <s.hauer@...gutronix.de>
> Sent: 2022年2月23日 16:09
> To: netdev@...r.kernel.org
> Cc: Joakim Zhang <qiangqing.zhang@....com>; David S . Miller
> <davem@...emloft.net>; kernel@...gutronix.de; Ahmad Fatoum
> <a.fatoum@...gutronix.de>; Sascha Hauer <s.hauer@...gutronix.de>
> Subject: [PATCH] net: fec: ethtool: fix unbalanced IRQ wake disable
> 
> From: Ahmad Fatoum <a.fatoum@...gutronix.de>
> 
> Userspace can trigger a kernel warning by using the ethtool ioctls to disable
> WoL, when it was not enabled before:
> 
>   $ ethtool -s eth0 wol d ; ethtool -s eth0 wol d
>   Unbalanced IRQ 54 wake disable
>   WARNING: CPU: 2 PID: 17532 at kernel/irq/manage.c:900
> irq_set_irq_wake+0x108/0x148
> 
> This is because fec_enet_set_wol happily calls disable_irq_wake, even if the
> wake IRQ is already disabled.

I have not found disable_irq_wake in fec_enet_set_wol.
https://elixir.bootlin.com/linux/v5.17-rc5/source/drivers/net/ethernet/freescale/fec_main.c#L2882

> Looking at other drivers, like lpc_eth, suggests the way to go is to do wake
> IRQ enabling/disabling in the suspend/resume callbacks.
> Doing so avoids the warning at no loss of functionality.

FEC done as this way:
https://elixir.bootlin.com/linux/v5.17-rc5/source/drivers/net/ethernet/freescale/fec_main.c#L4075
https://elixir.bootlin.com/linux/v5.17-rc5/source/drivers/net/ethernet/freescale/fec_main.c#L4119

> This only affects userspace with older ethtool versions. Newer ones use
> netlink and disabling before enabling will be refused before reaching the
> driver.

Ahh, what I misunderstand? All the description makes me confusion. Please use the latest kernel.

Best Regards,
Joakim Zhang
> Fixes: de40ed31b3c5 ("net: fec: add Wake-on-LAN support")
> Signed-off-by: Ahmad Fatoum <a.fatoum@...gutronix.de>
> Signed-off-by: Sascha Hauer <s.hauer@...gutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 796133de527e4..44a0c89d76dd6 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -4055,6 +4055,9 @@ static int __maybe_unused fec_suspend(struct
> device *dev)
>  	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> 
> +	if (device_may_wakeup(&ndev->dev) && fep->wake_irq > 0)
> +		enable_irq_wake(fep->wake_irq);
> +
>  	rtnl_lock();
>  	if (netif_running(ndev)) {
>  		if (fep->wol_flag & FEC_WOL_FLAG_ENABLE) @@ -4137,6 +4140,9
> @@ static int __maybe_unused fec_resume(struct device *dev)
>  	}
>  	rtnl_unlock();
> 
> +	if (device_may_wakeup(&ndev->dev) && fep->wake_irq > 0)
> +		disable_irq_wake(fep->wake_irq);
> +
>  	return 0;
> 
>  failed_clk:
> --
> 2.30.2