linux-kernel - Re: [PATCH 0/2] fix vma->anon_vma check for per-VMA locking; 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: <d25defcf-1f78-4099-a1c6-10fc24799621@paulmck-laptop>
Date:   Thu, 27 Jul 2023 09:16:48 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Will Deacon <will@...nel.org>, Jann Horn <jannh@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linus Torvalds <torvalds@...uxfoundation.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,
        Andrea Parri <parri.andrea@...il.com>,
        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>,
        Akira Yokosawa <akiyks@...il.com>,
        Daniel Lustig <dlustig@...dia.com>,
        Joel Fernandes <joel@...lfernandes.org>
Subject: Re: [PATCH 0/2] fix vma->anon_vma check for per-VMA locking; fix
 anon_vma memory ordering

On Thu, Jul 27, 2023 at 11:44:02AM -0400, Alan Stern wrote:
> On Thu, Jul 27, 2023 at 03:57:47PM +0100, Will Deacon wrote:
> > On Thu, Jul 27, 2023 at 04:39:34PM +0200, Jann Horn wrote:
> 
> > > Assume that we are holding some kind of lock that ensures that the
> > > only possible concurrent update to "vma->anon_vma" is that it changes
> > > from a NULL pointer to a non-NULL pointer (using smp_store_release()).
> > > 
> > > 
> > > if (READ_ONCE(vma->anon_vma) != NULL) {
> > >   // we now know that vma->anon_vma cannot change anymore
> > > 
> > >   // access the same memory location again with a plain load
> > >   struct anon_vma *a = vma->anon_vma;
> > > 
> > >   // this needs to be address-dependency-ordered against one of
> > >   // the loads from vma->anon_vma
> > >   struct anon_vma *root = a->root;
> > > }
> 
> This reads a little oddly, perhaps because it's a fragment from a larger 
> piece of code.  Still, if I were doing something like this, I'd write it 
> as:
> 
> struct anon_vma *a;
> 
> a = READ_ONCE(vma->anon_vma);
> if (a != NULL) {
> 	struct anon_vma *root = a->root;
> 	...
> 
> thus eliminating the possibility of confusion from multiple reads of the 
> same address.
> 
> In this situation, the ordering of the two reads is guaranteed by the 
> address dependency.  And people shouldn't worry too much about using 
> that sort of ordering; RCU relies on it critically, all the time.

Agreed.  In contrast, control dependencies require quite a bit more care
and feeding, and are usually best avoided.

But even with the normal RCU address/data dependencies, it is possible
to get in trouble.  For but one example, comparing a pointer obtained
from rcu_dereference() to the address of a static structure is a good
way to break your address dependency.  (Just yesterday evening I talked
to someone who had spent quite a bit of time chasing one of these down,
so yes, this is quite real.)

> > > Is this fine? If it is not fine just because the compiler might
> > > reorder the plain load of vma->anon_vma before the READ_ONCE() load,
> > > would it be fine after adding a barrier() directly after the
> > > READ_ONCE()?
> > 
> > I'm _very_ wary of mixing READ_ONCE() and plain loads to the same variable,
> > as I've run into cases where you have sequences such as:
> > 
> > 	// Assume *ptr is initially 0 and somebody else writes it to 1
> > 	// concurrently
> > 
> > 	foo = *ptr;
> > 	bar = READ_ONCE(*ptr);
> > 	baz = *ptr;
> > 
> > and you can get foo == baz == 0 but bar == 1 because the compiler only
> > ends up reading from memory twice.
> > 
> > That was the root cause behind f069faba6887 ("arm64: mm: Use READ_ONCE
> > when dereferencing pointer to pte table"), which was very unpleasant to
> > debug.
> 
> Indeed, that's the sort of thing that can happen when plain accesses are 
> involved in a race.

Agreed.  Furthermore, it is more important to comment plain C-language
accesses to shared variables than to comment the likes of READ_ONCE().
"OK, tell me again exactly why you think the compiler cannot mess you
up here?"

							Thanx, Paul