[PATCH 1/3] powerpc/code-patching: Add generic memory patching
Christophe Leroy
christophe.leroy at csgroup.eu
Thu Feb 9 18:15:21 AEDT 2023
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 <bgray at 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)
> {
More information about the Linuxppc-dev
mailing list