linux-kernel - Re: [PATCH v2] mm/hugetlb.c: make huge_pte_offset() consistent and document behaviour

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]

Date:   Wed, 30 Aug 2017 09:49:43 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Punit Agrawal <punit.agrawal@....com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
        Catalin Marinas <catalin.marinas@....com>,
        Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
        Steve Capper <steve.capper@....com>,
        Will Deacon <will.deacon@....com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Mike Kravetz <mike.kravetz@...cle.com>
Subject: Re: [PATCH v2] mm/hugetlb.c: make huge_pte_offset() consistent and
 document behaviour

On Fri 18-08-17 15:54:15, Punit Agrawal wrote:
> When walking the page tables to resolve an address that points to
> !p*d_present() entry, huge_pte_offset() returns inconsistent values
> depending on the level of page table (PUD or PMD).
> 
> It returns NULL in the case of a PUD entry while in the case of a PMD
> entry, it returns a pointer to the page table entry.
> 
> A similar inconsitency exists when handling swap entries - returns NULL
> for a PUD entry while a pointer to the pte_t is retured for the PMD entry.
> 
> Update huge_pte_offset() to make the behaviour consistent - return a
> pointer to the pte_t for hugepage or swap entries. Only return NULL in
> instances where we have a p*d_none() entry and the size parameter
> doesn't match the hugepage size at this level of the page table.
> 
> Document the behaviour to clarify the expected behaviour of this function.
> This is to set clear semantics for architecture specific implementations
> of huge_pte_offset().
> 
> Signed-off-by: Punit Agrawal <punit.agrawal@....com>
> Cc: Catalin Marinas <catalin.marinas@....com>
> Cc: Naoya Horiguchi <n-horiguchi@...jp.nec.com>
> Cc: Steve Capper <steve.capper@....com>
> Cc: Will Deacon <will.deacon@....com>
> Cc: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> Cc: Michal Hocko <mhocko@...e.com>
> Cc: Mike Kravetz <mike.kravetz@...cle.com>

I always thought that the weird semantic is a result of the hugetlb pte
sharing. But now that I dug into history it has been added by
02b0ccef903e ("[PATCH] hugetlb: check p?d_present in huge_pte_offset()")
for a completely different reason. I suspec the weird semantic just
wasn't noticed back then.

Anyway, I didn't find any problem with the patch
Acked-by: Michal Hocko <mhocko@...e.com>

> ---
> 
> Hi Andrew,
> 
> >From discussions on the arm64 implementation of huge_pte_offset()[0]
> we realised that there is benefit from returning a pte_t* in the case
> of p*d_none().
> 
> The fault handling code in hugetlb_fault() can handle p*d_none()
> entries and saves an extra round trip to huge_pte_alloc(). Other
> callers of huge_pte_offset() should be ok as well.
> 
> Apologies for sending a late update but I thought if we are defining
> the semantics, it's worth getting them right.
> 
> Could you please pick this version please?
> 
> Thanks,
> Punit
> 
> [0] http://www.spinics.net/lists/linux-mm/msg133699.html
> 
> v2: 
> 
>  mm/hugetlb.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 31e207cb399b..1d54a131bdd5 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4600,6 +4600,15 @@ pte_t *huge_pte_alloc(struct mm_struct *mm,
>  	return pte;
>  }
>  
> +/*
> + * huge_pte_offset() - Walk the page table to resolve the hugepage
> + * entry at address @addr
> + *
> + * Return: Pointer to page table or swap entry (PUD or PMD) for
> + * address @addr, or NULL if a p*d_none() entry is encountered and the
> + * size @sz doesn't match the hugepage size at this level of the page
> + * table.
> + */
>  pte_t *huge_pte_offset(struct mm_struct *mm,
>  		       unsigned long addr, unsigned long sz)
>  {
> @@ -4614,13 +4623,22 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>  	p4d = p4d_offset(pgd, addr);
>  	if (!p4d_present(*p4d))
>  		return NULL;
> +
>  	pud = pud_offset(p4d, addr);
> -	if (!pud_present(*pud))
> +	if (sz != PUD_SIZE && pud_none(*pud))
>  		return NULL;
> -	if (pud_huge(*pud))
> +	/* hugepage or swap? */
> +	if (pud_huge(*pud) || !pud_present(*pud))
>  		return (pte_t *)pud;
> +
>  	pmd = pmd_offset(pud, addr);
> -	return (pte_t *) pmd;
> +	if (sz != PMD_SIZE && pmd_none(*pmd))
> +		return NULL;
> +	/* hugepage or swap? */
> +	if (pmd_huge(*pmd) || !pmd_present(*pmd))
> +		return (pte_t *)pmd;
> +
> +	return NULL;
>  }
>  
>  #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
> -- 
> 2.13.2
> 

-- 
Michal Hocko
SUSE Labs