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

[Guilherme Giacomo Simoes]

Matthew Wilcox <willy@xxxxxxxxxxxxx> wrotes:
> > >> Fixes: 164b06f238b9 ("mm: call wp_page_copy() under the VMA lock")
> > >
> > > what makes you think this is the right commit for fixes?
> > Maybe I would should analyzed this better. I only seed the commit that introduce
> > this function (and consequently this reader)
> 
> That was what I thought, but it's not enough to determine if that's the
> start of the problem.  Look, that commit does:
> 
> -       if (unlikely(anon_vma_prepare(vma)))
> -               goto oom;
> +       ret = vmf_anon_prepare(vmf);
> +       if (unlikely(ret))
> +               goto out;
> 
> ... and anon_vma_prepare() does:
> 
>         if (likely(vma->anon_vma))
>                 return 0;
> 
> so either this race was already present in 164b06f238b9 (and you need to
> go back further) or it was actually introduced later (maybe the write
> side was introduced later?)

This commit introduce the vmf_anon_prepare():
+static vm_fault_t vmf_anon_prepare(struct vm_fault *vmf)
+{
+       struct vm_area_struct *vma = vmf->vma;
+
+       if (likely(vma->anon_vma))
+               return 0;
+       if (vmf->flags & FAULT_FLAG_VMA_LOCK) {
+               vma_end_read(vma);
+               return VM_FAULT_RETRY;
+       }
+       if (__anon_vma_prepare(vma))
+               return VM_FAULT_OOM;
+       return 0;
+}

in commit 2a058ab3286d (mm: change vmf_anon_prepare() to __vmf_anon_prepare())
the vmf_anon_prepare became __vmf_anon_prepare.
Where the race problem occours `if (likely(vma->anon_vma))`...
This commit 164b06f238b9 is introduced in 2023.

The write side is introduce in commit d5a187daf585 (mm, rmap: handle
anon_vma_prepare() common case inline) in 2016.

> The important thing to know is that the mmap_lock is a read-write lock.
> That means that two readers can be present at the same time.  So this race
> can happen when both threads hold the mmap_lock.  I don't know whether
> they do in the syzbot reproducer; probably not, but it doesn't matter.
> 
> The other important thing is that _we don't care_ what the value of
> vma->anon_vma is.  We only care whether it's NULL or not (this is a
> sufficiently common case that I wonder whether KCSAN shouldn't special-case
> it and decline to monitor it ...)  VMAs are created with a NULL anon_vma,
> and then if needed, anon_vma is set.  Once set, it is never changed (uhh
> ... at least I don't think it is.  Lorenzo, could you check me on this?
> I think all the places where we set vma->anon_vma to NULL are in
> situations where the VMA is not yet exposed to the page fault handler,
> like in the child side of fork()).
But if I have a write in the same time, this can be a problem, even though if
you only want to know if vma->anon_vma is NULL or not.

> 
> 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.) 

> 
> Also, as Lance said, I don't see how this is related to huge_page_alloc
> failing.  All I see is two threads calling  __vmf_anon_prepare() at the
> same time, which I presume is an attempt to COW a hugetlb page.
> 
> I don't think it's enough to just add a data_race() to this one read of
> vma->anon_vma.  I think it's quite prevalent.  There's probably other
> syzbot reports that mention it.
Yeah, I mentioned this on commit message because how is said by KCSAN the
__anon_vma_prepare() and __vmf_anon_prepare() is called after huge fault... My
interpretation might be wrong and if so, i will fix the commit message without
problem. 

Thanks Matthew and Lance for reviewing my patch and sharing your thoughts,