linux-kernel - Re: [RFC PATCH v2 01/47] hugetlb: don't set PageUptodate for UFFDIO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADrL8HXXT+7BBMj3kgR8g=z4RZv0q-AF5ykkg1vXF9aYCWg9MA@mail.gmail.com>
Date:   Mon, 21 Nov 2022 10:33:45 -0800
From:   James Houghton <jthoughton@...gle.com>
To:     Peter Xu <peterx@...hat.com>
Cc:     Mike Kravetz <mike.kravetz@...cle.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 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!

- James

>
> >
> >       /* Add shared, newly allocated pages to the page cache. */
> >       if (vm_shared && !is_continue) {
> > --
> > 2.38.0.135.g90850a2211-goog
> >
> >
>
> --
> Peter Xu
>