[PATCH 1/6] powerpc/code-patching: Implement generic text patching function

Christophe Leroy christophe.leroy at csgroup.eu
Mon Sep 19 16:04:22 AEST 2022



Le 16/09/2022 à 08:23, Benjamin Gray a écrit :
> Adds a generic text patching mechanism for patches of 1, 2, 4, or 8
> bytes. The patcher conditionally syncs the icache depending on if
> the content will be executed (as opposed to, e.g., read-only data).
> 
> The `patch_instruction` function is reimplemented in terms of this
> more generic function. This generic implementation allows patching of
> arbitrary 64-bit data, whereas the original `patch_instruction` decided
> the size based on the 'instruction' opcode, so was not suitable for
> arbitrary data.

With CONFIG_STRICT_KERNEL_RWX, this patches causes a 15% time increase 
for activation/deactivation of ftrace.

Without CONFIG_STRICT_KERNEL_RWX, it doesn't build.

> 
> Signed-off-by: Benjamin Gray <bgray at linux.ibm.com>
> ---
>   arch/powerpc/include/asm/code-patching.h |   1 +
>   arch/powerpc/lib/code-patching.c         | 135 +++++++++++++++--------
>   2 files changed, 89 insertions(+), 47 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> index 1c6316ec4b74..6a52c19dae46 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -76,6 +76,7 @@ int create_cond_branch(ppc_inst_t *instr, const u32 *addr,
>   int patch_branch(u32 *addr, unsigned long target, int flags);
>   int patch_instruction(u32 *addr, ppc_inst_t instr);
>   int raw_patch_instruction(u32 *addr, ppc_inst_t instr);
> +int patch_text_data(void *dest, const void *src, size_t size);
>   
>   static inline unsigned long patch_site_addr(s32 *site)
>   {
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index ad0cf3108dd0..a09a0898c2ce 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -3,6 +3,7 @@
>    *  Copyright 2008 Michael Ellerman, IBM Corporation.
>    */
>   
> +#include <linux/mm.h>
>   #include <linux/kprobes.h>
>   #include <linux/vmalloc.h>
>   #include <linux/init.h>
> @@ -14,32 +15,7 @@
>   #include <asm/page.h>
>   #include <asm/code-patching.h>
>   #include <asm/inst.h>
> -
> -static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr, u32 *patch_addr)
> -{
> -	if (!ppc_inst_prefixed(instr)) {
> -		u32 val = ppc_inst_val(instr);
> -
> -		__put_kernel_nofault(patch_addr, &val, u32, failed);
> -	} else {
> -		u64 val = ppc_inst_as_ulong(instr);
> -
> -		__put_kernel_nofault(patch_addr, &val, u64, failed);
> -	}
> -
> -	asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),
> -							    "r" (exec_addr));
> -
> -	return 0;
> -
> -failed:
> -	return -EPERM;
> -}
> -
> -int raw_patch_instruction(u32 *addr, ppc_inst_t instr)
> -{
> -	return __patch_instruction(addr, instr, addr);
> -}
> +#include <asm/cacheflush.h>
>   
>   #ifdef CONFIG_STRICT_KERNEL_RWX
>   static DEFINE_PER_CPU(struct vm_struct *, text_poke_area);
> @@ -147,16 +123,44 @@ static void unmap_patch_area(unsigned long addr)
>   	flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
>   }
>   
> -static int __do_patch_instruction(u32 *addr, ppc_inst_t instr)
> +static int __patch_text(void *dest, const void *src, size_t size, bool is_exec, void *exec_addr)

Is 'text' a good name ? For me text mean executable code. Should it be 
__patch_memory() ?


Why pass src as a void * ? This forces data to go via the stack. Can't 
you pass it as a 'long' ?

