linux-kernel - 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 for Android: free password hash cracker in your pocket

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

Date: Wed, 3 Apr 2024 10:01:02 +0800
From: cathy cai <cathycai0714@...il.com>
To: Romain Gantois <romain.gantois@...tlin.com>
Cc: Cathy Cai <cathy.cai@...soc.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

Hi Romain,

On Sun, Mar 31, 2024 at 4:35 PM Romain Gantois
<romain.gantois@...tlin.com> wrote:
>
> 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?
>
 Yes. It could also happen between the dev_open() and
clear_bit(STMMAC_DOWN) calls.
Although we did not reproduce this scenario, it should have happened
if we had increased
the number of test samples. In addition, I found that other people had
similar problems before.
The link is:
https://lore.kernel.org/all/20210208140820.10410-11-Sergey.Semin@baikalelectronics.ru/

>
> 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?
>
Clear interrupts unconditionally in the STMMAC_DOWN state directly
certainly won't cause this problem.
This may be too rough, maybe this design has other considerations.

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

 Best Regards,
Cathy