Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210513234309.366727-1-almasrymina@google.com>
 <CAHS8izNkBvS9gkSjy8FbWBOPDynwr8PXXXbMHt_2=5sZJsa6-Q@mail.gmail.com>
 <09dc0712-48e8-8ba2-f170-4c2febcfff83@oracle.com> <CAHS8izPFc+bSrKN-6gRguGefAqrj6kXaMUvgeUL5U7QxhXfWDw@mail.gmail.com>
 <f9c85756-62e1-3d5c-9fbc-f38c6e8f07f3@oracle.com> <YJ5tjWKyVZk2mvxo@t490s> <dd83c970-797e-6b7a-194d-790df5d53867@oracle.com>
In-Reply-To: <dd83c970-797e-6b7a-194d-790df5d53867@oracle.com>
From:   Axel Rasmussen <axelrasmussen@google.com>
Date:   Fri, 14 May 2021 11:30:39 -0700
Message-ID: <CAJHvVciTfZLgDCzsSdAf5FC5R5u4Og8Y72c4t0D53ACmJSdtRg@mail.gmail.com>
Subject: Re: [PATCH] mm, hugetlb: fix resv_huge_pages underflow on UFFDIO_COPY
To:     Mike Kravetz <mike.kravetz@oracle.com>
Cc:     Peter Xu <peterx@redhat.com>,
        Mina Almasry <almasrymina@google.com>,
        Linux-MM <linux-mm@kvack.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

It's complicated and would take some more time for me to be certain,
but after looking for half an hour or so this morning, I agree with
Mike that such a race is possible.

That is, we may back out into the retry path, and drop mmap_lock, and
leave a situation where a page is in the cache, but we have
!PageUptodate(). hugetlb_mcopy_atomic_pte clearly handles the
VM_SHARED case, so I don't see a reason why there can't be another
(non-userfaultfd-registered) mapping. If it were faulted at the right
time, it seems like such a fault would indeed zero the page, and then
the UFFDIO_COPY retry (once it acquired the lock again) would try to
reuse it.

On Fri, May 14, 2021 at 10:56 AM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> On 5/14/21 5:31 AM, Peter Xu wrote:
> > Hi, Mike,
> >
> > On Thu, May 13, 2021 at 09:02:15PM -0700, Mike Kravetz wrote:
> >
> > [...]
> >
> >> I am also concerned with the semantics of this approach and what happens
> >> when a fault races with the userfaultfd copy.  Previously I asked Peter
> >> if we could/should use a page found in the cache for the copy.  His
> >> answer was as follows:
> >>
> >>  AFAICT that's the expected behavior, and it need to be like that so as to avoid
> >>  silent data corruption (if the page cache existed, it means the page is not
> >>  "missing" at all, then it does not suite for a UFFDIO_COPY as it's only used
> >>  for uffd page missing case).
> >
> > I didn't follow the rest discussion in depth yet... but just to mention that
> > the above answer was for the question whether we can "update the page in the
> > page cache", rather than "use a page found in the page cache".
> >
> > I think reuse the page should be fine, however it'll definitely break existing
> > user interface (as it'll expect -EEXIST for now - we have kselftest covers
> > that), meanwhile I don't see why the -EEXIST bothers a lot: it still tells the
> > user that this page was filled in already.  Normally it was filled in by
> > another UFFDIO_COPY (as we could have multiple uffd service threads) along with
> > a valid pte, then this userspace thread can simply skip this message as it
> > means the event has been handled by some other servicing thread.
> >
> > (This also reminded me that there won't be a chance of UFFDIO_COPY race on page
> >  no page fault at least, since no page fault will always go into the uffd
> >  missing handling rather than filling in the page cache for a VM_UFFD_MISSING
> >  vma; while mmap read lock should guarantee VM_UFFD_MISSING be persistent)
>
> Perhaps I am missing something.
>
> Since this is a shared mapping, can we not have a 'regular' mapping to
> the same range that is uffd registered?  And, that regular mappings could
> fault and race with the uffd copy code?
>
> --
> Mike Kravetz