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: <1b2dd47e-184a-2dea-f62d-5417192f2710@gmail.com>
Date:   Thu, 22 Apr 2021 09:31:51 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
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:     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

- 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

- 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

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?
-- 
Florian