[PATCH 1/3] powerpc/nohash: Fix __ptep_set_access_flags() and ptep_set_wrprotect()

[PATCH 1/3] powerpc/nohash: Fix __ptep_set_access_flags() and ptep_set_wrprotect()

[Christophe Leroy]

Commit 26973fa5ac0e ("powerpc/mm: use pte helpers in generic code")
changed those two functions to use pte helpers to determine which
bits to clear and which bits to set.

This change was based on the assumption that bits to be set/cleared
are always the same and can be determined by applying the pte
manipulation helpers on __pte(0).

But on platforms like book3e, the bits depend on whether the page
is a user page or not.

For the time being it more or less works because of _PAGE_EXEC being
used for user pages only and exec right being set at all time on
kernel page. But following patch will clean that and output of
pte_mkexec() will depend on the page being a user or kernel page.

Instead of trying to make an even more complicated helper where bits
would become dependent on the final pte value, come back to a more
static situation like before commit 26973fa5ac0e ("powerpc/mm: use
pte helpers in generic code"), by introducing an 8xx specific
version of __ptep_set_access_flags() and ptep_set_wrprotect().

Fixes: 26973fa5ac0e ("powerpc/mm: use pte helpers in generic code")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v3: No change
v2: New
---
 arch/powerpc/include/asm/nohash/32/pgtable.h | 17 +++++++--------
 arch/powerpc/include/asm/nohash/32/pte-8xx.h | 22 ++++++++++++++++++++
 2 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/include/asm/nohash/32/pgtable.h b/arch/powerpc/include/asm/nohash/32/pgtable.h
index 34ce50da1850..11c6849f7864 100644
--- a/arch/powerpc/include/asm/nohash/32/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/32/pgtable.h
@@ -306,30 +306,29 @@ static inline pte_t ptep_get_and_clear(struct mm_struct *mm, unsigned long addr,
 }
 
 #define __HAVE_ARCH_PTEP_SET_WRPROTECT
+#ifndef ptep_set_wrprotect
 static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr,
 				      pte_t *ptep)
 {
-	unsigned long clr = ~pte_val(pte_wrprotect(__pte(~0)));
-	unsigned long set = pte_val(pte_wrprotect(__pte(0)));
-
-	pte_update(mm, addr, ptep, clr, set, 0);
+	pte_update(mm, addr, ptep, _PAGE_RW, 0, 0);
 }
+#endif
 
+#ifndef __ptep_set_access_flags
 static inline void __ptep_set_access_flags(struct vm_area_struct *vma,
 					   pte_t *ptep, pte_t entry,
 					   unsigned long address,
 					   int psize)
 {
-	pte_t pte_set = pte_mkyoung(pte_mkdirty(pte_mkwrite(pte_mkexec(__pte(0)))));
-	pte_t pte_clr = pte_mkyoung(pte_mkdirty(pte_mkwrite(pte_mkexec(__pte(~0)))));
-	unsigned long set = pte_val(entry) & pte_val(pte_set);
-	unsigned long clr = ~pte_val(entry) & ~pte_val(pte_clr);
+	unsigned long set = pte_val(entry) &
+			    (_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_RW | _PAGE_EXEC);
 	int huge = psize > mmu_virtual_psize ? 1 : 0;
 
-	pte_update(vma->vm_mm, address, ptep, clr, set, huge);
+	pte_update(vma->vm_mm, address, ptep, 0, set, huge);
 
 	flush_tlb_page(vma, address);
 }
