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@xxxxxxxxxx>
> > ---
> > 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