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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 23 Apr 2022 06:45:08 -0700 From: Eric Dumazet <edumazet@...gle.com> To: Peter Zijlstra <peterz@...radead.org> Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>, netdev <netdev@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Thomas Gleixner <tglx@...utronix.de> Subject: Re: [PATCH net] net: Use this_cpu_inc() to increment net->core_stats On Sat, Apr 23, 2022 at 2:24 AM Peter Zijlstra <peterz@...radead.org> wrote: > > On Thu, Apr 21, 2022 at 06:51:31PM +0200, Sebastian Andrzej Siewior wrote: > > On 2022-04-21 09:06:05 [-0700], Eric Dumazet wrote: > > > On Thu, Apr 21, 2022 at 7:00 AM Sebastian Andrzej Siewior > > > <bigeasy@...utronix.de> wrote: > > > > > > > > > > > for_each_possible_cpu(i) { > > > > core_stats = per_cpu_ptr(p, i); > > > > - storage->rx_dropped += local_read(&core_stats->rx_dropped); > > > > - storage->tx_dropped += local_read(&core_stats->tx_dropped); > > > > - storage->rx_nohandler += local_read(&core_stats->rx_nohandler); > > > > + storage->rx_dropped += core_stats->rx_dropped; > > > > + storage->tx_dropped += core_stats->tx_dropped; > > > > + storage->rx_nohandler += core_stats->rx_nohandler; > > > > > > I think that one of the reasons for me to use local_read() was that > > > it provided what was needed to avoid future syzbot reports. > > > > syzbot report due a plain read of a per-CPU variable which might be > > modified? > > > > > Perhaps use READ_ONCE() here ? > > > > > > Yes, we have many similar folding loops that are simply assuming > > > compiler won't do stupid things. > > > > I wasn't sure about that and added PeterZ to do some yelling here just > > in case. And yes, we have other sites doing exactly that. In > > Documentation/core-api/this_cpu_ops.rst > > there is nothing about remote-READ-access (only that there should be no > > writes (due to parallel this_cpu_inc() on the local CPU)). I know that a > > 32bit write can be optimized in two 16bit writes in certain cases but a > > read is a read. > > PeterZ? :) > > Eric is right. READ_ONCE() is 'required' to ensure the compiler doesn't > split the load and KCSAN konws about these things. More details can be found in https://lwn.net/Articles/793253/ Thanks !
Powered by blists - more mailing lists