+#endif
 
 static inline int pte_young(pte_t pte)
 {
diff --git a/arch/powerpc/include/asm/nohash/32/pte-8xx.h b/arch/powerpc/include/asm/nohash/32/pte-8xx.h
index fcc48d590d88..1a89ebdc3acc 100644
--- a/arch/powerpc/include/asm/nohash/32/pte-8xx.h
+++ b/arch/powerpc/include/asm/nohash/32/pte-8xx.h
@@ -136,6 +136,28 @@ static inline pte_t pte_mkhuge(pte_t pte)
 
 #define pte_mkhuge pte_mkhuge
 
+static inline pte_basic_t pte_update(struct mm_struct *mm, unsigned long addr, pte_t *p,
+				     unsigned long clr, unsigned long set, int huge);
+
+static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
+{
+	pte_update(mm, addr, ptep, 0, _PAGE_RO, 0);
+}
+#define ptep_set_wrprotect ptep_set_wrprotect
+
+static inline void __ptep_set_access_flags(struct vm_area_struct *vma, pte_t *ptep,
+					   pte_t entry, unsigned long address, int psize)
+{
+	unsigned long set = pte_val(entry) & (_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_EXEC);
+	unsigned long clr = ~pte_val(entry) & _PAGE_RO;
+	int huge = psize > mmu_virtual_psize ? 1 : 0;
+
+	pte_update(vma->vm_mm, address, ptep, clr, set, huge);
+
+	flush_tlb_page(vma, address);
+}
+#define __ptep_set_access_flags __ptep_set_access_flags
+
 static inline unsigned long pgd_leaf_size(pgd_t pgd)
 {
 	if (pgd_val(pgd) & _PMD_PAGE_8M)
-- 
2.31.1

[PATCH 2/3] powerpc/book3e: Fix set_memory_x() and set_memory_nx()

[Christophe Leroy]

set_memory_x() calls pte_mkexec() which sets _PAGE_EXEC.
set_memory_nx() calls pte_exprotec() which clears _PAGE_EXEC.

Book3e has 2 bits, UX and SX, which defines the exec rights
resp. for user (PR=1) and for kernel (PR=0).

_PAGE_EXEC is defined as UX only.

An executable kernel page is set with either _PAGE_KERNEL_RWX
or _PAGE_KERNEL_ROX, which both have SX set and UX cleared.

So set_memory_nx() call for an executable kernel page does
nothing because UX is already cleared.

And set_memory_x() on a non-executable kernel page makes it
executable for the user and keeps it non-executable for kernel.

Also, pte_exec() always returns 'false' on kernel pages, because
it checks _PAGE_EXEC which doesn't include SX, so for instance
the W+X check doesn't work.

To fix this:
- change tlb_low_64e.S to use _PAGE_BAP_UX instead of _PAGE_USER
- sets both UX and SX in _PAGE_EXEC so that pte_user() returns
true whenever one of the two bits is set and pte_exprotect()
clears both bits.
- Define a book3e specific version of pte_mkexec() which sets
either SX or UX based on UR.

Fixes: 1f9ad21c3b38 ("powerpc/mm: Implement set_memory() routines")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v3: Removed pte_mkexec() from nohash/64/pgtable.h
v2: New
---
 arch/powerpc/include/asm/nohash/32/pgtable.h |  2 ++
 arch/powerpc/include/asm/nohash/64/pgtable.h |  5 -----
 arch/powerpc/include/asm/nohash/pte-book3e.h | 18 ++++++++++++++----
 arch/powerpc/mm/nohash/tlb_low_64e.S         |  8 ++++----
 4 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/include/asm/nohash/32/pgtable.h b/arch/powerpc/include/asm/nohash/32/pgtable.h
index 11c6849f7864..b67742e2a9b2 100644
--- a/arch/powerpc/include/asm/nohash/32/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/32/pgtable.h
@@ -193,10 +193,12 @@ static inline pte_t pte_wrprotect(pte_t pte)
 }
 #endif
 
+#ifndef pte_mkexec
 static inline pte_t pte_mkexec(pte_t pte)
 {
 	return __pte(pte_val(pte) | _PAGE_EXEC);
 }
+#endif
 
 #define pmd_none(pmd)		(!pmd_val(pmd))
 #define	pmd_bad(pmd)		(pmd_val(pmd) & _PMD_BAD)
diff --git a/arch/powerpc/include/asm/nohash/64/pgtable.h b/arch/powerpc/include/asm/nohash/64/pgtable.h
index d081704b13fb..9d2905a47410 100644
--- a/arch/powerpc/include/asm/nohash/64/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/64/pgtable.h
@@ -118,11 +118,6 @@ static inline pte_t pte_wrprotect(pte_t pte)
 	return __pte(pte_val(pte) & ~_PAGE_RW);
 }
 
-static inline pte_t pte_mkexec(pte_t pte)
-{
-	return __pte(pte_val(pte) | _PAGE_EXEC);
-}
-
 #define PMD_BAD_BITS		(PTE_TABLE_SIZE-1)
 #define PUD_BAD_BITS		(PMD_TABLE_SIZE-1)
 
diff --git a/arch/powerpc/include/asm/nohash/pte-book3e.h b/arch/powerpc/include/asm/nohash/pte-book3e.h
index 813918f40765..f798640422c2 100644
--- a/arch/powerpc/include/asm/nohash/pte-book3e.h
+++ b/arch/powerpc/include/asm/nohash/pte-book3e.h
@@ -48,7 +48,7 @@
 #define _PAGE_WRITETHRU	0x800000 /* W: cache write-through */
 
 /* "Higher level" linux bit combinations */
-#define _PAGE_EXEC		_PAGE_BAP_UX /* .. and was cache cleaned */
+#define _PAGE_EXEC		(_PAGE_BAP_SX | _PAGE_BAP_UX) /* .. and was cache cleaned */
 #define _PAGE_RW		(_PAGE_BAP_SW | _PAGE_BAP_UW) /* User write permission */
 #define _PAGE_KERNEL_RW		(_PAGE_BAP_SW | _PAGE_BAP_SR | _PAGE_DIRTY)
 #define _PAGE_KERNEL_RO		(_PAGE_BAP_SR)
@@ -93,11 +93,11 @@
 /* Permission masks used to generate the __P and __S table */
 #define PAGE_NONE	__pgprot(_PAGE_BASE)
 #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_SHARED_X	__pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RW | _PAGE_BAP_UX)
 #define PAGE_COPY	__pgprot(_PAGE_BASE | _PAGE_USER)
-#define PAGE_COPY_X	__pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_EXEC)
+#define PAGE_COPY_X	__pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_BAP_UX)
 #define PAGE_READONLY	__pgprot(_PAGE_BASE | _PAGE_USER)
-#define PAGE_READONLY_X	__pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_EXEC)
+#define PAGE_READONLY_X	__pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_BAP_UX)
 
 #ifndef __ASSEMBLY__
 static inline pte_t pte_mkprivileged(pte_t pte)
@@ -113,6 +113,16 @@ static inline pte_t pte_mkuser(pte_t pte)
 }
 
 #define pte_mkuser pte_mkuser
+
+static inline pte_t pte_mkexec(pte_t pte)
+{
+	if (pte_val(pte) & _PAGE_BAP_UR)
+		return __pte((pte_val(pte) & ~_PAGE_BAP_SX) | _PAGE_BAP_UX);
+	else
+		return __pte((pte_val(pte) & ~_PAGE_BAP_UX) | _PAGE_BAP_SX);
+}
+#define pte_mkexec pte_mkexec
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __KERNEL__ */
diff --git a/arch/powerpc/mm/nohash/tlb_low_64e.S b/arch/powerpc/mm/nohash/tlb_low_64e.S
index bf24451f3e71..9235e720e357 100644
--- a/arch/powerpc/mm/nohash/tlb_low_64e.S
+++ b/arch/powerpc/mm/nohash/tlb_low_64e.S
@@ -222,7 +222,7 @@ tlb_miss_kernel_bolted:
 
 tlb_miss_fault_bolted:
 	/* We need to check if it was an instruction miss */
