Re: [PATCH v11 09/11] poweprc: mm: Implement *_user_accessible_page() for ptes

[Rohan McLure <rmclure@linux.ibm.com>]

On Thu, 2024-03-28 at 05:40 +0000, Christophe Leroy wrote:
> 
> 
> Le 28/03/2024 à 05:55, Rohan McLure a écrit :
> > Page table checking depends on architectures providing an
> > implementation of p{te,md,ud}_user_accessible_page. With
> > refactorisations made on powerpc/mm, the pte_access_permitted() and
> > similar methods verify whether a userland page is accessible with
> > the
> > required permissions.
> > 
> > Since page table checking is the only user of
> > p{te,md,ud}_user_accessible_page(), implement these for all
> > platforms,
> > using some of the same preliminary checks taken by
> > pte_access_permitted()
> > on that platform.
> > 
> > Since Commit 8e9bd41e4ce1 ("powerpc/nohash: Replace pte_user() by
> > pte_read()")
> > pte_user() is no longer required to be present on all platforms as
> > it
> > may be equivalent to or implied by pte_read(). Hence
> > implementations of
> > pte_user_accessible_page() are specialised.
> > 
> > Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> > ---
> > v9: New implementation
> > v10: Let book3s/64 use pte_user(), but otherwise default other
> > platforms
> > to using the address provided with the call to infer whether it is
> > a
> > user page or not. pmd/pud variants will warn on all other
> > platforms, as
> > they should not be used for user page mappings
> > v11: Conditionally define p{m,u}d_user_accessible_page(), as not
> > all
> > platforms have p{m,u}d_leaf(), p{m,u}d_pte() stubs.
> 
> See my comment to v10 patch 10.
> 
> p{m,u}d_leaf() is defined for all platforms (There is a fallback 
> definition in include/linux/pgtable.h) so
> p{m,u}d_user_accessible_page() 
> can be defined for all platforms, no need for a conditionally define.

The issue I see is that the definition in include/linux/pgtable.h
occurs after this header is included. Prior to the removal of a local
definition of p{m,u}d_leaf() etc we didn't run into this issue, but we
still do now.

Not insistent on doing it this way with ifndef, so amenable to
suggestions if you have a preference.

> 
> > ---
> >   arch/powerpc/include/asm/book3s/32/pgtable.h |  5 +++++
> >   arch/powerpc/include/asm/book3s/64/pgtable.h | 17
> > +++++++++++++++++
> >   arch/powerpc/include/asm/nohash/pgtable.h    |  5 +++++
> >   arch/powerpc/include/asm/pgtable.h           |  8 ++++++++
> >   4 files changed, 35 insertions(+)
> > 
> > diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h
> > b/arch/powerpc/include/asm/book3s/32/pgtable.h
> > index 52971ee30717..83f7b98ef49f 100644
> > --- a/arch/powerpc/include/asm/book3s/32/pgtable.h
> > +++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
> > @@ -436,6 +436,11 @@ static inline bool pte_access_permitted(pte_t
> > pte, bool write)
> >   	return true;
> >   }
> >   
> > +static inline bool pte_user_accessible_page(pte_t pte, unsigned
> > long addr)
> > +{
> > +	return pte_present(pte) && !is_kernel_addr(addr);
> > +}
> > +
> >   /* 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/book3s/64/pgtable.h
> > b/arch/powerpc/include/asm/book3s/64/pgtable.h
> > index fac5615e6bc5..d8640ddbcad1 100644
> > --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> > +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> > @@ -538,6 +538,11 @@ static inline bool pte_access_permitted(pte_t
> > pte, bool write)
> >   	return arch_pte_access_permitted(pte_val(pte), write, 0);
> >   }
> >   
> > +static inline bool pte_user_accessible_page(pte_t pte, unsigned
> > long addr)
> > +{
> > +	return pte_present(pte) && pte_user(pte);
> > +}
> > +
> >   /*
> >    * Conversion functions: convert a page and protection to a page
> > entry,
> >    * and a page entry and page directory to the page they refer to.
> > @@ -1441,5 +1446,17 @@ static inline bool pud_leaf(pud_t pud)
> >   	return !!(pud_raw(pud) & cpu_to_be64(_PAGE_PTE));
> >   }
> >   
> > +#define pmd_user_accessible_page pmd_user_accessible_page
> > +static inline bool pmd_user_accessible_page(pmd_t pmd, unsigned
> > long addr)
> > +{
> > +	return pmd_leaf(pmd) &&
> > pte_user_accessible_page(pmd_pte(pmd), addr);
> > +}
> > +
> > +#define pud_user_accessible_page pud_user_accessible_page
> > +static inline bool pud_user_accessible_page(pud_t pud, unsigned
> > long addr)
> > +{
> > +	return pud_leaf(pud) &&
> > pte_user_accessible_page(pud_pte(pud), addr);
> > +}
> > +
> >   #endif /* __ASSEMBLY__ */
> >   #endif /* _ASM_POWERPC_BOOK3S_64_PGTABLE_H_ */
> > diff --git a/arch/powerpc/include/asm/nohash/pgtable.h
> > b/arch/powerpc/include/asm/nohash/pgtable.h
> > index 427db14292c9..413d01a51e6f 100644
> > --- a/arch/powerpc/include/asm/nohash/pgtable.h
> > +++ b/arch/powerpc/include/asm/nohash/pgtable.h
> > @@ -213,6 +213,11 @@ static inline bool pte_access_permitted(pte_t
> > pte, bool write)
> >   	return true;
> >   }
> >   
> > +static inline bool pte_user_accessible_page(pte_t pte, unsigned
> > long addr)
> > +{
> > +	return pte_present(pte) && !is_kernel_addr(addr);
> > +}
> > +
> >   /* 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/pgtable.h
> > b/arch/powerpc/include/asm/pgtable.h
> > index ee8c82c0528f..f1ceae778cb1 100644
> > --- a/arch/powerpc/include/asm/pgtable.h
> > +++ b/arch/powerpc/include/asm/pgtable.h
> > @@ -219,6 +219,14 @@ static inline int pud_pfn(pud_t pud)
> >   }
> >   #endif
> >   
> > +#ifndef pmd_user_accessible_page
> > +#define pmd_user_accessible_page(pmd, addr)	false
> > +#endif
> > +
> > +#ifndef pud_user_accessible_page
> > +#define pud_user_accessible_page(pud, addr)	false
> > +#endif
> > +
> >   #endif /* __ASSEMBLY__ */
> >   
> >   #endif /* _ASM_POWERPC_PGTABLE_H */