[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y5JrS4o5Detzid9V@monkey>
Date: Thu, 8 Dec 2022 14:55:07 -0800
From: Mike Kravetz <mike.kravetz@...cle.com>
To: James Houghton <jthoughton@...gle.com>
Cc: Peter Xu <peterx@...hat.com>,
Muchun Song <songmuchun@...edance.com>,
David Hildenbrand <david@...hat.com>,
David Rientjes <rientjes@...gle.com>,
Axel Rasmussen <axelrasmussen@...gle.com>,
Mina Almasry <almasrymina@...gle.com>,
Zach O'Keefe <zokeefe@...gle.com>,
Manish Mishra <manish.mishra@...anix.com>,
Naoya Horiguchi <naoya.horiguchi@....com>,
"Dr . David Alan Gilbert" <dgilbert@...hat.com>,
"Matthew Wilcox (Oracle)" <willy@...radead.org>,
Vlastimil Babka <vbabka@...e.cz>,
Baolin Wang <baolin.wang@...ux.alibaba.com>,
Miaohe Lin <linmiaohe@...wei.com>,
Yang Shi <shy828301@...il.com>,
Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2 01/47] hugetlb: don't set PageUptodate for
UFFDIO_CONTINUE
On 11/21/22 10:33, James Houghton wrote:
> On Wed, Nov 16, 2022 at 8:30 AM Peter Xu <peterx@...hat.com> wrote:
> >
> > On Fri, Oct 21, 2022 at 04:36:17PM +0000, James Houghton wrote:
> > > This is how it should have been to begin with. It would be very bad if
> > > we actually set PageUptodate with a UFFDIO_CONTINUE, as UFFDIO_CONTINUE
> > > doesn't actually set/update the contents of the page, so we would be
> > > exposing a non-zeroed page to the user.
> > >
> > > The reason this change is being made now is because UFFDIO_CONTINUEs on
> > > subpages definitely shouldn't set this page flag on the head page.
> > >
> > > Signed-off-by: James Houghton <jthoughton@...gle.com>
> > > ---
> > > mm/hugetlb.c | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > > index 1a7dc7b2e16c..650761cdd2f6 100644
> > > --- a/mm/hugetlb.c
> > > +++ b/mm/hugetlb.c
> > > @@ -6097,7 +6097,10 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> > > * preceding stores to the page contents become visible before
> > > * the set_pte_at() write.
> > > */
> > > - __SetPageUptodate(page);
> > > + if (!is_continue)
> > > + __SetPageUptodate(page);
> > > + else
> > > + VM_WARN_ON_ONCE_PAGE(!PageUptodate(page), page);
> >
> > Yeah the old code looks wrong, I'm just wondering whether we can 100%
> > guarantee this for hugetlb. E.g. for shmem that won't hold when we
> > uffd-continue on a not used page (e.g. by an over-sized fallocate()).
> >
> > Another safer approach is simply fail the ioctl if !uptodate, but if you're
> > certain then WARN_ON_ONCE sounds all good too. At least I did have a quick
> > look on hugetlb fallocate() and pages will be uptodate immediately.
>
> Failing the ioctl sounds better than only WARNing. I'll do that and
> drop the WARN_ON_ONCE for v1. Thanks!
>
Sorry for the VERY late reply ...
After checking all the code paths, I do not think it is possible for a
!PageUptodate to be in the cache (target of continue).
ACK to failing the ioctl if not set, although I don't think it is possible
in current code.
--
Mike Kravetz
Powered by blists - more mailing lists