-	andi.	r10,r11,_PAGE_EXEC|_PAGE_BAP_SX
+	andi.	r10,r11,_PAGE_BAP_UX|_PAGE_BAP_SX
 	bne	itlb_miss_fault_bolted
 dtlb_miss_fault_bolted:
 	tlb_epilog_bolted
@@ -239,7 +239,7 @@ itlb_miss_fault_bolted:
 	srdi	r15,r16,60		/* get region */
 	bne-	itlb_miss_fault_bolted
 
-	li	r11,_PAGE_PRESENT|_PAGE_EXEC	/* Base perm */
+	li	r11,_PAGE_PRESENT|_PAGE_BAP_UX	/* Base perm */
 
 	/* We do the user/kernel test for the PID here along with the RW test
 	 */
@@ -614,7 +614,7 @@ itlb_miss_fault_e6500:
 
 	/* We do the user/kernel test for the PID here along with the RW test
 	 */
-	li	r11,_PAGE_PRESENT|_PAGE_EXEC	/* Base perm */
+	li	r11,_PAGE_PRESENT|_PAGE_BAP_UX	/* Base perm */
 	oris	r11,r11,_PAGE_ACCESSED@h
 
 	cmpldi	cr0,r15,0			/* Check for user region */
@@ -734,7 +734,7 @@ normal_tlb_miss_done:
 
 normal_tlb_miss_access_fault:
 	/* We need to check if it was an instruction miss */
-	andi.	r10,r11,_PAGE_EXEC
+	andi.	r10,r11,_PAGE_BAP_UX
 	bne	1f
 	ld	r14,EX_TLB_DEAR(r12)
 	ld	r15,EX_TLB_ESR(r12)
-- 
2.31.1

[PATCH 3/3] powerpc/fsl_booke: Fix setting of exec flag when setting TLBCAMs

[Christophe Leroy]

Building tqm8541_defconfig results in:

	arch/powerpc/mm/nohash/fsl_book3e.c: In function 'settlbcam':
	arch/powerpc/mm/nohash/fsl_book3e.c:126:40: error: '_PAGE_BAP_SX' undeclared (first use in this function)
	  126 |         TLBCAM[index].MAS3 |= (flags & _PAGE_BAP_SX) ? MAS3_SX : 0;
	      |                                        ^~~~~~~~~~~~
	arch/powerpc/mm/nohash/fsl_book3e.c:126:40: note: each undeclared identifier is reported only once for each function it appears in
	make[3]: *** [scripts/Makefile.build:277: arch/powerpc/mm/nohash/fsl_book3e.o] Error 1
	make[2]: *** [scripts/Makefile.build:540: arch/powerpc/mm/nohash] Error 2
	make[1]: *** [scripts/Makefile.build:540: arch/powerpc/mm] Error 2
	make: *** [Makefile:1868: arch/powerpc] Error 2

This is because _PAGE_BAP_SX is not defined when using 32 bits PTE.

Now that _PAGE_EXEC contains both _PAGE_BAP_SX and _PAGE_BAP_UX, it can be used instead.

