Message ID | 20220901055823.152983-2-bgray@linux.ibm.com (mailing list archive) |
---|---|
State | RFC |
Headers | show |
Series | Out-of-line static calls for powerpc64 ELF V2 | expand |
Le 01/09/2022 à 07:58, Benjamin Gray a écrit : > From: Russell Currey <ruscur@russell.cc> > > powerpc allocates a text poke area of one page that is used by > patch_instruction() to modify read-only text when STRICT_KERNEL_RWX > is enabled. > > patch_instruction() is only designed for instructions, > so writing data using the text poke area can only happen 4 bytes > at a time - each with a page map/unmap, pte flush and syncs. > > This patch introduces patch_memory(), implementing the same > interface as memcpy(), similar to x86's text_poke() and s390's > s390_kernel_write(). patch_memory() only needs to map the text > poke area once, unless the write would cross a page boundary. > > Signed-off-by: Russell Currey <ruscur@russell.cc> > Signed-off-by: Benjamin Gray <bgray@linux.ibm.com> > --- > arch/powerpc/include/asm/code-patching.h | 1 + > arch/powerpc/lib/code-patching.c | 65 ++++++++++++++++++++++++ > 2 files changed, 66 insertions(+) > > diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h > index 1c6316ec4b74..3de90748bce7 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); > +void *patch_memory(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 6edf0697a526..0cca39af44cb 100644 > --- a/arch/powerpc/lib/code-patching.c > +++ b/arch/powerpc/lib/code-patching.c > @@ -14,6 +14,7 @@ > #include <asm/page.h> > #include <asm/code-patching.h> > #include <asm/inst.h> > +#include <asm/cacheflush.h> > > static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr, u32 *patch_addr) > { > @@ -183,6 +184,65 @@ static int do_patch_instruction(u32 *addr, ppc_inst_t instr) > > return err; > } > + > +static int do_patch_memory(void *dest, const void *src, size_t size) > +{ > + int err; > + unsigned long text_poke_addr, patch_addr; > + > + text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr; > + > + err = map_patch_area(dest, text_poke_addr); This is not in line with the optimisation done by https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20220815114840.1468656-1-mpe@ellerman.id.au/ > + if (err) > + return err; > + > + patch_addr = text_poke_addr + offset_in_page(dest); > + copy_to_kernel_nofault((u8 *)patch_addr, src, size); copy_to_kernel_nofault() has a performance cost. > + > + flush_icache_range(patch_addr, size); Is that needed ? We are patching data, not text. > + unmap_patch_area(text_poke_addr); > + > + return 0; > +} > + > +/** > + * patch_memory - write data using the text poke area > + * > + * @dest: destination address > + * @src: source address > + * @size: size in bytes > + * > + * like memcpy(), but using the text poke area. No atomicity guarantees. > + * Do not use for instructions, use patch_instruction() instead. > + * Handles crossing page boundaries, though you shouldn't need to. > + * > + * Return value: > + * @dest > + **/ > +void *patch_memory(void *dest, const void *src, size_t size) > +{ > + int err; > + unsigned long flags; > + size_t written, write_size; > + > + // If the poke area isn't set up, it's early boot and we can just memcpy. > + if (!this_cpu_read(text_poke_area)) > + return memcpy(dest, src, size); > + > + for (written = 0; written < size; written += write_size) { > + // Write as much as possible without crossing a page boundary. > + write_size = min_t(size_t, size - written, > + PAGE_SIZE - offset_in_page(dest + written)); > + > + local_irq_save(flags); > + err = do_patch_memory(dest + written, src + written, write_size); > + local_irq_restore(flags); > + if (err) > + return ERR_PTR(err); > + } > + > + return dest; > +} > #else /* !CONFIG_STRICT_KERNEL_RWX */ > > static int do_patch_instruction(u32 *addr, ppc_inst_t instr) > @@ -190,6 +250,11 @@ 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) > +{ > + return memcpy(dest, src, size); In do_patch_memory() you have flush_icache_range(patch_addr, size); If that's really needed there, why don't we need it here as well ? > +} > + > #endif /* CONFIG_STRICT_KERNEL_RWX */ > > __ro_after_init DEFINE_STATIC_KEY_FALSE(init_mem_is_free); > -- > 2.37.2 >
On Thu, 2022-09-01 at 07:01 +0000, Christophe Leroy wrote: > > > Le 01/09/2022 à 07:58, Benjamin Gray a écrit : > > From: Russell Currey <ruscur@russell.cc> > > > > powerpc allocates a text poke area of one page that is used by > > patch_instruction() to modify read-only text when STRICT_KERNEL_RWX > > is enabled. > > > > patch_instruction() is only designed for instructions, > > so writing data using the text poke area can only happen 4 bytes > > at a time - each with a page map/unmap, pte flush and syncs. > > > > This patch introduces patch_memory(), implementing the same > > interface as memcpy(), similar to x86's text_poke() and s390's > > s390_kernel_write(). patch_memory() only needs to map the text > > poke area once, unless the write would cross a page boundary. > > > > Signed-off-by: Russell Currey <ruscur@russell.cc> > > Signed-off-by: Benjamin Gray <bgray@linux.ibm.com> > > --- > > arch/powerpc/include/asm/code-patching.h | 1 + > > arch/powerpc/lib/code-patching.c | 65 > > ++++++++++++++++++++++++ > > 2 files changed, 66 insertions(+) > > > > diff --git a/arch/powerpc/include/asm/code-patching.h > > b/arch/powerpc/include/asm/code-patching.h > > index 1c6316ec4b74..3de90748bce7 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); > > +void *patch_memory(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 6edf0697a526..0cca39af44cb 100644 > > --- a/arch/powerpc/lib/code-patching.c > > +++ b/arch/powerpc/lib/code-patching.c > > @@ -14,6 +14,7 @@ > > #include <asm/page.h> > > #include <asm/code-patching.h> > > #include <asm/inst.h> > > +#include <asm/cacheflush.h> > > > > static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr, > > u32 *patch_addr) > > { > > @@ -183,6 +184,65 @@ static int do_patch_instruction(u32 *addr, > > ppc_inst_t instr) > > > > return err; > > } > > + > > +static int do_patch_memory(void *dest, const void *src, size_t > > size) > > +{ > > + int err; > > + unsigned long text_poke_addr, patch_addr; > > + > > + text_poke_addr = (unsigned > > long)__this_cpu_read(text_poke_area)->addr; > > + > > + err = map_patch_area(dest, text_poke_addr); > > This is not in line with the optimisation done by > https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20220815114840.1468656-1-mpe@ellerman.id.au/ This patch hasn't changed since last year, thanks for the pointer. > > > + if (err) > > + return err; > > + > > + patch_addr = text_poke_addr + offset_in_page(dest); > > + copy_to_kernel_nofault((u8 *)patch_addr, src, size); > > copy_to_kernel_nofault() has a performance cost. > > > + > > + flush_icache_range(patch_addr, size); > > Is that needed ? We are patching data, not text. It's necessary if it gets used to patch text, which it might. Maybe we should add a variable and only flush if the caller thinks it's needed. The comment below should be updated for that too. > > + unmap_patch_area(text_poke_addr); > > + > > + return 0; > > +} > > + > > +/** > > + * patch_memory - write data using the text poke area > > + * > > + * @dest: destination address > > + * @src: source address > > + * @size: size in bytes > > + * > > + * like memcpy(), but using the text poke area. No atomicity > > guarantees. > > + * Do not use for instructions, use patch_instruction() instead. > > + * Handles crossing page boundaries, though you shouldn't need to. > > + * > > + * Return value: > > + * @dest > > + **/ > > +void *patch_memory(void *dest, const void *src, size_t size) > > +{ > > + int err; > > + unsigned long flags; > > + size_t written, write_size; > > + > > + // If the poke area isn't set up, it's early boot and we > > can just memcpy. > > + if (!this_cpu_read(text_poke_area)) > > + return memcpy(dest, src, size); > > + > > + for (written = 0; written < size; written += write_size) { > > + // Write as much as possible without crossing a > > page boundary. > > + write_size = min_t(size_t, size - written, > > + PAGE_SIZE - offset_in_page(dest > > + written)); > > + > > + local_irq_save(flags); > > + err = do_patch_memory(dest + written, src + > > written, write_size); > > + local_irq_restore(flags); > > + if (err) > > + return ERR_PTR(err); > > + } > > + > > + return dest; > > +} > > #else /* !CONFIG_STRICT_KERNEL_RWX */ > > > > static int do_patch_instruction(u32 *addr, ppc_inst_t instr) > > @@ -190,6 +250,11 @@ 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) > > +{ > > + return memcpy(dest, src, size); > > In do_patch_memory() you have flush_icache_range(patch_addr, size); > > If that's really needed there, why don't we need it here as well ? Good point. I might make the arguments (void *dest, const void *src, size_t size, bool text) and only do the icache flush if text is true. > > > +} > > + > > #endif /* CONFIG_STRICT_KERNEL_RWX */ > > > > __ro_after_init DEFINE_STATIC_KEY_FALSE(init_mem_is_free); > > -- > > 2.37.2
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h index 1c6316ec4b74..3de90748bce7 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); +void *patch_memory(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 6edf0697a526..0cca39af44cb 100644 --- a/arch/powerpc/lib/code-patching.c +++ b/arch/powerpc/lib/code-patching.c @@ -14,6 +14,7 @@ #include <asm/page.h> #include <asm/code-patching.h> #include <asm/inst.h> +#include <asm/cacheflush.h> static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr, u32 *patch_addr) { @@ -183,6 +184,65 @@ static int do_patch_instruction(u32 *addr, ppc_inst_t instr) return err; } + +static int do_patch_memory(void *dest, const void *src, size_t size) +{ + int err; + unsigned long text_poke_addr, patch_addr; + + text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr; + + err = map_patch_area(dest, text_poke_addr); + if (err) + return err; + + patch_addr = text_poke_addr + offset_in_page(dest); + copy_to_kernel_nofault((u8 *)patch_addr, src, size); + + flush_icache_range(patch_addr, size); + unmap_patch_area(text_poke_addr); + + return 0; +} + +/** + * patch_memory - write data using the text poke area + * + * @dest: destination address + * @src: source address + * @size: size in bytes + * + * like memcpy(), but using the text poke area. No atomicity guarantees. + * Do not use for instructions, use patch_instruction() instead. + * Handles crossing page boundaries, though you shouldn't need to. + * + * Return value: + * @dest + **/ +void *patch_memory(void *dest, const void *src, size_t size) +{ + int err; + unsigned long flags; + size_t written, write_size; + + // If the poke area isn't set up, it's early boot and we can just memcpy. + if (!this_cpu_read(text_poke_area)) + return memcpy(dest, src, size); + + for (written = 0; written < size; written += write_size) { + // Write as much as possible without crossing a page boundary. + write_size = min_t(size_t, size - written, + PAGE_SIZE - offset_in_page(dest + written)); + + local_irq_save(flags); + err = do_patch_memory(dest + written, src + written, write_size); + local_irq_restore(flags); + if (err) + return ERR_PTR(err); + } + + return dest; +} #else /* !CONFIG_STRICT_KERNEL_RWX */ static int do_patch_instruction(u32 *addr, ppc_inst_t instr) @@ -190,6 +250,11 @@ 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) +{ + return memcpy(dest, src, size); +} + #endif /* CONFIG_STRICT_KERNEL_RWX */ __ro_after_init DEFINE_STATIC_KEY_FALSE(init_mem_is_free);