Re: [RFC PATCH] powerpc/lib: Fixing use a temporary mm for code patching

[Christopher M Riedl]

> On March 26, 2020 9:42 AM Christophe Leroy <christophe.leroy@xxxxxx> wrote:
> 
>  
> This patch fixes the RFC series identified below.
> It fixes three points:
> - Failure with CONFIG_PPC_KUAP
> - Failure to write do to lack of DIRTY bit set on the 8xx
> - Inadequaly complex WARN post verification
> 
> However, it has an impact on the CPU load. Here is the time
> needed on an 8xx to run the ftrace selftests without and
> with this series:
> - Without CONFIG_STRICT_KERNEL_RWX		==> 38 seconds
> - With CONFIG_STRICT_KERNEL_RWX			==> 40 seconds
> - With CONFIG_STRICT_KERNEL_RWX + this series	==> 43 seconds
> 
> Link: https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003
> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxx>
> ---
>  arch/powerpc/lib/code-patching.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index f156132e8975..4ccff427592e 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -97,6 +97,7 @@ static int map_patch(const void *addr, struct patch_mapping *patch_mapping)
>  	}
>  
>  	pte = mk_pte(page, pgprot);
> +	pte = pte_mkdirty(pte);
>  	set_pte_at(patching_mm, patching_addr, ptep, pte);
>  
>  	init_temp_mm(&patch_mapping->temp_mm, patching_mm);
> @@ -168,7 +169,9 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>  			(offset_in_page((unsigned long)addr) /
>  				sizeof(unsigned int));
>  
> +	allow_write_to_user(patch_addr, sizeof(instr));
>  	__patch_instruction(addr, instr, patch_addr);
> +	prevent_write_to_user(patch_addr, sizeof(instr));
> 

On radix we can map the page with PAGE_KERNEL protection which ends up
setting EAA[0] in the radix PTE. This means the KUAP (AMR) protection is
ignored (ISA v3.0b Fig. 35) since we are accessing the page from MSR[PR]=0.

Can we employ a similar approach on the 8xx? I would prefer *not* to wrap
the __patch_instruction() with the allow_/prevent_write_to_user() KUAP things
because this is a temporary kernel mapping which really isn't userspace in
the usual sense.
 
>  	err = unmap_patch(&patch_mapping);
>  	if (err)
> @@ -179,7 +182,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>  	 * think we just wrote.
>  	 * XXX: BUG_ON() instead?
>  	 */
> -	WARN_ON(memcmp(addr, &instr, sizeof(instr)));
> +	WARN_ON(*addr != instr);
>  
>  out:
>  	local_irq_restore(flags);
> -- 
> 2.25.0