[RFC PATCH] powerpc/mm: Return NULL for not present hugetlb page

[RFC PATCH] powerpc/mm: Return NULL for not present hugetlb page

[Aneesh Kumar K.V]

We need to check whether pte is present in follow_huge_addr and
properly return NULL if mapping is not present. Also use READ_ONCE
when dereferencing pte_t address.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 arch/powerpc/mm/hugetlbpage.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index 0ce968b00b7c..f5688423bc69 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c
@@ -689,27 +689,34 @@ void hugetlb_free_pgd_range(struct mmu_gather *tlb,
 struct page *
 follow_huge_addr(struct mm_struct *mm, unsigned long address, int write)
 {
-	pte_t *ptep;
-	struct page *page;
+	pte_t *ptep, pte;
 	unsigned shift;
 	unsigned long mask, flags;
+	struct page *page = ERR_PTR(-EINVAL);
+
+	local_irq_save(flags);
+	ptep = find_linux_pte_or_hugepte(mm->pgd, address, &shift);
+	if (!ptep)
+		goto no_page;
+	pte = READ_ONCE(*ptep);
 	/*
+	 * Verify it is a huge page else bail.
 	 * Transparent hugepages are handled by generic code. We can skip them
 	 * here.
 	 */
-	local_irq_save(flags);
-	ptep = find_linux_pte_or_hugepte(mm->pgd, address, &shift);
+	if (!shift || pmd_trans_huge((pmd_t)pte))
+		goto no_page;
 
-	/* Verify it is a huge page else bail. */
-	if (!ptep || !shift || pmd_trans_huge(*(pmd_t *)ptep)) {
-		local_irq_restore(flags);
-		return ERR_PTR(-EINVAL);
+	if (!pte_present(pte)) {
+		page = NULL;
+		goto no_page;
 	}
 	mask = (1UL << shift) - 1;
-	page = pte_page(*ptep);
+	page = pte_page(pte);
 	if (page)
 		page += (address & mask) / PAGE_SIZE;
 
+no_page:
 	local_irq_restore(flags);
 	return page;
 }
-- 
2.1.4

Re: [RFC PATCH] powerpc/mm: Return NULL for not present hugetlb page

[Benjamin Herrenschmidt]

On Thu, 2015-05-07 at 12:46 +0530, Aneesh Kumar K.V wrote:
> We need to check whether pte is present in follow_huge_addr and
> properly return NULL if mapping is not present. Also use READ_ONCE
> when dereferencing pte_t address.

Do that need to go to stable as well ?

> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
>  arch/powerpc/mm/hugetlbpage.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
> index 0ce968b00b7c..f5688423bc69 100644
> --- a/arch/powerpc/mm/hugetlbpage.c
> +++ b/arch/powerpc/mm/hugetlbpage.c
> @@ -689,27 +689,34 @@ void hugetlb_free_pgd_range(struct mmu_gather *tlb,
>  struct page *
>  follow_huge_addr(struct mm_struct *mm, unsigned long address, int write)
>  {
> -	pte_t *ptep;
> -	struct page *page;
> +	pte_t *ptep, pte;
>  	unsigned shift;
>  	unsigned long mask, flags;
> +	struct page *page = ERR_PTR(-EINVAL);
> +
> +	local_irq_save(flags);
> +	ptep = find_linux_pte_or_hugepte(mm->pgd, address, &shift);
> +	if (!ptep)
> +		goto no_page;
> +	pte = READ_ONCE(*ptep);
>  	/*
> +	 * Verify it is a huge page else bail.
>  	 * Transparent hugepages are handled by generic code. We can skip them
>  	 * here.
>  	 */
> -	local_irq_save(flags);
> -	ptep = find_linux_pte_or_hugepte(mm->pgd, address, &shift);
> +	if (!shift || pmd_trans_huge((pmd_t)pte))
> +		goto no_page;
>  
> -	/* Verify it is a huge page else bail. */
> -	if (!ptep || !shift || pmd_trans_huge(*(pmd_t *)ptep)) {
> -		local_irq_restore(flags);
> -		return ERR_PTR(-EINVAL);
> +	if (!pte_present(pte)) {
> +		page = NULL;
> +		goto no_page;
>  	}
>  	mask = (1UL << shift) - 1;
> -	page = pte_page(*ptep);
> +	page = pte_page(pte);
>  	if (page)
>  		page += (address & mask) / PAGE_SIZE;
>  
> +no_page:
>  	local_irq_restore(flags);
>  	return page;
>  }

Re: [RFC PATCH] powerpc/mm: Return NULL for not present hugetlb page

[Aneesh Kumar K.V]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> On Thu, 2015-05-07 at 12:46 +0530, Aneesh Kumar K.V wrote:
>> We need to check whether pte is present in follow_huge_addr and
>> properly return NULL if mapping is not present. Also use READ_ONCE
>> when dereferencing pte_t address.
>
> Do that need to go to stable as well ?

Yes. I will like David to take a look at this and give his feedback.
W.r.t patch itself I hit a build failure on mpc85xx_smp_defconfig. I will
resent after the test build finish on all configs

-aneesh

Re: [RFC PATCH] powerpc/mm: Return NULL for not present hugetlb page

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2365 bytes --]

On Thu, May 07, 2015 at 12:46:21PM +0530, Aneesh Kumar K.V wrote:
> We need to check whether pte is present in follow_huge_addr and
> properly return NULL if mapping is not present. Also use READ_ONCE
> when dereferencing pte_t address.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Looks sane.  It's a long time since I worked with this so I don't
really remember, but I have a suspicion that at the time hugepage PTEs
could never exist but be non-present.

> ---
>  arch/powerpc/mm/hugetlbpage.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
> index 0ce968b00b7c..f5688423bc69 100644
> --- a/arch/powerpc/mm/hugetlbpage.c
> +++ b/arch/powerpc/mm/hugetlbpage.c
> @@ -689,27 +689,34 @@ void hugetlb_free_pgd_range(struct mmu_gather *tlb,
>  struct page *
>  follow_huge_addr(struct mm_struct *mm, unsigned long address, int write)
>  {
> -	pte_t *ptep;
> -	struct page *page;
> +	pte_t *ptep, pte;
>  	unsigned shift;
>  	unsigned long mask, flags;
> +	struct page *page = ERR_PTR(-EINVAL);
> +
> +	local_irq_save(flags);
> +	ptep = find_linux_pte_or_hugepte(mm->pgd, address, &shift);
> +	if (!ptep)
> +		goto no_page;
> +	pte = READ_ONCE(*ptep);
>  	/*
> +	 * Verify it is a huge page else bail.
>  	 * Transparent hugepages are handled by generic code. We can skip them
>  	 * here.
>  	 */
> -	local_irq_save(flags);
> -	ptep = find_linux_pte_or_hugepte(mm->pgd, address, &shift);
> +	if (!shift || pmd_trans_huge((pmd_t)pte))
> +		goto no_page;
>  
> -	/* Verify it is a huge page else bail. */
> -	if (!ptep || !shift || pmd_trans_huge(*(pmd_t *)ptep)) {
> -		local_irq_restore(flags);
> -		return ERR_PTR(-EINVAL);
> +	if (!pte_present(pte)) {
> +		page = NULL;
> +		goto no_page;
>  	}
>  	mask = (1UL << shift) - 1;
> -	page = pte_page(*ptep);
> +	page = pte_page(pte);
>  	if (page)
>  		page += (address & mask) / PAGE_SIZE;
>  
> +no_page:
>  	local_irq_restore(flags);
>  	return page;
>  }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]