| Message ID | 20231016050147.115686-2-bgray@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add generic data patching functions | expand |
On Mon, Oct 16, 2023 at 04:01:45PM +1100, Benjamin Gray wrote: > 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). That's debatable. While it is nice to be able to activate function tracing quickly, it is not necessarily a hot path. On the flip side, I do have a use case for data patching for 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@linux.ibm.com> > > --- > > v2: * Deduplicate patch_32() definition > * Use u32 for val32 > * Remove noinline > --- > arch/powerpc/include/asm/code-patching.h | 33 ++++++++++++ > arch/powerpc/lib/code-patching.c | 66 ++++++++++++++++++------ > 2 files changed, 83 insertions(+), 16 deletions(-) > > diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h > index 3f881548fb61..7c6056bb1706 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. Should we enforce alignment requirements, especially for patch_ulong() on 64-bit powerpc? I am not sure if there are use cases for unaligned 64-bit writes. That should also ensure that the write doesn't cross a cacheline. > + * > + * 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); > +int patch_ulong(void *addr, unsigned long val); > + > +#define patch_u64 patch_ulong > + > +#else > + > +static inline int patch_uint(u32 *addr, unsigned int val) Is there a reason to use u32 * here? > +{ > + return patch_instruction(addr, ppc_inst(val)); > +} > + > +static inline int patch_ulong(void *addr, unsigned long val) > +{ > + return patch_instruction(addr, ppc_inst(val)); > +} > + > +#endif > + > +#define patch_u32 patch_uint > + > 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..60289332412f 100644 > --- a/arch/powerpc/lib/code-patching.c > +++ b/arch/powerpc/lib/code-patching.c > @@ -20,15 +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); > + if (!IS_ENABLED(CONFIG_PPC64) || likely(!is_dword)) { > + u32 val32 = val; Would be good to add a comment indicating the need for this for BE. - Naveen
On Thu, 2023-11-30 at 15:55 +0530, Naveen N Rao wrote: > On Mon, Oct 16, 2023 at 04:01:45PM +1100, Benjamin Gray wrote: > > 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). > > That's debatable. While it is nice to be able to activate function > tracing quickly, it is not necessarily a hot path. On the flip side, > I > do have a use case for data patching for ftrace activation :) > True, but it's still correct to do so at least. Having a different path for data writes is definitely a possibility, but would be added for performance. This series is motivated by fixing a correctness issue with data write sizes. > > 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@linux.ibm.com> > > > > --- > > > > v2: * Deduplicate patch_32() definition > > * Use u32 for val32 > > * Remove noinline > > --- > > arch/powerpc/include/asm/code-patching.h | 33 ++++++++++++ > > arch/powerpc/lib/code-patching.c | 66 ++++++++++++++++++-- > > ---- > > 2 files changed, 83 insertions(+), 16 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/code-patching.h > > b/arch/powerpc/include/asm/code-patching.h > > index 3f881548fb61..7c6056bb1706 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. > > Should we enforce alignment requirements, especially for > patch_ulong() > on 64-bit powerpc? I am not sure if there are use cases for unaligned > 64-bit writes. That should also ensure that the write doesn't cross a > cacheline. Yeah, the current description is more just the technical restrictions, not an endorsement of usage. If the caller isn't working with aligned data, it seems unlikely it would still be cacheline aligned. The caller should break it into 32bit patches if this is the case. By enforce, are you suggesting a WARN_ON in the code too? > > + * > > + * 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); > > +int patch_ulong(void *addr, unsigned long val); > > + > > +#define patch_u64 patch_ulong > > + > > +#else > > + > > +static inline int patch_uint(u32 *addr, unsigned int val) > > Is there a reason to use u32 * here? > No, fixed to void* for next series > > +{ > > + return patch_instruction(addr, ppc_inst(val)); > > +} > > + > > +static inline int patch_ulong(void *addr, unsigned long val) > > +{ > > + return patch_instruction(addr, ppc_inst(val)); > > +} > > + > > +#endif > > + > > +#define patch_u32 patch_uint > > + > > 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..60289332412f 100644 > > --- a/arch/powerpc/lib/code-patching.c > > +++ b/arch/powerpc/lib/code-patching.c > > @@ -20,15 +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); > > + if (!IS_ENABLED(CONFIG_PPC64) || likely(!is_dword)) { > > + u32 val32 = val; > > Would be good to add a comment indicating the need for this for BE. > Yup, added > - Naveen >
On Mon, Feb 05, 2024 at 01:30:46PM +1100, Benjamin Gray wrote: > On Thu, 2023-11-30 at 15:55 +0530, Naveen N Rao wrote: > > On Mon, Oct 16, 2023 at 04:01:45PM +1100, Benjamin Gray wrote: > > > > > > diff --git a/arch/powerpc/include/asm/code-patching.h > > > b/arch/powerpc/include/asm/code-patching.h > > > index 3f881548fb61..7c6056bb1706 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. > > > > Should we enforce alignment requirements, especially for > > patch_ulong() > > on 64-bit powerpc? I am not sure if there are use cases for unaligned > > 64-bit writes. That should also ensure that the write doesn't cross a > > cacheline. > > Yeah, the current description is more just the technical restrictions, > not an endorsement of usage. If the caller isn't working with aligned > data, it seems unlikely it would still be cacheline aligned. The caller > should break it into 32bit patches if this is the case. > > By enforce, are you suggesting a WARN_ON in the code too? No, just detecting and returning an error code should help detect incorrect usage. - Naveen
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h index 3f881548fb61..7c6056bb1706 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); +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)); +} + +static inline int patch_ulong(void *addr, unsigned long val) +{ + return patch_instruction(addr, ppc_inst(val)); +} + +#endif + +#define patch_u32 patch_uint + 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..60289332412f 100644 --- a/arch/powerpc/lib/code-patching.c +++ b/arch/powerpc/lib/code-patching.c @@ -20,15 +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); + if (!IS_ENABLED(CONFIG_PPC64) || likely(!is_dword)) { + u32 val32 = val; - __put_kernel_nofault(patch_addr, &val, u32, failed); + __put_kernel_nofault(patch_addr, &val32, u32, failed); } else { - u64 val = ppc_inst_as_ulong(instr); - __put_kernel_nofault(patch_addr, &val, u64, failed); } @@ -43,7 +42,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 +280,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 +309,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 +330,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 +347,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 +355,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 +367,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 + +int patch_instruction(u32 *addr, ppc_inst_t instr) +{ + return patch_memory(addr, ppc_inst_val(instr), false); +} +NOKPROBE_SYMBOL(patch_instruction) + +#endif int patch_branch(u32 *addr, unsigned long target, int flags) {
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@linux.ibm.com> --- v2: * Deduplicate patch_32() definition * Use u32 for val32 * Remove noinline --- arch/powerpc/include/asm/code-patching.h | 33 ++++++++++++ arch/powerpc/lib/code-patching.c | 66 ++++++++++++++++++------ 2 files changed, 83 insertions(+), 16 deletions(-)