netdev - Re: [RFC PATCH] net: stmmac: Fix the problem about interrupt storm

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]

Date: Sun, 31 Mar 2024 10:35:32 +0200 (CEST)
From: Romain Gantois <romain.gantois@...tlin.com>
To: Cathy Cai <cathy.cai@...soc.com>
cc: cathycai0714@...il.com, davem@...emloft.net, edumazet@...gle.com, 
    kuba@...nel.org, pabeni@...hat.com, mcoquelin.stm32@...il.com, 
    netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
    xuewen.yan94@...il.com, cixi.geng1@...soc.com, wade.shu@...soc.com, 
    zhiguo.niu@...soc.com, alexandre.torgue@...s.st.com, joabreu@...opsys.com, 
    linux-stm32@...md-mailman.stormreply.com, 
    linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC PATCH] net: stmmac: Fix the problem about interrupt storm

Hello Cathy,

On Wed, 27 Mar 2024, Cathy Cai wrote:

> Tx queue time out then reset adapter. When reset the adapter, stmmac driver
> sets the state to STMMAC_DOWN and calls dev_close() function. If an interrupt
> is triggered at this instant after setting state to STMMAC_DOWN, before the
> dev_close() call.
> 
...
> -	set_bit(STMMAC_DOWN, &priv->state);
>  	dev_close(priv->dev);
> +	set_bit(STMMAC_DOWN, &priv->state);
>  	dev_open(priv->dev, NULL);
>  	clear_bit(STMMAC_DOWN, &priv->state);
>  	clear_bit(STMMAC_RESETING, &priv->state);

If this IRQ issue can happen whenever STMMAC_DOWN is set while the net device is 
open, then it could also happen between the dev_open() and 
clear_bit(STMMAC_DOWN) calls right? So you'd have to clear STMMAC_DOWN before 
calling dev_open() but then I don't see the usefulness of setting STMMAC_DOWN 
and clearing it immediately. Maybe closing and opening the net device should be 
enough?

Moreover, it seems strange to me that stmmac_interrupt() unconditionnally 
ignores interrupts when the driver is in STMMAC_DOWN state. This seems like 
dangerous behaviour, since it could cause IRQ storm issues whenever something 
in the driver sets this state. I'm not too familiar with the interrupt handling 
in this driver, but maybe stmmac_interrupt() could clear interrupts 
unconditionnally in the STMMAC_DOWN state?

Best Regards,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com