[PATCH] powerpc/book3s: Remove a few page table update interfaces.

[PATCH] powerpc/book3s: Remove a few page table update interfaces.

[Aneesh Kumar K.V]

When updating page tables, we need to make sure we fill the page table
entry valid bit. We should be using page table populate interface for
updating the table entries. The page table 'set' interface allows
updating the raw value of page table entry. This can result in
updating the entry wrongly. Remove the 'set' interface so that we avoid
its future usage.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 arch/powerpc/include/asm/book3s/64/pgalloc.h |  8 ++++----
 arch/powerpc/include/asm/book3s/64/pgtable.h | 14 --------------
 2 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/arch/powerpc/include/asm/book3s/64/pgalloc.h b/arch/powerpc/include/asm/book3s/64/pgalloc.h
index 9c1173283b96..138bc2ecc0c4 100644
--- a/arch/powerpc/include/asm/book3s/64/pgalloc.h
+++ b/arch/powerpc/include/asm/book3s/64/pgalloc.h
@@ -111,7 +111,7 @@ static inline void pgd_free(struct mm_struct *mm, pgd_t *pgd)
 
 static inline void pgd_populate(struct mm_struct *mm, pgd_t *pgd, pud_t *pud)
 {
-	pgd_set(pgd, __pgtable_ptr_val(pud) | PGD_VAL_BITS);
+	*pgd =  __pgd(__pgtable_ptr_val(pud) | PGD_VAL_BITS);
 }
 
 static inline pud_t *pud_alloc_one(struct mm_struct *mm, unsigned long addr)
