[PATCH 2/3] powerpc: mm: add p{te,md,ud}_user_accessible_page helpers
Add the following helpers for detecting whether a page table entry
is a leaf and is accessible to user space.
* pte_user_accessible_page
* pmd_user_accessible_page
* pud_user_accessible_page
The heavy lifting is done by pte_user, which checks user accessibility
on a per-mmu level, provided prior to this commit.
On 32-bit systems, provide stub implementations for these methods, with
BUG(), as debug features such as page table checks will emit functions
that call p{md,ud}_user_accessible_page but must not be used.
Signed-off-by: Rohan McLure
---
arch/powerpc/include/asm/pgtable.h | 35
1 file changed, 35 insertions(+)
diff --git a/arch/powerpc/include/asm/pgtable.h
b/arch/powerpc/include/asm/pgtable.h
index 522145b16a07..8c1f5feb9360 100644
--- a/arch/powerpc/include/asm/pgtable.h
+++ b/arch/powerpc/include/asm/pgtable.h
@@ -170,6 +170,41 @@ static inline int pud_pfn(pud_t pud)
return 0;
}
+static inline bool pte_user_accessible_page(pte_t pte)
+{
+ return (pte_val(pte) & _PAGE_PRESENT) && pte_user(pte);
+}
+
+#ifdef CONFIG_PPC64
+
+static inline bool pmd_user_accessible_page(pmd_t pmd)
+{
+ return pmd_is_leaf(pmd) && pmd_present(pmd)
+ && pte_user(pmd_pte(pmd));
+}
+
+static inline bool pud_user_accessible_page(pud_t pud)
+{
+ return pud_is_leaf(pud) && pud_present(pud)
+ && pte_user(pud_pte(pud));
+}
+
+#else
+
+static inline bool pmd_user_accessible_page(pmd_t pmd)
+{
+ BUG();
+ return false;
+}
+
+static inline bool pud_user_accessible_page(pud_t pud)
+{
+ BUG();
+ return false;
+}
+
+#endif /* CONFIG_PPC64 */
+
#endif /* __ASSEMBLY__ */
#endif /* _ASM_POWERPC_PGTABLE_H */
--
2.34.1
Re: [PATCH 2/3] powerpc: mm: add p{te,md,ud}_user_accessible_page helpers
Le 12/09/2022 à 03:47, Rohan McLure a écrit :
> Add the following helpers for detecting whether a page table entry
> is a leaf and is accessible to user space.
>
> * pte_user_accessible_page
> * pmd_user_accessible_page
> * pud_user_accessible_page
>
> The heavy lifting is done by pte_user, which checks user accessibility
> on a per-mmu level, provided prior to this commit.
Not in this series it seems, so I guess that commit is already in the
tree. Can you reference it ?
>
> On 32-bit systems, provide stub implementations for these methods, with
> BUG(), as debug features such as page table checks will emit functions
> that call p{md,ud}_user_accessible_page but must not be used.
Please please please no new BUG or BUG_ON, please see the following
discussion and especially the position of Linus Torvalds :
https://lore.kernel.org/all/CAHk-=wg40eazofo16eviaj7mfqdhz2gvebvfsmf6gyzsprj...@mail.gmail.com/
>
> Signed-off-by: Rohan McLure
> ---
> arch/powerpc/include/asm/pgtable.h | 35
> 1 file changed, 35 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/pgtable.h
> b/arch/powerpc/include/asm/pgtable.h
> index 522145b16a07..8c1f5feb9360 100644
> --- a/arch/powerpc/include/asm/pgtable.h
> +++ b/arch/powerpc/include/asm/pgtable.h
> @@ -170,6 +170,41 @@ static inline int pud_pfn(pud_t pud)
> return 0;
> }
>
> +static inline bool pte_user_accessible_page(pte_t pte)
> +{
> + return (pte_val(pte) & _PAGE_PRESENT) && pte_user(pte);
> +}
> +
> +#ifdef CONFIG_PPC64
> +
> +static inline bool pmd_user_accessible_page(pmd_t pmd)
> +{
> + return pmd_is_leaf(pmd) && pmd_present(pmd)
> + && pte_user(pmd_pte(pmd));
The && goes on previous line.
> +}
> +
> +static inline bool pud_user_accessible_page(pud_t pud)
> +{
> + return pud_is_leaf(pud) && pud_present(pud)
> + && pte_user(pud_pte(pud));
Same
> +}
> +
> +#else
> +
> +static inline bool pmd_user_accessible_page(pmd_t pmd)
> +{
> + BUG();
Can you use BUILD_BUG() instead ?
> + return false;
> +}
> +
> +static inline bool pud_user_accessible_page(pud_t pud)
> +{
> + BUG();
Same.
> + return false;
> +}
> +
> +#endif /* CONFIG_PPC64 */
> +
> #endif /* __ASSEMBLY__ */
>
> #endif /* _ASM_POWERPC_PGTABLE_H */
By the way, there is a great tool that can help you :
$ ./arch/powerpc/tools/checkpatch.sh --strict -g HEAD~
CHECK:LOGICAL_CONTINUATIONS: Logical continuations should be on the
previous line
#43: FILE: arch/powerpc/include/asm/pgtable.h:183:
+ return pmd_is_leaf(pmd) && pmd_present(pmd)
+ && pte_user(pmd_pte(pmd));
CHECK:LOGICAL_CONTINUATIONS: Logical continuations should be on the
previous line
#49: FILE: arch/powerpc/include/asm/pgtable.h:189:
+ return pud_is_leaf(pud) && pud_present(pud)
+ && pte_user(pud_pte(pud));
WARNING:AVOID_BUG: Avoid crashing the kernel - try using WARN_ON &
recovery code rather than BUG() or BUG_ON()
#56: FILE: arch/powerpc/include/asm/pgtable.h:196:
+ BUG();
WARNING:AVOID_BUG: Avoid crashing the kernel - try using WARN_ON &
recovery code rather than BUG() or BUG_ON()
#62: FILE: arch/powerpc/include/asm/pgtable.h:202:
+ BUG();
Re: [PATCH 2/3] powerpc: mm: add p{te,md,ud}_user_accessible_page helpers
>> +static inline bool pmd_user_accessible_page(pmd_t pmd)
>> +{
>> +return pmd_is_leaf(pmd) && pmd_present(pmd)
>> +&& pte_user(pmd_pte(pmd));
>
> The && goes on previous line.
> By the way, there is a great tool that can help you :
>
> $ ./arch/powerpc/tools/checkpatch.sh --strict -g HEAD~
Thank you. Yes I should be using --strict with checkpatch.
>
>> +}
>> +
>> +#else
>> +
>> +static inline bool pmd_user_accessible_page(pmd_t pmd)
>> +{
>> +BUG();
>
> Can you use BUILD_BUG() instead ?
As it stands, p{m,u}d_user_accessible page is invoked by
__page_table_check_p{m,u}d_{set,clear}. So unfortunately this
function will have to be compiled, but its body could be replaced
with a WARN() and return false on 32-bit.
Re: [PATCH 2/3] powerpc: mm: add p{te,md,ud}_user_accessible_page helpers
Le 12/09/2022 à 03:47, Rohan McLure a écrit :
> Add the following helpers for detecting whether a page table entry
> is a leaf and is accessible to user space.
>
> * pte_user_accessible_page
> * pmd_user_accessible_page
> * pud_user_accessible_page
>
> The heavy lifting is done by pte_user, which checks user accessibility
> on a per-mmu level, provided prior to this commit.
>
> On 32-bit systems, provide stub implementations for these methods, with
> BUG(), as debug features such as page table checks will emit functions
> that call p{md,ud}_user_accessible_page but must not be used.
Why "must not be used" ?
>
> Signed-off-by: Rohan McLure
> ---
> arch/powerpc/include/asm/pgtable.h | 35
> 1 file changed, 35 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/pgtable.h
> b/arch/powerpc/include/asm/pgtable.h
> index 522145b16a07..8c1f5feb9360 100644
> --- a/arch/powerpc/include/asm/pgtable.h
> +++ b/arch/powerpc/include/asm/pgtable.h
> @@ -170,6 +170,41 @@ static inline int pud_pfn(pud_t pud)
> return 0;
> }
>
> +static inline bool pte_user_accessible_page(pte_t pte)
> +{
> + return (pte_val(pte) & _PAGE_PRESENT) && pte_user(pte);
Should use pte_present() instead ?
> +}
> +
> +#ifdef CONFIG_PPC64
I think the functions could be valid for PPC32 as well with below changes:
> +
> +static inline bool pmd_user_accessible_page(pmd_t pmd)
> +{
> + return pmd_is_leaf(pmd) && pmd_present(pmd)
> + && pte_user(pmd_pte(pmd));
Define and use pmd_user()
> +}
> +
> +static inline bool pud_user_accessible_page(pud_t pud)
> +{
> + return pud_is_leaf(pud) && pud_present(pud)
> + && pte_user(pud_pte(pud));
Use pud_user().
For ppc64 you have to define it in
arch/powerpc/include/asm/book3s/64/pgtable.h and
arch/powerpc/include/asm/nohash/64/pgtable.h
For ppc32 it is already defined in include/asm-generic/pgtable-nopmd.h
And use pud_leaf() instead of pud_is_leaf(). For ppc32 it is already
defined in include/asm-generic/pgtable-nopmd.h
> +}
> +
> +#else
> +
> +static inline bool pmd_user_accessible_page(pmd_t pmd)
> +{
> + BUG();
> + return false;
> +}
> +
> +static inline bool pud_user_accessible_page(pud_t pud)
> +{
> + BUG();
> + return false;
> +}
> +
> +#endif /* CONFIG_PPC64 */
> +
> #endif /* __ASSEMBLY__ */
>
> #endif /* _ASM_POWERPC_PGTABLE_H */
