netdev - RE: [RFC net-next] net: stmmac: should not modify RX descriptor when STMMAC resume

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: <DB8PR04MB67954D37A59B2D91C69BF6A9E6489@DB8PR04MB6795.eurprd04.prod.outlook.com>
Date:   Tue, 20 Apr 2021 01:49:19 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Jon Hunter <jonathanh@...dia.com>,
        "peppe.cavallaro@...com" <peppe.cavallaro@...com>,
        "alexandre.torgue@...s.st.com" <alexandre.torgue@...s.st.com>,
        "joabreu@...opsys.com" <joabreu@...opsys.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "mcoquelin.stm32@...il.com" <mcoquelin.stm32@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>
CC:     dl-linux-imx <linux-imx@....com>,
        "treding@...dia.com" <treding@...dia.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [RFC net-next] net: stmmac: should not modify RX descriptor when
 STMMAC resume


Hi Jon,

> -----Original Message-----
> From: Jon Hunter <jonathanh@...dia.com>
> Sent: 2021年4月19日 23:12
> To: Joakim Zhang <qiangqing.zhang@....com>; peppe.cavallaro@...com;
> alexandre.torgue@...s.st.com; joabreu@...opsys.com;
> davem@...emloft.net; kuba@...nel.org; mcoquelin.stm32@...il.com;
> andrew@...n.ch; f.fainelli@...il.com
> Cc: dl-linux-imx <linux-imx@....com>; treding@...dia.com;
> netdev@...r.kernel.org
> Subject: Re: [RFC net-next] net: stmmac: should not modify RX descriptor when
> STMMAC resume
> 
> Hi Joakim,
> 
> On 19/04/2021 12:59, Joakim Zhang wrote:
> > When system resume back, STMMAC will clear RX descriptors:
> > stmmac_resume()
> > 	->stmmac_clear_descriptors()
> > 		->stmmac_clear_rx_descriptors()
> > 			->stmmac_init_rx_desc()
> > 				->dwmac4_set_rx_owner()
> > 				//p->des3 |= cpu_to_le32(RDES3_OWN |
> RDES3_BUFFER1_VALID_ADDR); It
> > only assets OWN and BUF1V bits in desc3 field, doesn't clear desc0/1/2 fields.
> >
> > Let's take a case into account, when system suspend, it is possible
> > that there are packets have not received yet, so the RX descriptors
> > are wrote back by DMA, e.g.
> > 008 [0x00000000c4310080]: 0x0 0x40 0x0 0x34010040
> >
> > When system resume back, after above process, it became a broken
> > descriptor:
> > 008 [0x00000000c4310080]: 0x0 0x40 0x0 0xb5010040
> >
> > The issue is that it only changes the owner of this descriptor, but do
> > nothing about desc0/1/2 fields. The descriptor of STMMAC a bit
> > special, applicaton prepares RX descriptors for DMA, after DMA recevie
> > the packets, it will write back the descriptors, so the same field of
> > a descriptor have different meanings to application and DMA. It should
> > be a software bug there, and may not easy to reproduce, but there is a
> > certain probability that it will occur.
> >
> > Commit 9c63faaa931e ("net: stmmac: re-init rx buffers when mac resume
> > back") tried to re-init desc0/desc1 (buffer address fields) to fix
> > this issue, but it is not a proper solution, and made regression on Jetson TX2
> boards.
> >
> > It is unreasonable to modify RX descriptors outside of
> > stmmac_rx_refill() function, where it will clear all desc0/desc1/desc2/desc3
> fields together.
> >
> > This patch removes RX descriptors modification when STMMAC resume.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@....com>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 9f396648d76f..b784304a22e8 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -7186,6 +7186,8 @@ static void stmmac_reset_queues_param(struct
> stmmac_priv *priv)
> >  		tx_q->mss = 0;
> >
> >  		netdev_tx_reset_queue(netdev_get_tx_queue(priv->dev, queue));
> > +
> > +		stmmac_clear_tx_descriptors(priv, queue);
> >  	}
> >  }
> >
> > @@ -7250,7 +7252,6 @@ int stmmac_resume(struct device *dev)
> >  	stmmac_reset_queues_param(priv);
> >
> >  	stmmac_free_tx_skbufs(priv);
> > -	stmmac_clear_descriptors(priv);
> >
> >  	stmmac_hw_setup(ndev, false);
> >  	stmmac_init_coalesce(priv);
> >
> 
> 
> I have tested this patch, but unfortunately the board still fails to resume
> correctly. So it appears to suffer with the same issue we saw on the previous
> implementation.

Could I double check with you? Have you reverted Commit 9c63faaa931e ("net: stmmac: re-init rx buffers when mac resume back") and then apply above patch to do the test?

If yes, you still saw the same issue with Commit 9c63faaa931e? Let's recall the problem, system suspended, but system hang when STMMAC resume back, right?

Best Regards,
Joakim Zhang
> Jon
> 
> --
> nvpublic