| Message ID | 20190527190450.14988-2-svens@stackframe.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Dynamic FTRACE for PA-RISC | expand |
Sven Schnelle wrote: > add patch_text_multiple() which allows to patch multiple > text words in memory. This can be used to copy functions. > > Signed-off-by: Sven Schnelle <svens@stackframe.org> > --- > arch/parisc/include/asm/patch.h | 4 +- > arch/parisc/kernel/patch.c | 75 ++++++++++++++++++++++++++------- > 2 files changed, 62 insertions(+), 17 deletions(-) > > diff --git a/arch/parisc/include/asm/patch.h > b/arch/parisc/include/asm/patch.h > index 685b58a13968..1156fe11249a 100644 > --- a/arch/parisc/include/asm/patch.h > +++ b/arch/parisc/include/asm/patch.h > @@ -4,8 +4,10 @@ > > /* stop machine and patch kernel text */ > void patch_text(void *addr, unsigned int insn); > +void patch_text_multiple(void *addr, u32 *insn, int len); > > /* patch kernel text with machine already stopped (e.g. in kgdb) */ > -void __patch_text(void *addr, unsigned int insn); > +void __patch_text(void *addr, u32 insn); > +void __patch_text_multiple(void *addr, u32 *insn, int len); A signed length always looks suspicious to me. > #endif > diff --git a/arch/parisc/kernel/patch.c b/arch/parisc/kernel/patch.c > index cdcd981278b3..eaef5515f5b6 100644 > --- a/arch/parisc/kernel/patch.c > +++ b/arch/parisc/kernel/patch.c > @@ -17,15 +17,18 @@ > > struct patch { > void *addr; > - unsigned int insn; > + u32 *insn; > + int len; > }; > > -static void __kprobes *patch_map(void *addr, int fixmap) > -{ > +static DEFINE_RAW_SPINLOCK(patch_lock); > + > +static void __kprobes *patch_map(void *addr, int fixmap, int > *need_unmap) > unsigned long uintaddr = (uintptr_t) addr; > bool module = !core_kernel_text(uintaddr); > struct page *page; > > + *need_unmap = 0; > if (module && IS_ENABLED(CONFIG_STRICT_MODULE_RWX)) > page = vmalloc_to_page(addr); > else if (!module && IS_ENABLED(CONFIG_STRICT_KERNEL_RWX)) > @@ -33,6 +36,7 @@ static void __kprobes *patch_map(void *addr, int > fixmap) > else > return addr; > > + *need_unmap = 1; > set_fixmap(fixmap, page_to_phys(page)); > > return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); > @@ -43,34 +47,73 @@ static void __kprobes patch_unmap(int fixmap) > clear_fixmap(fixmap); > } > > -void __kprobes __patch_text(void *addr, unsigned int insn) > +void __kprobes __patch_text_multiple(void *addr, u32 *insn, int len) > +{ > + unsigned long start = (unsigned long)addr; > + unsigned long end = (unsigned long)addr + len; > + u32 *p, *fixmap; > + int mapped; > + > + /* Make sure we don't have any aliases in cache */ > + flush_kernel_vmap_range(addr, len); > + flush_icache_range(start, end); > + > + p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &mapped); > + > + while (len > 0) { > + *p++ = *insn++; > + addr += 4; > + len -= sizeof(u32); I wonder if this can't just use memcpy inside the pages? If not there should be a comment describing what's going on here. Another nitpick: the "+4" and "-sizeof(u32)" are just the same at the end, but why do they use entirely different wording? What do we need "addr" for anyway, one could just look at "p" which would cross a page boundary at the same time, no? > + if (len && !((unsigned long)addr & ~PAGE_MASK)) { > + /* > + * We're crossing a page boundary, so > + * need to remap > + */ > + flush_kernel_vmap_range((void *)fixmap, > + (p-fixmap) * sizeof(*p)); > + if (mapped) > + patch_unmap(FIX_TEXT_POKE0); > + p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &mapped); > + } > + } > + > + flush_kernel_vmap_range((void *)fixmap, (p-fixmap) * sizeof(*p)); > + if (mapped) > + patch_unmap(FIX_TEXT_POKE0); > + flush_icache_range(start, end); > +} > + > +void __kprobes __patch_text(void *addr, u32 insn) > { > - void *waddr = addr; > - int size; > - > - waddr = patch_map(addr, FIX_TEXT_POKE0); > - *(u32 *)waddr = insn; > - size = sizeof(u32); > - flush_kernel_vmap_range(waddr, size); > - patch_unmap(FIX_TEXT_POKE0); > - flush_icache_range((uintptr_t)(addr), > - (uintptr_t)(addr) + size); > + __patch_text_multiple(addr, &insn, sizeof(insn)); > } > > static int __kprobes patch_text_stop_machine(void *data) > { > struct patch *patch = data; > > - __patch_text(patch->addr, patch->insn); > - > + __patch_text_multiple(patch->addr, patch->insn, patch->len); > return 0; > } > > void __kprobes patch_text(void *addr, unsigned int insn) > { > + struct patch patch = { > + .addr = addr, > + .insn = &insn, > + .len = 4 > + }; sizeof(insn)? I don't know if this makes it more readable, I personally tend to understand where the number is coming from faster if it's written this way. > + stop_machine_cpuslocked(patch_text_stop_machine, &patch, NULL); > +} > + > +void __kprobes patch_text_multiple(void *addr, u32 *insn, int len) > +{ > + > struct patch patch = { > .addr = addr, > .insn = insn, > + .len = len > }; > > stop_machine_cpuslocked(patch_text_stop_machine, &patch, NULL); Eike
Hi, On Tue, May 28, 2019 at 10:19:11AM +0200, Rolf Eike Beer wrote: > Sven Schnelle wrote: > > add patch_text_multiple() which allows to patch multiple > > text words in memory. This can be used to copy functions. > > +void __patch_text(void *addr, u32 insn); > > +void __patch_text_multiple(void *addr, u32 *insn, int len); > > A signed length always looks suspicious to me. Agreed. Will change. > > + p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &mapped); > > + > > + while (len > 0) { > > + *p++ = *insn++; > > + addr += 4; > > + len -= sizeof(u32); > > I wonder if this can't just use memcpy inside the pages? I think using memcpy here makes things just more complicated and harder to read. We would need to extract the amount of bytes to copy, and call memcpy multiple times. As this code is not performance critical and usually only copies only a few bytes i doubt that it's worth the effort. > If not there should be a comment describing what's going on here. Is it that complicated? It's just a copy loop like in every C beginner book, the only things that makes things more complicated is the need to remap when crossing a page. > Another nitpick: the "+4" and "-sizeof(u32)" are just the same at the end, > but why do they use entirely different wording? What do we need "addr" for > anyway, one could just look at "p" which would cross a page boundary at the > same time, no? You can't, because of the patch_map() call below which updates the fixed mapping. That call needs the real virtual address, while *p holds the virtual address of the fixed mapping for patching. > > + if (len && !((unsigned long)addr & ~PAGE_MASK)) { > > + /* > > + * We're crossing a page boundary, so > > + * need to remap > > + */ > > + flush_kernel_vmap_range((void *)fixmap, > > + (p-fixmap) * sizeof(*p)); > > + if (mapped) > > + patch_unmap(FIX_TEXT_POKE0); > > + p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &mapped); > > + } > > + } > > + Regards Sven
Sven Schnelle wrote: > On Tue, May 28, 2019 at 10:19:11AM +0200, Rolf Eike Beer wrote: > > Sven Schnelle wrote: > > > + p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &mapped); > > > + > > > + while (len > 0) { > > > + *p++ = *insn++; > > > + addr += 4; > > > + len -= sizeof(u32); > > > > I wonder if this can't just use memcpy inside the pages? > > I think using memcpy here makes things just more complicated and harder to > read. We would need to extract the amount of bytes to copy, and call memcpy > multiple times. As this code is not performance critical and usually only > copies only a few bytes i doubt that it's worth the effort. > > > If not there should be a comment describing what's going on here. > > Is it that complicated? It's just a copy loop like in every C beginner book, > the only things that makes things more complicated is the need to remap > when crossing a page. The copy loop not. But things like "why are you doing it backwards" come to mind. Be careful when you change the length to unsigned, your loop will not work this way anymore afterwards. > > Another nitpick: the "+4" and "-sizeof(u32)" are just the same at the end, > > but why do they use entirely different wording? What do we need "addr" for > > anyway, one could just look at "p" which would cross a page boundary at > > the > > same time, no? > > You can't, because of the patch_map() call below which updates the fixed > mapping. That call needs the real virtual address, while *p holds the > virtual address of the fixed mapping for patching. Can that remapping really place it at a non-zero offset regarding to the underlying page? That it moves the page descriptor around is normal, but it will keep the low order bits intact, so the page boundary crossing will be still the same, no? Eike
On Wed, May 29, 2019 at 07:58:57PM +0200, Rolf Eike Beer wrote: > Sven Schnelle wrote: > > On Tue, May 28, 2019 at 10:19:11AM +0200, Rolf Eike Beer wrote: > > > Another nitpick: the "+4" and "-sizeof(u32)" are just the same at the end, > > > but why do they use entirely different wording? What do we need "addr" for > > > anyway, one could just look at "p" which would cross a page boundary at > > > the > > > same time, no? > > > > You can't, because of the patch_map() call below which updates the fixed > > mapping. That call needs the real virtual address, while *p holds the > > virtual address of the fixed mapping for patching. > > Can that remapping really place it at a non-zero offset regarding to the > underlying page? That it moves the page descriptor around is normal, but it > will keep the low order bits intact, so the page boundary crossing will be > still the same, no? For the page crossing check it doesn't make a difference whether you check p or addr, but for the patch_map() you can only use addr. So we have to update both variables. Regards Sven
diff --git a/arch/parisc/include/asm/patch.h b/arch/parisc/include/asm/patch.h index 685b58a13968..1156fe11249a 100644 --- a/arch/parisc/include/asm/patch.h +++ b/arch/parisc/include/asm/patch.h @@ -4,8 +4,10 @@ /* stop machine and patch kernel text */ void patch_text(void *addr, unsigned int insn); +void patch_text_multiple(void *addr, u32 *insn, int len); /* patch kernel text with machine already stopped (e.g. in kgdb) */ -void __patch_text(void *addr, unsigned int insn); +void __patch_text(void *addr, u32 insn); +void __patch_text_multiple(void *addr, u32 *insn, int len); #endif diff --git a/arch/parisc/kernel/patch.c b/arch/parisc/kernel/patch.c index cdcd981278b3..eaef5515f5b6 100644 --- a/arch/parisc/kernel/patch.c +++ b/arch/parisc/kernel/patch.c @@ -17,15 +17,18 @@ struct patch { void *addr; - unsigned int insn; + u32 *insn; + int len; }; -static void __kprobes *patch_map(void *addr, int fixmap) -{ +static DEFINE_RAW_SPINLOCK(patch_lock); + +static void __kprobes *patch_map(void *addr, int fixmap, int *need_unmap) unsigned long uintaddr = (uintptr_t) addr; bool module = !core_kernel_text(uintaddr); struct page *page; + *need_unmap = 0; if (module && IS_ENABLED(CONFIG_STRICT_MODULE_RWX)) page = vmalloc_to_page(addr); else if (!module && IS_ENABLED(CONFIG_STRICT_KERNEL_RWX)) @@ -33,6 +36,7 @@ static void __kprobes *patch_map(void *addr, int fixmap) else return addr; + *need_unmap = 1; set_fixmap(fixmap, page_to_phys(page)); return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); @@ -43,34 +47,73 @@ static void __kprobes patch_unmap(int fixmap) clear_fixmap(fixmap); } -void __kprobes __patch_text(void *addr, unsigned int insn) +void __kprobes __patch_text_multiple(void *addr, u32 *insn, int len) +{ + unsigned long start = (unsigned long)addr; + unsigned long end = (unsigned long)addr + len; + u32 *p, *fixmap; + int mapped; + + /* Make sure we don't have any aliases in cache */ + flush_kernel_vmap_range(addr, len); + flush_icache_range(start, end); + + p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &mapped); + + while (len > 0) { + *p++ = *insn++; + addr += 4; + len -= sizeof(u32); + if (len && !((unsigned long)addr & ~PAGE_MASK)) { + /* + * We're crossing a page boundary, so + * need to remap + */ + flush_kernel_vmap_range((void *)fixmap, + (p-fixmap) * sizeof(*p)); + if (mapped) + patch_unmap(FIX_TEXT_POKE0); + p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &mapped); + } + } + + flush_kernel_vmap_range((void *)fixmap, (p-fixmap) * sizeof(*p)); + if (mapped) + patch_unmap(FIX_TEXT_POKE0); + flush_icache_range(start, end); +} + +void __kprobes __patch_text(void *addr, u32 insn) { - void *waddr = addr; - int size; - - waddr = patch_map(addr, FIX_TEXT_POKE0); - *(u32 *)waddr = insn; - size = sizeof(u32); - flush_kernel_vmap_range(waddr, size); - patch_unmap(FIX_TEXT_POKE0); - flush_icache_range((uintptr_t)(addr), - (uintptr_t)(addr) + size); + __patch_text_multiple(addr, &insn, sizeof(insn)); } static int __kprobes patch_text_stop_machine(void *data) { struct patch *patch = data; - __patch_text(patch->addr, patch->insn); - + __patch_text_multiple(patch->addr, patch->insn, patch->len); return 0; } void __kprobes patch_text(void *addr, unsigned int insn) { + struct patch patch = { + .addr = addr, + .insn = &insn, + .len = 4 + }; + + stop_machine_cpuslocked(patch_text_stop_machine, &patch, NULL); +} + +void __kprobes patch_text_multiple(void *addr, u32 *insn, int len) +{ + struct patch patch = { .addr = addr, .insn = insn, + .len = len }; stop_machine_cpuslocked(patch_text_stop_machine, &patch, NULL);
add patch_text_multiple() which allows to patch multiple text words in memory. This can be used to copy functions. Signed-off-by: Sven Schnelle <svens@stackframe.org> --- arch/parisc/include/asm/patch.h | 4 +- arch/parisc/kernel/patch.c | 75 ++++++++++++++++++++++++++------- 2 files changed, 62 insertions(+), 17 deletions(-)