netdev - Re: [REGRESSION] [PATCH net-next v5 2/2] net: stmmac: use per-queue 64 bit statistics where necessary

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: <2fcc9fb0e40ceff8ea4ae55cca3ce0aff75a20ca.camel@sipsolutions.net>
Date: Tue, 12 Sep 2023 11:30:14 +0200
From: Johannes Berg <johannes@...solutions.net>
To: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>, 
	Lucas Stach <l.stach@...gutronix.de>, Jisheng Zhang <jszhang@...nel.org>
Cc: Maxime Coquelin <mcoquelin.stm32@...il.com>, Eric Dumazet
 <edumazet@...gle.com>, Samuel Holland <samuel@...lland.org>, 
 netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>, Alexandre
 Torgue <alexandre.torgue@...s.st.com>, Jernej Skrabec
 <jernej.skrabec@...il.com>,  linux-kernel@...r.kernel.org, Chen-Yu Tsai
 <wens@...e.org>, Jose Abreu <joabreu@...opsys.com>, kernel@...gutronix.de,
 Giuseppe Cavallaro <peppe.cavallaro@...com>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 linux-sunxi@...ts.linux.dev,  linux-stm32@...md-mailman.stormreply.com,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [REGRESSION] [PATCH net-next v5 2/2] net: stmmac: use per-queue
 64 bit statistics where necessary

On Tue, 2023-09-12 at 11:24 +0200, Uwe Kleine-König wrote:
> > 
> > The newly added "struct u64_stats_sync syncp" uses a seqlock
> > internally, which is broken into multiple words on 32bit machines, and
> > needs to be initialized properly. You need to call u64_stats_init on
> > syncp before first usage.
> 
> This is done. The problematic thing is that in stmmac_open() ->
> __stmmac_open() the syncp initialized before is overwritten by
> 
> 	memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf));
> 
> Do I need to point out that this is ugly?

I think it also leaks the (lockdep) state since it reinits the syncp
(and a lot of other state) doing this. This is also called when the MTU
changes.

Also, I couldn't convince myself that it's even race-free? Even if it
is, it's not really obvious, IMHO.

So it seems to me that really this needs to be split into data that
actually should be reinitialized, and data that shouldn't, or just not
use memcpy() here but copy only the relevant state?

But anyway, I have no skin in this game - just reviewing this because I
was trying to help out Uwe.

johannes