Re: [PATCH] mm: fix the race on huge alloc failed

[Guilherme Giacomo Simoes]

Please, forgive the delay. I really needed spend a long time to understand your
explanation about why this race is safe.

Pedro Falcato <pfalcato@xxxxxxx> wrote:
> > > So it's inappropriate to use READ_ONCE() / WRITE_ONCE() to "solve"
> > > this problem, because we don't need those semantics.  It's sufficient
> > > to wrap the read side in data_race() to indicate to KCSAN that we know
> > > what we're doing.
> > you sure? 
> > 
> > the __anon_vma_prepare(..) is write on vma->anon_vma and the
> > __vmf_anon_prepare(..) is reade from the same vma->anon_vma at the same time,
> > you sure that is not a problem? (I'm asking as a curious layperson.) 
> 
> 99.9% sure. Here's the basic logic laid out:
> 
> 1) Fault needs to fault in anonymous pages
> 2) Fault needs to possibly create an anon_vma
>  2a) Thus it does the lockless check, where indeed we only
>      care if it's non-null or not.
>  2b) if the lockless check fails, we get into __anon_vma_prepare()
>      logic, which crucially takes the page_table_lock to write the
>      anon_vma to the vma. If it takes the lock and something is already
>      there, it backs out.
> 3) Now, into the weeds of anon page faulting, we end up in __folio_set_anon(),
>    which reads the anon_vma from vma. This function always (AFAIK?) runs with
>    the PTE lock held. Thus we can be sure the anon_vma value is correct. In
>    any case, we only need to have held the page table lock once in the fault
>    for it to be valid; any change to its value from non-null to null needs
>    the vma/mmap write lock. Because we take a bunch of locks and do a bunch of
>    stuff between that initial check in __vmf_anon_prepare and this, the compiler
>    cannot validly cache the load (which can, in theory, tear).
> 
> Now, for memory ordering and its wonderful transitive properties:
> 1) writing anon_vma takes the page_table_lock. therefore if you acquire
>    page_table_lock, you obsreve the anon_vma store and all preceding stores
>    (due to spin_unlock providing RELEASE semantics, and spin_lock providing
>    ACQUIRE semantics)
> 2) say you install e.g a PUD entry, you take the page_table_lock. So you fully
>    observe the anon_vma that was installed (by doing an ACQUIRE on the lock).
>    you also issue a smp_wmb() which makes sure the ptdesc setup is visible.
> 3) others using that PUD entry will (should?) transitively observe everything
>    you have observed, data-dependent loads will help you there. If we _ever_
>    observe a page table without seeing an associated anon_vma, it's broken.
> 
> [Yes, I spent quite a bit of time thinking through this; it isn't trivial to prove
> that 2->3 transition is correct, but it looks vaguely _handwavely_ correct]

So, this race condition is safe just because the ordering (very briefly):

if `if (likely(vma->anon_vma))` is TRUE return 0 (OK)
if `if (likely(vma->anon_vma))` is fail __anon_vma_prepare() is called
6 lines below and inside __anon_vma_prepare() he try to get a lock
`spin_lock(&mm->page_table_lock);` that have ACQUIRE semantics and ensure the
ordering mapping...

if `if (likely(!vma->anon_vma))` (recheck the anon_vma like __vmf_anon_prepare)
fail then `spin_unlock(&mm->page_table_lock);` else alloc anon_mmap.
	
Thanks for spend time to explain this in detail for me Pedro.