Message ID | 20220830185313.76402-1-song@kernel.org (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v5] livepatch: Clear relocation targets on a module removal | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 10 jobs. |
snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 10 jobs. |
snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
Le 30/08/2022 à 20:53, Song Liu a écrit : > From: Miroslav Benes <mbenes@suse.cz> > > Josh reported a bug: > > When the object to be patched is a module, and that module is > rmmod'ed and reloaded, it fails to load with: > > module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c > livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) > livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' > > The livepatch module has a relocation which references a symbol > in the _previous_ loading of nfsd. When apply_relocate_add() > tries to replace the old relocation with a new one, it sees that > the previous one is nonzero and it errors out. > > On ppc64le, we have a similar issue: > > module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd] > livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) > livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' > > He also proposed three different solutions. We could remove the error > check in apply_relocate_add() introduced by commit eda9cec4c9a1 > ("x86/module: Detect and skip invalid relocations"). However the check > is useful for detecting corrupted modules. > > We could also deny the patched modules to be removed. If it proved to be > a major drawback for users, we could still implement a different > approach. The solution would also complicate the existing code a lot. > > We thus decided to reverse the relocation patching (clear all relocation > targets on x86_64). The solution is not > universal and is too much arch-specific, but it may prove to be simpler > in the end. > > Reported-by: Josh Poimboeuf <jpoimboe@redhat.com> > Signed-off-by: Miroslav Benes <mbenes@suse.cz> > Signed-off-by: Song Liu <song@kernel.org> > > --- > > NOTE: powerpc code has not be tested. > > Changes v4 = v5: > 1. Fix compile with powerpc. Not completely it seems. CC kernel/livepatch/core.o kernel/livepatch/core.c: In function 'klp_clear_object_relocations': kernel/livepatch/core.c:352:50: error: passing argument 1 of 'clear_relocate_add' from incompatible pointer type [-Werror=incompatible-pointer-types] 352 | clear_relocate_add(pmod->klp_info->sechdrs, | ~~~~~~~~~~~~~~^~~~~~~~~ | | | Elf32_Shdr * {aka struct elf32_shdr *} In file included from kernel/livepatch/core.c:19: ./include/linux/moduleloader.h:76:37: note: expected 'Elf64_Shdr *' {aka 'struct elf64_shdr *'} but argument is of type 'Elf32_Shdr *' {aka 'struct elf32_shdr *'} 76 | void clear_relocate_add(Elf64_Shdr *sechdrs, | ~~~~~~~~~~~~^~~~~~~ cc1: some warnings being treated as errors Fixup: diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h index d22b36b84b4b..958e6da7f475 100644 --- a/include/linux/moduleloader.h +++ b/include/linux/moduleloader.h @@ -73,7 +73,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs, unsigned int relsec, struct module *mod); #ifdef CONFIG_LIVEPATCH -void clear_relocate_add(Elf64_Shdr *sechdrs, +void clear_relocate_add(Elf_Shdr *sechdrs, const char *strtab, unsigned int symindex, unsigned int relsec, But then the link fails. LD .tmp_vmlinux.kallsyms1 powerpc64-linux-ld: kernel/livepatch/core.o: in function `klp_cleanup_module_patches_limited': core.c:(.text+0xdb4): undefined reference to `clear_relocate_add' Christophe > > Changes v3 = v4: > 1. Reuse __apply_relocate_add to make it more reliable in long term. > (Josh Poimboeuf) > 2. Add back ppc64 logic from v2, with changes to match current code. > (Josh Poimboeuf) > > Changes v2 => v3: > 1. Rewrite x86 changes to match current code style. > 2. Remove powerpc changes as there is no test coverage in v3. > 3. Only keep 1/3 of v2. > > v2: https://lore.kernel.org/all/20190905124514.8944-1-mbenes@suse.cz/T/#u > --- > arch/powerpc/kernel/module_64.c | 49 +++++++++++++++ > arch/s390/kernel/module.c | 8 +++ > arch/x86/kernel/module.c | 102 +++++++++++++++++++++++--------- > include/linux/moduleloader.h | 7 +++ > kernel/livepatch/core.c | 41 ++++++++++++- > 5 files changed, 179 insertions(+), 28 deletions(-) > > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c > index 7e45dc98df8a..6aaf5720070d 100644 > --- a/arch/powerpc/kernel/module_64.c > +++ b/arch/powerpc/kernel/module_64.c > @@ -739,6 +739,55 @@ int apply_relocate_add(Elf64_Shdr *sechdrs, > return 0; > } > > +#ifdef CONFIG_LIVEPATCH > +void clear_relocate_add(Elf64_Shdr *sechdrs, > + const char *strtab, > + unsigned int symindex, > + unsigned int relsec, > + struct module *me) > +{ > + unsigned int i; > + Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr; > + Elf64_Sym *sym; > + unsigned long *location; > + const char *symname; > + u32 *instruction; > + > + pr_debug("Clearing ADD relocate section %u to %u\n", relsec, > + sechdrs[relsec].sh_info); > + > + for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) { > + location = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr > + + rela[i].r_offset; > + sym = (Elf64_Sym *)sechdrs[symindex].sh_addr > + + ELF64_R_SYM(rela[i].r_info); > + symname = me->core_kallsyms.strtab > + + sym->st_name; > + > + if (ELF64_R_TYPE(rela[i].r_info) != R_PPC_REL24) > + continue; > + /* > + * reverse the operations in apply_relocate_add() for case > + * R_PPC_REL24. > + */ > + if (sym->st_shndx != SHN_UNDEF && > + sym->st_shndx != SHN_LIVEPATCH) > + continue; > + > + instruction = (u32 *)location; > + if (is_mprofile_ftrace_call(symname)) > + continue; > + > + if (!instr_is_relative_link_branch(ppc_inst(*instruction))) > + continue; > + > + instruction += 1; > + *instruction = PPC_RAW_NOP(); > + } > + > +} > +#endif > + > #ifdef CONFIG_DYNAMIC_FTRACE > int module_trampoline_target(struct module *mod, unsigned long addr, > unsigned long *target) > diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c > index 2d159b32885b..cc6784fbc1ac 100644 > --- a/arch/s390/kernel/module.c > +++ b/arch/s390/kernel/module.c > @@ -500,6 +500,14 @@ static int module_alloc_ftrace_hotpatch_trampolines(struct module *me, > } > #endif /* CONFIG_FUNCTION_TRACER */ > > +#ifdef CONFIG_LIVEPATCH > +void clear_relocate_add(Elf64_Shdr *sechdrs, const char *strtab, > + unsigned int symindex, unsigned int relsec, > + struct module *me) > +{ > +} > +#endif > + > int module_finalize(const Elf_Ehdr *hdr, > const Elf_Shdr *sechdrs, > struct module *me) > diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c > index b1abf663417c..f9632afbb84c 100644 > --- a/arch/x86/kernel/module.c > +++ b/arch/x86/kernel/module.c > @@ -128,18 +128,20 @@ int apply_relocate(Elf32_Shdr *sechdrs, > return 0; > } > #else /*X86_64*/ > -static int __apply_relocate_add(Elf64_Shdr *sechdrs, > +static int __apply_clear_relocate_add(Elf64_Shdr *sechdrs, > const char *strtab, > unsigned int symindex, > unsigned int relsec, > struct module *me, > - void *(*write)(void *dest, const void *src, size_t len)) > + void *(*write)(void *dest, const void *src, size_t len), > + bool clear) > { > unsigned int i; > Elf64_Rela *rel = (void *)sechdrs[relsec].sh_addr; > Elf64_Sym *sym; > void *loc; > u64 val; > + u64 zero = 0ULL; > > DEBUGP("Applying relocate section %u to %u\n", > relsec, sechdrs[relsec].sh_info); > @@ -163,40 +165,60 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs, > case R_X86_64_NONE: > break; > case R_X86_64_64: > - if (*(u64 *)loc != 0) > - goto invalid_relocation; > - write(loc, &val, 8); > + if (!clear) { > + if (*(u64 *)loc != 0) > + goto invalid_relocation; > + write(loc, &val, 8); > + } else { > + write(loc, &zero, 8); > + } > break; > case R_X86_64_32: > - if (*(u32 *)loc != 0) > - goto invalid_relocation; > - write(loc, &val, 4); > - if (val != *(u32 *)loc) > - goto overflow; > + if (!clear) { > + if (*(u32 *)loc != 0) > + goto invalid_relocation; > + write(loc, &val, 4); > + if (val != *(u32 *)loc) > + goto overflow; > + } else { > + write(loc, &zero, 4); > + } > break; > case R_X86_64_32S: > - if (*(s32 *)loc != 0) > - goto invalid_relocation; > - write(loc, &val, 4); > - if ((s64)val != *(s32 *)loc) > - goto overflow; > + if (!clear) { > + if (*(s32 *)loc != 0) > + goto invalid_relocation; > + write(loc, &val, 4); > + if ((s64)val != *(s32 *)loc) > + goto overflow; > + } else { > + write(loc, &zero, 4); > + } > break; > case R_X86_64_PC32: > case R_X86_64_PLT32: > - if (*(u32 *)loc != 0) > - goto invalid_relocation; > - val -= (u64)loc; > - write(loc, &val, 4); > + if (!clear) { > + if (*(u32 *)loc != 0) > + goto invalid_relocation; > + val -= (u64)loc; > + write(loc, &val, 4); > #if 0 > - if ((s64)val != *(s32 *)loc) > - goto overflow; > + if ((s64)val != *(s32 *)loc) > + goto overflow; > #endif > + } else { > + write(loc, &zero, 4); > + } > break; > case R_X86_64_PC64: > - if (*(u64 *)loc != 0) > - goto invalid_relocation; > - val -= (u64)loc; > - write(loc, &val, 8); > + if (!clear) { > + if (*(u64 *)loc != 0) > + goto invalid_relocation; > + val -= (u64)loc; > + write(loc, &val, 8); > + } else { > + write(loc, &zero, 8); > + } > break; > default: > pr_err("%s: Unknown rela relocation: %llu\n", > @@ -234,8 +256,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs, > mutex_lock(&text_mutex); > } > > - ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me, > - write); > + ret = __apply_clear_relocate_add(sechdrs, strtab, symindex, relsec, me, > + write, false /* clear */); > > if (!early) { > text_poke_sync(); > @@ -245,6 +267,32 @@ int apply_relocate_add(Elf64_Shdr *sechdrs, > return ret; > } > > +#ifdef CONFIG_LIVEPATCH > + > +void clear_relocate_add(Elf64_Shdr *sechdrs, > + const char *strtab, > + unsigned int symindex, > + unsigned int relsec, > + struct module *me) > +{ > + bool early = me->state == MODULE_STATE_UNFORMED; > + void *(*write)(void *, const void *, size_t) = memcpy; > + > + if (!early) { > + write = text_poke; > + mutex_lock(&text_mutex); > + } > + > + __apply_clear_relocate_add(sechdrs, strtab, symindex, relsec, me, > + write, true /* clear */); > + > + if (!early) { > + text_poke_sync(); > + mutex_unlock(&text_mutex); > + } > +} > +#endif > + > #endif > > int module_finalize(const Elf_Ehdr *hdr, > diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h > index 9e09d11ffe5b..d22b36b84b4b 100644 > --- a/include/linux/moduleloader.h > +++ b/include/linux/moduleloader.h > @@ -72,6 +72,13 @@ int apply_relocate_add(Elf_Shdr *sechdrs, > unsigned int symindex, > unsigned int relsec, > struct module *mod); > +#ifdef CONFIG_LIVEPATCH > +void clear_relocate_add(Elf64_Shdr *sechdrs, Should be Elf_Shdr, otherwise I get : > + const char *strtab, > + unsigned int symindex, > + unsigned int relsec, > + struct module *me); > +#endif > #else > static inline int apply_relocate_add(Elf_Shdr *sechdrs, > const char *strtab, > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index bc475e62279d..5c0d8a4eba13 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -316,6 +316,45 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs, > return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod); > } > > +static void klp_clear_object_relocations(struct module *pmod, > + struct klp_object *obj) > +{ > + int i, cnt; > + const char *objname, *secname; > + char sec_objname[MODULE_NAME_LEN]; > + Elf_Shdr *sec; > + > + objname = klp_is_module(obj) ? obj->name : "vmlinux"; > + > + /* For each klp relocation section */ > + for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) { > + sec = pmod->klp_info->sechdrs + i; > + secname = pmod->klp_info->secstrings + sec->sh_name; > + if (!(sec->sh_flags & SHF_RELA_LIVEPATCH)) > + continue; > + > + /* > + * Format: .klp.rela.sec_objname.section_name > + * See comment in klp_resolve_symbols() for an explanation > + * of the selected field width value. > + */ > + secname = pmod->klp_info->secstrings + sec->sh_name; > + cnt = sscanf(secname, ".klp.rela.%55[^.]", sec_objname); > + if (cnt != 1) { > + pr_err("section %s has an incorrectly formatted name\n", > + secname); > + continue; > + } > + > + if (strcmp(objname, sec_objname)) > + continue; > + > + clear_relocate_add(pmod->klp_info->sechdrs, > + pmod->core_kallsyms.strtab, > + pmod->klp_info->symndx, i, pmod); > + } > +} > + > /* > * Sysfs Interface > * > @@ -1154,7 +1193,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod, > klp_unpatch_object(obj); > > klp_post_unpatch_callback(obj); > - > + klp_clear_object_relocations(patch->mod, obj); > klp_free_object_loaded(obj); > break; > }
On Wed, Aug 31, 2022 at 1:01 AM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > > Le 30/08/2022 à 20:53, Song Liu a écrit : > > From: Miroslav Benes <mbenes@suse.cz> > > > > Josh reported a bug: > > > > When the object to be patched is a module, and that module is > > rmmod'ed and reloaded, it fails to load with: > > > > module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c > > livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) > > livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' > > > > The livepatch module has a relocation which references a symbol > > in the _previous_ loading of nfsd. When apply_relocate_add() > > tries to replace the old relocation with a new one, it sees that > > the previous one is nonzero and it errors out. > > > > On ppc64le, we have a similar issue: > > > > module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd] > > livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) > > livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' > > > > He also proposed three different solutions. We could remove the error > > check in apply_relocate_add() introduced by commit eda9cec4c9a1 > > ("x86/module: Detect and skip invalid relocations"). However the check > > is useful for detecting corrupted modules. > > > > We could also deny the patched modules to be removed. If it proved to be > > a major drawback for users, we could still implement a different > > approach. The solution would also complicate the existing code a lot. > > > > We thus decided to reverse the relocation patching (clear all relocation > > targets on x86_64). The solution is not > > universal and is too much arch-specific, but it may prove to be simpler > > in the end. > > > > Reported-by: Josh Poimboeuf <jpoimboe@redhat.com> > > Signed-off-by: Miroslav Benes <mbenes@suse.cz> > > Signed-off-by: Song Liu <song@kernel.org> > > > > --- > > > > NOTE: powerpc code has not be tested. > > > > Changes v4 = v5: > > 1. Fix compile with powerpc. > > Not completely it seems. > > CC kernel/livepatch/core.o > kernel/livepatch/core.c: In function 'klp_clear_object_relocations': > kernel/livepatch/core.c:352:50: error: passing argument 1 of > 'clear_relocate_add' from incompatible pointer type > [-Werror=incompatible-pointer-types] > 352 | clear_relocate_add(pmod->klp_info->sechdrs, > | ~~~~~~~~~~~~~~^~~~~~~~~ > | | > | Elf32_Shdr * > {aka struct elf32_shdr *} > In file included from kernel/livepatch/core.c:19: > ./include/linux/moduleloader.h:76:37: note: expected 'Elf64_Shdr *' {aka > 'struct elf64_shdr *'} but argument is of type 'Elf32_Shdr *' {aka > 'struct elf32_shdr *'} > 76 | void clear_relocate_add(Elf64_Shdr *sechdrs, > | ~~~~~~~~~~~~^~~~~~~ > cc1: some warnings being treated as errors > > > Fixup: > > diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h > index d22b36b84b4b..958e6da7f475 100644 > --- a/include/linux/moduleloader.h > +++ b/include/linux/moduleloader.h > @@ -73,7 +73,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs, > unsigned int relsec, > struct module *mod); > #ifdef CONFIG_LIVEPATCH > -void clear_relocate_add(Elf64_Shdr *sechdrs, > +void clear_relocate_add(Elf_Shdr *sechdrs, > const char *strtab, > unsigned int symindex, > unsigned int relsec, > > > But then the link fails. > > LD .tmp_vmlinux.kallsyms1 > powerpc64-linux-ld: kernel/livepatch/core.o: in function > `klp_cleanup_module_patches_limited': > core.c:(.text+0xdb4): undefined reference to `clear_relocate_add' Hmm.. I am not seeing either error. Could you please share your .config file? Thanks, Song
Le 31/08/2022 à 19:05, Song Liu a écrit : > On Wed, Aug 31, 2022 at 1:01 AM Christophe Leroy > <christophe.leroy@csgroup.eu> wrote: >> >> >> >> Le 30/08/2022 à 20:53, Song Liu a écrit : >>> From: Miroslav Benes <mbenes@suse.cz> >>> >>> Josh reported a bug: >>> >>> When the object to be patched is a module, and that module is >>> rmmod'ed and reloaded, it fails to load with: >>> >>> module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c >>> livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) >>> livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' >>> >>> The livepatch module has a relocation which references a symbol >>> in the _previous_ loading of nfsd. When apply_relocate_add() >>> tries to replace the old relocation with a new one, it sees that >>> the previous one is nonzero and it errors out. >>> >>> On ppc64le, we have a similar issue: >>> >>> module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd] >>> livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) >>> livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' >>> >>> He also proposed three different solutions. We could remove the error >>> check in apply_relocate_add() introduced by commit eda9cec4c9a1 >>> ("x86/module: Detect and skip invalid relocations"). However the check >>> is useful for detecting corrupted modules. >>> >>> We could also deny the patched modules to be removed. If it proved to be >>> a major drawback for users, we could still implement a different >>> approach. The solution would also complicate the existing code a lot. >>> >>> We thus decided to reverse the relocation patching (clear all relocation >>> targets on x86_64). The solution is not >>> universal and is too much arch-specific, but it may prove to be simpler >>> in the end. >>> >>> Reported-by: Josh Poimboeuf <jpoimboe@redhat.com> >>> Signed-off-by: Miroslav Benes <mbenes@suse.cz> >>> Signed-off-by: Song Liu <song@kernel.org> >>> >>> --- >>> >>> NOTE: powerpc code has not be tested. >>> >>> Changes v4 = v5: >>> 1. Fix compile with powerpc. >> >> Not completely it seems. >> >> CC kernel/livepatch/core.o >> kernel/livepatch/core.c: In function 'klp_clear_object_relocations': >> kernel/livepatch/core.c:352:50: error: passing argument 1 of >> 'clear_relocate_add' from incompatible pointer type >> [-Werror=incompatible-pointer-types] >> 352 | clear_relocate_add(pmod->klp_info->sechdrs, >> | ~~~~~~~~~~~~~~^~~~~~~~~ >> | | >> | Elf32_Shdr * >> {aka struct elf32_shdr *} >> In file included from kernel/livepatch/core.c:19: >> ./include/linux/moduleloader.h:76:37: note: expected 'Elf64_Shdr *' {aka >> 'struct elf64_shdr *'} but argument is of type 'Elf32_Shdr *' {aka >> 'struct elf32_shdr *'} >> 76 | void clear_relocate_add(Elf64_Shdr *sechdrs, >> | ~~~~~~~~~~~~^~~~~~~ >> cc1: some warnings being treated as errors >> >> >> Fixup: >> >> diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h >> index d22b36b84b4b..958e6da7f475 100644 >> --- a/include/linux/moduleloader.h >> +++ b/include/linux/moduleloader.h >> @@ -73,7 +73,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs, >> unsigned int relsec, >> struct module *mod); >> #ifdef CONFIG_LIVEPATCH >> -void clear_relocate_add(Elf64_Shdr *sechdrs, >> +void clear_relocate_add(Elf_Shdr *sechdrs, >> const char *strtab, >> unsigned int symindex, >> unsigned int relsec, >> >> >> But then the link fails. >> >> LD .tmp_vmlinux.kallsyms1 >> powerpc64-linux-ld: kernel/livepatch/core.o: in function >> `klp_cleanup_module_patches_limited': >> core.c:(.text+0xdb4): undefined reference to `clear_relocate_add' > > Hmm.. I am not seeing either error. Could you please share your .config file? > defconfig follows: # CONFIG_LOCALVERSION_AUTO is not set CONFIG_SYSVIPC=y CONFIG_POSIX_MQUEUE=y CONFIG_NO_HZ=y CONFIG_HIGH_RES_TIMERS=y CONFIG_IKCONFIG=y CONFIG_IKCONFIG_PROC=y CONFIG_LOG_BUF_SHIFT=14 CONFIG_BLK_DEV_INITRD=y CONFIG_KALLSYMS_ALL=y CONFIG_PROFILING=y CONFIG_ALTIVEC=y # CONFIG_PPC_CHRP is not set CONFIG_CPU_FREQ=y CONFIG_CPU_FREQ_GOV_POWERSAVE=y CONFIG_CPU_FREQ_GOV_USERSPACE=y CONFIG_CPU_FREQ_PMAC=y CONFIG_GEN_RTC=y CONFIG_HIGHMEM=y CONFIG_HIBERNATION=y CONFIG_PM_DEBUG=y CONFIG_APM_EMULATION=y CONFIG_LIVEPATCH=y CONFIG_MODULES=y CONFIG_MODULE_UNLOAD=y CONFIG_MODULE_FORCE_UNLOAD=y CONFIG_PARTITION_ADVANCED=y CONFIG_BINFMT_MISC=m # CONFIG_COMPAT_BRK is not set CONFIG_NET=y CONFIG_PACKET=y CONFIG_UNIX=y CONFIG_XFRM_USER=y CONFIG_NET_KEY=y CONFIG_INET=y CONFIG_IP_MULTICAST=y CONFIG_SYN_COOKIES=y CONFIG_INET_AH=y CONFIG_INET_ESP=y # CONFIG_IPV6 is not set CONFIG_NETFILTER=y CONFIG_NF_CONNTRACK=m CONFIG_NF_CONNTRACK_FTP=m CONFIG_NF_CONNTRACK_IRC=m CONFIG_NF_CONNTRACK_TFTP=m CONFIG_NF_CT_NETLINK=m CONFIG_NETFILTER_XT_TARGET_CLASSIFY=m CONFIG_NETFILTER_XT_TARGET_MARK=m CONFIG_NETFILTER_XT_TARGET_NFLOG=m CONFIG_NETFILTER_XT_TARGET_NFQUEUE=m CONFIG_NETFILTER_XT_TARGET_TRACE=m CONFIG_NETFILTER_XT_TARGET_TCPMSS=m CONFIG_NETFILTER_XT_TARGET_TCPOPTSTRIP=m CONFIG_NETFILTER_XT_MATCH_COMMENT=m CONFIG_NETFILTER_XT_MATCH_CONNLIMIT=m CONFIG_NETFILTER_XT_MATCH_CONNTRACK=m CONFIG_NETFILTER_XT_MATCH_DSCP=m CONFIG_NETFILTER_XT_MATCH_ESP=m CONFIG_NETFILTER_XT_MATCH_HELPER=m CONFIG_NETFILTER_XT_MATCH_IPRANGE=m CONFIG_NETFILTER_XT_MATCH_LENGTH=m CONFIG_NETFILTER_XT_MATCH_LIMIT=m CONFIG_NETFILTER_XT_MATCH_MAC=m CONFIG_NETFILTER_XT_MATCH_MARK=m CONFIG_NETFILTER_XT_MATCH_MULTIPORT=m CONFIG_NETFILTER_XT_MATCH_OWNER=m CONFIG_NETFILTER_XT_MATCH_POLICY=m CONFIG_NETFILTER_XT_MATCH_PKTTYPE=m CONFIG_NETFILTER_XT_MATCH_RATEEST=m CONFIG_NETFILTER_XT_MATCH_REALM=m CONFIG_NETFILTER_XT_MATCH_RECENT=m CONFIG_NETFILTER_XT_MATCH_SCTP=m CONFIG_NETFILTER_XT_MATCH_STRING=m CONFIG_NETFILTER_XT_MATCH_TCPMSS=m CONFIG_NETFILTER_XT_MATCH_TIME=m CONFIG_NETFILTER_XT_MATCH_U32=m CONFIG_IP_NF_IPTABLES=m CONFIG_IP_NF_MATCH_AH=m CONFIG_IP_NF_MATCH_ECN=m CONFIG_IP_NF_MATCH_TTL=m CONFIG_IP_NF_FILTER=m CONFIG_IP_NF_TARGET_REJECT=m CONFIG_IP_NF_MANGLE=m CONFIG_IP_NF_TARGET_ECN=m CONFIG_IP_NF_TARGET_TTL=m CONFIG_IP_NF_RAW=m CONFIG_IP_NF_ARPTABLES=m CONFIG_IP_NF_ARPFILTER=m CONFIG_IP_NF_ARP_MANGLE=m CONFIG_IP_DCCP=m CONFIG_BT=m CONFIG_BT_RFCOMM=m CONFIG_BT_RFCOMM_TTY=y CONFIG_BT_BNEP=m CONFIG_BT_BNEP_MC_FILTER=y CONFIG_BT_BNEP_PROTO_FILTER=y CONFIG_BT_HIDP=m CONFIG_BT_HCIBCM203X=m CONFIG_BT_HCIBFUSB=m CONFIG_CFG80211=m CONFIG_MAC80211=m CONFIG_MAC80211_LEDS=y CONFIG_PCCARD=m CONFIG_YENTA=m # CONFIG_STANDALONE is not set CONFIG_CONNECTOR=y CONFIG_MAC_FLOPPY=m CONFIG_BLK_DEV_LOOP=y CONFIG_BLK_DEV_RAM=y CONFIG_BLK_DEV_SD=y CONFIG_CHR_DEV_ST=y CONFIG_BLK_DEV_SR=y CONFIG_CHR_DEV_SG=y CONFIG_SCSI_CONSTANTS=y CONFIG_SCSI_FC_ATTRS=y CONFIG_SCSI_AIC7XXX=m CONFIG_AIC7XXX_CMDS_PER_DEVICE=253 CONFIG_AIC7XXX_RESET_DELAY_MS=15000 CONFIG_SCSI_SYM53C8XX_2=y CONFIG_SCSI_SYM53C8XX_DMA_ADDRESSING_MODE=0 CONFIG_SCSI_MESH=y CONFIG_SCSI_MAC53C94=y CONFIG_ATA=y CONFIG_PATA_MACIO=y CONFIG_PATA_PDC2027X=y CONFIG_PATA_WINBOND=y CONFIG_PATA_PCMCIA=m CONFIG_ATA_GENERIC=y CONFIG_MD=y CONFIG_BLK_DEV_MD=m CONFIG_MD_LINEAR=m CONFIG_MD_RAID0=m CONFIG_MD_RAID1=m CONFIG_MD_RAID10=m CONFIG_MD_MULTIPATH=m CONFIG_MD_FAULTY=m CONFIG_BLK_DEV_DM=m CONFIG_DM_CRYPT=m CONFIG_DM_SNAPSHOT=m CONFIG_DM_MIRROR=m CONFIG_DM_ZERO=m CONFIG_ADB=y CONFIG_ADB_CUDA=y CONFIG_ADB_PMU=y CONFIG_ADB_PMU_LED=y CONFIG_ADB_PMU_LED_DISK=y CONFIG_PMAC_APM_EMU=m CONFIG_PMAC_MEDIABAY=y CONFIG_PMAC_BACKLIGHT=y CONFIG_PMAC_BACKLIGHT_LEGACY=y CONFIG_INPUT_ADBHID=y CONFIG_MAC_EMUMOUSEBTN=y CONFIG_THERM_WINDTUNNEL=m CONFIG_THERM_ADT746X=m CONFIG_PMAC_RACKMETER=m CONFIG_NETDEVICES=y CONFIG_DUMMY=m CONFIG_TUN=m CONFIG_PCNET32=y CONFIG_MACE=y CONFIG_BMAC=y CONFIG_SUNGEM=y CONFIG_PPP=y CONFIG_PPP_BSDCOMP=m CONFIG_PPP_DEFLATE=y CONFIG_PPP_MULTILINK=y CONFIG_PPP_ASYNC=y CONFIG_PPP_SYNC_TTY=m CONFIG_USB_USBNET=m # CONFIG_USB_NET_CDC_SUBSET is not set CONFIG_B43=m CONFIG_B43LEGACY=m CONFIG_P54_COMMON=m CONFIG_INPUT_EVDEV=y # CONFIG_KEYBOARD_ATKBD is not set # CONFIG_MOUSE_PS2 is not set CONFIG_MOUSE_APPLETOUCH=y # CONFIG_SERIO_I8042 is not set # CONFIG_SERIO_SERPORT is not set CONFIG_SERIAL_8250=m CONFIG_SERIAL_PMACZILOG=m CONFIG_SERIAL_PMACZILOG_TTYS=y CONFIG_I2C_CHARDEV=m CONFIG_APM_POWER=y CONFIG_BATTERY_PMU=y CONFIG_HWMON=m CONFIG_AGP=m CONFIG_AGP_UNINORTH=m CONFIG_DRM=m CONFIG_DRM_RADEON=m CONFIG_DRM_LEGACY=y CONFIG_DRM_R128=m CONFIG_FB=y CONFIG_FB_OF=y CONFIG_FB_CONTROL=y CONFIG_FB_PLATINUM=y CONFIG_FB_VALKYRIE=y CONFIG_FB_CT65550=y CONFIG_FB_IMSTT=y CONFIG_FB_NVIDIA=y CONFIG_FB_NVIDIA_I2C=y CONFIG_FB_MATROX=y CONFIG_FB_MATROX_MILLENIUM=y CONFIG_FB_MATROX_MYSTIQUE=y CONFIG_FB_RADEON=y CONFIG_FB_ATY128=y CONFIG_FB_ATY=y CONFIG_FB_ATY_CT=y CONFIG_FB_ATY_GX=y CONFIG_FB_3DFX=y # CONFIG_VGA_CONSOLE is not set CONFIG_LOGO=y CONFIG_SOUND=m CONFIG_SND=m CONFIG_SND_OSSEMUL=y CONFIG_SND_MIXER_OSS=m CONFIG_SND_PCM_OSS=m CONFIG_SND_SEQUENCER=m CONFIG_SND_SEQ_DUMMY=m CONFIG_SND_SEQUENCER_OSS=m CONFIG_SND_DUMMY=m CONFIG_SND_POWERMAC=m CONFIG_SND_AOA=m CONFIG_SND_AOA_FABRIC_LAYOUT=m CONFIG_SND_AOA_ONYX=m CONFIG_SND_AOA_TAS=m CONFIG_SND_AOA_TOONIE=m CONFIG_SND_USB_AUDIO=m CONFIG_HID_GYRATION=y CONFIG_HID_NTRIG=y CONFIG_HID_PANTHERLORD=y CONFIG_HID_PETALYNX=y CONFIG_HID_SAMSUNG=y CONFIG_HID_SONY=y CONFIG_HID_SUNPLUS=y CONFIG_HID_TOPSEED=y CONFIG_USB_DYNAMIC_MINORS=y CONFIG_USB_MON=y CONFIG_USB_EHCI_HCD=m CONFIG_USB_EHCI_ROOT_HUB_TT=y # CONFIG_USB_EHCI_HCD_PPC_OF is not set CONFIG_USB_OHCI_HCD=y CONFIG_USB_ACM=m CONFIG_USB_PRINTER=m CONFIG_USB_STORAGE=m CONFIG_USB_STORAGE_ONETOUCH=m CONFIG_USB_SERIAL=m CONFIG_USB_SERIAL_VISOR=m CONFIG_USB_SERIAL_IPAQ=m CONFIG_USB_SERIAL_KEYSPAN_PDA=m CONFIG_USB_SERIAL_KEYSPAN=m CONFIG_USB_APPLEDISPLAY=m CONFIG_LEDS_TRIGGER_DEFAULT_ON=y CONFIG_EXT2_FS=y CONFIG_EXT4_FS=y CONFIG_EXT4_FS_POSIX_ACL=y CONFIG_AUTOFS4_FS=m CONFIG_FUSE_FS=m CONFIG_ISO9660_FS=y CONFIG_JOLIET=y CONFIG_ZISOFS=y CONFIG_UDF_FS=m CONFIG_MSDOS_FS=m CONFIG_VFAT_FS=m CONFIG_PROC_KCORE=y CONFIG_TMPFS=y CONFIG_HFS_FS=m CONFIG_HFSPLUS_FS=m CONFIG_NFS_FS=y CONFIG_NFS_V3_ACL=y CONFIG_NFS_V4=y CONFIG_NFSD=m CONFIG_NFSD_V3_ACL=y CONFIG_NFSD_V4=y CONFIG_NLS_CODEPAGE_437=m CONFIG_NLS_ISO8859_1=m CONFIG_CRYPTO_PCBC=m CONFIG_CRYPTO_MD4=m CONFIG_CRYPTO_WP512=m CONFIG_CRYPTO_BLOWFISH=m CONFIG_CRYPTO_CAST5=m CONFIG_CRYPTO_CAST6=m CONFIG_CRYPTO_SERPENT=m CONFIG_CRYPTO_TWOFISH=m CONFIG_CRYPTO_DEFLATE=m CONFIG_CRC_T10DIF=y CONFIG_DEBUG_KERNEL=y CONFIG_MAGIC_SYSRQ=y CONFIG_DETECT_HUNG_TASK=y CONFIG_FUNCTION_TRACER=y CONFIG_XMON=y CONFIG_XMON_DEFAULT=y CONFIG_BOOTX_TEXT=y
On Tue, Aug 30, 2022 at 11:53:13AM -0700, Song Liu wrote: > From: Miroslav Benes <mbenes@suse.cz> > > Josh reported a bug: > > When the object to be patched is a module, and that module is > rmmod'ed and reloaded, it fails to load with: > > module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c > livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) > livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' > > The livepatch module has a relocation which references a symbol > in the _previous_ loading of nfsd. When apply_relocate_add() > tries to replace the old relocation with a new one, it sees that > the previous one is nonzero and it errors out. > > On ppc64le, we have a similar issue: > > module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd] > livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) > livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' > > He also proposed three different solutions. We could remove the error > check in apply_relocate_add() introduced by commit eda9cec4c9a1 > ("x86/module: Detect and skip invalid relocations"). However the check > is useful for detecting corrupted modules. > > We could also deny the patched modules to be removed. If it proved to be > a major drawback for users, we could still implement a different > approach. The solution would also complicate the existing code a lot. > > We thus decided to reverse the relocation patching (clear all relocation > targets on x86_64). The solution is not > universal and is too much arch-specific, but it may prove to be simpler > in the end. > > Reported-by: Josh Poimboeuf <jpoimboe@redhat.com> > Signed-off-by: Miroslav Benes <mbenes@suse.cz> > Signed-off-by: Song Liu <song@kernel.org> > > --- > > NOTE: powerpc code has not be tested. > > Changes v4 = v5: > 1. Fix compile with powerpc. > > Changes v3 = v4: > 1. Reuse __apply_relocate_add to make it more reliable in long term. > (Josh Poimboeuf) > 2. Add back ppc64 logic from v2, with changes to match current code. > (Josh Poimboeuf) > > Changes v2 => v3: > 1. Rewrite x86 changes to match current code style. > 2. Remove powerpc changes as there is no test coverage in v3. > 3. Only keep 1/3 of v2. > > v2: https://lore.kernel.org/all/20190905124514.8944-1-mbenes@suse.cz/T/#u > --- > arch/powerpc/kernel/module_64.c | 49 +++++++++++++++ > arch/s390/kernel/module.c | 8 +++ > arch/x86/kernel/module.c | 102 +++++++++++++++++++++++--------- > include/linux/moduleloader.h | 7 +++ > kernel/livepatch/core.c | 41 ++++++++++++- > 5 files changed, 179 insertions(+), 28 deletions(-) > > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c > index 7e45dc98df8a..6aaf5720070d 100644 > --- a/arch/powerpc/kernel/module_64.c > +++ b/arch/powerpc/kernel/module_64.c > @@ -739,6 +739,55 @@ int apply_relocate_add(Elf64_Shdr *sechdrs, > return 0; > } > > +#ifdef CONFIG_LIVEPATCH > +void clear_relocate_add(Elf64_Shdr *sechdrs, > + const char *strtab, > + unsigned int symindex, > + unsigned int relsec, > + struct module *me) > +{ > + unsigned int i; > + Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr; > + Elf64_Sym *sym; > + unsigned long *location; > + const char *symname; > + u32 *instruction; > + > + pr_debug("Clearing ADD relocate section %u to %u\n", relsec, > + sechdrs[relsec].sh_info); > + > + for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) { > + location = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr > + + rela[i].r_offset; > + sym = (Elf64_Sym *)sechdrs[symindex].sh_addr > + + ELF64_R_SYM(rela[i].r_info); > + symname = me->core_kallsyms.strtab > + + sym->st_name; > + > + if (ELF64_R_TYPE(rela[i].r_info) != R_PPC_REL24) > + continue; > + /* > + * reverse the operations in apply_relocate_add() for case > + * R_PPC_REL24. > + */ > + if (sym->st_shndx != SHN_UNDEF && > + sym->st_shndx != SHN_LIVEPATCH) > + continue; > + > + instruction = (u32 *)location; > + if (is_mprofile_ftrace_call(symname)) > + continue; > + > + if (!instr_is_relative_link_branch(ppc_inst(*instruction))) > + continue; > + > + instruction += 1; > + *instruction = PPC_RAW_NOP(); > + } > + > +} > +#endif > + > #ifdef CONFIG_DYNAMIC_FTRACE > int module_trampoline_target(struct module *mod, unsigned long addr, > unsigned long *target) > diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c > index 2d159b32885b..cc6784fbc1ac 100644 > --- a/arch/s390/kernel/module.c > +++ b/arch/s390/kernel/module.c > @@ -500,6 +500,14 @@ static int module_alloc_ftrace_hotpatch_trampolines(struct module *me, > } > #endif /* CONFIG_FUNCTION_TRACER */ > > +#ifdef CONFIG_LIVEPATCH > +void clear_relocate_add(Elf64_Shdr *sechdrs, const char *strtab, > + unsigned int symindex, unsigned int relsec, > + struct module *me) > +{ > +} > +#endif > + > int module_finalize(const Elf_Ehdr *hdr, > const Elf_Shdr *sechdrs, > struct module *me) > diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c > index b1abf663417c..f9632afbb84c 100644 > --- a/arch/x86/kernel/module.c > +++ b/arch/x86/kernel/module.c > @@ -128,18 +128,20 @@ int apply_relocate(Elf32_Shdr *sechdrs, > return 0; > } > #else /*X86_64*/ > -static int __apply_relocate_add(Elf64_Shdr *sechdrs, > +static int __apply_clear_relocate_add(Elf64_Shdr *sechdrs, > const char *strtab, > unsigned int symindex, > unsigned int relsec, > struct module *me, > - void *(*write)(void *dest, const void *src, size_t len)) > + void *(*write)(void *dest, const void *src, size_t len), > + bool clear) > { > unsigned int i; > Elf64_Rela *rel = (void *)sechdrs[relsec].sh_addr; > Elf64_Sym *sym; > void *loc; > u64 val; > + u64 zero = 0ULL; > > DEBUGP("Applying relocate section %u to %u\n", > relsec, sechdrs[relsec].sh_info); > @@ -163,40 +165,60 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs, > case R_X86_64_NONE: > break; > case R_X86_64_64: > - if (*(u64 *)loc != 0) > - goto invalid_relocation; > - write(loc, &val, 8); > + if (!clear) { > + if (*(u64 *)loc != 0) > + goto invalid_relocation; > + write(loc, &val, 8); > + } else { > + write(loc, &zero, 8); > + } > break; > case R_X86_64_32: > - if (*(u32 *)loc != 0) > - goto invalid_relocation; > - write(loc, &val, 4); > - if (val != *(u32 *)loc) > - goto overflow; > + if (!clear) { > + if (*(u32 *)loc != 0) > + goto invalid_relocation; > + write(loc, &val, 4); > + if (val != *(u32 *)loc) > + goto overflow; > + } else { > + write(loc, &zero, 4); > + } > break; > case R_X86_64_32S: > - if (*(s32 *)loc != 0) > - goto invalid_relocation; > - write(loc, &val, 4); > - if ((s64)val != *(s32 *)loc) > - goto overflow; > + if (!clear) { > + if (*(s32 *)loc != 0) > + goto invalid_relocation; > + write(loc, &val, 4); > + if ((s64)val != *(s32 *)loc) > + goto overflow; > + } else { > + write(loc, &zero, 4); > + } > break; > case R_X86_64_PC32: > case R_X86_64_PLT32: > - if (*(u32 *)loc != 0) > - goto invalid_relocation; > - val -= (u64)loc; > - write(loc, &val, 4); > + if (!clear) { > + if (*(u32 *)loc != 0) > + goto invalid_relocation; > + val -= (u64)loc; > + write(loc, &val, 4); > #if 0 > - if ((s64)val != *(s32 *)loc) > - goto overflow; > + if ((s64)val != *(s32 *)loc) > + goto overflow; > #endif > + } else { > + write(loc, &zero, 4); > + } > break; > case R_X86_64_PC64: > - if (*(u64 *)loc != 0) > - goto invalid_relocation; > - val -= (u64)loc; > - write(loc, &val, 8); > + if (!clear) { > + if (*(u64 *)loc != 0) > + goto invalid_relocation; > + val -= (u64)loc; > + write(loc, &val, 8); > + } else { > + write(loc, &zero, 8); > + } > break; > default: > pr_err("%s: Unknown rela relocation: %llu\n", > @@ -234,8 +256,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs, > mutex_lock(&text_mutex); > } > > - ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me, > - write); > + ret = __apply_clear_relocate_add(sechdrs, strtab, symindex, relsec, me, > + write, false /* clear */); > > if (!early) { > text_poke_sync(); > @@ -245,6 +267,32 @@ int apply_relocate_add(Elf64_Shdr *sechdrs, > return ret; > } > > +#ifdef CONFIG_LIVEPATCH > + > +void clear_relocate_add(Elf64_Shdr *sechdrs, > + const char *strtab, > + unsigned int symindex, > + unsigned int relsec, > + struct module *me) > +{ > + bool early = me->state == MODULE_STATE_UNFORMED; > + void *(*write)(void *, const void *, size_t) = memcpy; > + > + if (!early) { > + write = text_poke; > + mutex_lock(&text_mutex); > + } > + > + __apply_clear_relocate_add(sechdrs, strtab, symindex, relsec, me, > + write, true /* clear */); > + > + if (!early) { > + text_poke_sync(); > + mutex_unlock(&text_mutex); > + } > +} > +#endif > + > #endif > > int module_finalize(const Elf_Ehdr *hdr, > diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h > index 9e09d11ffe5b..d22b36b84b4b 100644 > --- a/include/linux/moduleloader.h > +++ b/include/linux/moduleloader.h > @@ -72,6 +72,13 @@ int apply_relocate_add(Elf_Shdr *sechdrs, > unsigned int symindex, > unsigned int relsec, > struct module *mod); > +#ifdef CONFIG_LIVEPATCH > +void clear_relocate_add(Elf64_Shdr *sechdrs, > + const char *strtab, > + unsigned int symindex, > + unsigned int relsec, > + struct module *me); > +#endif > #else > static inline int apply_relocate_add(Elf_Shdr *sechdrs, > const char *strtab, > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index bc475e62279d..5c0d8a4eba13 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -316,6 +316,45 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs, > return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod); > } > > +static void klp_clear_object_relocations(struct module *pmod, > + struct klp_object *obj) > +{ > + int i, cnt; > + const char *objname, *secname; > + char sec_objname[MODULE_NAME_LEN]; > + Elf_Shdr *sec; > + > + objname = klp_is_module(obj) ? obj->name : "vmlinux"; > + > + /* For each klp relocation section */ > + for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) { > + sec = pmod->klp_info->sechdrs + i; > + secname = pmod->klp_info->secstrings + sec->sh_name; > + if (!(sec->sh_flags & SHF_RELA_LIVEPATCH)) > + continue; > + > + /* > + * Format: .klp.rela.sec_objname.section_name > + * See comment in klp_resolve_symbols() for an explanation > + * of the selected field width value. > + */ > + secname = pmod->klp_info->secstrings + sec->sh_name; > + cnt = sscanf(secname, ".klp.rela.%55[^.]", sec_objname); > + if (cnt != 1) { > + pr_err("section %s has an incorrectly formatted name\n", > + secname); > + continue; > + } > + > + if (strcmp(objname, sec_objname)) > + continue; > + > + clear_relocate_add(pmod->klp_info->sechdrs, > + pmod->core_kallsyms.strtab, > + pmod->klp_info->symndx, i, pmod); > + } > +} > + > /* > * Sysfs Interface > * > @@ -1154,7 +1193,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod, > klp_unpatch_object(obj); > > klp_post_unpatch_callback(obj); > - > + klp_clear_object_relocations(patch->mod, obj); > klp_free_object_loaded(obj); > break; > } > -- > 2.30.2 > Hi Song, Applying your patch on top of my latest klp-convert-tree branch [1], I modified a few of its late module patching tests (tools/testing/selftests/livepatch/test-song.sh) such that: 1 - A livepatch module is loaded - this module contains klp-relocations to objects in (2) 2 - A target test module is loaded 3 - Unload the test target module - Clear klp-relocations in (1) 4 - Repeat target module load (2) / unload (3) a few times 5 - Unload livepatch module The results: x86_64 : pass s390x : pass ppc64le : crash I suspect Power 32-bit would suffer the same fate, but I don't have hardware to verify. See the kernel log from the crash below... ===== TEST: klp-convert symbols (late module patching) ===== % modprobe test_klp_convert1 test_klp_convert1: tainting kernel with TAINT_LIVEPATCH livepatch: enabling patch 'test_klp_convert1' livepatch: 'test_klp_convert1': starting patching transition livepatch: 'test_klp_convert1': patching complete % modprobe test_klp_convert_mod livepatch: applying patch 'test_klp_convert1' to loading module 'test_klp_convert_mod' test_klp_convert1: saved_command_line, 0: BOOT_IMAGE=(ieee1275//vdevice/v-scsi@30000003/disk@8100000000000000,msdos2)/vmlinuz-5.19.0+ root=/dev/mapper/rhel_ibm--p9z--18--lp7-root ro crashkernel=2G-4G:384M,4G-16G:512M,16G-64G:1G,64G-128G:2G,128G-:4G rd.lvm.lv=rhel_ibm-p9z-18-lp7/root rd.lvm.lv=rhel_ibm-p9z-18-lp7/swap test_klp_convert1: driver_name, 0: test_klp_convert_mod test_klp_convert1: test_klp_get_driver_name(), 0: test_klp_convert_mod test_klp_convert1: homonym_string, 1: homonym string A test_klp_convert1: get_homonym_string(), 1: homonym string A test_klp_convert1: klp_string.12345 = lib/livepatch/test_klp_convert_mod_a.c static string test_klp_convert1: klp_string.67890 = lib/livepatch/test_klp_convert_mod_b.c static string % rmmod test_klp_convert_mod livepatch: reverting patch 'test_klp_convert1' on unloading module 'test_klp_convert_mod' module_64: Clearing ADD relocate section 48 to 6 BUG: Unable to handle kernel data access on write at 0xc008000002140150 Faulting instruction address: 0xc00000000005659c Oops: Kernel access of bad area, sig: 11 [#1] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries Modules linked in: test_klp_convert_mod(-) test_klp_convert1(K) bonding tls rfkill pseries_rng drm fuse drm_panel_orientation_quirks xfs libcrc32c sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp vmx_crypto dm_mirror dm_region_hash dm_log dm_mod CPU: 6 PID: 4766 Comm: rmmod Kdump: loaded Tainted: G K 5.19.0+ #1 NIP: c00000000005659c LR: c000000000056590 CTR: 0000000000000024 REGS: c000000007223840 TRAP: 0300 Tainted: G K (5.19.0+) MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 48008282 XER: 0000000a CFAR: c0000000000a87e0 DAR: c008000002140150 DSISR: 0a000000 IRQMASK: 0 GPR00: c000000000056568 c000000007223ae0 c000000002a68a00 0000000000000001 GPR04: c0080000021706f0 000000000000002d 0000000000000000 0000000000000000 GPR08: 0000000000000066 0000001200000010 0000000000000000 0000000000008000 GPR12: 0000000000000000 c00000000ffca080 0000000000000000 0000000000000000 GPR16: 0000010005bf1810 000000010c0f7370 c0000000011b7e50 c0000000011b7e68 GPR20: c0080000021501c8 c008000002150228 0000000000000030 0000000060000000 GPR24: c008000002160380 c000000056b43000 000000000000ff20 c000000056b43c00 GPR28: aaaaaaaaaaaaaaab c000000056b43b40 0000000000000000 c00800000214014c NIP [c00000000005659c] clear_relocate_add+0x11c/0x1c0 LR [c000000000056590] clear_relocate_add+0x110/0x1c0 Call Trace: [c000000007223ae0] [ffffffffffffffff] 0xffffffffffffffff (unreliable) [c000000007223ba0] [c00000000021e3a8] klp_cleanup_module_patches_limited+0x448/0x480 [c000000007223cb0] [c000000000220278] klp_module_going+0x68/0x94 [c000000007223ce0] [c00000000022f480] __do_sys_delete_module.constprop.0+0x1d0/0x390 [c000000007223db0] [c00000000002f004] system_call_exception+0x164/0x340 [c000000007223e10] [c00000000000be68] system_call_vectored_common+0xe8/0x278 --- interrupt: 3000 at 0x7fffa178fb6c NIP: 00007fffa178fb6c LR: 0000000000000000 CTR: 0000000000000000 REGS: c000000007223e80 TRAP: 3000 Tainted: G K (5.19.0+) MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48002482 XER: 00000000 IRQMASK: 0 GPR00: 0000000000000081 00007ffff2d1b720 00007fffa1887200 0000010005bf1878 GPR04: 0000000000000800 000000000000000a 0000000000000000 00000000000000da GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 GPR12: 0000000000000000 00007fffa201c540 0000000000000000 0000000000000000 GPR16: 0000010005bf1810 000000010c0f7370 000000010c0f8090 000000010c0f8078 GPR20: 000000010c0f8050 000000010c0f80a8 000000010c0f7518 000000010c0f80d0 GPR24: 00007ffff2d1b830 00007ffff2d1efbb 0000000000000000 0000010005bf02a0 GPR28: 00007ffff2d1be50 0000000000000000 0000010005bf1810 0000000000100000 NIP [00007fffa178fb6c] 0x7fffa178fb6c LR [0000000000000000] 0x0 --- interrupt: 3000 Instruction dump: 40820044 813b002c 7ff5f82a 79293664 7d394a14 e9290010 7c69f82e 7fe9fa14 48052235 60000000 2c030000 41820008 <92ff0004> eadb0020 60000000 60000000 ---[ end trace 0000000000000000 ]--- $ addr2line 0xc00000000005659c -e vmlinux /root/klp-convert-tree/arch/powerpc/kernel/module_64.c:785 743 void clear_relocate_add(Elf64_Shdr *sechdrs, 744 const char *strtab, 745 unsigned int symindex, 746 unsigned int relsec, 747 struct module *me) 748 { ... 759 for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) { ... 785 *instruction = PPC_RAW_NOP(); 786 } $ readelf --wide --sections test_klp_convert1.ko | grep -e '\[ 6\]' -e '\[48\]' [ 6] .text.unlikely PROGBITS 0000000000000000 0001b8 0001cc 00 AX 0 0 4 [48] .klp.rela.test_klp_convert_mod..text.unlikely RELA 0000000000000000 041358 000030 18 Ao 45 6 8 $ readelf --wide --relocs test_klp_convert1.ko ... Relocation section '.klp.rela.test_klp_convert_mod..text.unlikely' at offset 0x41358 contains 2 entries: Offset Info Type Symbol's Value Symbol's Name + Addend 00000000000000f0 000000490000000a R_PPC64_REL24 0000000000000000 .klp.sym.test_klp_convert_mod.test_klp_get_driver_name,0 + 0 0000000000000158 000000450000000a R_PPC64_REL24 0000000000000000 .klp.sym.test_klp_convert_mod.get_homonym_string,1 + 0 PS - I will be OOTO for a few weeks in Sept [1] https://github.com/joe-lawrence/klp-convert-tree/tree/klp-convert-v7-devel+song -- Joe
On Wed, Aug 31, 2022 at 10:05:43AM -0700, Song Liu wrote: > On Wed, Aug 31, 2022 at 1:01 AM Christophe Leroy > <christophe.leroy@csgroup.eu> wrote: > > > > > > > > Le 30/08/2022 à 20:53, Song Liu a écrit : > > > From: Miroslav Benes <mbenes@suse.cz> > > > > > > Josh reported a bug: > > > > > > When the object to be patched is a module, and that module is > > > rmmod'ed and reloaded, it fails to load with: > > > > > > module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c > > > livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) > > > livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' > > > > > > The livepatch module has a relocation which references a symbol > > > in the _previous_ loading of nfsd. When apply_relocate_add() > > > tries to replace the old relocation with a new one, it sees that > > > the previous one is nonzero and it errors out. > > > > > > On ppc64le, we have a similar issue: > > > > > > module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd] > > > livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) > > > livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' > > > > > > He also proposed three different solutions. We could remove the error > > > check in apply_relocate_add() introduced by commit eda9cec4c9a1 > > > ("x86/module: Detect and skip invalid relocations"). However the check > > > is useful for detecting corrupted modules. > > > > > > We could also deny the patched modules to be removed. If it proved to be > > > a major drawback for users, we could still implement a different > > > approach. The solution would also complicate the existing code a lot. > > > > > > We thus decided to reverse the relocation patching (clear all relocation > > > targets on x86_64). The solution is not > > > universal and is too much arch-specific, but it may prove to be simpler > > > in the end. > > > > > > Reported-by: Josh Poimboeuf <jpoimboe@redhat.com> > > > Signed-off-by: Miroslav Benes <mbenes@suse.cz> > > > Signed-off-by: Song Liu <song@kernel.org> > > > > > > --- > > > > > > NOTE: powerpc code has not be tested. > > > > > > Changes v4 = v5: > > > 1. Fix compile with powerpc. > > > > Not completely it seems. > > > > CC kernel/livepatch/core.o > > kernel/livepatch/core.c: In function 'klp_clear_object_relocations': > > kernel/livepatch/core.c:352:50: error: passing argument 1 of > > 'clear_relocate_add' from incompatible pointer type > > [-Werror=incompatible-pointer-types] > > 352 | clear_relocate_add(pmod->klp_info->sechdrs, > > | ~~~~~~~~~~~~~~^~~~~~~~~ > > | | > > | Elf32_Shdr * > > {aka struct elf32_shdr *} > > In file included from kernel/livepatch/core.c:19: > > ./include/linux/moduleloader.h:76:37: note: expected 'Elf64_Shdr *' {aka > > 'struct elf64_shdr *'} but argument is of type 'Elf32_Shdr *' {aka > > 'struct elf32_shdr *'} > > 76 | void clear_relocate_add(Elf64_Shdr *sechdrs, > > | ~~~~~~~~~~~~^~~~~~~ > > cc1: some warnings being treated as errors > > > > > > Fixup: > > > > diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h > > index d22b36b84b4b..958e6da7f475 100644 > > --- a/include/linux/moduleloader.h > > +++ b/include/linux/moduleloader.h > > @@ -73,7 +73,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs, > > unsigned int relsec, > > struct module *mod); > > #ifdef CONFIG_LIVEPATCH > > -void clear_relocate_add(Elf64_Shdr *sechdrs, > > +void clear_relocate_add(Elf_Shdr *sechdrs, > > const char *strtab, > > unsigned int symindex, > > unsigned int relsec, > > > > > > But then the link fails. > > > > LD .tmp_vmlinux.kallsyms1 > > powerpc64-linux-ld: kernel/livepatch/core.o: in function > > `klp_cleanup_module_patches_limited': > > core.c:(.text+0xdb4): undefined reference to `clear_relocate_add' > > Hmm.. I am not seeing either error. Could you please share your .config file? > > Thanks, > Song > If it's any help, I see the same build error Christophe reported using the 'cross-dev' script that's in my klp-convert-tree [1]. $ BUILD_ARCHES="ppc32" ./cross-dev config $ BUILD_ARCHES="ppc32" ./cross-dev build -j$(nproc) (The kernel will be built in /tmp/klp-convert-ppc32 btw.) Applying the header file fix results in the same linker error, too. [1] https://github.com/joe-lawrence/klp-convert-tree/tree/klp-convert-v7-devel+song -- Joe
Joe Lawrence <joe.lawrence@redhat.com> writes: > On Tue, Aug 30, 2022 at 11:53:13AM -0700, Song Liu wrote: >> From: Miroslav Benes <mbenes@suse.cz> >> >> Josh reported a bug: >> >> When the object to be patched is a module, and that module is >> rmmod'ed and reloaded, it fails to load with: >> >> module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c >> livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) >> livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' >> >> The livepatch module has a relocation which references a symbol >> in the _previous_ loading of nfsd. When apply_relocate_add() >> tries to replace the old relocation with a new one, it sees that >> the previous one is nonzero and it errors out. >> >> On ppc64le, we have a similar issue: >> >> module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd] >> livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) >> livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' ... >> > > Hi Song, > > Applying your patch on top of my latest klp-convert-tree branch [1], I > modified a few of its late module patching tests > (tools/testing/selftests/livepatch/test-song.sh) such that: > > 1 - A livepatch module is loaded > - this module contains klp-relocations to objects in (2) > 2 - A target test module is loaded > 3 - Unload the test target module > - Clear klp-relocations in (1) > 4 - Repeat target module load (2) / unload (3) a few times > 5 - Unload livepatch module If you push that test code somewhere I could test it on ppc64le. > The results: > > x86_64 : pass > s390x : pass > ppc64le : crash > > I suspect Power 32-bit would suffer the same fate, but I don't have > hardware to verify. See the kernel log from the crash below... > > > ===== TEST: klp-convert symbols (late module patching) ===== > % modprobe test_klp_convert1 > test_klp_convert1: tainting kernel with TAINT_LIVEPATCH > livepatch: enabling patch 'test_klp_convert1' > livepatch: 'test_klp_convert1': starting patching transition > livepatch: 'test_klp_convert1': patching complete > % modprobe test_klp_convert_mod > livepatch: applying patch 'test_klp_convert1' to loading module 'test_klp_convert_mod' > test_klp_convert1: saved_command_line, 0: BOOT_IMAGE=(ieee1275//vdevice/v-scsi@30000003/disk@8100000000000000,msdos2)/vmlinuz-5.19.0+ root=/dev/mapper/rhel_ibm--p9z--18--lp7-root ro crashkernel=2G-4G:384M,4G-16G:512M,16G-64G:1G,64G-128G:2G,128G-:4G rd.lvm.lv=rhel_ibm-p9z-18-lp7/root rd.lvm.lv=rhel_ibm-p9z-18-lp7/swap > test_klp_convert1: driver_name, 0: test_klp_convert_mod > test_klp_convert1: test_klp_get_driver_name(), 0: test_klp_convert_mod > test_klp_convert1: homonym_string, 1: homonym string A > test_klp_convert1: get_homonym_string(), 1: homonym string A > test_klp_convert1: klp_string.12345 = lib/livepatch/test_klp_convert_mod_a.c static string > test_klp_convert1: klp_string.67890 = lib/livepatch/test_klp_convert_mod_b.c static string > % rmmod test_klp_convert_mod > livepatch: reverting patch 'test_klp_convert1' on unloading module 'test_klp_convert_mod' > module_64: Clearing ADD relocate section 48 to 6 > BUG: Unable to handle kernel data access on write at 0xc008000002140150 > Faulting instruction address: 0xc00000000005659c > Oops: Kernel access of bad area, sig: 11 [#1] > LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries > Modules linked in: test_klp_convert_mod(-) test_klp_convert1(K) bonding tls rfkill pseries_rng drm fuse drm_panel_orientation_quirks xfs libcrc32c sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp vmx_crypto dm_mirror dm_region_hash dm_log dm_mod > CPU: 6 PID: 4766 Comm: rmmod Kdump: loaded Tainted: G K 5.19.0+ #1 > NIP: c00000000005659c LR: c000000000056590 CTR: 0000000000000024 > REGS: c000000007223840 TRAP: 0300 Tainted: G K (5.19.0+) > MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 48008282 XER: 0000000a > CFAR: c0000000000a87e0 DAR: c008000002140150 DSISR: 0a000000 IRQMASK: 0 This is saying you don't have permissions to write at that address. > GPR00: c000000000056568 c000000007223ae0 c000000002a68a00 0000000000000001 > GPR04: c0080000021706f0 000000000000002d 0000000000000000 0000000000000000 > GPR08: 0000000000000066 0000001200000010 0000000000000000 0000000000008000 > GPR12: 0000000000000000 c00000000ffca080 0000000000000000 0000000000000000 > GPR16: 0000010005bf1810 000000010c0f7370 c0000000011b7e50 c0000000011b7e68 > GPR20: c0080000021501c8 c008000002150228 0000000000000030 0000000060000000 > GPR24: c008000002160380 c000000056b43000 000000000000ff20 c000000056b43c00 > GPR28: aaaaaaaaaaaaaaab c000000056b43b40 0000000000000000 c00800000214014c > NIP [c00000000005659c] clear_relocate_add+0x11c/0x1c0 > LR [c000000000056590] clear_relocate_add+0x110/0x1c0 > Call Trace: > [c000000007223ae0] [ffffffffffffffff] 0xffffffffffffffff (unreliable) > [c000000007223ba0] [c00000000021e3a8] klp_cleanup_module_patches_limited+0x448/0x480 > [c000000007223cb0] [c000000000220278] klp_module_going+0x68/0x94 > [c000000007223ce0] [c00000000022f480] __do_sys_delete_module.constprop.0+0x1d0/0x390 > [c000000007223db0] [c00000000002f004] system_call_exception+0x164/0x340 > [c000000007223e10] [c00000000000be68] system_call_vectored_common+0xe8/0x278 > --- interrupt: 3000 at 0x7fffa178fb6c > NIP: 00007fffa178fb6c LR: 0000000000000000 CTR: 0000000000000000 > REGS: c000000007223e80 TRAP: 3000 Tainted: G K (5.19.0+) > MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48002482 XER: 00000000 > IRQMASK: 0 > GPR00: 0000000000000081 00007ffff2d1b720 00007fffa1887200 0000010005bf1878 > GPR04: 0000000000000800 000000000000000a 0000000000000000 00000000000000da > GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > GPR12: 0000000000000000 00007fffa201c540 0000000000000000 0000000000000000 > GPR16: 0000010005bf1810 000000010c0f7370 000000010c0f8090 000000010c0f8078 > GPR20: 000000010c0f8050 000000010c0f80a8 000000010c0f7518 000000010c0f80d0 > GPR24: 00007ffff2d1b830 00007ffff2d1efbb 0000000000000000 0000010005bf02a0 > GPR28: 00007ffff2d1be50 0000000000000000 0000010005bf1810 0000000000100000 > NIP [00007fffa178fb6c] 0x7fffa178fb6c > LR [0000000000000000] 0x0 > --- interrupt: 3000 > Instruction dump: > 40820044 813b002c 7ff5f82a 79293664 7d394a14 e9290010 7c69f82e 7fe9fa14 > 48052235 60000000 2c030000 41820008 <92ff0004> eadb0020 60000000 60000000 > ---[ end trace 0000000000000000 ]--- > > $ addr2line 0xc00000000005659c -e vmlinux > /root/klp-convert-tree/arch/powerpc/kernel/module_64.c:785 > > 743 void clear_relocate_add(Elf64_Shdr *sechdrs, > 744 const char *strtab, > 745 unsigned int symindex, > 746 unsigned int relsec, > 747 struct module *me) > 748 { > ... > 759 for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) { > ... > 785 *instruction = PPC_RAW_NOP(); > 786 } Has the module text been marked RW prior to this? I suspect not? In which case you need to use patch_instruction() here. cheers
On Wed, Aug 31, 2022 at 3:30 PM Michael Ellerman <mpe@ellerman.id.au> wrote: > > Joe Lawrence <joe.lawrence@redhat.com> writes: > > On Tue, Aug 30, 2022 at 11:53:13AM -0700, Song Liu wrote: > >> From: Miroslav Benes <mbenes@suse.cz> > >> > >> Josh reported a bug: > >> > >> When the object to be patched is a module, and that module is > >> rmmod'ed and reloaded, it fails to load with: > >> > >> module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c > >> livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) > >> livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' > >> > >> The livepatch module has a relocation which references a symbol > >> in the _previous_ loading of nfsd. When apply_relocate_add() > >> tries to replace the old relocation with a new one, it sees that > >> the previous one is nonzero and it errors out. > >> > >> On ppc64le, we have a similar issue: > >> > >> module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd] > >> livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) > >> livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' > ... > >> > > > > Hi Song, > > > > Applying your patch on top of my latest klp-convert-tree branch [1], I > > modified a few of its late module patching tests > > (tools/testing/selftests/livepatch/test-song.sh) such that: > > > > 1 - A livepatch module is loaded > > - this module contains klp-relocations to objects in (2) > > 2 - A target test module is loaded > > 3 - Unload the test target module > > - Clear klp-relocations in (1) > > 4 - Repeat target module load (2) / unload (3) a few times > > 5 - Unload livepatch module > > If you push that test code somewhere I could test it on ppc64le. > > > The results: > > > > x86_64 : pass > > s390x : pass > > ppc64le : crash > > > > I suspect Power 32-bit would suffer the same fate, but I don't have > > hardware to verify. See the kernel log from the crash below... > > > > > > ===== TEST: klp-convert symbols (late module patching) ===== > > % modprobe test_klp_convert1 > > test_klp_convert1: tainting kernel with TAINT_LIVEPATCH > > livepatch: enabling patch 'test_klp_convert1' > > livepatch: 'test_klp_convert1': starting patching transition > > livepatch: 'test_klp_convert1': patching complete > > % modprobe test_klp_convert_mod > > livepatch: applying patch 'test_klp_convert1' to loading module 'test_klp_convert_mod' > > test_klp_convert1: saved_command_line, 0: BOOT_IMAGE=(ieee1275//vdevice/v-scsi@30000003/disk@8100000000000000,msdos2)/vmlinuz-5.19.0+ root=/dev/mapper/rhel_ibm--p9z--18--lp7-root ro crashkernel=2G-4G:384M,4G-16G:512M,16G-64G:1G,64G-128G:2G,128G-:4G rd.lvm.lv=rhel_ibm-p9z-18-lp7/root rd.lvm.lv=rhel_ibm-p9z-18-lp7/swap > > test_klp_convert1: driver_name, 0: test_klp_convert_mod > > test_klp_convert1: test_klp_get_driver_name(), 0: test_klp_convert_mod > > test_klp_convert1: homonym_string, 1: homonym string A > > test_klp_convert1: get_homonym_string(), 1: homonym string A > > test_klp_convert1: klp_string.12345 = lib/livepatch/test_klp_convert_mod_a.c static string > > test_klp_convert1: klp_string.67890 = lib/livepatch/test_klp_convert_mod_b.c static string > > % rmmod test_klp_convert_mod > > livepatch: reverting patch 'test_klp_convert1' on unloading module 'test_klp_convert_mod' > > module_64: Clearing ADD relocate section 48 to 6 > > BUG: Unable to handle kernel data access on write at 0xc008000002140150 > > Faulting instruction address: 0xc00000000005659c > > Oops: Kernel access of bad area, sig: 11 [#1] > > LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries > > Modules linked in: test_klp_convert_mod(-) test_klp_convert1(K) bonding tls rfkill pseries_rng drm fuse drm_panel_orientation_quirks xfs libcrc32c sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp vmx_crypto dm_mirror dm_region_hash dm_log dm_mod > > CPU: 6 PID: 4766 Comm: rmmod Kdump: loaded Tainted: G K 5.19.0+ #1 > > NIP: c00000000005659c LR: c000000000056590 CTR: 0000000000000024 > > REGS: c000000007223840 TRAP: 0300 Tainted: G K (5.19.0+) > > MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 48008282 XER: 0000000a > > CFAR: c0000000000a87e0 DAR: c008000002140150 DSISR: 0a000000 IRQMASK: 0 > > This is saying you don't have permissions to write at that address. > > > GPR00: c000000000056568 c000000007223ae0 c000000002a68a00 0000000000000001 > > GPR04: c0080000021706f0 000000000000002d 0000000000000000 0000000000000000 > > GPR08: 0000000000000066 0000001200000010 0000000000000000 0000000000008000 > > GPR12: 0000000000000000 c00000000ffca080 0000000000000000 0000000000000000 > > GPR16: 0000010005bf1810 000000010c0f7370 c0000000011b7e50 c0000000011b7e68 > > GPR20: c0080000021501c8 c008000002150228 0000000000000030 0000000060000000 > > GPR24: c008000002160380 c000000056b43000 000000000000ff20 c000000056b43c00 > > GPR28: aaaaaaaaaaaaaaab c000000056b43b40 0000000000000000 c00800000214014c > > NIP [c00000000005659c] clear_relocate_add+0x11c/0x1c0 > > LR [c000000000056590] clear_relocate_add+0x110/0x1c0 > > Call Trace: > > [c000000007223ae0] [ffffffffffffffff] 0xffffffffffffffff (unreliable) > > [c000000007223ba0] [c00000000021e3a8] klp_cleanup_module_patches_limited+0x448/0x480 > > [c000000007223cb0] [c000000000220278] klp_module_going+0x68/0x94 > > [c000000007223ce0] [c00000000022f480] __do_sys_delete_module.constprop.0+0x1d0/0x390 > > [c000000007223db0] [c00000000002f004] system_call_exception+0x164/0x340 > > [c000000007223e10] [c00000000000be68] system_call_vectored_common+0xe8/0x278 > > --- interrupt: 3000 at 0x7fffa178fb6c > > NIP: 00007fffa178fb6c LR: 0000000000000000 CTR: 0000000000000000 > > REGS: c000000007223e80 TRAP: 3000 Tainted: G K (5.19.0+) > > MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48002482 XER: 00000000 > > IRQMASK: 0 > > GPR00: 0000000000000081 00007ffff2d1b720 00007fffa1887200 0000010005bf1878 > > GPR04: 0000000000000800 000000000000000a 0000000000000000 00000000000000da > > GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > > GPR12: 0000000000000000 00007fffa201c540 0000000000000000 0000000000000000 > > GPR16: 0000010005bf1810 000000010c0f7370 000000010c0f8090 000000010c0f8078 > > GPR20: 000000010c0f8050 000000010c0f80a8 000000010c0f7518 000000010c0f80d0 > > GPR24: 00007ffff2d1b830 00007ffff2d1efbb 0000000000000000 0000010005bf02a0 > > GPR28: 00007ffff2d1be50 0000000000000000 0000010005bf1810 0000000000100000 > > NIP [00007fffa178fb6c] 0x7fffa178fb6c > > LR [0000000000000000] 0x0 > > --- interrupt: 3000 > > Instruction dump: > > 40820044 813b002c 7ff5f82a 79293664 7d394a14 e9290010 7c69f82e 7fe9fa14 > > 48052235 60000000 2c030000 41820008 <92ff0004> eadb0020 60000000 60000000 > > ---[ end trace 0000000000000000 ]--- > > > > $ addr2line 0xc00000000005659c -e vmlinux > > /root/klp-convert-tree/arch/powerpc/kernel/module_64.c:785 > > > > 743 void clear_relocate_add(Elf64_Shdr *sechdrs, > > 744 const char *strtab, > > 745 unsigned int symindex, > > 746 unsigned int relsec, > > 747 struct module *me) > > 748 { > > ... > > 759 for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) { > > ... > > 785 *instruction = PPC_RAW_NOP(); > > 786 } > > Has the module text been marked RW prior to this? I suspect not? > > In which case you need to use patch_instruction() here. > > cheers Thanks folks! I guess something like this would fix compile for ppc32 and fix crash for ppc64. I also pushed it to https://git.kernel.org/pub/scm/linux/kernel/git/song/linux.git/log/?h=klp-module-reload This includes Joe's klp-convert patches and this patch. Thanks! Song diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c index ea6536171778..e3c312770453 100644 --- a/arch/powerpc/kernel/module_32.c +++ b/arch/powerpc/kernel/module_32.c @@ -285,6 +285,16 @@ int apply_relocate_add(Elf32_Shdr *sechdrs, return 0; } +#ifdef CONFIG_LIVEPATCH +void clear_relocate_add(Elf32_Shdr *sechdrs, + const char *strtab, + unsigned int symindex, + unsigned int relsec, + struct module *me) +{ +} +#endif + #ifdef CONFIG_DYNAMIC_FTRACE notrace int module_trampoline_target(struct module *mod, unsigned long addr, unsigned long *target) diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c index 6aaf5720070d..4d55f0e52704 100644 --- a/arch/powerpc/kernel/module_64.c +++ b/arch/powerpc/kernel/module_64.c @@ -782,7 +782,7 @@ void clear_relocate_add(Elf64_Shdr *sechdrs, continue; instruction += 1; - *instruction = PPC_RAW_NOP(); + patch_instruction(instruction, PPC_RAW_NOP()); } } diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h index d22b36b84b4b..958e6da7f475 100644 --- a/include/linux/moduleloader.h +++ b/include/linux/moduleloader.h @@ -73,7 +73,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs, unsigned int relsec, struct module *mod); #ifdef CONFIG_LIVEPATCH -void clear_relocate_add(Elf64_Shdr *sechdrs, +void clear_relocate_add(Elf_Shdr *sechdrs, const char *strtab, unsigned int symindex, unsigned int relsec,
On Wed, Aug 31, 2022 at 03:48:26PM -0700, Song Liu wrote: > On Wed, Aug 31, 2022 at 3:30 PM Michael Ellerman <mpe@ellerman.id.au> wrote: > > > > Joe Lawrence <joe.lawrence@redhat.com> writes: > > > On Tue, Aug 30, 2022 at 11:53:13AM -0700, Song Liu wrote: > > >> From: Miroslav Benes <mbenes@suse.cz> > > >> > > >> Josh reported a bug: > > >> > > >> When the object to be patched is a module, and that module is > > >> rmmod'ed and reloaded, it fails to load with: > > >> > > >> module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c > > >> livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) > > >> livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' > > >> > > >> The livepatch module has a relocation which references a symbol > > >> in the _previous_ loading of nfsd. When apply_relocate_add() > > >> tries to replace the old relocation with a new one, it sees that > > >> the previous one is nonzero and it errors out. > > >> > > >> On ppc64le, we have a similar issue: > > >> > > >> module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd] > > >> livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) > > >> livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' > > ... > > >> > > > > > > Hi Song, > > > > > > Applying your patch on top of my latest klp-convert-tree branch [1], I > > > modified a few of its late module patching tests > > > (tools/testing/selftests/livepatch/test-song.sh) such that: > > > > > > 1 - A livepatch module is loaded > > > - this module contains klp-relocations to objects in (2) > > > 2 - A target test module is loaded > > > 3 - Unload the test target module > > > - Clear klp-relocations in (1) > > > 4 - Repeat target module load (2) / unload (3) a few times > > > 5 - Unload livepatch module > > > > If you push that test code somewhere I could test it on ppc64le. > > > > > The results: > > > > > > x86_64 : pass > > > s390x : pass > > > ppc64le : crash > > > > > > I suspect Power 32-bit would suffer the same fate, but I don't have > > > hardware to verify. See the kernel log from the crash below... > > > > > > > > > ===== TEST: klp-convert symbols (late module patching) ===== > > > % modprobe test_klp_convert1 > > > test_klp_convert1: tainting kernel with TAINT_LIVEPATCH > > > livepatch: enabling patch 'test_klp_convert1' > > > livepatch: 'test_klp_convert1': starting patching transition > > > livepatch: 'test_klp_convert1': patching complete > > > % modprobe test_klp_convert_mod > > > livepatch: applying patch 'test_klp_convert1' to loading module 'test_klp_convert_mod' > > > test_klp_convert1: saved_command_line, 0: BOOT_IMAGE=(ieee1275//vdevice/v-scsi@30000003/disk@8100000000000000,msdos2)/vmlinuz-5.19.0+ root=/dev/mapper/rhel_ibm--p9z--18--lp7-root ro crashkernel=2G-4G:384M,4G-16G:512M,16G-64G:1G,64G-128G:2G,128G-:4G rd.lvm.lv=rhel_ibm-p9z-18-lp7/root rd.lvm.lv=rhel_ibm-p9z-18-lp7/swap > > > test_klp_convert1: driver_name, 0: test_klp_convert_mod > > > test_klp_convert1: test_klp_get_driver_name(), 0: test_klp_convert_mod > > > test_klp_convert1: homonym_string, 1: homonym string A > > > test_klp_convert1: get_homonym_string(), 1: homonym string A > > > test_klp_convert1: klp_string.12345 = lib/livepatch/test_klp_convert_mod_a.c static string > > > test_klp_convert1: klp_string.67890 = lib/livepatch/test_klp_convert_mod_b.c static string > > > % rmmod test_klp_convert_mod > > > livepatch: reverting patch 'test_klp_convert1' on unloading module 'test_klp_convert_mod' > > > module_64: Clearing ADD relocate section 48 to 6 > > > BUG: Unable to handle kernel data access on write at 0xc008000002140150 > > > Faulting instruction address: 0xc00000000005659c > > > Oops: Kernel access of bad area, sig: 11 [#1] > > > LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries > > > Modules linked in: test_klp_convert_mod(-) test_klp_convert1(K) bonding tls rfkill pseries_rng drm fuse drm_panel_orientation_quirks xfs libcrc32c sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp vmx_crypto dm_mirror dm_region_hash dm_log dm_mod > > > CPU: 6 PID: 4766 Comm: rmmod Kdump: loaded Tainted: G K 5.19.0+ #1 > > > NIP: c00000000005659c LR: c000000000056590 CTR: 0000000000000024 > > > REGS: c000000007223840 TRAP: 0300 Tainted: G K (5.19.0+) > > > MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 48008282 XER: 0000000a > > > CFAR: c0000000000a87e0 DAR: c008000002140150 DSISR: 0a000000 IRQMASK: 0 > > > > This is saying you don't have permissions to write at that address. > > > > > GPR00: c000000000056568 c000000007223ae0 c000000002a68a00 0000000000000001 > > > GPR04: c0080000021706f0 000000000000002d 0000000000000000 0000000000000000 > > > GPR08: 0000000000000066 0000001200000010 0000000000000000 0000000000008000 > > > GPR12: 0000000000000000 c00000000ffca080 0000000000000000 0000000000000000 > > > GPR16: 0000010005bf1810 000000010c0f7370 c0000000011b7e50 c0000000011b7e68 > > > GPR20: c0080000021501c8 c008000002150228 0000000000000030 0000000060000000 > > > GPR24: c008000002160380 c000000056b43000 000000000000ff20 c000000056b43c00 > > > GPR28: aaaaaaaaaaaaaaab c000000056b43b40 0000000000000000 c00800000214014c > > > NIP [c00000000005659c] clear_relocate_add+0x11c/0x1c0 > > > LR [c000000000056590] clear_relocate_add+0x110/0x1c0 > > > Call Trace: > > > [c000000007223ae0] [ffffffffffffffff] 0xffffffffffffffff (unreliable) > > > [c000000007223ba0] [c00000000021e3a8] klp_cleanup_module_patches_limited+0x448/0x480 > > > [c000000007223cb0] [c000000000220278] klp_module_going+0x68/0x94 > > > [c000000007223ce0] [c00000000022f480] __do_sys_delete_module.constprop.0+0x1d0/0x390 > > > [c000000007223db0] [c00000000002f004] system_call_exception+0x164/0x340 > > > [c000000007223e10] [c00000000000be68] system_call_vectored_common+0xe8/0x278 > > > --- interrupt: 3000 at 0x7fffa178fb6c > > > NIP: 00007fffa178fb6c LR: 0000000000000000 CTR: 0000000000000000 > > > REGS: c000000007223e80 TRAP: 3000 Tainted: G K (5.19.0+) > > > MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48002482 XER: 00000000 > > > IRQMASK: 0 > > > GPR00: 0000000000000081 00007ffff2d1b720 00007fffa1887200 0000010005bf1878 > > > GPR04: 0000000000000800 000000000000000a 0000000000000000 00000000000000da > > > GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > > > GPR12: 0000000000000000 00007fffa201c540 0000000000000000 0000000000000000 > > > GPR16: 0000010005bf1810 000000010c0f7370 000000010c0f8090 000000010c0f8078 > > > GPR20: 000000010c0f8050 000000010c0f80a8 000000010c0f7518 000000010c0f80d0 > > > GPR24: 00007ffff2d1b830 00007ffff2d1efbb 0000000000000000 0000010005bf02a0 > > > GPR28: 00007ffff2d1be50 0000000000000000 0000010005bf1810 0000000000100000 > > > NIP [00007fffa178fb6c] 0x7fffa178fb6c > > > LR [0000000000000000] 0x0 > > > --- interrupt: 3000 > > > Instruction dump: > > > 40820044 813b002c 7ff5f82a 79293664 7d394a14 e9290010 7c69f82e 7fe9fa14 > > > 48052235 60000000 2c030000 41820008 <92ff0004> eadb0020 60000000 60000000 > > > ---[ end trace 0000000000000000 ]--- > > > > > > $ addr2line 0xc00000000005659c -e vmlinux > > > /root/klp-convert-tree/arch/powerpc/kernel/module_64.c:785 > > > > > > 743 void clear_relocate_add(Elf64_Shdr *sechdrs, > > > 744 const char *strtab, > > > 745 unsigned int symindex, > > > 746 unsigned int relsec, > > > 747 struct module *me) > > > 748 { > > > ... > > > 759 for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) { > > > ... > > > 785 *instruction = PPC_RAW_NOP(); > > > 786 } > > > > Has the module text been marked RW prior to this? I suspect not? > > > > In which case you need to use patch_instruction() here. > > > > cheers > > Thanks folks! > > I guess something like this would fix compile for ppc32 and fix crash for ppc64. > > I also pushed it to > > https://git.kernel.org/pub/scm/linux/kernel/git/song/linux.git/log/?h=klp-module-reload > > This includes Joe's klp-convert patches and this patch. > > Thanks! > Song > > > > diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c > index ea6536171778..e3c312770453 100644 > --- a/arch/powerpc/kernel/module_32.c > +++ b/arch/powerpc/kernel/module_32.c > @@ -285,6 +285,16 @@ int apply_relocate_add(Elf32_Shdr *sechdrs, > return 0; > } > > +#ifdef CONFIG_LIVEPATCH > +void clear_relocate_add(Elf32_Shdr *sechdrs, > + const char *strtab, > + unsigned int symindex, > + unsigned int relsec, > + struct module *me) > +{ > +} > +#endif > + > #ifdef CONFIG_DYNAMIC_FTRACE > notrace int module_trampoline_target(struct module *mod, unsigned long addr, > unsigned long *target) > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c > index 6aaf5720070d..4d55f0e52704 100644 > --- a/arch/powerpc/kernel/module_64.c > +++ b/arch/powerpc/kernel/module_64.c > @@ -782,7 +782,7 @@ void clear_relocate_add(Elf64_Shdr *sechdrs, > continue; > > instruction += 1; > - *instruction = PPC_RAW_NOP(); > + patch_instruction(instruction, PPC_RAW_NOP()); Close. I believe PPC_RAW_NOP() needs to be passed to ppc_inst() like: diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c index 4d55f0e52..514951f97 100644 --- a/arch/powerpc/kernel/module_64.c +++ b/arch/powerpc/kernel/module_64.c @@ -782,7 +782,7 @@ void clear_relocate_add(Elf64_Shdr *sechdrs, continue; instruction += 1; - patch_instruction(instruction, PPC_RAW_NOP()); + patch_instruction(instruction, ppc_inst(PPC_RAW_NOP())); } } And with that tweak, new result: ppc64le : pass Tested-by: Joe Lawrence <joe.lawrence@redhat.com> # x86_64, s390x, ppc64le Thanks guys, -- Joe > } > > } > diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h > index d22b36b84b4b..958e6da7f475 100644 > --- a/include/linux/moduleloader.h > +++ b/include/linux/moduleloader.h > @@ -73,7 +73,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs, > unsigned int relsec, > struct module *mod); > #ifdef CONFIG_LIVEPATCH > -void clear_relocate_add(Elf64_Shdr *sechdrs, > +void clear_relocate_add(Elf_Shdr *sechdrs, > const char *strtab, > unsigned int symindex, > unsigned int relsec, >
On Thu, Sep 01, 2022 at 08:30:44AM +1000, Michael Ellerman wrote: > Joe Lawrence <joe.lawrence@redhat.com> writes: > > On Tue, Aug 30, 2022 at 11:53:13AM -0700, Song Liu wrote: > >> From: Miroslav Benes <mbenes@suse.cz> > >> > >> Josh reported a bug: > >> > >> When the object to be patched is a module, and that module is > >> rmmod'ed and reloaded, it fails to load with: > >> > >> module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c > >> livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) > >> livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' > >> > >> The livepatch module has a relocation which references a symbol > >> in the _previous_ loading of nfsd. When apply_relocate_add() > >> tries to replace the old relocation with a new one, it sees that > >> the previous one is nonzero and it errors out. > >> > >> On ppc64le, we have a similar issue: > >> > >> module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd] > >> livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) > >> livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' > ... > >> > > > > Hi Song, > > > > Applying your patch on top of my latest klp-convert-tree branch [1], I > > modified a few of its late module patching tests > > (tools/testing/selftests/livepatch/test-song.sh) such that: > > > > 1 - A livepatch module is loaded > > - this module contains klp-relocations to objects in (2) > > 2 - A target test module is loaded > > 3 - Unload the test target module > > - Clear klp-relocations in (1) > > 4 - Repeat target module load (2) / unload (3) a few times > > 5 - Unload livepatch module > > If you push that test code somewhere I could test it on ppc64le. > > > The results: > > > > x86_64 : pass > > s390x : pass > > ppc64le : crash > > > > I suspect Power 32-bit would suffer the same fate, but I don't have > > hardware to verify. See the kernel log from the crash below... > > > > > > ===== TEST: klp-convert symbols (late module patching) ===== > > % modprobe test_klp_convert1 > > test_klp_convert1: tainting kernel with TAINT_LIVEPATCH > > livepatch: enabling patch 'test_klp_convert1' > > livepatch: 'test_klp_convert1': starting patching transition > > livepatch: 'test_klp_convert1': patching complete > > % modprobe test_klp_convert_mod > > livepatch: applying patch 'test_klp_convert1' to loading module 'test_klp_convert_mod' > > test_klp_convert1: saved_command_line, 0: BOOT_IMAGE=(ieee1275//vdevice/v-scsi@30000003/disk@8100000000000000,msdos2)/vmlinuz-5.19.0+ root=/dev/mapper/rhel_ibm--p9z--18--lp7-root ro crashkernel=2G-4G:384M,4G-16G:512M,16G-64G:1G,64G-128G:2G,128G-:4G rd.lvm.lv=rhel_ibm-p9z-18-lp7/root rd.lvm.lv=rhel_ibm-p9z-18-lp7/swap > > test_klp_convert1: driver_name, 0: test_klp_convert_mod > > test_klp_convert1: test_klp_get_driver_name(), 0: test_klp_convert_mod > > test_klp_convert1: homonym_string, 1: homonym string A > > test_klp_convert1: get_homonym_string(), 1: homonym string A > > test_klp_convert1: klp_string.12345 = lib/livepatch/test_klp_convert_mod_a.c static string > > test_klp_convert1: klp_string.67890 = lib/livepatch/test_klp_convert_mod_b.c static string > > % rmmod test_klp_convert_mod > > livepatch: reverting patch 'test_klp_convert1' on unloading module 'test_klp_convert_mod' > > module_64: Clearing ADD relocate section 48 to 6 > > BUG: Unable to handle kernel data access on write at 0xc008000002140150 > > Faulting instruction address: 0xc00000000005659c > > Oops: Kernel access of bad area, sig: 11 [#1] > > LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries > > Modules linked in: test_klp_convert_mod(-) test_klp_convert1(K) bonding tls rfkill pseries_rng drm fuse drm_panel_orientation_quirks xfs libcrc32c sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp vmx_crypto dm_mirror dm_region_hash dm_log dm_mod > > CPU: 6 PID: 4766 Comm: rmmod Kdump: loaded Tainted: G K 5.19.0+ #1 > > NIP: c00000000005659c LR: c000000000056590 CTR: 0000000000000024 > > REGS: c000000007223840 TRAP: 0300 Tainted: G K (5.19.0+) > > MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 48008282 XER: 0000000a > > CFAR: c0000000000a87e0 DAR: c008000002140150 DSISR: 0a000000 IRQMASK: 0 > > This is saying you don't have permissions to write at that address. > > > GPR00: c000000000056568 c000000007223ae0 c000000002a68a00 0000000000000001 > > GPR04: c0080000021706f0 000000000000002d 0000000000000000 0000000000000000 > > GPR08: 0000000000000066 0000001200000010 0000000000000000 0000000000008000 > > GPR12: 0000000000000000 c00000000ffca080 0000000000000000 0000000000000000 > > GPR16: 0000010005bf1810 000000010c0f7370 c0000000011b7e50 c0000000011b7e68 > > GPR20: c0080000021501c8 c008000002150228 0000000000000030 0000000060000000 > > GPR24: c008000002160380 c000000056b43000 000000000000ff20 c000000056b43c00 > > GPR28: aaaaaaaaaaaaaaab c000000056b43b40 0000000000000000 c00800000214014c > > NIP [c00000000005659c] clear_relocate_add+0x11c/0x1c0 > > LR [c000000000056590] clear_relocate_add+0x110/0x1c0 > > Call Trace: > > [c000000007223ae0] [ffffffffffffffff] 0xffffffffffffffff (unreliable) > > [c000000007223ba0] [c00000000021e3a8] klp_cleanup_module_patches_limited+0x448/0x480 > > [c000000007223cb0] [c000000000220278] klp_module_going+0x68/0x94 > > [c000000007223ce0] [c00000000022f480] __do_sys_delete_module.constprop.0+0x1d0/0x390 > > [c000000007223db0] [c00000000002f004] system_call_exception+0x164/0x340 > > [c000000007223e10] [c00000000000be68] system_call_vectored_common+0xe8/0x278 > > --- interrupt: 3000 at 0x7fffa178fb6c > > NIP: 00007fffa178fb6c LR: 0000000000000000 CTR: 0000000000000000 > > REGS: c000000007223e80 TRAP: 3000 Tainted: G K (5.19.0+) > > MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48002482 XER: 00000000 > > IRQMASK: 0 > > GPR00: 0000000000000081 00007ffff2d1b720 00007fffa1887200 0000010005bf1878 > > GPR04: 0000000000000800 000000000000000a 0000000000000000 00000000000000da > > GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > > GPR12: 0000000000000000 00007fffa201c540 0000000000000000 0000000000000000 > > GPR16: 0000010005bf1810 000000010c0f7370 000000010c0f8090 000000010c0f8078 > > GPR20: 000000010c0f8050 000000010c0f80a8 000000010c0f7518 000000010c0f80d0 > > GPR24: 00007ffff2d1b830 00007ffff2d1efbb 0000000000000000 0000010005bf02a0 > > GPR28: 00007ffff2d1be50 0000000000000000 0000010005bf1810 0000000000100000 > > NIP [00007fffa178fb6c] 0x7fffa178fb6c > > LR [0000000000000000] 0x0 > > --- interrupt: 3000 > > Instruction dump: > > 40820044 813b002c 7ff5f82a 79293664 7d394a14 e9290010 7c69f82e 7fe9fa14 > > 48052235 60000000 2c030000 41820008 <92ff0004> eadb0020 60000000 60000000 > > ---[ end trace 0000000000000000 ]--- > > > > $ addr2line 0xc00000000005659c -e vmlinux > > /root/klp-convert-tree/arch/powerpc/kernel/module_64.c:785 > > > > 743 void clear_relocate_add(Elf64_Shdr *sechdrs, > > 744 const char *strtab, > > 745 unsigned int symindex, > > 746 unsigned int relsec, > > 747 struct module *me) > > 748 { > > ... > > 759 for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) { > > ... > > 785 *instruction = PPC_RAW_NOP(); > > 786 } > > Has the module text been marked RW prior to this? I suspect not? > > In which case you need to use patch_instruction() here. > Hi Michael, While we're on the topic of klp-relocations and Power, I saw a similar access problem when writing (late) relocations into .data..ro_after_init. I'm not entirely convinced this should be allowed (ie, is it really read-only after .init or ???), but it seems that other arches currently allow it ... $ ./tools/testing/selftests/livepatch/test-shadow-vars.sh ===== TEST: klp-convert data relocations (late module patching) ===== % modprobe test_klp_convert_data livepatch: enabling patch 'test_klp_convert_data' livepatch: 'test_klp_convert_data': starting patching transition livepatch: 'test_klp_convert_data': patching complete % modprobe test_klp_convert_mod ... module_64: Applying ADD relocate section 54 to 20 module_64: RELOC at 000000008482d02a: 38-type as .klp.sym.test_klp_convert_mod.static_ro_after_init,0 (0xc0080000016d0084) + 0 BUG: Unable to handle kernel data access on write at 0xc0080000021d0000 Faulting instruction address: 0xc000000000055f14 Oops: Kernel access of bad area, sig: 11 [#1] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries Modules linked in: test_klp_convert_mod(+) test_klp_convert_data(K) bonding rfkill tls pseries_rng drm fuse drm_panel_orientation_quirks xfs libcrc32c sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp vmx_crypto dm_mirror dm_region_hash dm_log dm_mod [last unloaded: test_klp_convert_mod] CPU: 0 PID: 17089 Comm: modprobe Kdump: loaded Tainted: G K 5.19.0+ #1 NIP: c000000000055f14 LR: c00000000021ef28 CTR: c000000000055f14 REGS: c0000000387af5a0 TRAP: 0300 Tainted: G K (5.19.0+) MSR: 8000000002009033 <SF,VEC,EE,ME,IR,DR,RI,LE> CR: 88228444 XER: 00000000 CFAR: c000000000055e04 DAR: c0080000021d0000 DSISR: 42000000 IRQMASK: 0 GPR00: c00000000021ef28 c0000000387af840 c000000002a68a00 c0000000088b3000 GPR04: c008000002230084 0000000000000026 0000000000000036 c0080000021e0480 GPR08: 000000007c426214 c000000000055f14 c000000000055e08 0000000000000d80 GPR12: c00000000021d9b0 c000000002d90000 c0000000088b3000 c0080000021f0810 GPR16: c0080000021c0638 c0000000088b3d80 00000000ffffffff c000000001181e38 GPR20: c0000000029dc088 c0080000021e0480 c0080000021f0870 aaaaaaaaaaaaaaab GPR24: c0000000088b3c40 c0080000021d0000 c0080000021f0000 0000000000000000 GPR28: c0080000021d0000 0000000000000000 c0080000021c0638 0000000000000810 NIP [c000000000055f14] apply_relocate_add+0x474/0x9e0 LR [c00000000021ef28] klp_apply_section_relocs+0x208/0x2d0 Call Trace: [c0000000387af840] [c0000000387af920] 0xc0000000387af920 (unreliable) [c0000000387af940] [c00000000021ef28] klp_apply_section_relocs+0x208/0x2d0 [c0000000387afa30] [c00000000021f080] klp_init_object_loaded+0x90/0x1e0 [c0000000387afac0] [c0000000002200ac] klp_module_coming+0x3dc/0x5c0 [c0000000387afb70] [c000000000231414] load_module+0xf64/0x13a0 [c0000000387afc90] [c000000000231b8c] __do_sys_finit_module+0xdc/0x180 [c0000000387afdb0] [c00000000002f004] system_call_exception+0x164/0x340 [c0000000387afe10] [c00000000000be68] system_call_vectored_common+0xe8/0x278 --- interrupt: 3000 at 0x7fffb6af4710 NIP: 00007fffb6af4710 LR: 0000000000000000 CTR: 0000000000000000 REGS: c0000000387afe80 TRAP: 3000 Tainted: G K (5.19.0+) MSR: 800000000000f033 <SF,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48224244 XER: 00000000 IRQMASK: 0 GPR00: 0000000000000161 00007fffe06f5550 00007fffb6bf7200 0000000000000005 GPR04: 0000000105f36ca0 0000000000000000 0000000000000005 0000000000000000 GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 GPR12: 0000000000000000 00007fffb738c540 0000000000000020 0000000000000000 GPR16: 0000010024d31de0 0000000000000000 0000000105f37d50 0000010024d302f8 GPR20: 0000000000000001 0000000000000908 0000010024d32020 0000010024d319b0 GPR24: 0000000000000000 0000000000000000 0000010024d32040 0000010024d303f0 GPR28: 0000010024d31e00 0000000105f36ca0 0000000000040000 0000010024d319b0 NIP [00007fffb6af4710] 0x7fffb6af4710 LR [0000000000000000] 0x0 --- interrupt: 3000 Instruction dump: 0000061c 0000061c 0000061c 0000061c 0000061c 0000061c 0000061c 0000061c 00000288 00000248 60000000 7c992050 <7c9ce92a> 60000000 60000000 e9310020 ---[ end trace 0000000000000000 ]--- $ readelf --wide --sections lib/livepatch/test_klp_convert_data.ko | grep -e '\[20\]' -e '\[54\]' [20] .data..ro_after_init PROGBITS 0000000000000000 001a58 000008 00 WA 0 0 8 [54] .klp.rela.test_klp_convert_mod..data..ro_after_init RELA 0000000000000000 0426e8 000018 18 Ao 49 20 8 I can push a branch up to github if you'd like to try it yourself. Regards, -- Joe
Joe Lawrence <joe.lawrence@redhat.com> writes: > On Thu, Sep 01, 2022 at 08:30:44AM +1000, Michael Ellerman wrote: >> Joe Lawrence <joe.lawrence@redhat.com> writes: ... > > Hi Michael, > > While we're on the topic of klp-relocations and Power, I saw a similar > access problem when writing (late) relocations into > .data..ro_after_init. I'm not entirely convinced this should be allowed > (ie, is it really read-only after .init or ???), but it seems that other > arches currently allow it ... I guess that's because we didn't properly fix apply_relocate_add() in https://github.com/linuxppc/issues/issues/375 ? If other arches allow it then we don't want to be the odd one out :) So I guess we need to implement it. > $ ./tools/testing/selftests/livepatch/test-shadow-vars.sh > > ===== TEST: klp-convert data relocations (late module patching) ===== > % modprobe test_klp_convert_data > livepatch: enabling patch 'test_klp_convert_data' > livepatch: 'test_klp_convert_data': starting patching transition > livepatch: 'test_klp_convert_data': patching complete > % modprobe test_klp_convert_mod > ... > module_64: Applying ADD relocate section 54 to 20 > module_64: RELOC at 000000008482d02a: 38-type as .klp.sym.test_klp_convert_mod.static_ro_after_init,0 (0xc0080000016d0084) + 0 > BUG: Unable to handle kernel data access on write at 0xc0080000021d0000 > Faulting instruction address: 0xc000000000055f14 > Oops: Kernel access of bad area, sig: 11 [#1] > LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries > Modules linked in: test_klp_convert_mod(+) test_klp_convert_data(K) bonding rfkill tls pseries_rng drm fuse drm_panel_orientation_quirks xfs libcrc32c sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp vmx_crypto dm_mirror dm_region_hash dm_log dm_mod [last unloaded: test_klp_convert_mod] > CPU: 0 PID: 17089 Comm: modprobe Kdump: loaded Tainted: G K 5.19.0+ #1 > NIP: c000000000055f14 LR: c00000000021ef28 CTR: c000000000055f14 > REGS: c0000000387af5a0 TRAP: 0300 Tainted: G K (5.19.0+) > MSR: 8000000002009033 <SF,VEC,EE,ME,IR,DR,RI,LE> CR: 88228444 XER: 00000000 > CFAR: c000000000055e04 DAR: c0080000021d0000 DSISR: 42000000 IRQMASK: 0 > GPR00: c00000000021ef28 c0000000387af840 c000000002a68a00 c0000000088b3000 > GPR04: c008000002230084 0000000000000026 0000000000000036 c0080000021e0480 > GPR08: 000000007c426214 c000000000055f14 c000000000055e08 0000000000000d80 > GPR12: c00000000021d9b0 c000000002d90000 c0000000088b3000 c0080000021f0810 > GPR16: c0080000021c0638 c0000000088b3d80 00000000ffffffff c000000001181e38 > GPR20: c0000000029dc088 c0080000021e0480 c0080000021f0870 aaaaaaaaaaaaaaab > GPR24: c0000000088b3c40 c0080000021d0000 c0080000021f0000 0000000000000000 > GPR28: c0080000021d0000 0000000000000000 c0080000021c0638 0000000000000810 > NIP [c000000000055f14] apply_relocate_add+0x474/0x9e0 > LR [c00000000021ef28] klp_apply_section_relocs+0x208/0x2d0 > Call Trace: > [c0000000387af840] [c0000000387af920] 0xc0000000387af920 (unreliable) > [c0000000387af940] [c00000000021ef28] klp_apply_section_relocs+0x208/0x2d0 > [c0000000387afa30] [c00000000021f080] klp_init_object_loaded+0x90/0x1e0 > [c0000000387afac0] [c0000000002200ac] klp_module_coming+0x3dc/0x5c0 > [c0000000387afb70] [c000000000231414] load_module+0xf64/0x13a0 > [c0000000387afc90] [c000000000231b8c] __do_sys_finit_module+0xdc/0x180 > [c0000000387afdb0] [c00000000002f004] system_call_exception+0x164/0x340 > [c0000000387afe10] [c00000000000be68] system_call_vectored_common+0xe8/0x278 > --- interrupt: 3000 at 0x7fffb6af4710 > NIP: 00007fffb6af4710 LR: 0000000000000000 CTR: 0000000000000000 > REGS: c0000000387afe80 TRAP: 3000 Tainted: G K (5.19.0+) > MSR: 800000000000f033 <SF,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48224244 XER: 00000000 > IRQMASK: 0 > GPR00: 0000000000000161 00007fffe06f5550 00007fffb6bf7200 0000000000000005 > GPR04: 0000000105f36ca0 0000000000000000 0000000000000005 0000000000000000 > GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > GPR12: 0000000000000000 00007fffb738c540 0000000000000020 0000000000000000 > GPR16: 0000010024d31de0 0000000000000000 0000000105f37d50 0000010024d302f8 > GPR20: 0000000000000001 0000000000000908 0000010024d32020 0000010024d319b0 > GPR24: 0000000000000000 0000000000000000 0000010024d32040 0000010024d303f0 > GPR28: 0000010024d31e00 0000000105f36ca0 0000000000040000 0000010024d319b0 > NIP [00007fffb6af4710] 0x7fffb6af4710 > LR [0000000000000000] 0x0 > --- interrupt: 3000 > Instruction dump: > 0000061c 0000061c 0000061c 0000061c 0000061c 0000061c 0000061c 0000061c > 00000288 00000248 60000000 7c992050 <7c9ce92a> 60000000 60000000 e9310020 > ---[ end trace 0000000000000000 ]--- > > $ readelf --wide --sections lib/livepatch/test_klp_convert_data.ko | grep -e '\[20\]' -e '\[54\]' > [20] .data..ro_after_init PROGBITS 0000000000000000 001a58 000008 00 WA 0 0 8 > [54] .klp.rela.test_klp_convert_mod..data..ro_after_init RELA 0000000000000000 0426e8 000018 18 Ao 49 20 8 > > I can push a branch up to github if you'd like to try it yourself. That would help thanks. cheers
On Thu, Sep 01, 2022 at 01:39:02PM +1000, Michael Ellerman wrote: > Joe Lawrence <joe.lawrence@redhat.com> writes: > > On Thu, Sep 01, 2022 at 08:30:44AM +1000, Michael Ellerman wrote: > >> Joe Lawrence <joe.lawrence@redhat.com> writes: > ... > > > > Hi Michael, > > > > While we're on the topic of klp-relocations and Power, I saw a similar > > access problem when writing (late) relocations into > > .data..ro_after_init. I'm not entirely convinced this should be allowed > > (ie, is it really read-only after .init or ???), but it seems that other > > arches currently allow it ... > > I guess that's because we didn't properly fix apply_relocate_add() in > https://github.com/linuxppc/issues/issues/375 ? > > If other arches allow it then we don't want to be the odd one out :) > > So I guess we need to implement it. > FWIW, I think it this particular relocation is pretty rare, we haven't seen it in real patches nor do we have a kpatch test that generates it. I only hit a crash as I was trying to write a more exhaustive test for the klp-convert implementation. > > ===== TEST: klp-convert data relocations (late module patching) ===== > > % modprobe test_klp_convert_data > > livepatch: enabling patch 'test_klp_convert_data' > > livepatch: 'test_klp_convert_data': starting patching transition > > livepatch: 'test_klp_convert_data': patching complete > > % modprobe test_klp_convert_mod > > ... > > module_64: Applying ADD relocate section 54 to 20 > > module_64: RELOC at 000000008482d02a: 38-type as .klp.sym.test_klp_convert_mod.static_ro_after_init,0 (0xc0080000016d0084) + 0 > > BUG: Unable to handle kernel data access on write at 0xc0080000021d0000 > > Faulting instruction address: 0xc000000000055f14 > > Oops: Kernel access of bad area, sig: 11 [#1] > > LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries > > Modules linked in: test_klp_convert_mod(+) test_klp_convert_data(K) bonding rfkill tls pseries_rng drm fuse drm_panel_orientation_quirks xfs libcrc32c sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp vmx_crypto dm_mirror dm_region_hash dm_log dm_mod [last unloaded: test_klp_convert_mod] > > CPU: 0 PID: 17089 Comm: modprobe Kdump: loaded Tainted: G K 5.19.0+ #1 > > NIP: c000000000055f14 LR: c00000000021ef28 CTR: c000000000055f14 > > REGS: c0000000387af5a0 TRAP: 0300 Tainted: G K (5.19.0+) > > MSR: 8000000002009033 <SF,VEC,EE,ME,IR,DR,RI,LE> CR: 88228444 XER: 00000000 > > CFAR: c000000000055e04 DAR: c0080000021d0000 DSISR: 42000000 IRQMASK: 0 > > GPR00: c00000000021ef28 c0000000387af840 c000000002a68a00 c0000000088b3000 > > GPR04: c008000002230084 0000000000000026 0000000000000036 c0080000021e0480 > > GPR08: 000000007c426214 c000000000055f14 c000000000055e08 0000000000000d80 > > GPR12: c00000000021d9b0 c000000002d90000 c0000000088b3000 c0080000021f0810 > > GPR16: c0080000021c0638 c0000000088b3d80 00000000ffffffff c000000001181e38 > > GPR20: c0000000029dc088 c0080000021e0480 c0080000021f0870 aaaaaaaaaaaaaaab > > GPR24: c0000000088b3c40 c0080000021d0000 c0080000021f0000 0000000000000000 > > GPR28: c0080000021d0000 0000000000000000 c0080000021c0638 0000000000000810 > > NIP [c000000000055f14] apply_relocate_add+0x474/0x9e0 > > LR [c00000000021ef28] klp_apply_section_relocs+0x208/0x2d0 > > Call Trace: > > [c0000000387af840] [c0000000387af920] 0xc0000000387af920 (unreliable) > > [c0000000387af940] [c00000000021ef28] klp_apply_section_relocs+0x208/0x2d0 > > [c0000000387afa30] [c00000000021f080] klp_init_object_loaded+0x90/0x1e0 > > [c0000000387afac0] [c0000000002200ac] klp_module_coming+0x3dc/0x5c0 > > [c0000000387afb70] [c000000000231414] load_module+0xf64/0x13a0 > > [c0000000387afc90] [c000000000231b8c] __do_sys_finit_module+0xdc/0x180 > > [c0000000387afdb0] [c00000000002f004] system_call_exception+0x164/0x340 > > [c0000000387afe10] [c00000000000be68] system_call_vectored_common+0xe8/0x278 > > --- interrupt: 3000 at 0x7fffb6af4710 > > NIP: 00007fffb6af4710 LR: 0000000000000000 CTR: 0000000000000000 > > REGS: c0000000387afe80 TRAP: 3000 Tainted: G K (5.19.0+) > > MSR: 800000000000f033 <SF,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48224244 XER: 00000000 > > IRQMASK: 0 > > GPR00: 0000000000000161 00007fffe06f5550 00007fffb6bf7200 0000000000000005 > > GPR04: 0000000105f36ca0 0000000000000000 0000000000000005 0000000000000000 > > GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > > GPR12: 0000000000000000 00007fffb738c540 0000000000000020 0000000000000000 > > GPR16: 0000010024d31de0 0000000000000000 0000000105f37d50 0000010024d302f8 > > GPR20: 0000000000000001 0000000000000908 0000010024d32020 0000010024d319b0 > > GPR24: 0000000000000000 0000000000000000 0000010024d32040 0000010024d303f0 > > GPR28: 0000010024d31e00 0000000105f36ca0 0000000000040000 0000010024d319b0 > > NIP [00007fffb6af4710] 0x7fffb6af4710 > > LR [0000000000000000] 0x0 > > --- interrupt: 3000 > > Instruction dump: > > 0000061c 0000061c 0000061c 0000061c 0000061c 0000061c 0000061c 0000061c > > 00000288 00000248 60000000 7c992050 <7c9ce92a> 60000000 60000000 e9310020 > > ---[ end trace 0000000000000000 ]--- > > > > $ readelf --wide --sections lib/livepatch/test_klp_convert_data.ko | grep -e '\[20\]' -e '\[54\]' > > [20] .data..ro_after_init PROGBITS 0000000000000000 001a58 000008 00 WA 0 0 8 > > [54] .klp.rela.test_klp_convert_mod..data..ro_after_init RELA 0000000000000000 0426e8 000018 18 Ao 49 20 8 > > > > I can push a branch up to github if you'd like to try it yourself. > > That would help thanks. > This branch should do it: https://github.com/joe-lawrence/klp-convert-tree/tree/klp-convert-v7-devel Boot that and then run tools/testing/selftests/livepatch/test-livepatch.sh -- Joe
On Wed, Aug 31, 2022 at 7:05 PM Joe Lawrence <joe.lawrence@redhat.com> wrote: > > On Wed, Aug 31, 2022 at 03:48:26PM -0700, Song Liu wrote: > > On Wed, Aug 31, 2022 at 3:30 PM Michael Ellerman <mpe@ellerman.id.au> wrote: > > > > > > Joe Lawrence <joe.lawrence@redhat.com> writes: > > > > On Tue, Aug 30, 2022 at 11:53:13AM -0700, Song Liu wrote: > > > >> From: Miroslav Benes <mbenes@suse.cz> > > > >> > > > >> Josh reported a bug: > > > >> > > > >> When the object to be patched is a module, and that module is > > > >> rmmod'ed and reloaded, it fails to load with: > > > >> > > > >> module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c > > > >> livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) > > > >> livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' > > > >> > > > >> The livepatch module has a relocation which references a symbol > > > >> in the _previous_ loading of nfsd. When apply_relocate_add() > > > >> tries to replace the old relocation with a new one, it sees that > > > >> the previous one is nonzero and it errors out. > > > >> > > > >> On ppc64le, we have a similar issue: > > > >> > > > >> module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd] > > > >> livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) > > > >> livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' > > > ... > > > >> > > > > > > > > Hi Song, > > > > > > > > Applying your patch on top of my latest klp-convert-tree branch [1], I > > > > modified a few of its late module patching tests > > > > (tools/testing/selftests/livepatch/test-song.sh) such that: > > > > > > > > 1 - A livepatch module is loaded > > > > - this module contains klp-relocations to objects in (2) > > > > 2 - A target test module is loaded > > > > 3 - Unload the test target module > > > > - Clear klp-relocations in (1) > > > > 4 - Repeat target module load (2) / unload (3) a few times > > > > 5 - Unload livepatch module > > > > > > If you push that test code somewhere I could test it on ppc64le. > > > > > > > The results: > > > > > > > > x86_64 : pass > > > > s390x : pass > > > > ppc64le : crash > > > > > > > > I suspect Power 32-bit would suffer the same fate, but I don't have > > > > hardware to verify. See the kernel log from the crash below... > > > > > > > > > > > > ===== TEST: klp-convert symbols (late module patching) ===== > > > > % modprobe test_klp_convert1 > > > > test_klp_convert1: tainting kernel with TAINT_LIVEPATCH > > > > livepatch: enabling patch 'test_klp_convert1' > > > > livepatch: 'test_klp_convert1': starting patching transition > > > > livepatch: 'test_klp_convert1': patching complete > > > > % modprobe test_klp_convert_mod > > > > livepatch: applying patch 'test_klp_convert1' to loading module 'test_klp_convert_mod' > > > > test_klp_convert1: saved_command_line, 0: BOOT_IMAGE=(ieee1275//vdevice/v-scsi@30000003/disk@8100000000000000,msdos2)/vmlinuz-5.19.0+ root=/dev/mapper/rhel_ibm--p9z--18--lp7-root ro crashkernel=2G-4G:384M,4G-16G:512M,16G-64G:1G,64G-128G:2G,128G-:4G rd.lvm.lv=rhel_ibm-p9z-18-lp7/root rd.lvm.lv=rhel_ibm-p9z-18-lp7/swap > > > > test_klp_convert1: driver_name, 0: test_klp_convert_mod > > > > test_klp_convert1: test_klp_get_driver_name(), 0: test_klp_convert_mod > > > > test_klp_convert1: homonym_string, 1: homonym string A > > > > test_klp_convert1: get_homonym_string(), 1: homonym string A > > > > test_klp_convert1: klp_string.12345 = lib/livepatch/test_klp_convert_mod_a.c static string > > > > test_klp_convert1: klp_string.67890 = lib/livepatch/test_klp_convert_mod_b.c static string > > > > % rmmod test_klp_convert_mod > > > > livepatch: reverting patch 'test_klp_convert1' on unloading module 'test_klp_convert_mod' > > > > module_64: Clearing ADD relocate section 48 to 6 > > > > BUG: Unable to handle kernel data access on write at 0xc008000002140150 > > > > Faulting instruction address: 0xc00000000005659c > > > > Oops: Kernel access of bad area, sig: 11 [#1] > > > > LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries > > > > Modules linked in: test_klp_convert_mod(-) test_klp_convert1(K) bonding tls rfkill pseries_rng drm fuse drm_panel_orientation_quirks xfs libcrc32c sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp vmx_crypto dm_mirror dm_region_hash dm_log dm_mod > > > > CPU: 6 PID: 4766 Comm: rmmod Kdump: loaded Tainted: G K 5.19.0+ #1 > > > > NIP: c00000000005659c LR: c000000000056590 CTR: 0000000000000024 > > > > REGS: c000000007223840 TRAP: 0300 Tainted: G K (5.19.0+) > > > > MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 48008282 XER: 0000000a > > > > CFAR: c0000000000a87e0 DAR: c008000002140150 DSISR: 0a000000 IRQMASK: 0 > > > > > > This is saying you don't have permissions to write at that address. > > > > > > > GPR00: c000000000056568 c000000007223ae0 c000000002a68a00 0000000000000001 > > > > GPR04: c0080000021706f0 000000000000002d 0000000000000000 0000000000000000 > > > > GPR08: 0000000000000066 0000001200000010 0000000000000000 0000000000008000 > > > > GPR12: 0000000000000000 c00000000ffca080 0000000000000000 0000000000000000 > > > > GPR16: 0000010005bf1810 000000010c0f7370 c0000000011b7e50 c0000000011b7e68 > > > > GPR20: c0080000021501c8 c008000002150228 0000000000000030 0000000060000000 > > > > GPR24: c008000002160380 c000000056b43000 000000000000ff20 c000000056b43c00 > > > > GPR28: aaaaaaaaaaaaaaab c000000056b43b40 0000000000000000 c00800000214014c > > > > NIP [c00000000005659c] clear_relocate_add+0x11c/0x1c0 > > > > LR [c000000000056590] clear_relocate_add+0x110/0x1c0 > > > > Call Trace: > > > > [c000000007223ae0] [ffffffffffffffff] 0xffffffffffffffff (unreliable) > > > > [c000000007223ba0] [c00000000021e3a8] klp_cleanup_module_patches_limited+0x448/0x480 > > > > [c000000007223cb0] [c000000000220278] klp_module_going+0x68/0x94 > > > > [c000000007223ce0] [c00000000022f480] __do_sys_delete_module.constprop.0+0x1d0/0x390 > > > > [c000000007223db0] [c00000000002f004] system_call_exception+0x164/0x340 > > > > [c000000007223e10] [c00000000000be68] system_call_vectored_common+0xe8/0x278 > > > > --- interrupt: 3000 at 0x7fffa178fb6c > > > > NIP: 00007fffa178fb6c LR: 0000000000000000 CTR: 0000000000000000 > > > > REGS: c000000007223e80 TRAP: 3000 Tainted: G K (5.19.0+) > > > > MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48002482 XER: 00000000 > > > > IRQMASK: 0 > > > > GPR00: 0000000000000081 00007ffff2d1b720 00007fffa1887200 0000010005bf1878 > > > > GPR04: 0000000000000800 000000000000000a 0000000000000000 00000000000000da > > > > GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > > > > GPR12: 0000000000000000 00007fffa201c540 0000000000000000 0000000000000000 > > > > GPR16: 0000010005bf1810 000000010c0f7370 000000010c0f8090 000000010c0f8078 > > > > GPR20: 000000010c0f8050 000000010c0f80a8 000000010c0f7518 000000010c0f80d0 > > > > GPR24: 00007ffff2d1b830 00007ffff2d1efbb 0000000000000000 0000010005bf02a0 > > > > GPR28: 00007ffff2d1be50 0000000000000000 0000010005bf1810 0000000000100000 > > > > NIP [00007fffa178fb6c] 0x7fffa178fb6c > > > > LR [0000000000000000] 0x0 > > > > --- interrupt: 3000 > > > > Instruction dump: > > > > 40820044 813b002c 7ff5f82a 79293664 7d394a14 e9290010 7c69f82e 7fe9fa14 > > > > 48052235 60000000 2c030000 41820008 <92ff0004> eadb0020 60000000 60000000 > > > > ---[ end trace 0000000000000000 ]--- > > > > > > > > $ addr2line 0xc00000000005659c -e vmlinux > > > > /root/klp-convert-tree/arch/powerpc/kernel/module_64.c:785 > > > > > > > > 743 void clear_relocate_add(Elf64_Shdr *sechdrs, > > > > 744 const char *strtab, > > > > 745 unsigned int symindex, > > > > 746 unsigned int relsec, > > > > 747 struct module *me) > > > > 748 { > > > > ... > > > > 759 for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) { > > > > ... > > > > 785 *instruction = PPC_RAW_NOP(); > > > > 786 } > > > > > > Has the module text been marked RW prior to this? I suspect not? > > > > > > In which case you need to use patch_instruction() here. > > > > > > cheers > > > > Thanks folks! > > > > I guess something like this would fix compile for ppc32 and fix crash for ppc64. > > > > I also pushed it to > > > > https://git.kernel.org/pub/scm/linux/kernel/git/song/linux.git/log/?h=klp-module-reload > > > > This includes Joe's klp-convert patches and this patch. > > > > Thanks! > > Song > > > > > > > > diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c > > index ea6536171778..e3c312770453 100644 > > --- a/arch/powerpc/kernel/module_32.c > > +++ b/arch/powerpc/kernel/module_32.c > > @@ -285,6 +285,16 @@ int apply_relocate_add(Elf32_Shdr *sechdrs, > > return 0; > > } > > > > +#ifdef CONFIG_LIVEPATCH > > +void clear_relocate_add(Elf32_Shdr *sechdrs, > > + const char *strtab, > > + unsigned int symindex, > > + unsigned int relsec, > > + struct module *me) > > +{ > > +} > > +#endif > > + > > #ifdef CONFIG_DYNAMIC_FTRACE > > notrace int module_trampoline_target(struct module *mod, unsigned long addr, > > unsigned long *target) > > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c > > index 6aaf5720070d..4d55f0e52704 100644 > > --- a/arch/powerpc/kernel/module_64.c > > +++ b/arch/powerpc/kernel/module_64.c > > @@ -782,7 +782,7 @@ void clear_relocate_add(Elf64_Shdr *sechdrs, > > continue; > > > > instruction += 1; > > - *instruction = PPC_RAW_NOP(); > > + patch_instruction(instruction, PPC_RAW_NOP()); > > Close. I believe PPC_RAW_NOP() needs to be passed to ppc_inst() like: > > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c > index 4d55f0e52..514951f97 100644 > --- a/arch/powerpc/kernel/module_64.c > +++ b/arch/powerpc/kernel/module_64.c > @@ -782,7 +782,7 @@ void clear_relocate_add(Elf64_Shdr *sechdrs, > continue; > > instruction += 1; > - patch_instruction(instruction, PPC_RAW_NOP()); > + patch_instruction(instruction, ppc_inst(PPC_RAW_NOP())); > } > > } > > And with that tweak, new result: > > ppc64le : pass > > Tested-by: Joe Lawrence <joe.lawrence@redhat.com> # x86_64, s390x, ppc64le Thanks! I will fold this in and send v6. Song
On Thu, 2022-09-01 at 08:42 -0400, Joe Lawrence wrote: > On Thu, Sep 01, 2022 at 01:39:02PM +1000, Michael Ellerman wrote: > > Joe Lawrence <joe.lawrence@redhat.com> writes: > > > On Thu, Sep 01, 2022 at 08:30:44AM +1000, Michael Ellerman wrote: > > > > Joe Lawrence <joe.lawrence@redhat.com> writes: > > ... > > > > > > Hi Michael, > > > > > > While we're on the topic of klp-relocations and Power, I saw a > > > similar > > > access problem when writing (late) relocations into > > > .data..ro_after_init. I'm not entirely convinced this should be > > > allowed > > > (ie, is it really read-only after .init or ???), but it seems > > > that other > > > arches currently allow it ... > > > > I guess that's because we didn't properly fix apply_relocate_add() > > in > > https://github.com/linuxppc/issues/issues/375 ? > > > > If other arches allow it then we don't want to be the odd one out > > :) > > > > So I guess we need to implement it. > > > > FWIW, I think it this particular relocation is pretty rare, we > haven't > seen it in real patches nor do we have a kpatch test that generates > it. > I only hit a crash as I was trying to write a more exhaustive test > for > the klp-convert implementation. I'll revive my proper fix. I stopped working on it since my previous version was hitting endian bugs with some relocations & it didn't seem necessary at the time. Shouldn't take too much to get it going again. > > > > ===== TEST: klp-convert data relocations (late module patching) > > > ===== > > > % modprobe test_klp_convert_data > > > livepatch: enabling patch 'test_klp_convert_data' > > > livepatch: 'test_klp_convert_data': starting patching transition > > > livepatch: 'test_klp_convert_data': patching complete > > > % modprobe test_klp_convert_mod > > > ... > > > module_64: Applying ADD relocate section 54 to 20 > > > module_64: RELOC at 000000008482d02a: 38-type as > > > .klp.sym.test_klp_convert_mod.static_ro_after_init,0 > > > (0xc0080000016d0084) + 0 > > > BUG: Unable to handle kernel data access on write at > > > 0xc0080000021d0000 > > > Faulting instruction address: 0xc000000000055f14 > > > Oops: Kernel access of bad area, sig: 11 [#1] > > > LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries > > > Modules linked in: test_klp_convert_mod(+) > > > test_klp_convert_data(K) bonding rfkill tls pseries_rng drm fuse > > > drm_panel_orientation_quirks xfs libcrc32c sd_mod t10_pi sg > > > ibmvscsi ibmveth scsi_transport_srp vmx_crypto dm_mirror > > > dm_region_hash dm_log dm_mod [last unloaded: > > > test_klp_convert_mod] > > > CPU: 0 PID: 17089 Comm: modprobe Kdump: loaded Tainted: > > > G K 5.19.0+ #1 > > > NIP: c000000000055f14 LR: c00000000021ef28 CTR: c000000000055f14 > > > REGS: c0000000387af5a0 TRAP: 0300 Tainted: G K > > > (5.19.0+) > > > MSR: 8000000002009033 <SF,VEC,EE,ME,IR,DR,RI,LE> CR: 88228444 > > > XER: 00000000 > > > CFAR: c000000000055e04 DAR: c0080000021d0000 DSISR: 42000000 > > > IRQMASK: 0 > > > GPR00: c00000000021ef28 c0000000387af840 c000000002a68a00 > > > c0000000088b3000 > > > GPR04: c008000002230084 0000000000000026 0000000000000036 > > > c0080000021e0480 > > > GPR08: 000000007c426214 c000000000055f14 c000000000055e08 > > > 0000000000000d80 > > > GPR12: c00000000021d9b0 c000000002d90000 c0000000088b3000 > > > c0080000021f0810 > > > GPR16: c0080000021c0638 c0000000088b3d80 00000000ffffffff > > > c000000001181e38 > > > GPR20: c0000000029dc088 c0080000021e0480 c0080000021f0870 > > > aaaaaaaaaaaaaaab > > > GPR24: c0000000088b3c40 c0080000021d0000 c0080000021f0000 > > > 0000000000000000 > > > GPR28: c0080000021d0000 0000000000000000 c0080000021c0638 > > > 0000000000000810 > > > NIP [c000000000055f14] apply_relocate_add+0x474/0x9e0 > > > LR [c00000000021ef28] klp_apply_section_relocs+0x208/0x2d0 > > > Call Trace: > > > [c0000000387af840] [c0000000387af920] 0xc0000000387af920 > > > (unreliable) > > > [c0000000387af940] [c00000000021ef28] > > > klp_apply_section_relocs+0x208/0x2d0 > > > [c0000000387afa30] [c00000000021f080] > > > klp_init_object_loaded+0x90/0x1e0 > > > [c0000000387afac0] [c0000000002200ac] > > > klp_module_coming+0x3dc/0x5c0 > > > [c0000000387afb70] [c000000000231414] load_module+0xf64/0x13a0 > > > [c0000000387afc90] [c000000000231b8c] > > > __do_sys_finit_module+0xdc/0x180 > > > [c0000000387afdb0] [c00000000002f004] > > > system_call_exception+0x164/0x340 > > > [c0000000387afe10] [c00000000000be68] > > > system_call_vectored_common+0xe8/0x278 > > > --- interrupt: 3000 at 0x7fffb6af4710 > > > NIP: 00007fffb6af4710 LR: 0000000000000000 CTR: 0000000000000000 > > > REGS: c0000000387afe80 TRAP: 3000 Tainted: G K > > > (5.19.0+) > > > MSR: 800000000000f033 <SF,EE,PR,FP,ME,IR,DR,RI,LE> CR: > > > 48224244 XER: 00000000 > > > IRQMASK: 0 > > > GPR00: 0000000000000161 00007fffe06f5550 00007fffb6bf7200 > > > 0000000000000005 > > > GPR04: 0000000105f36ca0 0000000000000000 0000000000000005 > > > 0000000000000000 > > > GPR08: 0000000000000000 0000000000000000 0000000000000000 > > > 0000000000000000 > > > GPR12: 0000000000000000 00007fffb738c540 0000000000000020 > > > 0000000000000000 > > > GPR16: 0000010024d31de0 0000000000000000 0000000105f37d50 > > > 0000010024d302f8 > > > GPR20: 0000000000000001 0000000000000908 0000010024d32020 > > > 0000010024d319b0 > > > GPR24: 0000000000000000 0000000000000000 0000010024d32040 > > > 0000010024d303f0 > > > GPR28: 0000010024d31e00 0000000105f36ca0 0000000000040000 > > > 0000010024d319b0 > > > NIP [00007fffb6af4710] 0x7fffb6af4710 > > > LR [0000000000000000] 0x0 > > > --- interrupt: 3000 > > > Instruction dump: > > > 0000061c 0000061c 0000061c 0000061c 0000061c 0000061c 0000061c > > > 0000061c > > > 00000288 00000248 60000000 7c992050 <7c9ce92a> 60000000 60000000 > > > e9310020 > > > ---[ end trace 0000000000000000 ]--- > > > > > > $ readelf --wide --sections > > > lib/livepatch/test_klp_convert_data.ko | grep -e '\[20\]' -e > > > '\[54\]' > > > [20] .data..ro_after_init > > > PROGBITS 0000000000000000 001a58 000008 00 WA 0 0 8 > > > [54] .klp.rela.test_klp_convert_mod..data..ro_after_init > > > RELA 0000000000000000 0426e8 000018 18 Ao 49 20 8 > > > > > > I can push a branch up to github if you'd like to try it > > > yourself. > > > > That would help thanks. > > > > This branch should do it: > > https://github.com/joe-lawrence/klp-convert-tree/tree/klp-convert-v7-devel > > Boot that and then run tools/testing/selftests/livepatch/test- > livepatch.sh Was able to reproduce, and confirm that using text patching for R_PPC64_ADDR64 works fixes it. - Russell > > -- Joe >
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c index 7e45dc98df8a..6aaf5720070d 100644 --- a/arch/powerpc/kernel/module_64.c +++ b/arch/powerpc/kernel/module_64.c @@ -739,6 +739,55 @@ int apply_relocate_add(Elf64_Shdr *sechdrs, return 0; } +#ifdef CONFIG_LIVEPATCH +void clear_relocate_add(Elf64_Shdr *sechdrs, + const char *strtab, + unsigned int symindex, + unsigned int relsec, + struct module *me) +{ + unsigned int i; + Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr; + Elf64_Sym *sym; + unsigned long *location; + const char *symname; + u32 *instruction; + + pr_debug("Clearing ADD relocate section %u to %u\n", relsec, + sechdrs[relsec].sh_info); + + for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) { + location = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr + + rela[i].r_offset; + sym = (Elf64_Sym *)sechdrs[symindex].sh_addr + + ELF64_R_SYM(rela[i].r_info); + symname = me->core_kallsyms.strtab + + sym->st_name; + + if (ELF64_R_TYPE(rela[i].r_info) != R_PPC_REL24) + continue; + /* + * reverse the operations in apply_relocate_add() for case + * R_PPC_REL24. + */ + if (sym->st_shndx != SHN_UNDEF && + sym->st_shndx != SHN_LIVEPATCH) + continue; + + instruction = (u32 *)location; + if (is_mprofile_ftrace_call(symname)) + continue; + + if (!instr_is_relative_link_branch(ppc_inst(*instruction))) + continue; + + instruction += 1; + *instruction = PPC_RAW_NOP(); + } + +} +#endif + #ifdef CONFIG_DYNAMIC_FTRACE int module_trampoline_target(struct module *mod, unsigned long addr, unsigned long *target) diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c index 2d159b32885b..cc6784fbc1ac 100644 --- a/arch/s390/kernel/module.c +++ b/arch/s390/kernel/module.c @@ -500,6 +500,14 @@ static int module_alloc_ftrace_hotpatch_trampolines(struct module *me, } #endif /* CONFIG_FUNCTION_TRACER */ +#ifdef CONFIG_LIVEPATCH +void clear_relocate_add(Elf64_Shdr *sechdrs, const char *strtab, + unsigned int symindex, unsigned int relsec, + struct module *me) +{ +} +#endif + int module_finalize(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs, struct module *me) diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c index b1abf663417c..f9632afbb84c 100644 --- a/arch/x86/kernel/module.c +++ b/arch/x86/kernel/module.c @@ -128,18 +128,20 @@ int apply_relocate(Elf32_Shdr *sechdrs, return 0; } #else /*X86_64*/ -static int __apply_relocate_add(Elf64_Shdr *sechdrs, +static int __apply_clear_relocate_add(Elf64_Shdr *sechdrs, const char *strtab, unsigned int symindex, unsigned int relsec, struct module *me, - void *(*write)(void *dest, const void *src, size_t len)) + void *(*write)(void *dest, const void *src, size_t len), + bool clear) { unsigned int i; Elf64_Rela *rel = (void *)sechdrs[relsec].sh_addr; Elf64_Sym *sym; void *loc; u64 val; + u64 zero = 0ULL; DEBUGP("Applying relocate section %u to %u\n", relsec, sechdrs[relsec].sh_info); @@ -163,40 +165,60 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs, case R_X86_64_NONE: break; case R_X86_64_64: - if (*(u64 *)loc != 0) - goto invalid_relocation; - write(loc, &val, 8); + if (!clear) { + if (*(u64 *)loc != 0) + goto invalid_relocation; + write(loc, &val, 8); + } else { + write(loc, &zero, 8); + } break; case R_X86_64_32: - if (*(u32 *)loc != 0) - goto invalid_relocation; - write(loc, &val, 4); - if (val != *(u32 *)loc) - goto overflow; + if (!clear) { + if (*(u32 *)loc != 0) + goto invalid_relocation; + write(loc, &val, 4); + if (val != *(u32 *)loc) + goto overflow; + } else { + write(loc, &zero, 4); + } break; case R_X86_64_32S: - if (*(s32 *)loc != 0) - goto invalid_relocation; - write(loc, &val, 4); - if ((s64)val != *(s32 *)loc) - goto overflow; + if (!clear) { + if (*(s32 *)loc != 0) + goto invalid_relocation; + write(loc, &val, 4); + if ((s64)val != *(s32 *)loc) + goto overflow; + } else { + write(loc, &zero, 4); + } break; case R_X86_64_PC32: case R_X86_64_PLT32: - if (*(u32 *)loc != 0) - goto invalid_relocation; - val -= (u64)loc; - write(loc, &val, 4); + if (!clear) { + if (*(u32 *)loc != 0) + goto invalid_relocation; + val -= (u64)loc; + write(loc, &val, 4); #if 0 - if ((s64)val != *(s32 *)loc) - goto overflow; + if ((s64)val != *(s32 *)loc) + goto overflow; #endif + } else { + write(loc, &zero, 4); + } break; case R_X86_64_PC64: - if (*(u64 *)loc != 0) - goto invalid_relocation; - val -= (u64)loc; - write(loc, &val, 8); + if (!clear) { + if (*(u64 *)loc != 0) + goto invalid_relocation; + val -= (u64)loc; + write(loc, &val, 8); + } else { + write(loc, &zero, 8); + } break; default: pr_err("%s: Unknown rela relocation: %llu\n", @@ -234,8 +256,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs, mutex_lock(&text_mutex); } - ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me, - write); + ret = __apply_clear_relocate_add(sechdrs, strtab, symindex, relsec, me, + write, false /* clear */); if (!early) { text_poke_sync(); @@ -245,6 +267,32 @@ int apply_relocate_add(Elf64_Shdr *sechdrs, return ret; } +#ifdef CONFIG_LIVEPATCH + +void clear_relocate_add(Elf64_Shdr *sechdrs, + const char *strtab, + unsigned int symindex, + unsigned int relsec, + struct module *me) +{ + bool early = me->state == MODULE_STATE_UNFORMED; + void *(*write)(void *, const void *, size_t) = memcpy; + + if (!early) { + write = text_poke; + mutex_lock(&text_mutex); + } + + __apply_clear_relocate_add(sechdrs, strtab, symindex, relsec, me, + write, true /* clear */); + + if (!early) { + text_poke_sync(); + mutex_unlock(&text_mutex); + } +} +#endif + #endif int module_finalize(const Elf_Ehdr *hdr, diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h index 9e09d11ffe5b..d22b36b84b4b 100644 --- a/include/linux/moduleloader.h +++ b/include/linux/moduleloader.h @@ -72,6 +72,13 @@ int apply_relocate_add(Elf_Shdr *sechdrs, unsigned int symindex, unsigned int relsec, struct module *mod); +#ifdef CONFIG_LIVEPATCH +void clear_relocate_add(Elf64_Shdr *sechdrs, + const char *strtab, + unsigned int symindex, + unsigned int relsec, + struct module *me); +#endif #else static inline int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab, diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index bc475e62279d..5c0d8a4eba13 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -316,6 +316,45 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs, return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod); } +static void klp_clear_object_relocations(struct module *pmod, + struct klp_object *obj) +{ + int i, cnt; + const char *objname, *secname; + char sec_objname[MODULE_NAME_LEN]; + Elf_Shdr *sec; + + objname = klp_is_module(obj) ? obj->name : "vmlinux"; + + /* For each klp relocation section */ + for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) { + sec = pmod->klp_info->sechdrs + i; + secname = pmod->klp_info->secstrings + sec->sh_name; + if (!(sec->sh_flags & SHF_RELA_LIVEPATCH)) + continue; + + /* + * Format: .klp.rela.sec_objname.section_name + * See comment in klp_resolve_symbols() for an explanation + * of the selected field width value. + */ + secname = pmod->klp_info->secstrings + sec->sh_name; + cnt = sscanf(secname, ".klp.rela.%55[^.]", sec_objname); + if (cnt != 1) { + pr_err("section %s has an incorrectly formatted name\n", + secname); + continue; + } + + if (strcmp(objname, sec_objname)) + continue; + + clear_relocate_add(pmod->klp_info->sechdrs, + pmod->core_kallsyms.strtab, + pmod->klp_info->symndx, i, pmod); + } +} + /* * Sysfs Interface * @@ -1154,7 +1193,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod, klp_unpatch_object(obj); klp_post_unpatch_callback(obj); - + klp_clear_object_relocations(patch->mod, obj); klp_free_object_loaded(obj); break; }