Le 07/02/2023 à 02:56, Benjamin Gray a écrit : > patch_instruction() is designed for patching instructions in otherwise > readonly memory. Other consumers also sometimes need to patch readonly > memory, so have abused patch_instruction() for arbitrary data patches. > > This is a problem on ppc64 as patch_instruction() decides on the patch > width using the 'instruction' opcode to see if it's a prefixed > instruction. Data that triggers this can lead to larger writes, possibly > crossing a page boundary and failing the write altogether. > > Introduce patch_uint(), and patch_ulong(), with aliases patch_u32(), and > patch_u64() (on ppc64) designed for aligned data patches. The patch > size is now determined by the called function, and is passed as an > additional parameter to generic internals. > > While the instruction flushing is not required for data patches, the > use cases for data patching (mainly module loading and static calls) > are less performance sensitive than for instruction patching > (ftrace activation). So the instruction flushing remains unconditional > in this patch. > > ppc32 does not support prefixed instructions, so is unaffected by the > original issue. Care is taken in not exposing the size parameter in the > public (non-static) interface, so the compiler can const-propagate it > away. > > Signed-off-by: Benjamin Gray <bg...@linux.ibm.com> > > --- > > GCC 12.2.1 compiled kernels show the following structure: > > - ppc64le_defconfig (ppc64, strict RWX): patch_{uint,ulong,instruction}() > are small wrappers that tail call into patch_memory() > > - ppc_defconfig (ppc32, strict RWX): patch_uint() is the only full RWX > aware implementation. All use of patch size is eliminated. > > Note: patch_instruction() is marked noinline to prevent making > patch_instruction() a wrapper of patch_memory(); GCC likes to tail > call patch_branch() -> patch_memory(), skipping patch_instruction() > and preventing patch_memory() from inlining inside patch_instruction(). > > - pmac32_defconfig (ppc32, no strict RWX): patch_uint() and > raw_patch_instruction() are both implemented but have the same > ~40 byte function body (as is the case already mind). > --- > arch/powerpc/include/asm/code-patching.h | 33 ++++++++++++ > arch/powerpc/lib/code-patching.c | 69 +++++++++++++++++------- > 2 files changed, 84 insertions(+), 18 deletions(-) > > diff --git a/arch/powerpc/include/asm/code-patching.h > b/arch/powerpc/include/asm/code-patching.h > index 3f881548fb61..89fc4c3f6ecf 100644 > --- a/arch/powerpc/include/asm/code-patching.h > +++ b/arch/powerpc/include/asm/code-patching.h > @@ -75,6 +75,39 @@ 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); > > +/* > + * patch_uint() and patch_ulong() should only be called on addresses where > the > + * patch does not cross a cacheline, otherwise it may not be flushed properly > + * and mixes of new and stale data may be observed. It cannot cross a page > + * boundary, as only the target page is mapped as writable. > + * > + * patch_instruction() and other instruction patchers automatically satisfy > this > + * requirement due to instruction alignment requirements. > + */ > + > +#ifdef CONFIG_PPC64 > + > +int patch_uint(void *addr, unsigned int val); > +#define patch_u32 patch_uint > + > +int patch_ulong(void *addr, unsigned long val); > +#define patch_u64 patch_ulong > + > +#else > + > +static inline int patch_uint(u32 *addr, unsigned int val) > +{ > + return patch_instruction(addr, ppc_inst(val));
Would it make more sense that patch_instruction() calls patch_uint() instead of the reverse ? > +} > +#define patch_u32 patch_uint You can put this outside the ifdef instead of duplicating it. > + > +static inline int patch_ulong(void *addr, unsigned long val) > +{ > + return patch_instruction(addr, ppc_inst(val)); > +} > + > +#endif > + > static inline unsigned long patch_site_addr(s32 *site) > { > return (unsigned long)site + *site; > diff --git a/arch/powerpc/lib/code-patching.c > b/arch/powerpc/lib/code-patching.c > index b00112d7ad46..0f7e9949faf0 100644 > --- a/arch/powerpc/lib/code-patching.c > +++ b/arch/powerpc/lib/code-patching.c > @@ -20,16 +20,14 @@ > #include <asm/code-patching.h> > #include <asm/inst.h> > > -static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr, u32 > *patch_addr) > +static int __patch_memory(void *exec_addr, unsigned long val, void > *patch_addr, > + bool is_dword) > { > - 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); > - > + if (IS_ENABLED(CONFIG_PPC64) && unlikely(is_dword)) { > __put_kernel_nofault(patch_addr, &val, u64, failed); > + } else { > + unsigned int val32 = val; Why unsigned int and not u32 as before ? > + __put_kernel_nofault(patch_addr, &val32, u32, failed); > } > > asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr), > @@ -43,7 +41,10 @@ static int __patch_instruction(u32 *exec_addr, ppc_inst_t > instr, u32 *patch_addr > > int raw_patch_instruction(u32 *addr, ppc_inst_t instr) > { > - return __patch_instruction(addr, instr, addr); > + if (ppc_inst_prefixed(instr)) > + return __patch_memory(addr, ppc_inst_as_ulong(instr), addr, > true); > + else > + return __patch_memory(addr, ppc_inst_val(instr), addr, false); > } > > struct patch_context { > @@ -278,7 +279,7 @@ static void unmap_patch_area(unsigned long addr) > flush_tlb_kernel_range(addr, addr + PAGE_SIZE); > } > > -static int __do_patch_instruction_mm(u32 *addr, ppc_inst_t instr) > +static int __do_patch_memory_mm(void *addr, unsigned long val, bool is_dword) > { > int err; > u32 *patch_addr; > @@ -307,7 +308,7 @@ static int __do_patch_instruction_mm(u32 *addr, > ppc_inst_t instr) > > orig_mm = start_using_temp_mm(patching_mm); > > - err = __patch_instruction(addr, instr, patch_addr); > + err = __patch_memory(addr, val, patch_addr, is_dword); > > /* hwsync performed by __patch_instruction (sync) if successful */ > if (err) > @@ -328,7 +329,7 @@ static int __do_patch_instruction_mm(u32 *addr, > ppc_inst_t instr) > return err; > } > > -static int __do_patch_instruction(u32 *addr, ppc_inst_t instr) > +static int __do_patch_memory(void *addr, unsigned long val, bool is_dword) > { > int err; > u32 *patch_addr; > @@ -345,7 +346,7 @@ 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); > + err = __patch_memory(addr, val, patch_addr, is_dword); > > pte_clear(&init_mm, text_poke_addr, pte); > flush_tlb_kernel_range(text_poke_addr, text_poke_addr + PAGE_SIZE); > @@ -353,7 +354,7 @@ static int __do_patch_instruction(u32 *addr, ppc_inst_t > instr) > return err; > } > > -int patch_instruction(u32 *addr, ppc_inst_t instr) > +static int patch_memory(void *addr, unsigned long val, bool is_dword) > { > int err; > unsigned long flags; > @@ -365,18 +366,50 @@ int patch_instruction(u32 *addr, ppc_inst_t instr) > */ > if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX) || > !static_branch_likely(&poking_init_done)) > - return raw_patch_instruction(addr, instr); > + return __patch_memory(addr, val, addr, is_dword); > > local_irq_save(flags); > if (mm_patch_enabled()) > - err = __do_patch_instruction_mm(addr, instr); > + err = __do_patch_memory_mm(addr, val, is_dword); > else > - err = __do_patch_instruction(addr, instr); > + err = __do_patch_memory(addr, val, is_dword); > local_irq_restore(flags); > > return err; > } > -NOKPROBE_SYMBOL(patch_instruction); > + > +#ifdef CONFIG_PPC64 > + > +int patch_instruction(u32 *addr, ppc_inst_t instr) > +{ > + if (ppc_inst_prefixed(instr)) > + return patch_memory(addr, ppc_inst_as_ulong(instr), true); > + else > + return patch_memory(addr, ppc_inst_val(instr), false); > +} > +NOKPROBE_SYMBOL(patch_instruction) > + > +int patch_uint(void *addr, unsigned int val) > +{ > + return patch_memory(addr, val, false); > +} > +NOKPROBE_SYMBOL(patch_uint) > + > +int patch_ulong(void *addr, unsigned long val) > +{ > + return patch_memory(addr, val, true); > +} > +NOKPROBE_SYMBOL(patch_ulong) > + > +#else > + > +noinline int patch_instruction(u32 *addr, ppc_inst_t instr) > +{ > + return patch_memory(addr, ppc_inst_val(instr), false); > +} A comment explaining the reason for the noinline would be welcome here. By the way, would the noinline change anything on PPC64 ? If not we could have a common function as ppc_inst_prefixed() constant folds to false in PPC32. > +NOKPROBE_SYMBOL(patch_instruction) > + > +#endif > > int patch_branch(u32 *addr, unsigned long target, int flags) > {