Re: [PATCH] powerpc/mm: Support execute-only memory on the Radix MMU

[Aneesh Kumar K V <aneesh.kumar@linux.ibm.com>]

On 8/8/22 5:28 PM, Russell Currey wrote:
> The Hash MMU already supports XOM (i.e. mmap with PROT_EXEC only)
> through the execute-only pkey.  A PROT_ONLY mapping will actually map to
> RX, and then the pkey will be applied on top of it.
> 
> Radix doesn't have pkeys, but it does have execute permissions built-in
> to the MMU, so all we have to do to support XOM is expose it.
> 
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
> quick test: https://raw.githubusercontent.com/ruscur/junkcode/main/mmap_test.c
> I can make it a selftest.
> 
>  arch/powerpc/include/asm/book3s/64/radix.h |  3 +++
>  arch/powerpc/mm/book3s64/radix_pgtable.c   |  4 ++++
>  arch/powerpc/mm/fault.c                    | 10 ++++++++++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/book3s/64/radix.h b/arch/powerpc/include/asm/book3s/64/radix.h
> index 686001eda936..bf316b773d73 100644
> --- a/arch/powerpc/include/asm/book3s/64/radix.h
> +++ b/arch/powerpc/include/asm/book3s/64/radix.h
> @@ -19,6 +19,9 @@
>  #include <asm/cpu_has_feature.h>
>  #endif
>  
> +/* Execute-only page protections, Hash can use RX + execute-only pkey */
> +#define PAGE_EXECONLY	__pgprot(_PAGE_BASE | _PAGE_EXEC)
> +
>  /* An empty PTE can still have a R or C writeback */
>  #define RADIX_PTE_NONE_MASK		(_PAGE_DIRTY | _PAGE_ACCESSED)
>  
> diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
> index 698274109c91..2edb56169805 100644
> --- a/arch/powerpc/mm/book3s64/radix_pgtable.c
> +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
> @@ -617,6 +617,10 @@ void __init radix__early_init_mmu(void)
>  	__pmd_frag_nr = RADIX_PMD_FRAG_NR;
>  	__pmd_frag_size_shift = RADIX_PMD_FRAG_SIZE_SHIFT;
>  
> +	/* Radix directly supports execute-only page protections */
> +	protection_map[VM_EXEC] = PAGE_EXECONLY;
> +	protection_map[VM_EXEC | VM_SHARED] = PAGE_EXECONLY;
> +
>  	radix_init_pgtable();
>  
>  	if (!firmware_has_feature(FW_FEATURE_LPAR)) {
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 014005428687..887c0cc45ca6 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -270,6 +270,16 @@ static bool access_error(bool is_write, bool is_exec, struct vm_area_struct *vma
>  		return false;
>  	}
>  
> +	if (unlikely(!(vma->vm_flags & VM_READ))) {
> +		/*
> +		 * If we're on Radix, then this could be a read attempt on
> +		 * execute-only memory.  On other MMUs, an "exec-only" page
> +		 * will be given RX flags, so this might be redundant.
> +		 */
> +		if (radix_enabled())
> +			return true;
> +	}
> +


should we do 

/* This cover both PROT_NONE (due to check above) and exec only mapping */
if (radix_enabled() && !(vma->vm_flags & VM_READ)) {
    return true;
/* PROT_NONE check */
else if (!vma_is_accessible(vma)) 
   return true;

return false;



>  	if (unlikely(!vma_is_accessible(vma)))
>  		return true;
>  	/*

-aneesh