From: Michael Ellerman <mpe@ellerman.id.au> To: Russell Currey <ruscur@russell.cc>, Joe Lawrence <joe.lawrence@redhat.com>, linuxppc-dev@lists.ozlabs.org Cc: jniethe5@gmail.com, naveen.n.rao@linux.vnet.ibm.com, christophe.leroy@csgroup.eu, live-patching@vger.kernel.org Subject: Re: [PATCH] powerpc/module_64: Fix livepatching for RO modules Date: Thu, 09 Dec 2021 18:00:30 +1100 [thread overview] Message-ID: <87y24umfe9.fsf@mpe.ellerman.id.au> (raw) In-Reply-To: <25d35b916e87ed7a71ebc6528259e2f0ed390cb2.camel@russell.cc> 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:
WARNING: multiple messages have this Message-ID (diff)
From: Michael Ellerman <mpe@ellerman.id.au> To: Russell Currey <ruscur@russell.cc>, Joe Lawrence <joe.lawrence@redhat.com>, linuxppc-dev@lists.ozlabs.org Cc: jniethe5@gmail.com, naveen.n.rao@linux.vnet.ibm.com, live-patching@vger.kernel.org Subject: Re: [PATCH] powerpc/module_64: Fix livepatching for RO modules Date: Thu, 09 Dec 2021 18:00:30 +1100 [thread overview] Message-ID: <87y24umfe9.fsf@mpe.ellerman.id.au> (raw) In-Reply-To: <25d35b916e87ed7a71ebc6528259e2f0ed390cb2.camel@russell.cc> 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:
next prev parent reply other threads:[~2021-12-09 7:00 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-11-23 8:15 [PATCH] powerpc/module_64: Fix livepatching for RO modules Russell Currey 2021-12-07 14:44 ` Joe Lawrence 2021-12-07 14:44 ` Joe Lawrence 2021-12-08 1:51 ` Russell Currey 2021-12-08 1:51 ` Russell Currey 2021-12-09 7:00 ` Michael Ellerman [this message] 2021-12-09 7:00 ` Michael Ellerman 2021-12-10 3:08 ` Joe Lawrence 2021-12-10 3:08 ` Joe Lawrence 2021-12-15 0:51 ` Michael Ellerman
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87y24umfe9.fsf@mpe.ellerman.id.au \ --to=mpe@ellerman.id.au \ --cc=christophe.leroy@csgroup.eu \ --cc=jniethe5@gmail.com \ --cc=joe.lawrence@redhat.com \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=live-patching@vger.kernel.org \ --cc=naveen.n.rao@linux.vnet.ibm.com \ --cc=ruscur@russell.cc \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.