linux-kernel - Re: [PATCH] hugetlb: simplify hugetlb handling in follow_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]

Date:   Tue, 30 Aug 2022 14:31:18 -0700
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        inuxppc-dev@...ts.ozlabs.org, linux-ia64@...r.kernel.org,
        Baolin Wang <baolin.wang@...ux.alibaba.com>,
        "Aneesh Kumar K . V" <aneesh.kumar@...ux.ibm.com>,
        Naoya Horiguchi <naoya.horiguchi@...ux.dev>,
        Michael Ellerman <mpe@...erman.id.au>,
        Muchun Song <songmuchun@...edance.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Christophe Leroy <christophe.leroy@...roup.eu>
Subject: Re: [PATCH] hugetlb: simplify hugetlb handling in follow_page_mask

On 08/30/22 09:52, Mike Kravetz wrote:
> On 08/30/22 10:11, David Hildenbrand wrote:
> > On 30.08.22 01:40, Mike Kravetz wrote:
> > > During discussions of this series [1], it was suggested that hugetlb
> > > handling code in follow_page_mask could be simplified.  At the beginning
> > 
> > Feel free to use a Suggested-by if you consider it appropriate.
> > 
> > > of follow_page_mask, there currently is a call to follow_huge_addr which
> > > 'may' handle hugetlb pages.  ia64 is the only architecture which provides
> > > a follow_huge_addr routine that does not return error.  Instead, at each
> > > level of the page table a check is made for a hugetlb entry.  If a hugetlb
> > > entry is found, a call to a routine associated with that entry is made.
> > > 
> > > Currently, there are two checks for hugetlb entries at each page table
> > > level.  The first check is of the form:
> > > 	if (p?d_huge())
> > > 		page = follow_huge_p?d();
> > > the second check is of the form:
> > > 	if (is_hugepd())
> > > 		page = follow_huge_pd().
> > 
> > BTW, what about all this hugepd stuff in mm/pagewalk.c?
> > 
> > Isn't this all dead code as we're essentially routing all hugetlb VMAs
> > via walk_hugetlb_range? [yes, all that hugepd stuff in generic code that
> > overcomplicates stuff has been annoying me for a long time]
> 
> I am 'happy' to look at cleaning up that code next.  Perhaps I will just
> create a cleanup series.
> 

Technically, that code is not dead IIUC.  The call to walk_hugetlb_range in
__walk_page_range is as follows:

	if (vma && is_vm_hugetlb_page(vma)) {
		if (ops->hugetlb_entry)
			err = walk_hugetlb_range(start, end, walk);
	} else
		err = walk_pgd_range(start, end, walk);

We also have the interface walk_page_range_novma() that will call
__walk_page_range without a value for vma.  So, in that case we would
end up calling walk_pgd_range, etc.  walk_pgd_range and related routines
do have those checks such as:

		if (is_hugepd(__hugepd(pmd_val(*pmd))))
			err = walk_hugepd_range((hugepd_t *)pmd, addr, next, walk, PMD_SHIFT);

So, it looks like in this case we would process 'hugepd' entries but not
'normal' hugetlb entries.  That does not seem right.

Christophe Leroy added this code with commit e17eae2b8399 "mm: pagewalk: fix
walk for hugepage tables".  This was part of the series "Convert powerpc to
GENERIC_PTDUMP".  And, the ptdump code uses the walk_page_range_novma
interface.  So, this code is certainly not dead.

Adding Christophe on Cc:

Christophe do you know if is_hugepd is true for all hugetlb entries, not
just hugepd?

On systems without hugepd entries, I guess ptdump skips all hugetlb entries.
Sigh!
-- 
Mike Kravetz