linux-kernel - Re: [PATCH 2/2] mm: Fix anon

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: <CAHk-=wiU7J8-tz_UaaAZfLeak7N2D_LPLq3OS7stSxY87nwPYg@mail.gmail.com>
Date:   Thu, 27 Jul 2023 11:25:40 -0700
From:   Linus Torvalds <torvalds@...uxfoundation.org>
To:     Jann Horn <jannh@...gle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Matthew Wilcox <willy@...radead.org>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Alan Stern <stern@...land.harvard.edu>,
        Andrea Parri <parri.andrea@...il.com>,
        Will Deacon <will@...nel.org>,
        Boqun Feng <boqun.feng@...il.com>,
        Nicholas Piggin <npiggin@...il.com>,
        David Howells <dhowells@...hat.com>,
        Jade Alglave <j.alglave@....ac.uk>,
        Luc Maranget <luc.maranget@...ia.fr>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Akira Yokosawa <akiyks@...il.com>,
        Daniel Lustig <dlustig@...dia.com>,
        Joel Fernandes <joel@...lfernandes.org>
Subject: Re: [PATCH 2/2] mm: Fix anon_vma memory ordering

On Wed, 26 Jul 2023 at 14:51, Jann Horn <jannh@...gle.com> wrote:
>
> Of course I only realize directly after sending this patch that this
> comment only holds... [..]
> ... if we move the smp_store_release() down by one line here.

So I've applied PATCH 1/2 as obvious, but am holding off on this one.

Partly because of the other discussion about memory ordering, but also
due to how the now sometimes much more complex-looking conditionals
could be made a bit visually simpler.

For example the patch does

-       if (!vma || !(vma->vm_flags & VM_MERGEABLE) || !vma->anon_vma)
+       /* see anon_vma_prepare() */
+       if (!vma || !(vma->vm_flags & VM_MERGEABLE) ||
+                       !smp_load_acquire(&vma->anon_vma))
                return NULL;

which is a understandably mindless "just add the smp_load_acquire()",
but it is also just unnecessarily hard to read.

And the comment placement is a bit misleading too, since it really
only refers to one part of the expression. And that's very obvious if
you know what it's about, but the whole point of that comment would be
that you don't necessarily know why the code is doing what it is
doing.

IOW, that would be much more legible just split up as

        if (!vma || !(vma->vm_flags & VM_MERGEABLE))
                return NULL;

        /* see anon_vma_prepare() */
        if (!smp_load_acquire(&vma->anon_vma))
                return NULL;

I feel.

Also, I'm now wondering about the ordering of that

>                 smp_store_release(&vma->anon_vma, anon_vma);
>                 anon_vma_chain_link(vma, avc, anon_vma);

sequence. Maybe we *do* want the anon_vma pointer to be set first, so
that once it's on that anon_vma chain (and is visible in the anon_vma
interval tree) it always has a proper anon_vma pointer.

*OR* maybe the rule needs to be that any other concurrent user that
sees 'anon_vma' as being valid also can rely on the links being set
up?

I *suspect* the ordering doesn't actually matter, but it just makes me
wonder more about this area.

                Linus