linux-kernel - Re: [PATCH v2 17/46] hugetlbfs: do a full walk to check if vma maps a page

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: <Y/6Ts386188x5Lm/@monkey>
Date:   Tue, 28 Feb 2023 15:52:19 -0800
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     James Houghton <jthoughton@...gle.com>
Cc:     Muchun Song <songmuchun@...edance.com>,
        Peter Xu <peterx@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        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>,
        Frank van der Linden <fvdl@...gle.com>,
        Jiaqi Yan <jiaqiyan@...gle.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 17/46] hugetlbfs: do a full walk to check if vma maps
 a page

On 02/22/23 07:46, James Houghton wrote:
> On Fri, Feb 17, 2023 at 4:29 PM James Houghton <jthoughton@...gle.com> wrote:
> >
> > Because it is safe to do so, do a full high-granularity page table walk
> > to check if the page is mapped.
> >
> > Signed-off-by: James Houghton <jthoughton@...gle.com>
> >
> > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> > index cfd09f95551b..c0ee69f0418e 100644
> > --- a/fs/hugetlbfs/inode.c
> > +++ b/fs/hugetlbfs/inode.c
> > @@ -386,17 +386,24 @@ static void hugetlb_delete_from_page_cache(struct folio *folio)
> >  static bool hugetlb_vma_maps_page(struct vm_area_struct *vma,
> >                                 unsigned long addr, struct page *page)
> >  {
> > -       pte_t *ptep, pte;
> > +       pte_t pte;
> > +       struct hugetlb_pte hpte;
> >
> > -       ptep = hugetlb_walk(vma, addr, huge_page_size(hstate_vma(vma)));
> > -       if (!ptep)
> > +       if (hugetlb_full_walk(&hpte, vma, addr))
> >                 return false;
> >
> > -       pte = huge_ptep_get(ptep);
> > +       pte = huge_ptep_get(hpte.ptep);
> >         if (huge_pte_none(pte) || !pte_present(pte))
> >                 return false;
> >
> > -       if (pte_page(pte) == page)
> > +       if (unlikely(!hugetlb_pte_present_leaf(&hpte, pte)))
> > +               /*
> > +                * We raced with someone splitting us, and the only case
> > +                * where this is impossible is when the pte was none.
> > +                */
> > +               return false;
> > +
> > +       if (compound_head(pte_page(pte)) == page)
> >                 return true;
> >
> >         return false;
> > --
> > 2.39.2.637.g21b0678d19-goog
> >
> 
> I think this patch is actually incorrect.
> 
> This function is *supposed* to check if the page is mapped at all in
> this VMA, but really we're only checking if the base address of the
> page is mapped.

The function is/was only checking if the page is mapped at the specific
address.  That is because when walking the interval tree, we know where
it would be mapped and only check there.

I suppose it would still be functionally correct if we checked for the
page being mapped anywhere in the vma.

>                 If we did the 'hugetlb_vma_maybe_maps_page' approach
> that I did previously and returned 'true' if
> !hugetlb_pte_present_leaf(), then this code would be correct again.
> 
> But what I really think this function should do is just call
> page_vma_mapped_walk(). We're sort of reimplementing it here anyway.
> Unless someone disagrees, I'll do this for v3.

Yes, I think page_vma_mapped_walk would provide the same functionality.
I did not consider this when writing hugetlb_vma_maps_page, and
hugetlb_vma_maps_page was pretty simple for the current hugetlb
possibilities.  Things get more complicated with HGM.
-- 
Mike Kravetz