[PATCH 1/3] arch/powerpc/hugetlb: Use pte_access_permitted for hugetlb access check

[PATCH 1/3] arch/powerpc/hugetlb: Use pte_access_permitted for hugetlb access check

[Aneesh Kumar K.V]

No functional change in this patch. This update gup_hugepte to use the
helper. This will help later when we add memory keys.

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

diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index a9b9083c5e49..c7e5afe5e118 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c
@@ -855,9 +855,7 @@ int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,
 
 	pte = READ_ONCE(*ptep);
 
-	if (!pte_present(pte) || !pte_read(pte))
-		return 0;
-	if (write && !pte_write(pte))
+	if (!pte_access_permitted(pte, write))
 		return 0;
 
 	/* hugepages are never "special" */
-- 
2.14.3

[PATCH 2/3] powerpc/mm/book3s/64: Add proper pte access check helper

[Aneesh Kumar K.V]

pte_access_premitted get called in get_user_pages_fast path. If we have marked
the pte PROT_NONE, we should not allow a read access on the address. With
the current implementation we are not checking the READ and only check for
WRITE. This is needed on archs like ppc64 that implement PROT_NONE using
RWX access instead of _PAGE_PRESENT. Also add pte_user check just to make sure
we are not accessing kernel mapping.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/book3s/64/pgtable.h | 41 ++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 2006fec40a00..efa5bd6bc1d3 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -520,6 +520,30 @@ static inline int pte_present(pte_t pte)
 {
 	return !!(pte_raw(pte) & cpu_to_be64(_PAGE_PRESENT));
 }
+
+#define pte_access_permitted pte_access_permitted
+static inline bool pte_access_permitted(pte_t pte, bool write)
+{
+	unsigned long pteval = pte_val(pte);
+	/* Also check for pte_user */
+	unsigned long clear_pte_bits = _PAGE_PRIVILEGED;
+	/*
+	 * _PAGE_READ is needed for any access and will be
+	 * cleared for PROT_NONE
+	 */
+	unsigned long need_pte_bits = _PAGE_PRESENT | _PAGE_READ;
+
+	if (write)
+		need_pte_bits |= _PAGE_WRITE;
+
+	if ((pteval & need_pte_bits) != need_pte_bits)
+		return false;
+
+	if ((pteval & clear_pte_bits) == clear_pte_bits)
+		return false;
+	return true;
+}
+
 /*
  * Conversion functions: convert a page and protection to a page entry,
  * and a page entry and page directory to the page they refer to.
@@ -824,6 +848,11 @@ static inline int pud_bad(pud_t pud)
 	return hash__pud_bad(pud);
 }
 
+#define pud_access_permitted pud_access_permitted
+static inline bool pud_access_permitted(pud_t pud, bool write)
+{
+	return pte_access_permitted(pud_pte(pud), write);
+}
 
 #define pgd_write(pgd)		pte_write(pgd_pte(pgd))
 static inline void pgd_set(pgd_t *pgdp, unsigned long val)
@@ -863,6 +892,12 @@ static inline int pgd_bad(pgd_t pgd)
 	return hash__pgd_bad(pgd);
 }
 
+#define pgd_access_permitted pgd_access_permitted
+static inline bool pgd_access_permitted(pgd_t pgd, bool write)
+{
+	return pte_access_permitted(pgd_pte(pgd), write);
+}
+
 extern struct page *pgd_page(pgd_t pgd);
 
 /* Pointers in the page table tree are physical addresses */
@@ -984,6 +1019,12 @@ static inline int pmd_protnone(pmd_t pmd)
 #define __pmd_write(pmd)	__pte_write(pmd_pte(pmd))
 #define pmd_savedwrite(pmd)	pte_savedwrite(pmd_pte(pmd))
 
+#define pmd_access_permitted pmd_access_permitted
+static inline bool pmd_access_permitted(pmd_t pmd, bool write)
+{
+	return pte_access_permitted(pmd_pte(pmd), write);
+}
+
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 extern pmd_t pfn_pmd(unsigned long pfn, pgprot_t pgprot);
 extern pmd_t mk_pmd(struct page *page, pgprot_t pgprot);
-- 
2.14.3

[PATCH 3/3] powerpc/mm/ Add proper pte access check helper

[Aneesh Kumar K.V]

pte_access_premitted get called in get_user_pages_fast path. If we have marked
the pte PROT_NONE, we should not allow a read access on the address. With
the current implementation we are not checking the READ and only check for
WRITE. This is needed on archs like ppc64 that implement PROT_NONE using
_PAGE_USER access instead of _PAGE_PRESENT. Also add pte_user check just to make sure
we are not accessing kernel mapping.

