netdev - Re: [PATCH] inet: frags: Remove unnecessary smp_store_release/READ

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, 31 May 2019 10:24:08 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Herbert Xu <herbert@...dor.apana.org.au>,
        "Paul E. McKenney" <paulmck@...ux.ibm.com>,
        Andrea Parri <andrea.parri@...rulasolutions.com>,
        Alan Stern <stern@...land.harvard.edu>
Cc:     Eric Dumazet <edumazet@...gle.com>,
        David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        syzbot <syzkaller@...glegroups.com>
Subject: Re: [PATCH] inet: frags: Remove unnecessary smp_store_release/READ_ONCE

On Wed, May 29, 2019 at 7:48 AM Herbert Xu <herbert@...dor.apana.org.au> wrote:
>
> On Wed, May 29, 2019 at 07:43:51AM +0200, Dmitry Vyukov wrote:
> >
> > If fqdir->dead read/write are concurrent, then this still needs to be
> > READ_ONCE/WRITE_ONCE. Ordering is orthogonal to atomicity.
>
> No they do not.  READ_ONCE/WRITE_ONCE are basically a more fine-tuned
> version of barrier().  In this case we already have an implicit
> barrier() call due to the memory barrier semantics so this is simply
> unnecessary.
>
> It's the same reason you don't need READ_ONCE/WRITE_ONCE when you do:
>
> CPU1                            CPU2
> ----                            ----
> spin_lock
> shared_var = 1                  spin_lock
> spin_unlock                     if (shared_var == 1)
>                                         ...
>                                 spin_unlock

+Paul, Andrea, Alan

OK, let's call it barrier. But we need more than a barrier here then.

1. The C standard is very clear here -- this new code causes undefined
behavior of kernel. Regardless of what one might think about the C
standard, it's still the current contract between us and compiler
writers and nobody created any better replacement.

2. Then Documentation/memory-barriers.txt (which one can't say is not
relevant here) effectively says the same:

 (*) It _must_not_ be assumed that the compiler will do what you want
     with memory references that are not protected by READ_ONCE() and
     WRITE_ONCE().  Without them, the compiler is within its rights to
     do all sorts of "creative" transformations, which are covered in
     the COMPILER BARRIER section.

3. The code is only correct under the naive execution (all code is
executed literally as written). But we don't want compiler to execute
code in such way, we want them to all employ all possible optimization
tricks to make it faster. As the result compiler can break this code.
It can reload the condition multiple times, including within the same
branch, it can use variables as scratch storage and do other things
that you and me can't think of now. Some of these optimizations are
reasonable, some are not reasonable but still legal and just a result
complex compiler logic giving surprising result on a corner case. Also
if current implementation details of rhashtable_remove_fast change,
surprising things can happen, e.g. executing just refcount_dec but not
rhashtable_remove_fast.
"Proving" impossibility of any of unexpected transformations it's just
not what we should be spending time on again and again. E.g. a hundred
of times people will skim through this code in future chasing some
bug.

4. Then READ_ONCE()/WRITE_ONCE() improve code self-documentation.
There is huge difference between a plain load and inter-thread
synchronization algorithms. This needs to be clearly visible in the
code. Especially when one hunts a racy memory corruption in large base
of code (which happens with kernel all the time).

5. Marking all shared access is required to enable data race detection
tooling. Which is an absolute must for kernel taking into account
amount and complexity of tricky multi-threaded code. We need this.

6. We need marking of all shared memory accesses for the kernel memory
model effort. There is no way the model can be defined without that.

7. This is very different from spin_lock example. spin_lock is a
synchronization primitive that effectively makes code inside of the
critical section single-threaded. But in this case read/write to dead
execute concurrently.

So what are the reasons to give up all these nice things and go into
the rabbit hole of "proving" that we don't see how compiler could
screw up things?
If there are no significant reasons that outweigh all of these points,
please use READ_ONCE()/WRITE_ONCE() for all shared accesses. Many will
say thank you.