linux-kernel - Re: [mm-unstable PATCH v7 2/8] mm/hugetlb: make pud_huge() and follow_huge

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] [day] [month] [year] [list]

Message-ID: <20221106235216.GA426682@hori.linux.bs1.fc.nec.co.jp>
Date:   Sun, 6 Nov 2022 23:52:19 +0000
From:   HORIGUCHI NAOYA(堀口　直也) 
        <naoya.horiguchi@....com>
To:     Ville Syrjälä <ville.syrjala@...ux.intel.com>
CC:     Naoya Horiguchi <naoya.horiguchi@...ux.dev>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        David Hildenbrand <david@...hat.com>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Miaohe Lin <linmiaohe@...wei.com>,
        Liu Shixin <liushixin2@...wei.com>,
        Yang Shi <shy828301@...il.com>,
        Oscar Salvador <osalvador@...e.de>,
        Muchun Song <songmuchun@...edance.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
        "regressions@...ts.linux.dev" <regressions@...ts.linux.dev>
Subject: Re: [mm-unstable PATCH v7 2/8] mm/hugetlb: make pud_huge() and
 follow_huge_pud() aware of non-present pud entry

On Sat, Nov 05, 2022 at 12:23:40AM +0200, Ville Syrjälä wrote:
> On Sat, Nov 05, 2022 at 12:59:30AM +0900, Naoya Horiguchi wrote:
> > On Wed, Nov 02, 2022 at 10:51:40PM +0200, Ville Syrjälä wrote:
> > > On Thu, Jul 14, 2022 at 01:24:14PM +0900, Naoya Horiguchi wrote:
> > > > +/*
> > > > + * pud_huge() returns 1 if @pud is hugetlb related entry, that is normal
> > > > + * hugetlb entry or non-present (migration or hwpoisoned) hugetlb entry.
> > > > + * Otherwise, returns 0.
> > > > + */
> > > >  int pud_huge(pud_t pud)
> > > >  {
> > > > -	return !!(pud_val(pud) & _PAGE_PSE);
> > > > +	return !pud_none(pud) &&
> > > > +		(pud_val(pud) & (_PAGE_PRESENT|_PAGE_PSE)) != _PAGE_PRESENT;
> > > >  }
> > > 
> > > Hi,
> > > 
> > > This causes i915 to trip a BUG_ON() on x86-32 when I start X.
> > 
> > Hello,
> > 
> > Thank you for finding and reporting the issue.
> > 
> > x86-32 does not enable CONFIG_ARCH_HAS_GIGANTIC_PAGE, so pud_huge() is
> > supposed to be false on x86-32.  Doing like below looks to me a fix
> > (reverting to the original behavior for x86-32):
> > diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
> > index 6b3033845c6d..bf73f25aaa32 100644
> > --- a/arch/x86/mm/hugetlbpage.c
> > +++ b/arch/x86/mm/hugetlbpage.c
> > @@ -37,8 +37,12 @@ int pmd_huge(pmd_t pmd)
> >   */
> >  int pud_huge(pud_t pud)
> >  {
> > +#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
> >         return !pud_none(pud) &&
> >                 (pud_val(pud) & (_PAGE_PRESENT|_PAGE_PSE)) != _PAGE_PRESENT;
> > +#else
> > +       return !!(pud_val(pud) & _PAGE_PSE);    // or "return 0;" ?
> > +#endif
> >  }
> > 
> >  #ifdef CONFIG_HUGETLB_PAGE
> > 
> > 
> > Let me guess what the PUD entry was there when triggering the issue.
> > Assuming that the original code (before 3a194f3f8ad0) was correct, the PSE
> > bit in pud_val(pud) should be always cleared.  So, when pud_huge() returns
> > true since 3a194f3f8ad0, the PRESENT bit should be clear and some other
> > bits (rather than PRESENT and PSE) are set so that pud_none() is false.
> > I'm not sure what such a non-present PUD entry does mean.
> 
> pud_val()==0 when it blows up, and pud_none() is false because
> pgtable-nopmd.h says so with 2 level paging.
> 
> And given that I just tested with PAE / 3 level paging, 
> and sure enough it no longer blows up.
> 
> So looks to me like maybe this new code just doesn't understand
> how the levels get folded.

OK, so branching based on "#if CONFIG_PGTABLE_LEVELS > 2" seems better.
Thank you for additional testing.

> 
> I might also be missing something obvious, but why is it even
> necessary to treat PRESENT==0+PSE==0 as a huge entry?

The format of pud entry differs based on PRESENT bit, and PSE bit is
checked before PRESENT bit.  So in order to distinguish from a normal
huge entry, we had to define that a non-present huge entry should have
its PSE bit cleared (although this sounds counter-intuitive).

Thanks,
Naoya Horiguchi