Even though there is code duplication, keeping the low level pte accessors
different for different platforms helps in code readability.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/book3s/32/pgtable.h | 23 +++++++++++++++++++++++
 arch/powerpc/include/asm/nohash/pgtable.h    | 23 +++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
index 016579ef16d3..30a155c0a6b0 100644
--- a/arch/powerpc/include/asm/book3s/32/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
@@ -311,6 +311,29 @@ static inline int pte_present(pte_t pte)
 	return pte_val(pte) & _PAGE_PRESENT;
 }
 
+/*
+ * We only find page table entry in the last level
+ * Hence no need for other accessors
+ */
+#define pte_access_permitted pte_access_permitted
+static inline bool pte_access_permitted(pte_t pte, bool write)
+{
+	unsigned long pteval = pte_val(pte);
+	/*
+	 * A read-only access is controlled by _PAGE_USER bit.
+	 * We have _PAGE_READ set for WRITE and EXECUTE
+	 */
+	unsigned long need_pte_bits = _PAGE_PRESENT | _PAGE_USER;
+
+	if (write)
+		need_pte_bits |= _PAGE_WRITE;
+
+	if ((pteval & need_pte_bits) != need_pte_bits)
+		return false;
+
+	return true;
+}
+
 /* Conversion functions: convert a page and protection to a page entry,
  * and a page entry and page directory to the page they refer to.
  *
diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
index 5c68f4a59f75..fc4376c8d444 100644
--- a/arch/powerpc/include/asm/nohash/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/pgtable.h
@@ -45,6 +45,29 @@ static inline int pte_present(pte_t pte)
 	return pte_val(pte) & _PAGE_PRESENT;
 }
 
+/*
+ * We only find page table entry in the last level
+ * Hence no need for other accessors
+ */
+#define pte_access_permitted pte_access_permitted
+static inline bool pte_access_permitted(pte_t pte, bool write)
+{
+	unsigned long pteval = pte_val(pte);
+	/*
+	 * A read-only access is controlled by _PAGE_USER bit.
+	 * We have _PAGE_READ set for WRITE and EXECUTE
+	 */
+	unsigned long need_pte_bits = _PAGE_PRESENT | _PAGE_USER;
+
+	if (write)
+		need_pte_bits |= _PAGE_WRITE;
+
+	if ((pteval & need_pte_bits) != need_pte_bits)
+		return false;
+
+	return true;
+}
+
 /* Conversion functions: convert a page and protection to a page entry,
  * and a page entry and page directory to the page they refer to.
  *
-- 
2.14.3

Re: [PATCH 3/3] powerpc/mm/ Add proper pte access check helper

[Christophe LEROY]

Le 04/12/2017 à 03:19, Aneesh Kumar K.V a écrit :
> pte_access_premitted get called in get_user_pages_fast path. If we have marked
> the pte PROT_NONE, we should not allow a read access on the address. With
> the current implementation we are not checking the READ and only check for
> WRITE. This is needed on archs like ppc64 that implement PROT_NONE using
> _PAGE_USER access instead of _PAGE_PRESENT. Also add pte_user check just to make sure
> we are not accessing kernel mapping.
> 
> Even though there is code duplication, keeping the low level pte accessors
> different for different platforms helps in code readability.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
>   arch/powerpc/include/asm/book3s/32/pgtable.h | 23 +++++++++++++++++++++++
>   arch/powerpc/include/asm/nohash/pgtable.h    | 23 +++++++++++++++++++++++
>   2 files changed, 46 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
> index 016579ef16d3..30a155c0a6b0 100644
> --- a/arch/powerpc/include/asm/book3s/32/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
> @@ -311,6 +311,29 @@ static inline int pte_present(pte_t pte)
>   	return pte_val(pte) & _PAGE_PRESENT;
>   }
>   
> +/*
> + * We only find page table entry in the last level
> + * Hence no need for other accessors
> + */
> +#define pte_access_permitted pte_access_permitted
> +static inline bool pte_access_permitted(pte_t pte, bool write)
> +{
> +	unsigned long pteval = pte_val(pte);
> +	/*
> +	 * A read-only access is controlled by _PAGE_USER bit.
> +	 * We have _PAGE_READ set for WRITE and EXECUTE
> +	 */
> +	unsigned long need_pte_bits = _PAGE_PRESENT | _PAGE_USER;
> +
> +	if (write)
> +		need_pte_bits |= _PAGE_WRITE;
> +
> +	if ((pteval & need_pte_bits) != need_pte_bits)
> +		return false;
> +
> +	return true;
> +}
> +
>   /* Conversion functions: convert a page and protection to a page entry,
>    * and a page entry and page directory to the page they refer to.
>    *
> diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
> index 5c68f4a59f75..fc4376c8d444 100644
> --- a/arch/powerpc/include/asm/nohash/pgtable.h
> +++ b/arch/powerpc/include/asm/nohash/pgtable.h
> @@ -45,6 +45,29 @@ static inline int pte_present(pte_t pte)
>   	return pte_val(pte) & _PAGE_PRESENT;
>   }
>   
> +/*
> + * We only find page table entry in the last level
> + * Hence no need for other accessors
> + */
> +#define pte_access_permitted pte_access_permitted
> +static inline bool pte_access_permitted(pte_t pte, bool write)
> +{
> +	unsigned long pteval = pte_val(pte);
> +	/*
> +	 * A read-only access is controlled by _PAGE_USER bit.
> +	 * We have _PAGE_READ set for WRITE and EXECUTE
> +	 */

Not fully right. asm/pte-common.h defines:

#define PAGE_NONE	__pgprot(_PAGE_BASE | _PAGE_NA)
#define PAGE_SHARED	__pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RW)
#define PAGE_SHARED_X	__pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RW | \
				 _PAGE_EXEC)