@@ -138,7 +138,7 @@ static inline void pud_free(struct mm_struct *mm, pud_t *pud)
 
 static inline void pud_populate(struct mm_struct *mm, pud_t *pud, pmd_t *pmd)
 {
-	pud_set(pud, __pgtable_ptr_val(pmd) | PUD_VAL_BITS);
+	*pud = __pud(__pgtable_ptr_val(pmd) | PUD_VAL_BITS);
 }
 
 static inline void __pud_free_tlb(struct mmu_gather *tlb, pud_t *pud,
@@ -176,13 +176,13 @@ static inline void __pmd_free_tlb(struct mmu_gather *tlb, pmd_t *pmd,
 static inline void pmd_populate_kernel(struct mm_struct *mm, pmd_t *pmd,
 				       pte_t *pte)
 {
-	pmd_set(pmd, __pgtable_ptr_val(pte) | PMD_VAL_BITS);
+	*pmd = __pmd(__pgtable_ptr_val(pte) | PMD_VAL_BITS);
 }
 
 static inline void pmd_populate(struct mm_struct *mm, pmd_t *pmd,
 				pgtable_t pte_page)
 {
-	pmd_set(pmd, __pgtable_ptr_val(pte_page) | PMD_VAL_BITS);
+	*pmd = __pmd(__pgtable_ptr_val(pte_page) | PMD_VAL_BITS);
 }
 
 static inline pgtable_t pmd_pgtable(pmd_t pmd)
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index dc71e2b92003..a24e00fb7fa7 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -853,11 +853,6 @@ static inline bool pte_ci(pte_t pte)
 	return false;
 }
 
-static inline void pmd_set(pmd_t *pmdp, unsigned long val)
-{
-	*pmdp = __pmd(val);
-}
-
 static inline void pmd_clear(pmd_t *pmdp)
 {
 	*pmdp = __pmd(0);
@@ -889,11 +884,6 @@ static inline int pmd_bad(pmd_t pmd)
 	return hash__pmd_bad(pmd);
 }
 
-static inline void pud_set(pud_t *pudp, unsigned long val)
-{
-	*pudp = __pud(val);
-}
-
 static inline void pud_clear(pud_t *pudp)
 {
 	*pudp = __pud(0);
@@ -936,10 +926,6 @@ static inline bool pud_access_permitted(pud_t pud, bool write)
 }
 
 #define pgd_write(pgd)		pte_write(pgd_pte(pgd))
-static inline void pgd_set(pgd_t *pgdp, unsigned long val)
-{
-	*pgdp = __pgd(val);
-}
 
 static inline void pgd_clear(pgd_t *pgdp)
 {
-- 
2.20.1

Re: [PATCH] powerpc/book3s: Remove a few page table update interfaces.

[Nicholas Piggin]

Aneesh Kumar K.V's on February 14, 2019 4:45 pm:
> When updating page tables, we need to make sure we fill the page table
> entry valid bit. We should be using page table populate interface for
> updating the table entries. The page table 'set' interface allows
> updating the raw value of page table entry. This can result in
> updating the entry wrongly. Remove the 'set' interface so that we avoid
> its future usage.

Removing a pointless indirection is fine by me, but I can't
figure out how this change could help to prevent entry being updated
incorrectly.

Thanks,
Nick

> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/book3s/64/pgalloc.h |  8 ++++----
>  arch/powerpc/include/asm/book3s/64/pgtable.h | 14 --------------
>  2 files changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/book3s/64/pgalloc.h b/arch/powerpc/include/asm/book3s/64/pgalloc.h
> index 9c1173283b96..138bc2ecc0c4 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgalloc.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgalloc.h
> @@ -111,7 +111,7 @@ static inline void pgd_free(struct mm_struct *mm, pgd_t *pgd)
>  
>  static inline void pgd_populate(struct mm_struct *mm, pgd_t *pgd, pud_t *pud)
>  {
> -	pgd_set(pgd, __pgtable_ptr_val(pud) | PGD_VAL_BITS);
> +	*pgd =  __pgd(__pgtable_ptr_val(pud) | PGD_VAL_BITS);
>  }
>  
>  static inline pud_t *pud_alloc_one(struct mm_struct *mm, unsigned long addr)
> @@ -138,7 +138,7 @@ static inline void pud_free(struct mm_struct *mm, pud_t *pud)
>  
>  static inline void pud_populate(struct mm_struct *mm, pud_t *pud, pmd_t *pmd)
>  {
> -	pud_set(pud, __pgtable_ptr_val(pmd) | PUD_VAL_BITS);
> +	*pud = __pud(__pgtable_ptr_val(pmd) | PUD_VAL_BITS);
>  }
>  
>  static inline void __pud_free_tlb(struct mmu_gather *tlb, pud_t *pud,
> @@ -176,13 +176,13 @@ static inline void __pmd_free_tlb(struct mmu_gather *tlb, pmd_t *pmd,
>  static inline void pmd_populate_kernel(struct mm_struct *mm, pmd_t *pmd,
>  				       pte_t *pte)
>  {
> -	pmd_set(pmd, __pgtable_ptr_val(pte) | PMD_VAL_BITS);
> +	*pmd = __pmd(__pgtable_ptr_val(pte) | PMD_VAL_BITS);
>  }
>  
>  static inline void pmd_populate(struct mm_struct *mm, pmd_t *pmd,
>  				pgtable_t pte_page)
>  {
> -	pmd_set(pmd, __pgtable_ptr_val(pte_page) | PMD_VAL_BITS);
> +	*pmd = __pmd(__pgtable_ptr_val(pte_page) | PMD_VAL_BITS);
>  }
>  
>  static inline pgtable_t pmd_pgtable(pmd_t pmd)
> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
> index dc71e2b92003..a24e00fb7fa7 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> @@ -853,11 +853,6 @@ static inline bool pte_ci(pte_t pte)
>  	return false;
>  }
>  
> -static inline void pmd_set(pmd_t *pmdp, unsigned long val)
> -{
> -	*pmdp = __pmd(val);
> -}
> -
>  static inline void pmd_clear(pmd_t *pmdp)
>  {
>  	*pmdp = __pmd(0);
> @@ -889,11 +884,6 @@ static inline int pmd_bad(pmd_t pmd)
>  	return hash__pmd_bad(pmd);
>  }
>  
> -static inline void pud_set(pud_t *pudp, unsigned long val)
> -{
> -	*pudp = __pud(val);
> -}
> -
>  static inline void pud_clear(pud_t *pudp)
>  {
>  	*pudp = __pud(0);
> @@ -936,10 +926,6 @@ static inline bool pud_access_permitted(pud_t pud, bool write)
>  }
>  
>  #define pgd_write(pgd)		pte_write(pgd_pte(pgd))
> -static inline void pgd_set(pgd_t *pgdp, unsigned long val)
> -{
> -	*pgdp = __pgd(val);
> -}
>  
>  static inline void pgd_clear(pgd_t *pgdp)
>  {
> -- 
> 2.20.1
> 
> 

Re: [PATCH] powerpc/book3s: Remove a few page table update interfaces.

[Aneesh Kumar K.V]

On 2/19/19 11:23 AM, Nicholas Piggin wrote:
> Aneesh Kumar K.V's on February 14, 2019 4:45 pm:
>> When updating page tables, we need to make sure we fill the page table
>> entry valid bit. We should be using page table populate interface for
>> updating the table entries. The page table 'set' interface allows
>> updating the raw value of page table entry. This can result in
>> updating the entry wrongly. Remove the 'set' interface so that we avoid
>> its future usage.
> 
> Removing a pointless indirection is fine by me, but I can't
> figure out how this change could help to prevent entry being updated
> incorrectly.


I guess my commit message is confusing. What I meant is this avoid the 
usage of pgd/pud_set from other kernel code without updating the page 
table entry valid bits. The idea is to make pgd_populate as the only 
interface for update pgd entries.

-aneesh

Re: powerpc/book3s: Remove a few page table update interfaces.

[Michael Ellerman]

On Thu, 2019-02-14 at 06:45:40 UTC, "Aneesh Kumar K.V" wrote:
> When updating page tables, we need to make sure we fill the page table
> entry valid bit. We should be using page table populate interface for
> updating the table entries. The page table 'set' interface allows
> updating the raw value of page table entry. This can result in
> updating the entry wrongly. Remove the 'set' interface so that we avoid
> its future usage.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/c746ca00f5eac6224eda02f39ebdc48f

cheers