> +{
> +	if (virt_to_pfn(dest) != virt_to_pfn(dest + size - 1))
> +		return -EFAULT;

Why do you need that new check ?

> +
> +	switch (size) {
> +		case 1:
> +			__put_kernel_nofault(dest, src, u8, failed);
> +			break;
> +		case 2:
> +			__put_kernel_nofault(dest, src, u16, failed);
> +			break;
> +		case 4:
> +			__put_kernel_nofault(dest, src, u32, failed);
> +			break;
> +		case 8:
> +			__put_kernel_nofault(dest, src, u64, failed);
> +			break;

Is case 8 needed for PPC32 ?

> +	}

Do you catch it when size if none of 1,2,4,8 ?

> +
> +	asm ("dcbst 0, %0; sync" :: "r" (dest));

Maybe write it in C:

	dcbst(dest);
	mb(); /* sync */

> +
> +	if (is_exec)
> +		asm ("icbi 0,%0; sync; isync" :: "r" (exec_addr));

Same, can be:

	if (is_exec) {
		icbi(exec_addr);
		mb(); /* sync */
		isync();
	}

Or keep it flat:

	if (!is_exec)
		return 0;

	icbi(exec_addr);
	mb(); /* sync */
	isync();

	return 0;

> +
> +	return 0;
> +
> +failed:
> +	return -EPERM;
> +}
> +
> +static pte_t *start_text_patch(void* dest, u32 **patch_addr)
>   {
> -	int err;
> -	u32 *patch_addr;
> -	unsigned long text_poke_addr;
>   	pte_t *pte;
> -	unsigned long pfn = get_patch_pfn(addr);
> +	unsigned long text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr & PAGE_MASK;
> +	unsigned long pfn = get_patch_pfn(dest);
>   
> -	text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr & PAGE_MASK;
> -	patch_addr = (u32 *)(text_poke_addr + offset_in_page(addr));
> +	*patch_addr = (u32 *)(text_poke_addr + offset_in_page(dest));
>   
>   	pte = virt_to_kpte(text_poke_addr);
>   	__set_pte_at(&init_mm, text_poke_addr, pte, pfn_pte(pfn, PAGE_KERNEL), 0);
> @@ -164,33 +168,63 @@ static int __do_patch_instruction(u32 *addr, ppc_inst_t instr)
>   	if (radix_enabled())
>   		asm volatile("ptesync": : :"memory");
>   
> -	err = __patch_instruction(addr, instr, patch_addr);
> +	return pte;
> +}
>   
> +static void finish_text_patch(pte_t *pte)
> +{
> +	unsigned long text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr & PAGE_MASK;

Leave a blank line after variables declaration.

>   	pte_clear(&init_mm, text_poke_addr, pte);
>   	flush_tlb_kernel_range(text_poke_addr, text_poke_addr + PAGE_SIZE);
> +}
> +
> +static int do_patch_text(void *dest, const void *src, size_t size, bool is_exec)
> +{
> +	int err;
> +	pte_t *pte;
> +	u32 *patch_addr;
> +
> +	pte = start_text_patch(dest, &patch_addr);
> +	err = __patch_text(patch_addr, src, size, is_exec, dest);
> +	finish_text_patch(pte);

Why do you need to split this function in three parts ? I can't see the 
added value, all it does is reduce readability.

Did you check the impact of calling __this_cpu_read() twice ?

>   
>   	return err;
>   }
>   
> -static int do_patch_instruction(u32 *addr, ppc_inst_t instr)
> +static int patch_text(void *dest, const void *src, size_t size, bool is_exec)

Same, do you need the source data to go via stack ?

>   {
>   	int err;
>   	unsigned long flags;
>   
> -	/*
> -	 * During early early boot patch_instruction is called
> -	 * when text_poke_area is not ready, but we still need
> -	 * to allow patching. We just do the plain old patching
> -	 */
> +	/* Make sure we aren't patching a freed init section */
> +	if (static_branch_likely(&init_mem_is_free) && init_section_contains(dest, 4))
> +		return 0;
> +
>   	if (!static_branch_likely(&poking_init_done))
> -		return raw_patch_instruction(addr, instr);
> +		return __patch_text(dest, src, size, is_exec, dest);
>   
>   	local_irq_save(flags);
> -	err = __do_patch_instruction(addr, instr);
> +	err = do_patch_text(dest, src, size, is_exec);
>   	local_irq_restore(flags);
>   
>   	return err;
>   }
> +
> +int patch_text_data(void *dest, const void *src, size_t size) {

{ must be on next line for functions start.

> +	return patch_text(dest, src, size, false);
> +}
> +
> +int raw_patch_instruction(u32 *addr, ppc_inst_t instr)
> +{
> +	if (!ppc_inst_prefixed(instr)) {
> +		u32 val = ppc_inst_val(instr);
> +		return __patch_text(addr, &val, sizeof(val), true, addr);
> +	} else {
> +		u64 val = ppc_inst_as_ulong(instr);
> +		return __patch_text(addr, &val, sizeof(val), true, addr);
> +	}
> +}
> +
>   #else /* !CONFIG_STRICT_KERNEL_RWX */
>   
>   static int do_patch_instruction(u32 *addr, ppc_inst_t instr)
> @@ -198,17 +232,24 @@ static int do_patch_instruction(u32 *addr, ppc_inst_t instr)
>   	return raw_patch_instruction(addr, instr);
>   }
>   
> +void *patch_memory(void *dest, const void *src, size_t size)

What is this function used for ?

> +{
> +	return memcpy(dest, src, size);
> +}
> +
>   #endif /* CONFIG_STRICT_KERNEL_RWX */
>   
>   __ro_after_init DEFINE_STATIC_KEY_FALSE(init_mem_is_free);
>   
>   int patch_instruction(u32 *addr, ppc_inst_t instr)
>   {
> -	/* Make sure we aren't patching a freed init section */
> -	if (static_branch_likely(&init_mem_is_free) && init_section_contains(addr, 4))
> -		return 0;
> -
> -	return do_patch_instruction(addr, instr);
> +	if (!ppc_inst_prefixed(instr)) {
> +		u32 val = ppc_inst_val(instr);
> +		return patch_text(addr, &val, sizeof(val), true);
> +	} else {
> +		u64 val = ppc_inst_as_ulong(instr);
> +		return patch_text(addr, &val, sizeof(val), true);
> +	}
>   }
>   NOKPROBE_SYMBOL(patch_instruction);
>   


More information about the Linuxppc-dev mailing list