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 <bg...@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); >