Re: [PATCH 2/3] powerpc: mm: add p{te,md,ud}_user_accessible_page helpers

[Christophe Leroy <christophe.leroy@csgroup.eu>]

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-=wg40EAZofO16Eviaj7mfqDhZ2gVEbvfsMf6gYzspRjYvw@mail.gmail.com/

> 
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
>   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();