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]

Date:   Fri, 23 Apr 2021 02:17:35 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Florian Fainelli <f.fainelli@...il.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>
CC:     dl-linux-imx <linux-imx@....com>,
        "jonathanh@...dia.com" <jonathanh@...dia.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 Florian,

Thanks for your comments.

> -----Original Message-----
> From: Florian Fainelli <f.fainelli@...il.com>
> Sent: 2021年4月23日 0:32
> 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
> Cc: dl-linux-imx <linux-imx@....com>; jonathanh@...dia.com;
> treding@...dia.com; netdev@...r.kernel.org
> Subject: Re: [RFC net-next] net: stmmac: should not modify RX descriptor when
> STMMAC resume
> 
> 
> 
> On 4/19/2021 4:59 AM, 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.
> 
> Your patch makes sense to me, however the explanation seems to highlight
> that you may have a few cases to consider while you suspend.
> 
> Usually you will turn off the RX DMA such that DMA into DRAM stops there,
> this may not an entirely atomic operation as the MAC may have to wait for a
> certain packet boundary to be crossed, that could leave you with descriptors in
> 3 states I believe:
> 
> - descriptor is ready for RX DMA to process and is owned by RX DMA, no need
> to do anything
Agree.

> - descriptor has been fully consumed by the CPU and is owned by the CPU, CPU
> should be putting the descriptor back on the ring and relinquish ownership
Yes, at this case, after stmmac resume, this descriptor would have chance to be refilled in stmmac_rx_refill() function.

> - descriptor has been written to DRAM but not processed by CPU, and it should
> be put back on the ring for RX DMA to use it
This case is descriptor is owned by CPU and rx buffer have not been consumed yet, I think it still has chance to be received from stmmac_rx() after stmmac resume.

> 
> Out of suspend, don't you need to deal with descriptors in cases 2 and 3
> somehow? Does the DMA skip over descriptors that are still marked as owned
> by the CPU or does it stop/stall?
Yes, AFAIK, DMA would skip over descriptors that are owned by CPU, only use the descriptors that owned by DMA. This is my understanding.

Best Regards,
Joakim Zhang
> --
> Florian