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

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(-)

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;
>  }

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

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;
>  }