#define PAGE_COPY	__pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RO)
#define PAGE_COPY_X	__pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RO | \
				 _PAGE_EXEC)
#define PAGE_READONLY	__pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RO)
#define PAGE_READONLY_X	__pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RO | \
				 _PAGE_EXEC)

On the 8xx, _PAGE_USER = 0

> +	unsigned long need_pte_bits = _PAGE_PRESENT | _PAGE_USER;
> +
> +	if (write)
> +		need_pte_bits |= _PAGE_WRITE;
> +
> +	if ((pteval & need_pte_bits) != need_pte_bits)
> +		return false;

This test is not fully correct:
- To check access(read) permission, you also have to check that _PAGE_NA 
is not set.
- To check write permission, you also have to check that neither 
_PAGE_NA nor _PAGE_RO are set.

On the 8xx, you have:
_PAGE_RW = _PAGE_WRITE = 0
_PAGE_NA = 0x0200
_PAGE_RO = 0x0600

Christophe

> +
> +	return true;
> +}
> +
>   /* Conversion functions: convert a page and protection to a page entry,
>    * and a page entry and page directory to the page they refer to.
>    *
> 

Re: [PATCH 3/3] powerpc/mm/ Add proper pte access check helper

[Christophe LEROY]

Le 17/08/2018 à 17:12, Christophe LEROY a écrit :
> 
> 
> Le 04/12/2017 à 03:19, Aneesh Kumar K.V a écrit :
>> pte_access_premitted get called in get_user_pages_fast path. If we 
>> have marked
>> the pte PROT_NONE, we should not allow a read access on the address. With
>> the current implementation we are not checking the READ and only check 
>> for
>> WRITE. This is needed on archs like ppc64 that implement PROT_NONE using
>> _PAGE_USER access instead of _PAGE_PRESENT. Also add pte_user check 
>> just to make sure
>> we are not accessing kernel mapping.
>>
>> Even though there is code duplication, keeping the low level pte 
>> accessors
>> different for different platforms helps in code readability.
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/include/asm/book3s/32/pgtable.h | 23 
>> +++++++++++++++++++++++
>>   arch/powerpc/include/asm/nohash/pgtable.h    | 23 
>> +++++++++++++++++++++++
>>   2 files changed, 46 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h 
>> b/arch/powerpc/include/asm/book3s/32/pgtable.h
>> index 016579ef16d3..30a155c0a6b0 100644
>> --- a/arch/powerpc/include/asm/book3s/32/pgtable.h
>> +++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
>> @@ -311,6 +311,29 @@ static inline int pte_present(pte_t pte)
>>       return pte_val(pte) & _PAGE_PRESENT;
>>   }
>> +/*
>> + * We only find page table entry in the last level
>> + * Hence no need for other accessors
>> + */
>> +#define pte_access_permitted pte_access_permitted
>> +static inline bool pte_access_permitted(pte_t pte, bool write)
>> +{
>> +    unsigned long pteval = pte_val(pte);
>> +    /*
>> +     * A read-only access is controlled by _PAGE_USER bit.
>> +     * We have _PAGE_READ set for WRITE and EXECUTE
>> +     */
>> +    unsigned long need_pte_bits = _PAGE_PRESENT | _PAGE_USER;
>> +
>> +    if (write)
>> +        need_pte_bits |= _PAGE_WRITE;
>> +
>> +    if ((pteval & need_pte_bits) != need_pte_bits)
>> +        return false;
>> +
>> +    return true;
>> +}
>> +
>>   /* Conversion functions: convert a page and protection to a page entry,
>>    * and a page entry and page directory to the page they refer to.
>>    *
>> diff --git a/arch/powerpc/include/asm/nohash/pgtable.h 
>> b/arch/powerpc/include/asm/nohash/pgtable.h
>> index 5c68f4a59f75..fc4376c8d444 100644
>> --- a/arch/powerpc/include/asm/nohash/pgtable.h
>> +++ b/arch/powerpc/include/asm/nohash/pgtable.h
>> @@ -45,6 +45,29 @@ static inline int pte_present(pte_t pte)
>>       return pte_val(pte) & _PAGE_PRESENT;
>>   }
>> +/*
>> + * We only find page table entry in the last level
>> + * Hence no need for other accessors
>> + */
>> +#define pte_access_permitted pte_access_permitted
>> +static inline bool pte_access_permitted(pte_t pte, bool write)
>> +{
>> +    unsigned long pteval = pte_val(pte);
>> +    /*
>> +     * A read-only access is controlled by _PAGE_USER bit.
>> +     * We have _PAGE_READ set for WRITE and EXECUTE
>> +     */
> 
> Not fully right. asm/pte-common.h defines:
> 
> #define PAGE_NONE    __pgprot(_PAGE_BASE | _PAGE_NA)
> #define PAGE_SHARED    __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RW)
> #define PAGE_SHARED_X    __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RW | \
>                   _PAGE_EXEC)
> #define PAGE_COPY    __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RO)
> #define PAGE_COPY_X    __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RO | \
>                   _PAGE_EXEC)
> #define PAGE_READONLY    __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RO)
> #define PAGE_READONLY_X    __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RO | \
>                   _PAGE_EXEC)
> 
> On the 8xx, _PAGE_USER = 0
> 
>> +    unsigned long need_pte_bits = _PAGE_PRESENT | _PAGE_USER;

