Re: [PATCH] mm/userfaultfd: selftests: Fix memory corruption with thp enabled

[Peter Xu]

On Fri, Sep 24, 2021 at 10:21:30AM -0700, Axel Rasmussen wrote:
> On Thu, Sep 23, 2021 at 4:25 PM Peter Xu <peterx@xxxxxxxxxx> wrote:
> >
> > In RHEL's gating selftests we've encountered memory corruption in the uffd
> > event test even with upstream kernel:
> >
> >         # ./userfaultfd anon 128 4
> >         nr_pages: 32768, nr_pages_per_cpu: 32768
> >         bounces: 3, mode: rnd racing read, userfaults: 6240 missing (6240) 14729 wp (14729)
> >         bounces: 2, mode: racing read, userfaults: 1444 missing (1444) 28877 wp (28877)
> >         bounces: 1, mode: rnd read, userfaults: 6055 missing (6055) 14699 wp (14699)
> >         bounces: 0, mode: read, userfaults: 82 missing (82) 25196 wp (25196)
> >         testing uffd-wp with pagemap (pgsize=4096): done
> >         testing uffd-wp with pagemap (pgsize=2097152): done
> >         testing events (fork, remap, remove): ERROR: nr 32427 memory corruption 0 1 (errno=0, line=963)
> >         ERROR: faulting process failed (errno=0, line=1117)
> >
> > It can be easily reproduced when global thp enabled, which is the default for
> > RHEL.
> >
> > It's also known as a side effect of commit 0db282ba2c12 ("selftest: use mmap
> > instead of posix_memalign to allocate memory", 2021-07-23), which is imho right
> > itself on using mmap() to make sure the addresses will be untagged even on arm.
> >
> > The problem is, for each test we allocate buffers using two allocate_area()
> > calls.  We assumed these two buffers won't affect each other, however they
> > could, because mmap() could have found that the two buffers are near each other
> > and having the same VMA flags, so they got merged into one VMA.
> >
> > It won't be a big problem if thp is not enabled, but when thp is agressively
> > enabled it means when initializing the src buffer it could accidentally setup
> > part of the dest buffer too when there's a shared THP that overlaps the two
> > regions.  Then some of the dest buffer won't be able to be trapped by
> > userfaultfd missing mode, then it'll cause memory corruption as described.
> >
> > To fix it, do release_pages() after initializing the src buffer.
> 
> But, if I understand correctly, release_pages() will just free the
> physical pages, but not touch the VMA(s). So, with the right
> max_ptes_none setting, why couldn't khugepaged just decide to
> re-collapse (with zero pages) immediately after we release the pages,
> causing the same problem? It seems to me this change just
> significantly narrows the race window (which explains why we see less
> of the issue), but doesn't fix it fundamentally.

Did you mean you can reproduce the issue even with this patch?

It is a good point anyway, indeed I don't see anything stops it from happening.

I wanted to prepare a v2 by releasing the pages after uffdio registration where
we'll do the vma split, but it won't simply work because release_pages() will
cause the process to hang death since that test registers with EVENT_REMOVE,
and release_pages() upon the thp will trigger synchronous EVENT_REMOVE which
cannot be handled by anyone.

Another solution is to map some PROT_NONE regions between the buffers, to make
sure they won't share a VMA.  I'll need to think more about which is better..

-- 
Peter Xu