| Message ID | 20211123081520.18843-1-ruscur@russell.cc (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/module_64: Fix livepatching for RO modules | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
On 11/23/21 3:15 AM, Russell Currey wrote: > Livepatching a loaded module involves applying relocations through > apply_relocate_add(), which attempts to write to read-only memory when > CONFIG_STRICT_MODULE_RWX=y. Work around this by performing these > writes through the text poke area by using patch_instruction(). > > R_PPC_REL24 is the only relocation type generated by the kpatch-build > userspace tool or klp-convert kernel tree that I observed applying a > relocation to a post-init module. > > A more comprehensive solution is planned, but using patch_instruction() > for R_PPC_REL24 on should serve as a sufficient fix. > > This does have a performance impact, I observed ~15% overhead in > module_load() on POWER8 bare metal with checksum verification off. > > Fixes: c35717c71e98 ("powerpc: Set ARCH_HAS_STRICT_MODULE_RWX") > Cc: stable@vger.kernel.org # v5.14+ > Reported-by: Joe Lawrence <joe.lawrence@redhat.com> > Signed-off-by: Russell Currey <ruscur@russell.cc> > --- > Intended to be a minimal fix that can go to stable. > > arch/powerpc/kernel/module_64.c | 30 ++++++++++++++++++++++-------- > 1 file changed, 22 insertions(+), 8 deletions(-) > > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c > index 6baa676e7cb6..c25ef36c3ef4 100644 > --- a/arch/powerpc/kernel/module_64.c > +++ b/arch/powerpc/kernel/module_64.c > @@ -422,11 +422,16 @@ static inline int create_stub(const Elf64_Shdr *sechdrs, > const char *name) > { > long reladdr; > + func_desc_t desc; > + int i; > > if (is_mprofile_ftrace_call(name)) > return create_ftrace_stub(entry, addr, me); > > - memcpy(entry->jump, ppc64_stub_insns, sizeof(ppc64_stub_insns)); > + for (i = 0; i < sizeof(ppc64_stub_insns) / sizeof(u32); i++) { > + patch_instruction(&entry->jump[i], > + ppc_inst(ppc64_stub_insns[i])); > + } > > /* Stub uses address relative to r2. */ > reladdr = (unsigned long)entry - my_r2(sechdrs, me); > @@ -437,10 +442,19 @@ static inline int create_stub(const Elf64_Shdr *sechdrs, > } > pr_debug("Stub %p get data from reladdr %li\n", entry, reladdr); > > - entry->jump[0] |= PPC_HA(reladdr); > - entry->jump[1] |= PPC_LO(reladdr); > - entry->funcdata = func_desc(addr); > - entry->magic = STUB_MAGIC; > + patch_instruction(&entry->jump[0], > + ppc_inst(entry->jump[0] | PPC_HA(reladdr))); > + patch_instruction(&entry->jump[1], > + ppc_inst(entry->jump[1] | PPC_LO(reladdr))); > + > + // func_desc_t is 8 bytes if ABIv2, else 16 bytes > + desc = func_desc(addr); > + for (i = 0; i < sizeof(func_desc_t) / sizeof(u32); i++) { > + patch_instruction(((u32 *)&entry->funcdata) + i, > + ppc_inst(((u32 *)(&desc))[i])); > + } > + > + patch_instruction(&entry->magic, ppc_inst(STUB_MAGIC)); > > return 1; > } > @@ -496,7 +510,7 @@ static int restore_r2(const char *name, u32 *instruction, struct module *me) > return 0; > } > /* ld r2,R2_STACK_OFFSET(r1) */ > - *instruction = PPC_INST_LD_TOC; > + patch_instruction(instruction, ppc_inst(PPC_INST_LD_TOC)); > return 1; > } > > @@ -636,9 +650,9 @@ int apply_relocate_add(Elf64_Shdr *sechdrs, > } > > /* Only replace bits 2 through 26 */ > - *(uint32_t *)location > - = (*(uint32_t *)location & ~0x03fffffc) > + value = (*(uint32_t *)location & ~0x03fffffc) > | (value & 0x03fffffc); > + patch_instruction((u32 *)location, ppc_inst(value)); > break; > > case R_PPC64_REL64: > [[ cc += livepatching list ]] Hi Russell, Thanks for writing a minimal fix for stable / backporting. As I mentioned on the github issue [1], this avoided the crashes I reported here and over on kpatch github [2]. I wasn't sure if this is the final version for stable, but feel free to add my: Tested-by: Joe Lawrence <joe.lawrence@redhat.com> [1] https://github.com/linuxppc/issues/issues/375 [2] https://github.com/dynup/kpatch/issues/1228
On Tue, 2021-12-07 at 09:44 -0500, Joe Lawrence wrote: > On 11/23/21 3:15 AM, Russell Currey wrote: > > [[ cc += livepatching list ]] > > Hi Russell, > > Thanks for writing a minimal fix for stable / backporting. As I > mentioned on the github issue [1], this avoided the crashes I > reported > here and over on kpatch github [2]. I wasn't sure if this is the > final > version for stable, but feel free to add my: > > Tested-by: Joe Lawrence <joe.lawrence@redhat.com> Thanks Joe, as per the discussions on GitHub I think we're fine to use this patch for a fix for stable (unless there's new issues found or additional community feedback etc). > > [1] https://github.com/linuxppc/issues/issues/375 > [2] https://github.com/dynup/kpatch/issues/1228 >
Russell Currey <ruscur@russell.cc> writes: > On Tue, 2021-12-07 at 09:44 -0500, Joe Lawrence wrote: >> On 11/23/21 3:15 AM, Russell Currey wrote: >> >> [[ cc += livepatching list ]] >> >> Hi Russell, >> >> Thanks for writing a minimal fix for stable / backporting. As I >> mentioned on the github issue [1], this avoided the crashes I >> reported >> here and over on kpatch github [2]. I wasn't sure if this is the >> final >> version for stable, but feel free to add my: >> >> Tested-by: Joe Lawrence <joe.lawrence@redhat.com> > > Thanks Joe, as per the discussions on GitHub I think we're fine to use > this patch for a fix for stable (unless there's new issues found or > additional community feedback etc). Hmm, I read the GitHub discussion as being that you were going to do another version, which is why I haven't picked this up. But I guess you and Christophe were talking about the non-minimal fix. I know we want this to be minimal, but I think it should be checking that patch_instruction() isn't failing. Incremental diff to do that is below. It boots OK, are you able to throw a livepatch test at it? cheers diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c index c25ef36c3ef4..5d77d3f5fbb5 100644 --- a/arch/powerpc/kernel/module_64.c +++ b/arch/powerpc/kernel/module_64.c @@ -429,8 +429,9 @@ static inline int create_stub(const Elf64_Shdr *sechdrs, return create_ftrace_stub(entry, addr, me); for (i = 0; i < sizeof(ppc64_stub_insns) / sizeof(u32); i++) { - patch_instruction(&entry->jump[i], - ppc_inst(ppc64_stub_insns[i])); + if (patch_instruction(&entry->jump[i], + ppc_inst(ppc64_stub_insns[i]))) + return 0; } /* Stub uses address relative to r2. */ @@ -442,19 +443,24 @@ static inline int create_stub(const Elf64_Shdr *sechdrs, } pr_debug("Stub %p get data from reladdr %li\n", entry, reladdr); - patch_instruction(&entry->jump[0], - ppc_inst(entry->jump[0] | PPC_HA(reladdr))); - patch_instruction(&entry->jump[1], - ppc_inst(entry->jump[1] | PPC_LO(reladdr))); + if (patch_instruction(&entry->jump[0], + ppc_inst(entry->jump[0] | PPC_HA(reladdr)))) + return 0; + + if (patch_instruction(&entry->jump[1], + ppc_inst(entry->jump[1] | PPC_LO(reladdr)))) + return 0; // func_desc_t is 8 bytes if ABIv2, else 16 bytes desc = func_desc(addr); for (i = 0; i < sizeof(func_desc_t) / sizeof(u32); i++) { - patch_instruction(((u32 *)&entry->funcdata) + i, - ppc_inst(((u32 *)(&desc))[i])); + if (patch_instruction(((u32 *)&entry->funcdata) + i, + ppc_inst(((u32 *)(&desc))[i]))) + return 0; } - patch_instruction(&entry->magic, ppc_inst(STUB_MAGIC)); + if (patch_instruction(&entry->magic, ppc_inst(STUB_MAGIC))) + return 0; return 1; } @@ -509,8 +515,11 @@ static int restore_r2(const char *name, u32 *instruction, struct module *me) me->name, *instruction, instruction); return 0; } + /* ld r2,R2_STACK_OFFSET(r1) */ - patch_instruction(instruction, ppc_inst(PPC_INST_LD_TOC)); + if (patch_instruction(instruction, ppc_inst(PPC_INST_LD_TOC))) + return 0; + return 1; } @@ -652,7 +661,10 @@ int apply_relocate_add(Elf64_Shdr *sechdrs, /* Only replace bits 2 through 26 */ value = (*(uint32_t *)location & ~0x03fffffc) | (value & 0x03fffffc); - patch_instruction((u32 *)location, ppc_inst(value)); + + if (patch_instruction((u32 *)location, ppc_inst(value))) + return -EFAULT; + break; case R_PPC64_REL64:
On 12/9/21 2:00 AM, Michael Ellerman wrote: > Russell Currey <ruscur@russell.cc> writes: >> On Tue, 2021-12-07 at 09:44 -0500, Joe Lawrence wrote: >>> On 11/23/21 3:15 AM, Russell Currey wrote: >>> >>> [[ cc += livepatching list ]] >>> >>> Hi Russell, >>> >>> Thanks for writing a minimal fix for stable / backporting. As I >>> mentioned on the github issue [1], this avoided the crashes I >>> reported >>> here and over on kpatch github [2]. I wasn't sure if this is the >>> final >>> version for stable, but feel free to add my: >>> >>> Tested-by: Joe Lawrence <joe.lawrence@redhat.com> >> >> Thanks Joe, as per the discussions on GitHub I think we're fine to use >> this patch for a fix for stable (unless there's new issues found or >> additional community feedback etc). > > Hmm, I read the GitHub discussion as being that you were going to do > another version, which is why I haven't picked this up. But I guess you > and Christophe were talking about the non-minimal fix. > > I know we want this to be minimal, but I think it should be checking > that patch_instruction() isn't failing. > > Incremental diff to do that is below. It boots OK, are you able to throw > a livepatch test at it? > No problem. The incremental patch update tests successful. Thanks,
On Tue, 23 Nov 2021 18:15:20 +1000, Russell Currey wrote: > Livepatching a loaded module involves applying relocations through > apply_relocate_add(), which attempts to write to read-only memory when > CONFIG_STRICT_MODULE_RWX=y. Work around this by performing these > writes through the text poke area by using patch_instruction(). > > R_PPC_REL24 is the only relocation type generated by the kpatch-build > userspace tool or klp-convert kernel tree that I observed applying a > relocation to a post-init module. > > [...] Applied to powerpc/fixes. [1/1] powerpc/module_64: Fix livepatching for RO modules https://git.kernel.org/powerpc/c/8734b41b3efe0fc6082c1937b0e88556c396dc96 cheers
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c index 6baa676e7cb6..c25ef36c3ef4 100644 --- a/arch/powerpc/kernel/module_64.c +++ b/arch/powerpc/kernel/module_64.c @@ -422,11 +422,16 @@ static inline int create_stub(const Elf64_Shdr *sechdrs, const char *name) { long reladdr; + func_desc_t desc; + int i; if (is_mprofile_ftrace_call(name)) return create_ftrace_stub(entry, addr, me); - memcpy(entry->jump, ppc64_stub_insns, sizeof(ppc64_stub_insns)); + for (i = 0; i < sizeof(ppc64_stub_insns) / sizeof(u32); i++) { + patch_instruction(&entry->jump[i], + ppc_inst(ppc64_stub_insns[i])); + } /* Stub uses address relative to r2. */ reladdr = (unsigned long)entry - my_r2(sechdrs, me); @@ -437,10 +442,19 @@ static inline int create_stub(const Elf64_Shdr *sechdrs, } pr_debug("Stub %p get data from reladdr %li\n", entry, reladdr); - entry->jump[0] |= PPC_HA(reladdr); - entry->jump[1] |= PPC_LO(reladdr); - entry->funcdata = func_desc(addr); - entry->magic = STUB_MAGIC; + patch_instruction(&entry->jump[0], + ppc_inst(entry->jump[0] | PPC_HA(reladdr))); + patch_instruction(&entry->jump[1], + ppc_inst(entry->jump[1] | PPC_LO(reladdr))); + + // func_desc_t is 8 bytes if ABIv2, else 16 bytes + desc = func_desc(addr); + for (i = 0; i < sizeof(func_desc_t) / sizeof(u32); i++) { + patch_instruction(((u32 *)&entry->funcdata) + i, + ppc_inst(((u32 *)(&desc))[i])); + } + + patch_instruction(&entry->magic, ppc_inst(STUB_MAGIC)); return 1; } @@ -496,7 +510,7 @@ static int restore_r2(const char *name, u32 *instruction, struct module *me) return 0; } /* ld r2,R2_STACK_OFFSET(r1) */ - *instruction = PPC_INST_LD_TOC; + patch_instruction(instruction, ppc_inst(PPC_INST_LD_TOC)); return 1; } @@ -636,9 +650,9 @@ int apply_relocate_add(Elf64_Shdr *sechdrs, } /* Only replace bits 2 through 26 */ - *(uint32_t *)location - = (*(uint32_t *)location & ~0x03fffffc) + value = (*(uint32_t *)location & ~0x03fffffc) | (value & 0x03fffffc); + patch_instruction((u32 *)location, ppc_inst(value)); break; case R_PPC64_REL64:
Livepatching a loaded module involves applying relocations through apply_relocate_add(), which attempts to write to read-only memory when CONFIG_STRICT_MODULE_RWX=y. Work around this by performing these writes through the text poke area by using patch_instruction(). R_PPC_REL24 is the only relocation type generated by the kpatch-build userspace tool or klp-convert kernel tree that I observed applying a relocation to a post-init module. A more comprehensive solution is planned, but using patch_instruction() for R_PPC_REL24 on should serve as a sufficient fix. This does have a performance impact, I observed ~15% overhead in module_load() on POWER8 bare metal with checksum verification off. Fixes: c35717c71e98 ("powerpc: Set ARCH_HAS_STRICT_MODULE_RWX") Cc: stable@vger.kernel.org # v5.14+ Reported-by: Joe Lawrence <joe.lawrence@redhat.com> Signed-off-by: Russell Currey <ruscur@russell.cc> --- Intended to be a minimal fix that can go to stable. arch/powerpc/kernel/module_64.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)