_PAGE_PRIVILEGED should be checked as well.

Indeed, it seems like our patches crossed each other. My patch for 
adding _PAGE_PRIVILEGED is date January 16th while your's was merged on 
the 17th.

I'm wondering if there are other places that are missing _PAGE_RO 
handling and _PAGE_PRIVILEGED handling. Do you remember if you added any 
recently ?

Christophe

>> +
>> +    if (write)
>> +        need_pte_bits |= _PAGE_WRITE;
>> +
>> +    if ((pteval & need_pte_bits) != need_pte_bits)
>> +        return false;
> 
> This test is not fully correct:
> - To check access(read) permission, you also have to check that _PAGE_NA 
> is not set.
> - To check write permission, you also have to check that neither 
> _PAGE_NA nor _PAGE_RO are set.
> 
> On the 8xx, you have:
> _PAGE_RW = _PAGE_WRITE = 0
> _PAGE_NA = 0x0200
> _PAGE_RO = 0x0600
> 
> Christophe
> 
>> +
>> +    return true;
>> +}
>> +
>>   /* Conversion functions: convert a page and protection to a page entry,
>>    * and a page entry and page directory to the page they refer to.
>>    *
>>

Re: [1/3] arch/powerpc/hugetlb: Use pte_access_permitted for hugetlb access check

[Michael Ellerman]

On Mon, 2017-12-04 at 02:19:10 UTC, "Aneesh Kumar K.V" wrote:
> No functional change in this patch. This update gup_hugepte to use the
> helper. This will help later when we add memory keys.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/5fa5b16be5b319184378870467352e

cheers

Re: [PATCH 1/3] arch/powerpc/hugetlb: Use pte_access_permitted for hugetlb access check

[Christophe LEROY]

Le 04/12/2017 à 03:19, Aneesh Kumar K.V a écrit :
> No functional change in this patch. This update gup_hugepte to use the
> helper. This will help later when we add memory keys.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
>   arch/powerpc/mm/hugetlbpage.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
> index a9b9083c5e49..c7e5afe5e118 100644
> --- a/arch/powerpc/mm/hugetlbpage.c
> +++ b/arch/powerpc/mm/hugetlbpage.c
> @@ -855,9 +855,7 @@ int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,
>   
>   	pte = READ_ONCE(*ptep);
>   
> -	if (!pte_present(pte) || !pte_read(pte))
> -		return 0;
> -	if (write && !pte_write(pte))
> +	if (!pte_access_permitted(pte, write))

Seems like pte_access_permitted() doesn't check _PAGE_RO whereas 
pte_write() does.

Christophe

>   		return 0;
>   
>   	/* hugepages are never "special" */
>