Reported-by: kernel test robot <lkp@intel.com>
Fixes: 01116e6e98b0 ("powerpc/fsl_booke: Take exec flag into account when setting TLBCAMs")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/mm/nohash/fsl_book3e.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/nohash/fsl_book3e.c b/arch/powerpc/mm/nohash/fsl_book3e.c
index 978e0bcdfa2c..b231a54f540c 100644
--- a/arch/powerpc/mm/nohash/fsl_book3e.c
+++ b/arch/powerpc/mm/nohash/fsl_book3e.c
@@ -123,7 +123,6 @@ static void settlbcam(int index, unsigned long virt, phys_addr_t phys,
 	TLBCAM[index].MAS2 |= (flags & _PAGE_ENDIAN) ? MAS2_E : 0;
 
 	TLBCAM[index].MAS3 = (phys & MAS3_RPN) | MAS3_SR;
-	TLBCAM[index].MAS3 |= (flags & _PAGE_BAP_SX) ? MAS3_SX : 0;
 	TLBCAM[index].MAS3 |= (flags & _PAGE_RW) ? MAS3_SW : 0;
 	if (mmu_has_feature(MMU_FTR_BIG_PHYS))
 		TLBCAM[index].MAS7 = (u64)phys >> 32;
@@ -133,6 +132,8 @@ static void settlbcam(int index, unsigned long virt, phys_addr_t phys,
 		TLBCAM[index].MAS3 |= MAS3_UR;
 		TLBCAM[index].MAS3 |= (flags & _PAGE_EXEC) ? MAS3_UX : 0;
 		TLBCAM[index].MAS3 |= (flags & _PAGE_RW) ? MAS3_UW : 0;
+	} else {
+		TLBCAM[index].MAS3 |= (flags & _PAGE_EXEC) ? MAS3_SX : 0;
 	}
 
 	tlbcam_addrs[index].start = virt;
-- 
2.31.1

Re: [PATCH 1/3] powerpc/nohash: Fix __ptep_set_access_flags() and ptep_set_wrprotect()

[Nicholas Piggin]

Excerpts from Christophe Leroy's message of October 26, 2021 3:39 pm:
> Commit 26973fa5ac0e ("powerpc/mm: use pte helpers in generic code")
> changed those two functions to use pte helpers to determine which
> bits to clear and which bits to set.
> 
> This change was based on the assumption that bits to be set/cleared
> are always the same and can be determined by applying the pte
> manipulation helpers on __pte(0).
> 
> But on platforms like book3e, the bits depend on whether the page
> is a user page or not.
> 
> For the time being it more or less works because of _PAGE_EXEC being
> used for user pages only and exec right being set at all time on
> kernel page. But following patch will clean that and output of
> pte_mkexec() will depend on the page being a user or kernel page.
> 
> Instead of trying to make an even more complicated helper where bits
> would become dependent on the final pte value, come back to a more
> static situation like before commit 26973fa5ac0e ("powerpc/mm: use
> pte helpers in generic code"), by introducing an 8xx specific
> version of __ptep_set_access_flags() and ptep_set_wrprotect().

What is this actually fixing? Does it change anything itself, or
just a preparation patch?

Thanks,
Nick

> 
> Fixes: 26973fa5ac0e ("powerpc/mm: use pte helpers in generic code")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> v3: No change
> v2: New
> ---
>  arch/powerpc/include/asm/nohash/32/pgtable.h | 17 +++++++--------
>  arch/powerpc/include/asm/nohash/32/pte-8xx.h | 22 ++++++++++++++++++++
>  2 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/nohash/32/pgtable.h b/arch/powerpc/include/asm/nohash/32/pgtable.h
> index 34ce50da1850..11c6849f7864 100644
> --- a/arch/powerpc/include/asm/nohash/32/pgtable.h
> +++ b/arch/powerpc/include/asm/nohash/32/pgtable.h
> @@ -306,30 +306,29 @@ static inline pte_t ptep_get_and_clear(struct mm_struct *mm, unsigned long addr,
>  }
>  
>  #define __HAVE_ARCH_PTEP_SET_WRPROTECT
> +#ifndef ptep_set_wrprotect
>  static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr,
>  				      pte_t *ptep)
>  {
> -	unsigned long clr = ~pte_val(pte_wrprotect(__pte(~0)));
> -	unsigned long set = pte_val(pte_wrprotect(__pte(0)));
> -
> -	pte_update(mm, addr, ptep, clr, set, 0);
> +	pte_update(mm, addr, ptep, _PAGE_RW, 0, 0);
>  }
> +#endif
>  
> +#ifndef __ptep_set_access_flags
>  static inline void __ptep_set_access_flags(struct vm_area_struct *vma,
>  					   pte_t *ptep, pte_t entry,
>  					   unsigned long address,
>  					   int psize)
>  {
> -	pte_t pte_set = pte_mkyoung(pte_mkdirty(pte_mkwrite(pte_mkexec(__pte(0)))));
> -	pte_t pte_clr = pte_mkyoung(pte_mkdirty(pte_mkwrite(pte_mkexec(__pte(~0)))));
> -	unsigned long set = pte_val(entry) & pte_val(pte_set);
> -	unsigned long clr = ~pte_val(entry) & ~pte_val(pte_clr);
> +	unsigned long set = pte_val(entry) &
> +			    (_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_RW | _PAGE_EXEC);
>  	int huge = psize > mmu_virtual_psize ? 1 : 0;
>  
> -	pte_update(vma->vm_mm, address, ptep, clr, set, huge);
> +	pte_update(vma->vm_mm, address, ptep, 0, set, huge);
>  
>  	flush_tlb_page(vma, address);
>  }
> +#endif
>  
>  static inline int pte_young(pte_t pte)
>  {
> diff --git a/arch/powerpc/include/asm/nohash/32/pte-8xx.h b/arch/powerpc/include/asm/nohash/32/pte-8xx.h
> index fcc48d590d88..1a89ebdc3acc 100644
> --- a/arch/powerpc/include/asm/nohash/32/pte-8xx.h
> +++ b/arch/powerpc/include/asm/nohash/32/pte-8xx.h
> @@ -136,6 +136,28 @@ static inline pte_t pte_mkhuge(pte_t pte)
>  
>  #define pte_mkhuge pte_mkhuge
>  
> +static inline pte_basic_t pte_update(struct mm_struct *mm, unsigned long addr, pte_t *p,
> +				     unsigned long clr, unsigned long set, int huge);
> +
> +static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
> +{
> +	pte_update(mm, addr, ptep, 0, _PAGE_RO, 0);
> +}
> +#define ptep_set_wrprotect ptep_set_wrprotect
> +
> +static inline void __ptep_set_access_flags(struct vm_area_struct *vma, pte_t *ptep,
> +					   pte_t entry, unsigned long address, int psize)
> +{
> +	unsigned long set = pte_val(entry) & (_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_EXEC);
> +	unsigned long clr = ~pte_val(entry) & _PAGE_RO;
> +	int huge = psize > mmu_virtual_psize ? 1 : 0;
> +
> +	pte_update(vma->vm_mm, address, ptep, clr, set, huge);
> +
> +	flush_tlb_page(vma, address);
> +}
> +#define __ptep_set_access_flags __ptep_set_access_flags
> +
>  static inline unsigned long pgd_leaf_size(pgd_t pgd)
>  {
>  	if (pgd_val(pgd) & _PMD_PAGE_8M)
> -- 
> 2.31.1
> 
> 

Re: [PATCH 1/3] powerpc/nohash: Fix __ptep_set_access_flags() and ptep_set_wrprotect()

[Christophe Leroy]

Le 27/10/2021 à 06:23, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of October 26, 2021 3:39 pm:
>> Commit 26973fa5ac0e ("powerpc/mm: use pte helpers in generic code")
>> changed those two functions to use pte helpers to determine which
>> bits to clear and which bits to set.
>>
>> This change was based on the assumption that bits to be set/cleared
>> are always the same and can be determined by applying the pte
>> manipulation helpers on __pte(0).
>>
>> But on platforms like book3e, the bits depend on whether the page
>> is a user page or not.
>>
>> For the time being it more or less works because of _PAGE_EXEC being
>> used for user pages only and exec right being set at all time on
>> kernel page. But following patch will clean that and output of
>> pte_mkexec() will depend on the page being a user or kernel page.
>>
>> Instead of trying to make an even more complicated helper where bits
>> would become dependent on the final pte value, come back to a more
>> static situation like before commit 26973fa5ac0e ("powerpc/mm: use
>> pte helpers in generic code"), by introducing an 8xx specific
>> version of __ptep_set_access_flags() and ptep_set_wrprotect().
> 
> What is this actually fixing? Does it change anything itself, or
> just a preparation patch?

Just a preparation patch I think.

I didn't flag it for stable.

Once patch 2 is applied, __ptep_set_access_flags() doesn't work anymore 
without this patch, because then pte_mkexec(__pte(0)) sets SX and clears 
UX while pte_mkexec(__pte(~0)) sets UX and clears SX

Christophe


> 
> Thanks,
> Nick
> 
>>
>> Fixes: 26973fa5ac0e ("powerpc/mm: use pte helpers in generic code")
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> v3: No change
>> v2: New
>> ---
>>   arch/powerpc/include/asm/nohash/32/pgtable.h | 17 +++++++--------
>>   arch/powerpc/include/asm/nohash/32/pte-8xx.h | 22 ++++++++++++++++++++
>>   2 files changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/nohash/32/pgtable.h b/arch/powerpc/include/asm/nohash/32/pgtable.h
>> index 34ce50da1850..11c6849f7864 100644
>> --- a/arch/powerpc/include/asm/nohash/32/pgtable.h
>> +++ b/arch/powerpc/include/asm/nohash/32/pgtable.h
>> @@ -306,30 +306,29 @@ static inline pte_t ptep_get_and_clear(struct mm_struct *mm, unsigned long addr,
>>   }
>>   
>>   #define __HAVE_ARCH_PTEP_SET_WRPROTECT
>> +#ifndef ptep_set_wrprotect
>>   static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr,
>>   				      pte_t *ptep)
>>   {
>> -	unsigned long clr = ~pte_val(pte_wrprotect(__pte(~0)));
>> -	unsigned long set = pte_val(pte_wrprotect(__pte(0)));
>> -
>> -	pte_update(mm, addr, ptep, clr, set, 0);
>> +	pte_update(mm, addr, ptep, _PAGE_RW, 0, 0);
>>   }
>> +#endif
>>   
>> +#ifndef __ptep_set_access_flags
>>   static inline void __ptep_set_access_flags(struct vm_area_struct *vma,
>>   					   pte_t *ptep, pte_t entry,
>>   					   unsigned long address,
>>   					   int psize)
>>   {
>> -	pte_t pte_set = pte_mkyoung(pte_mkdirty(pte_mkwrite(pte_mkexec(__pte(0)))));
>> -	pte_t pte_clr = pte_mkyoung(pte_mkdirty(pte_mkwrite(pte_mkexec(__pte(~0)))));
>> -	unsigned long set = pte_val(entry) & pte_val(pte_set);
>> -	unsigned long clr = ~pte_val(entry) & ~pte_val(pte_clr);
>> +	unsigned long set = pte_val(entry) &
>> +			    (_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_RW | _PAGE_EXEC);
>>   	int huge = psize > mmu_virtual_psize ? 1 : 0;
>>   
>> -	pte_update(vma->vm_mm, address, ptep, clr, set, huge);
>> +	pte_update(vma->vm_mm, address, ptep, 0, set, huge);
>>   
>>   	flush_tlb_page(vma, address);
>>   }
>> +#endif
>>   
>>   static inline int pte_young(pte_t pte)
>>   {
>> diff --git a/arch/powerpc/include/asm/nohash/32/pte-8xx.h b/arch/powerpc/include/asm/nohash/32/pte-8xx.h
>> index fcc48d590d88..1a89ebdc3acc 100644
>> --- a/arch/powerpc/include/asm/nohash/32/pte-8xx.h
>> +++ b/arch/powerpc/include/asm/nohash/32/pte-8xx.h
>> @@ -136,6 +136,28 @@ static inline pte_t pte_mkhuge(pte_t pte)
>>   
>>   #define pte_mkhuge pte_mkhuge
>>   
>> +static inline pte_basic_t pte_update(struct mm_struct *mm, unsigned long addr, pte_t *p,
>> +				     unsigned long clr, unsigned long set, int huge);
>> +
>> +static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>> +{
>> +	pte_update(mm, addr, ptep, 0, _PAGE_RO, 0);
>> +}
>> +#define ptep_set_wrprotect ptep_set_wrprotect
>> +
>> +static inline void __ptep_set_access_flags(struct vm_area_struct *vma, pte_t *ptep,
>> +					   pte_t entry, unsigned long address, int psize)
>> +{
>> +	unsigned long set = pte_val(entry) & (_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_EXEC);
>> +	unsigned long clr = ~pte_val(entry) & _PAGE_RO;
>> +	int huge = psize > mmu_virtual_psize ? 1 : 0;
>> +
>> +	pte_update(vma->vm_mm, address, ptep, clr, set, huge);
>> +
>> +	flush_tlb_page(vma, address);
>> +}
>> +#define __ptep_set_access_flags __ptep_set_access_flags
>> +
>>   static inline unsigned long pgd_leaf_size(pgd_t pgd)
>>   {
>>   	if (pgd_val(pgd) & _PMD_PAGE_8M)
>> -- 
>> 2.31.1
>>
>>

Re: [PATCH 2/3] powerpc/book3e: Fix set_memory_x() and set_memory_nx()

[Nicholas Piggin]

Excerpts from Christophe Leroy's message of October 26, 2021 3:39 pm:
> set_memory_x() calls pte_mkexec() which sets _PAGE_EXEC.
> set_memory_nx() calls pte_exprotec() which clears _PAGE_EXEC.
> 
> Book3e has 2 bits, UX and SX, which defines the exec rights
> resp. for user (PR=1) and for kernel (PR=0).
> 
> _PAGE_EXEC is defined as UX only.
> 
> An executable kernel page is set with either _PAGE_KERNEL_RWX
> or _PAGE_KERNEL_ROX, which both have SX set and UX cleared.
> 
> So set_memory_nx() call for an executable kernel page does
> nothing because UX is already cleared.
> 
> And set_memory_x() on a non-executable kernel page makes it
> executable for the user and keeps it non-executable for kernel.
> 
> Also, pte_exec() always returns 'false' on kernel pages, because
> it checks _PAGE_EXEC which doesn't include SX, so for instance
> the W+X check doesn't work.
> 
> To fix this:
> - change tlb_low_64e.S to use _PAGE_BAP_UX instead of _PAGE_USER
> - sets both UX and SX in _PAGE_EXEC so that pte_user() returns
> true whenever one of the two bits is set

I don't understand this change. Which pte_user() returns true after
this change? Or do you mean pte_exec()?

Does this filter through in some cases at least for kernel executable
PTEs will get both bits set? Seems cleaner to distinguish user and
kernel exec for that but maybe it's a lot of churn?

Thanks,
Nick

> and pte_exprotect()
> clears both bits.
> - Define a book3e specific version of pte_mkexec() which sets
> either SX or UX based on UR.
> 
> Fixes: 1f9ad21c3b38 ("powerpc/mm: Implement set_memory() routines")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> v3: Removed pte_mkexec() from nohash/64/pgtable.h
> v2: New
> ---

Re: [PATCH 2/3] powerpc/book3e: Fix set_memory_x() and set_memory_nx()

[Christophe Leroy]

Le 27/10/2021 à 06:44, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of October 26, 2021 3:39 pm:
>> set_memory_x() calls pte_mkexec() which sets _PAGE_EXEC.
>> set_memory_nx() calls pte_exprotec() which clears _PAGE_EXEC.
>>
>> Book3e has 2 bits, UX and SX, which defines the exec rights
>> resp. for user (PR=1) and for kernel (PR=0).
>>
>> _PAGE_EXEC is defined as UX only.
>>
>> An executable kernel page is set with either _PAGE_KERNEL_RWX
>> or _PAGE_KERNEL_ROX, which both have SX set and UX cleared.
>>
>> So set_memory_nx() call for an executable kernel page does
>> nothing because UX is already cleared.
>>
>> And set_memory_x() on a non-executable kernel page makes it
>> executable for the user and keeps it non-executable for kernel.
>>
>> Also, pte_exec() always returns 'false' on kernel pages, because
>> it checks _PAGE_EXEC which doesn't include SX, so for instance
>> the W+X check doesn't work.
>>
>> To fix this:
>> - change tlb_low_64e.S to use _PAGE_BAP_UX instead of _PAGE_USER
>> - sets both UX and SX in _PAGE_EXEC so that pte_user() returns
>> true whenever one of the two bits is set
> 
> I don't understand this change. Which pte_user() returns true after
> this change? Or do you mean pte_exec()?

Oops, yes, I mean pte_exec()

Unless I have to re-spin, can Michael eventually fix that typo while 
applying ?

> 
> Does this filter through in some cases at least for kernel executable
> PTEs will get both bits set? Seems cleaner to distinguish user and
> kernel exec for that but maybe it's a lot of churn?

Didn't understand what you mean.

I did it like that to be able to continue using _PAGE_EXEC for checking 
executability regardless of whether this is user or kernel, and then 
continue using the generic nohash pte_exec() helper.

Other solution would be to get rid of _PAGE_EXEC completely for book3e 
and implement both pte_exec() and pte_mkexec() with _PAGE_BAP_UX and 
_PAGE_BAP_SX, but I'm not sure it is worth the churn as you say. It 
would also mean different helpers for book3s/32 when it is using 32 bits 
PTE (CONFIG_PTE_64BIT=n)

Christophe

> 
> Thanks,
> Nick
> 
>> and pte_exprotect()
>> clears both bits.
>> - Define a book3e specific version of pte_mkexec() which sets
>> either SX or UX based on UR.
>>
>> Fixes: 1f9ad21c3b38 ("powerpc/mm: Implement set_memory() routines")
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> v3: Removed pte_mkexec() from nohash/64/pgtable.h
>> v2: New
>> ---

Re: [PATCH 2/3] powerpc/book3e: Fix set_memory_x() and set_memory_nx()

[Nicholas Piggin]

Excerpts from Christophe Leroy's message of October 27, 2021 2:55 pm:
> 
> 
> Le 27/10/2021 à 06:44, Nicholas Piggin a écrit :
>> Excerpts from Christophe Leroy's message of October 26, 2021 3:39 pm:
>>> set_memory_x() calls pte_mkexec() which sets _PAGE_EXEC.
>>> set_memory_nx() calls pte_exprotec() which clears _PAGE_EXEC.
>>>
>>> Book3e has 2 bits, UX and SX, which defines the exec rights
>>> resp. for user (PR=1) and for kernel (PR=0).
>>>
>>> _PAGE_EXEC is defined as UX only.
>>>
>>> An executable kernel page is set with either _PAGE_KERNEL_RWX
>>> or _PAGE_KERNEL_ROX, which both have SX set and UX cleared.
>>>
>>> So set_memory_nx() call for an executable kernel page does
>>> nothing because UX is already cleared.
>>>
>>> And set_memory_x() on a non-executable kernel page makes it
>>> executable for the user and keeps it non-executable for kernel.
>>>
>>> Also, pte_exec() always returns 'false' on kernel pages, because
>>> it checks _PAGE_EXEC which doesn't include SX, so for instance
>>> the W+X check doesn't work.
>>>
>>> To fix this:
>>> - change tlb_low_64e.S to use _PAGE_BAP_UX instead of _PAGE_USER
>>> - sets both UX and SX in _PAGE_EXEC so that pte_user() returns
>>> true whenever one of the two bits is set
>> 
>> I don't understand this change. Which pte_user() returns true after
>> this change? Or do you mean pte_exec()?
> 
> Oops, yes, I mean pte_exec()
> 
> Unless I have to re-spin, can Michael eventually fix that typo while 
> applying ?
> 
>> 
>> Does this filter through in some cases at least for kernel executable
>> PTEs will get both bits set? Seems cleaner to distinguish user and
>> kernel exec for that but maybe it's a lot of churn?
> 
> Didn't understand what you mean.
> 
> I did it like that to be able to continue using _PAGE_EXEC for checking 
> executability regardless of whether this is user or kernel, and then 
> continue using the generic nohash pte_exec() helper.
> 
> Other solution would be to get rid of _PAGE_EXEC completely for book3e 
> and implement both pte_exec() and pte_mkexec() with _PAGE_BAP_UX and 
> _PAGE_BAP_SX, but I'm not sure it is worth the churn as you say. It 
> would also mean different helpers for book3s/32 when it is using 32 bits 
> PTE (CONFIG_PTE_64BIT=n)

That's basically what I mean. And _PAGE_KERNEL_ROX etc would then not 
set the UX bit. But at least for now it seems to be an improvement.

Thanks,
Nick

Re: [PATCH 2/3] powerpc/book3e: Fix set_memory_x() and set_memory_nx()

[Christophe Leroy]

Le 27/10/2021 à 07:27, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of October 27, 2021 2:55 pm:
>>
>>
>> Le 27/10/2021 à 06:44, Nicholas Piggin a écrit :
>>> Excerpts from Christophe Leroy's message of October 26, 2021 3:39 pm:
>>>> set_memory_x() calls pte_mkexec() which sets _PAGE_EXEC.
>>>> set_memory_nx() calls pte_exprotec() which clears _PAGE_EXEC.
>>>>
>>>> Book3e has 2 bits, UX and SX, which defines the exec rights
>>>> resp. for user (PR=1) and for kernel (PR=0).
>>>>
>>>> _PAGE_EXEC is defined as UX only.
>>>>
>>>> An executable kernel page is set with either _PAGE_KERNEL_RWX
>>>> or _PAGE_KERNEL_ROX, which both have SX set and UX cleared.
>>>>
>>>> So set_memory_nx() call for an executable kernel page does
>>>> nothing because UX is already cleared.
>>>>
>>>> And set_memory_x() on a non-executable kernel page makes it
>>>> executable for the user and keeps it non-executable for kernel.
>>>>
>>>> Also, pte_exec() always returns 'false' on kernel pages, because
>>>> it checks _PAGE_EXEC which doesn't include SX, so for instance
>>>> the W+X check doesn't work.
>>>>
>>>> To fix this:
>>>> - change tlb_low_64e.S to use _PAGE_BAP_UX instead of _PAGE_USER
>>>> - sets both UX and SX in _PAGE_EXEC so that pte_user() returns
>>>> true whenever one of the two bits is set
>>>
>>> I don't understand this change. Which pte_user() returns true after
>>> this change? Or do you mean pte_exec()?
>>
>> Oops, yes, I mean pte_exec()
>>
>> Unless I have to re-spin, can Michael eventually fix that typo while
>> applying ?
>>
>>>
>>> Does this filter through in some cases at least for kernel executable
>>> PTEs will get both bits set? Seems cleaner to distinguish user and
>>> kernel exec for that but maybe it's a lot of churn?
>>
>> Didn't understand what you mean.
>>
>> I did it like that to be able to continue using _PAGE_EXEC for checking
>> executability regardless of whether this is user or kernel, and then
>> continue using the generic nohash pte_exec() helper.
>>
>> Other solution would be to get rid of _PAGE_EXEC completely for book3e
>> and implement both pte_exec() and pte_mkexec() with _PAGE_BAP_UX and
>> _PAGE_BAP_SX, but I'm not sure it is worth the churn as you say. It
>> would also mean different helpers for book3s/32 when it is using 32 bits
>> PTE (CONFIG_PTE_64BIT=n)
> 
> That's basically what I mean. And _PAGE_KERNEL_ROX etc would then not
> set the UX bit. But at least for now it seems to be an improvement.
> 

That's already the case:

#define _PAGE_KERNEL_RWX	(_PAGE_BAP_SW | _PAGE_BAP_SR | _PAGE_DIRTY | 
_PAGE_BAP_SX)
#define _PAGE_KERNEL_ROX	(_PAGE_BAP_SR | _PAGE_BAP_SX)

Christophe

Re: [PATCH 2/3] powerpc/book3e: Fix set_memory_x() and set_memory_nx()

[Nicholas Piggin]

Excerpts from Christophe Leroy's message of October 27, 2021 3:50 pm:
> 
> 
> Le 27/10/2021 à 07:27, Nicholas Piggin a écrit :
>> Excerpts from Christophe Leroy's message of October 27, 2021 2:55 pm:
>>>
>>>
>>> Le 27/10/2021 à 06:44, Nicholas Piggin a écrit :
>>>> Excerpts from Christophe Leroy's message of October 26, 2021 3:39 pm:
>>>>> set_memory_x() calls pte_mkexec() which sets _PAGE_EXEC.
>>>>> set_memory_nx() calls pte_exprotec() which clears _PAGE_EXEC.
>>>>>
>>>>> Book3e has 2 bits, UX and SX, which defines the exec rights
>>>>> resp. for user (PR=1) and for kernel (PR=0).
>>>>>
>>>>> _PAGE_EXEC is defined as UX only.
>>>>>
>>>>> An executable kernel page is set with either _PAGE_KERNEL_RWX
>>>>> or _PAGE_KERNEL_ROX, which both have SX set and UX cleared.
>>>>>
>>>>> So set_memory_nx() call for an executable kernel page does
>>>>> nothing because UX is already cleared.
>>>>>
>>>>> And set_memory_x() on a non-executable kernel page makes it
>>>>> executable for the user and keeps it non-executable for kernel.
>>>>>
>>>>> Also, pte_exec() always returns 'false' on kernel pages, because
>>>>> it checks _PAGE_EXEC which doesn't include SX, so for instance
>>>>> the W+X check doesn't work.
>>>>>
>>>>> To fix this:
>>>>> - change tlb_low_64e.S to use _PAGE_BAP_UX instead of _PAGE_USER
>>>>> - sets both UX and SX in _PAGE_EXEC so that pte_user() returns
>>>>> true whenever one of the two bits is set
>>>>
>>>> I don't understand this change. Which pte_user() returns true after
>>>> this change? Or do you mean pte_exec()?
>>>
>>> Oops, yes, I mean pte_exec()
>>>
>>> Unless I have to re-spin, can Michael eventually fix that typo while
>>> applying ?
>>>
>>>>
>>>> Does this filter through in some cases at least for kernel executable
>>>> PTEs will get both bits set? Seems cleaner to distinguish user and
>>>> kernel exec for that but maybe it's a lot of churn?
>>>
>>> Didn't understand what you mean.
>>>
>>> I did it like that to be able to continue using _PAGE_EXEC for checking
>>> executability regardless of whether this is user or kernel, and then
>>> continue using the generic nohash pte_exec() helper.
>>>
>>> Other solution would be to get rid of _PAGE_EXEC completely for book3e
>>> and implement both pte_exec() and pte_mkexec() with _PAGE_BAP_UX and
>>> _PAGE_BAP_SX, but I'm not sure it is worth the churn as you say. It
>>> would also mean different helpers for book3s/32 when it is using 32 bits
>>> PTE (CONFIG_PTE_64BIT=n)
>> 
>> That's basically what I mean. And _PAGE_KERNEL_ROX etc would then not
>> set the UX bit. But at least for now it seems to be an improvement.
>> 
> 
> That's already the case:
> 
> #define _PAGE_KERNEL_RWX	(_PAGE_BAP_SW | _PAGE_BAP_SR | _PAGE_DIRTY | 
> _PAGE_BAP_SX)
> #define _PAGE_KERNEL_ROX	(_PAGE_BAP_SR | _PAGE_BAP_SX)

So it is, I was looking at the wrong header.

Looks okay to me then, for what it's worth.

Thanks,
Nick

Re: [PATCH 2/3] powerpc/book3e: Fix set_memory_x() and set_memory_nx()

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 27/10/2021 à 06:44, Nicholas Piggin a écrit :
>> Excerpts from Christophe Leroy's message of October 26, 2021 3:39 pm:
>>> set_memory_x() calls pte_mkexec() which sets _PAGE_EXEC.
>>> set_memory_nx() calls pte_exprotec() which clears _PAGE_EXEC.
>>>
>>> Book3e has 2 bits, UX and SX, which defines the exec rights
>>> resp. for user (PR=1) and for kernel (PR=0).
>>>
>>> _PAGE_EXEC is defined as UX only.
>>>
>>> An executable kernel page is set with either _PAGE_KERNEL_RWX
>>> or _PAGE_KERNEL_ROX, which both have SX set and UX cleared.
>>>
>>> So set_memory_nx() call for an executable kernel page does
>>> nothing because UX is already cleared.
>>>
>>> And set_memory_x() on a non-executable kernel page makes it
>>> executable for the user and keeps it non-executable for kernel.
>>>
>>> Also, pte_exec() always returns 'false' on kernel pages, because
>>> it checks _PAGE_EXEC which doesn't include SX, so for instance
>>> the W+X check doesn't work.
>>>
>>> To fix this:
>>> - change tlb_low_64e.S to use _PAGE_BAP_UX instead of _PAGE_USER
>>> - sets both UX and SX in _PAGE_EXEC so that pte_user() returns
>>> true whenever one of the two bits is set
>> 
>> I don't understand this change. Which pte_user() returns true after
>> this change? Or do you mean pte_exec()?
>
> Oops, yes, I mean pte_exec()
>
> Unless I have to re-spin, can Michael eventually fix that typo while 
> applying ?

I did.

cheers

Re: [PATCH 1/3] powerpc/nohash: Fix __ptep_set_access_flags() and ptep_set_wrprotect()

[Michael Ellerman]

On Tue, 26 Oct 2021 07:39:24 +0200, Christophe Leroy wrote:
> Commit 26973fa5ac0e ("powerpc/mm: use pte helpers in generic code")
> changed those two functions to use pte helpers to determine which
> bits to clear and which bits to set.
> 
> This change was based on the assumption that bits to be set/cleared
> are always the same and can be determined by applying the pte
> manipulation helpers on __pte(0).
> 
> [...]

Applied to powerpc/next.

[1/3] powerpc/nohash: Fix __ptep_set_access_flags() and ptep_set_wrprotect()
      https://git.kernel.org/powerpc/c/b1b93cb7e794e914787bf7d9936b57a149cdee4f
[2/3] powerpc/book3e: Fix set_memory_x() and set_memory_nx()
      https://git.kernel.org/powerpc/c/b6cb20fdc2735f8b2e082937066c33fe376c2ee2
[3/3] powerpc/fsl_booke: Fix setting of exec flag when setting TLBCAMs
      https://git.kernel.org/powerpc/c/44c14509b0dabb909ad1ec28800893ea71762732

cheers