* [PATCH v7] livepatch: Clear relocation targets on a module removal
@ 2022-12-14 17:40 Song Liu
2023-01-03 17:00 ` Song Liu
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Song Liu @ 2022-12-14 17:40 UTC (permalink / raw)
To: live-patching
Cc: jpoimboe, jikos, pmladek, joe.lawrence, Miroslav Benes, Song Liu,
Josh Poimboeuf
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.
Signed-off-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Song Liu <song@kernel.org>
Reported-by: Josh Poimboeuf <jpoimboe@redhat.com>
Tested-by: Joe Lawrence <joe.lawrence@redhat.com> # x86_64, s390x, ppc64le
---
NOTE: powerpc32 code is only compile tested.
Changes v6 = v7:
1. Reduce code duplication in livepatch/core.c and x86/kernel/module.c.
2. Add more comments to powerpc/kernel/module_64.c.
3. Added Joe's Tested-by (which I should have added in v6).
Changes v5 = v6:
1. Fix powerpc64.
2. Fix compile for powerpc32.
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_32.c | 10 +++
arch/powerpc/kernel/module_64.c | 61 ++++++++++++++++++
arch/s390/kernel/module.c | 8 +++
arch/x86/kernel/module.c | 108 ++++++++++++++++++++++----------
include/linux/moduleloader.h | 7 +++
kernel/livepatch/core.c | 85 ++++++++++++++++---------
6 files changed, 217 insertions(+), 62 deletions(-)
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 7e45dc98df8a..83e6c226009c 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -739,6 +739,67 @@ 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;
+
+ /* skip mprofile and ftrace calls, same as restore_r2() */
+ if (is_mprofile_ftrace_call(symname))
+ continue;
+
+ instruction = (u32 *)location;
+ /* skip sibling call, same as restore_r2() */
+ if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
+ continue;
+
+ instruction += 1;
+ /*
+ * Patch location + 1 back to NOP so the next
+ * apply_relocate_add() call (reload the module) will not
+ * fail the sanity check in restore_r2():
+ *
+ * if (*instruction != PPC_RAW_NOP()) {
+ * pr_err(...);
+ * return 0;
+ * }
+ */
+ patch_instruction(instruction, ppc_inst(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 c032edcd3d95..8f997959e526 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 __write_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 apply)
{
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 (apply) {
+ 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 (apply) {
+ 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 (apply) {
+ 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 (apply) {
+ 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 (apply) {
+ 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",
@@ -219,11 +241,12 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs,
return -ENOEXEC;
}
-int apply_relocate_add(Elf64_Shdr *sechdrs,
- const char *strtab,
- unsigned int symindex,
- unsigned int relsec,
- struct module *me)
+static int write_relocate_add(Elf64_Shdr *sechdrs,
+ const char *strtab,
+ unsigned int symindex,
+ unsigned int relsec,
+ struct module *me,
+ bool apply)
{
int ret;
bool early = me->state == MODULE_STATE_UNFORMED;
@@ -234,8 +257,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
mutex_lock(&text_mutex);
}
- ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
- write);
+ ret = __write_relocate_add(sechdrs, strtab, symindex, relsec, me,
+ write, apply);
if (!early) {
text_poke_sync();
@@ -245,6 +268,27 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
return ret;
}
+int apply_relocate_add(Elf64_Shdr *sechdrs,
+ const char *strtab,
+ unsigned int symindex,
+ unsigned int relsec,
+ struct module *me)
+{
+ return write_relocate_add(sechdrs, strtab, symindex, relsec, me, true);
+}
+
+#ifdef CONFIG_LIVEPATCH
+
+void clear_relocate_add(Elf64_Shdr *sechdrs,
+ const char *strtab,
+ unsigned int symindex,
+ unsigned int relsec,
+ struct module *me)
+{
+ write_relocate_add(sechdrs, strtab, symindex, relsec, me, false);
+}
+#endif
+
#endif
int module_finalize(const Elf_Ehdr *hdr,
diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 9e09d11ffe5b..958e6da7f475 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(Elf_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 9ada0bc5247b..8cd04643f988 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -261,6 +261,41 @@ static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
return 0;
}
+static int klp_write_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
+ const char *shstrtab, const char *strtab,
+ unsigned int symndx, unsigned int secndx,
+ const char *objname, bool apply)
+{
+ int cnt, ret;
+ char sec_objname[MODULE_NAME_LEN];
+ Elf_Shdr *sec = sechdrs + secndx;
+
+ /*
+ * Format: .klp.rela.sec_objname.section_name
+ * See comment in klp_resolve_symbols() for an explanation
+ * of the selected field width value.
+ */
+ cnt = sscanf(shstrtab + sec->sh_name, ".klp.rela.%55[^.]",
+ sec_objname);
+ if (cnt != 1) {
+ pr_err("section %s has an incorrectly formatted name\n",
+ shstrtab + sec->sh_name);
+ return -EINVAL;
+ }
+
+ if (strcmp(objname ? objname : "vmlinux", sec_objname))
+ return 0;
+
+ ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
+ if (ret)
+ return ret;
+
+ if (apply)
+ return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
+ clear_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
+ return 0;
+}
+
/*
* At a high-level, there are two types of klp relocation sections: those which
* reference symbols which live in vmlinux; and those which reference symbols
@@ -289,31 +324,8 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
unsigned int symndx, unsigned int secndx,
const char *objname)
{
- int cnt, ret;
- char sec_objname[MODULE_NAME_LEN];
- Elf_Shdr *sec = sechdrs + secndx;
-
- /*
- * Format: .klp.rela.sec_objname.section_name
- * See comment in klp_resolve_symbols() for an explanation
- * of the selected field width value.
- */
- cnt = sscanf(shstrtab + sec->sh_name, ".klp.rela.%55[^.]",
- sec_objname);
- if (cnt != 1) {
- pr_err("section %s has an incorrectly formatted name\n",
- shstrtab + sec->sh_name);
- return -EINVAL;
- }
-
- if (strcmp(objname ? objname : "vmlinux", sec_objname))
- return 0;
-
- ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
- if (ret)
- return ret;
-
- return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
+ return klp_write_section_relocs(pmod, sechdrs, shstrtab, strtab, symndx,
+ secndx, objname, true);
}
/*
@@ -762,8 +774,9 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
func->old_sympos ? func->old_sympos : 1);
}
-static int klp_apply_object_relocs(struct klp_patch *patch,
- struct klp_object *obj)
+static int klp_write_object_relocs(struct klp_patch *patch,
+ struct klp_object *obj,
+ bool apply)
{
int i, ret;
struct klp_modinfo *info = patch->mod->klp_info;
@@ -774,10 +787,10 @@ static int klp_apply_object_relocs(struct klp_patch *patch,
if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
continue;
- ret = klp_apply_section_relocs(patch->mod, info->sechdrs,
+ ret = klp_write_section_relocs(patch->mod, info->sechdrs,
info->secstrings,
patch->mod->core_kallsyms.strtab,
- info->symndx, i, obj->name);
+ info->symndx, i, obj->name, apply);
if (ret)
return ret;
}
@@ -785,6 +798,18 @@ static int klp_apply_object_relocs(struct klp_patch *patch,
return 0;
}
+static int klp_apply_object_relocs(struct klp_patch *patch,
+ struct klp_object *obj)
+{
+ return klp_write_object_relocs(patch, obj, true);
+}
+
+static void klp_clear_object_relocs(struct klp_patch *patch,
+ struct klp_object *obj)
+{
+ klp_write_object_relocs(patch, obj, false);
+}
+
/* parts of the initialization that is done only when the object is loaded */
static int klp_init_object_loaded(struct klp_patch *patch,
struct klp_object *obj)
@@ -1172,7 +1197,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
klp_unpatch_object(obj);
klp_post_unpatch_callback(obj);
-
+ klp_clear_object_relocs(patch, obj);
klp_free_object_loaded(obj);
break;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v7] livepatch: Clear relocation targets on a module removal
2022-12-14 17:40 [PATCH v7] livepatch: Clear relocation targets on a module removal Song Liu
@ 2023-01-03 17:00 ` Song Liu
2023-01-03 22:39 ` Joe Lawrence
2023-01-04 10:26 ` Petr Mladek
2023-01-05 13:03 ` Petr Mladek
2 siblings, 1 reply; 17+ messages in thread
From: Song Liu @ 2023-01-03 17:00 UTC (permalink / raw)
To: live-patching
Cc: jpoimboe, jikos, pmladek, joe.lawrence, Miroslav Benes, Josh Poimboeuf
Hi folks,
Happy New Year!
Could you please share your comments/suggestions on this version?
Thanks,
Song
On Wed, Dec 14, 2022 at 9:41 AM Song Liu <song@kernel.org> 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.
>
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> Signed-off-by: Song Liu <song@kernel.org>
> Reported-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Tested-by: Joe Lawrence <joe.lawrence@redhat.com> # x86_64, s390x, ppc64le
>
> ---
>
> NOTE: powerpc32 code is only compile tested.
>
> Changes v6 = v7:
> 1. Reduce code duplication in livepatch/core.c and x86/kernel/module.c.
> 2. Add more comments to powerpc/kernel/module_64.c.
> 3. Added Joe's Tested-by (which I should have added in v6).
>
> Changes v5 = v6:
> 1. Fix powerpc64.
> 2. Fix compile for powerpc32.
>
> 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_32.c | 10 +++
> arch/powerpc/kernel/module_64.c | 61 ++++++++++++++++++
> arch/s390/kernel/module.c | 8 +++
> arch/x86/kernel/module.c | 108 ++++++++++++++++++++++----------
> include/linux/moduleloader.h | 7 +++
> kernel/livepatch/core.c | 85 ++++++++++++++++---------
> 6 files changed, 217 insertions(+), 62 deletions(-)
>
> 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 7e45dc98df8a..83e6c226009c 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -739,6 +739,67 @@ 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;
> +
> + /* skip mprofile and ftrace calls, same as restore_r2() */
> + if (is_mprofile_ftrace_call(symname))
> + continue;
> +
> + instruction = (u32 *)location;
> + /* skip sibling call, same as restore_r2() */
> + if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
> + continue;
> +
> + instruction += 1;
> + /*
> + * Patch location + 1 back to NOP so the next
> + * apply_relocate_add() call (reload the module) will not
> + * fail the sanity check in restore_r2():
> + *
> + * if (*instruction != PPC_RAW_NOP()) {
> + * pr_err(...);
> + * return 0;
> + * }
> + */
> + patch_instruction(instruction, ppc_inst(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 c032edcd3d95..8f997959e526 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 __write_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 apply)
> {
> 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 (apply) {
> + 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 (apply) {
> + 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 (apply) {
> + 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 (apply) {
> + 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 (apply) {
> + 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",
> @@ -219,11 +241,12 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs,
> return -ENOEXEC;
> }
>
> -int apply_relocate_add(Elf64_Shdr *sechdrs,
> - const char *strtab,
> - unsigned int symindex,
> - unsigned int relsec,
> - struct module *me)
> +static int write_relocate_add(Elf64_Shdr *sechdrs,
> + const char *strtab,
> + unsigned int symindex,
> + unsigned int relsec,
> + struct module *me,
> + bool apply)
> {
> int ret;
> bool early = me->state == MODULE_STATE_UNFORMED;
> @@ -234,8 +257,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> mutex_lock(&text_mutex);
> }
>
> - ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
> - write);
> + ret = __write_relocate_add(sechdrs, strtab, symindex, relsec, me,
> + write, apply);
>
> if (!early) {
> text_poke_sync();
> @@ -245,6 +268,27 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> return ret;
> }
>
> +int apply_relocate_add(Elf64_Shdr *sechdrs,
> + const char *strtab,
> + unsigned int symindex,
> + unsigned int relsec,
> + struct module *me)
> +{
> + return write_relocate_add(sechdrs, strtab, symindex, relsec, me, true);
> +}
> +
> +#ifdef CONFIG_LIVEPATCH
> +
> +void clear_relocate_add(Elf64_Shdr *sechdrs,
> + const char *strtab,
> + unsigned int symindex,
> + unsigned int relsec,
> + struct module *me)
> +{
> + write_relocate_add(sechdrs, strtab, symindex, relsec, me, false);
> +}
> +#endif
> +
> #endif
>
> int module_finalize(const Elf_Ehdr *hdr,
> diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
> index 9e09d11ffe5b..958e6da7f475 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(Elf_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 9ada0bc5247b..8cd04643f988 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -261,6 +261,41 @@ static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
> return 0;
> }
>
> +static int klp_write_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
> + const char *shstrtab, const char *strtab,
> + unsigned int symndx, unsigned int secndx,
> + const char *objname, bool apply)
> +{
> + int cnt, ret;
> + char sec_objname[MODULE_NAME_LEN];
> + Elf_Shdr *sec = sechdrs + secndx;
> +
> + /*
> + * Format: .klp.rela.sec_objname.section_name
> + * See comment in klp_resolve_symbols() for an explanation
> + * of the selected field width value.
> + */
> + cnt = sscanf(shstrtab + sec->sh_name, ".klp.rela.%55[^.]",
> + sec_objname);
> + if (cnt != 1) {
> + pr_err("section %s has an incorrectly formatted name\n",
> + shstrtab + sec->sh_name);
> + return -EINVAL;
> + }
> +
> + if (strcmp(objname ? objname : "vmlinux", sec_objname))
> + return 0;
> +
> + ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
> + if (ret)
> + return ret;
> +
> + if (apply)
> + return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
> + clear_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
> + return 0;
> +}
> +
> /*
> * At a high-level, there are two types of klp relocation sections: those which
> * reference symbols which live in vmlinux; and those which reference symbols
> @@ -289,31 +324,8 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
> unsigned int symndx, unsigned int secndx,
> const char *objname)
> {
> - int cnt, ret;
> - char sec_objname[MODULE_NAME_LEN];
> - Elf_Shdr *sec = sechdrs + secndx;
> -
> - /*
> - * Format: .klp.rela.sec_objname.section_name
> - * See comment in klp_resolve_symbols() for an explanation
> - * of the selected field width value.
> - */
> - cnt = sscanf(shstrtab + sec->sh_name, ".klp.rela.%55[^.]",
> - sec_objname);
> - if (cnt != 1) {
> - pr_err("section %s has an incorrectly formatted name\n",
> - shstrtab + sec->sh_name);
> - return -EINVAL;
> - }
> -
> - if (strcmp(objname ? objname : "vmlinux", sec_objname))
> - return 0;
> -
> - ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
> - if (ret)
> - return ret;
> -
> - return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
> + return klp_write_section_relocs(pmod, sechdrs, shstrtab, strtab, symndx,
> + secndx, objname, true);
> }
>
> /*
> @@ -762,8 +774,9 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> func->old_sympos ? func->old_sympos : 1);
> }
>
> -static int klp_apply_object_relocs(struct klp_patch *patch,
> - struct klp_object *obj)
> +static int klp_write_object_relocs(struct klp_patch *patch,
> + struct klp_object *obj,
> + bool apply)
> {
> int i, ret;
> struct klp_modinfo *info = patch->mod->klp_info;
> @@ -774,10 +787,10 @@ static int klp_apply_object_relocs(struct klp_patch *patch,
> if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
> continue;
>
> - ret = klp_apply_section_relocs(patch->mod, info->sechdrs,
> + ret = klp_write_section_relocs(patch->mod, info->sechdrs,
> info->secstrings,
> patch->mod->core_kallsyms.strtab,
> - info->symndx, i, obj->name);
> + info->symndx, i, obj->name, apply);
> if (ret)
> return ret;
> }
> @@ -785,6 +798,18 @@ static int klp_apply_object_relocs(struct klp_patch *patch,
> return 0;
> }
>
> +static int klp_apply_object_relocs(struct klp_patch *patch,
> + struct klp_object *obj)
> +{
> + return klp_write_object_relocs(patch, obj, true);
> +}
> +
> +static void klp_clear_object_relocs(struct klp_patch *patch,
> + struct klp_object *obj)
> +{
> + klp_write_object_relocs(patch, obj, false);
> +}
> +
> /* parts of the initialization that is done only when the object is loaded */
> static int klp_init_object_loaded(struct klp_patch *patch,
> struct klp_object *obj)
> @@ -1172,7 +1197,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
> klp_unpatch_object(obj);
>
> klp_post_unpatch_callback(obj);
> -
> + klp_clear_object_relocs(patch, obj);
> klp_free_object_loaded(obj);
> break;
> }
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7] livepatch: Clear relocation targets on a module removal
2023-01-03 17:00 ` Song Liu
@ 2023-01-03 22:39 ` Joe Lawrence
2023-01-03 23:29 ` Song Liu
0 siblings, 1 reply; 17+ messages in thread
From: Joe Lawrence @ 2023-01-03 22:39 UTC (permalink / raw)
To: Song Liu, live-patching
Cc: jpoimboe, jikos, pmladek, Miroslav Benes, Josh Poimboeuf
On 1/3/23 12:00, Song Liu wrote:
> Hi folks,
>
> Happy New Year!
>
> Could you please share your comments/suggestions on this version?
>
Hi Song,
I recently rebased the klp-convert-v7-devel development branch on top of
upstream v6.1 [1]. That branch includes a bunch of selftests that will
generate klp-relocations for several livepatches. I started a second
branch, klp-convert-v7-devel-song-v7 [2], which adds a few more commits
to the first one:
1 - (song) livepatch: Clear relocation targets on a module removal -
your v7 patch from this thread
2 - (song, tests) livepatch/selftests: add klp-relocation clearing tests
- modify the selftests to reload modules providing klp-relocation resolution
3 - (song, tests) livepatch/selftests: stricter klp-relocation clearing
tests - modify the selftests to verify that the relocations were
actually cleared
4 - (song, x86_64 suggestions) livepatch: Clear relocation targets on a
module removal - a few option tweaks to the x86 code, which I'll
commentate inline below
5 - (song, ppc64le suggestions, wip) livepatch: Clear relocation targets
on a module removal - a similar attempt at the same for ppc64le code,
which I didn't complete.
[1]
https://github.com/joe-lawrence/klp-convert-tree/tree/klp-convert-v7-devel
[2]
https://github.com/joe-lawrence/klp-convert-tree/tree/klp-convert-v7-devel-song-v7
more inline below ...
> Thanks,
> Song
>
> On Wed, Dec 14, 2022 at 9:41 AM Song Liu <song@kernel.org> 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.
>>
>> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
>> Signed-off-by: Song Liu <song@kernel.org>
>> Reported-by: Josh Poimboeuf <jpoimboe@redhat.com>
>> Tested-by: Joe Lawrence <joe.lawrence@redhat.com> # x86_64, s390x, ppc64le
Since there is still some work required for ppc64le and possibly s390x,
let's strip the tested-by tag. Each version should be re-tested and
then we can let the maintainer add it on the final version.
>>
>> ---
>>
>> NOTE: powerpc32 code is only compile tested.
>>
>> Changes v6 = v7:
>> 1. Reduce code duplication in livepatch/core.c and x86/kernel/module.c.
>> 2. Add more comments to powerpc/kernel/module_64.c.
>> 3. Added Joe's Tested-by (which I should have added in v6).
>>
>> Changes v5 = v6:
>> 1. Fix powerpc64.
>> 2. Fix compile for powerpc32.
>>
>> 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_32.c | 10 +++
>> arch/powerpc/kernel/module_64.c | 61 ++++++++++++++++++
>> arch/s390/kernel/module.c | 8 +++
>> arch/x86/kernel/module.c | 108 ++++++++++++++++++++++----------
>> include/linux/moduleloader.h | 7 +++
>> kernel/livepatch/core.c | 85 ++++++++++++++++---------
>> 6 files changed, 217 insertions(+), 62 deletions(-)
>>
>> 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 7e45dc98df8a..83e6c226009c 100644
>> --- a/arch/powerpc/kernel/module_64.c
>> +++ b/arch/powerpc/kernel/module_64.c
>> @@ -739,6 +739,67 @@ 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;
Ppc64le will need to handle additional relocation types.
While debugging a related issue on ppc64le regarding
CONFIG_STRICT_MODULE_RWX [3], these were the extent of the
klp-relocation types generated by kpatch-build and klp-convert-tree:
- R_PPC64_REL24 to symbols in other .text sections
- R_PPC64_ADDR64 to symbols thru .TOC
- R_PPC64_REL64 to static key symbols
I believe R_PPC64_ADDR64 and R_PPC64_REL64 can be simply be reset to 0.
[3] https://github.com/linuxppc/issues/issues/375#issuecomment-1233698835
>> + /*
>> + * reverse the operations in apply_relocate_add() for case
>> + * R_PPC_REL24.
>> + */
>> + if (sym->st_shndx != SHN_UNDEF &&
>> + sym->st_shndx != SHN_LIVEPATCH)
>> + continue;
>> +
>> + /* skip mprofile and ftrace calls, same as restore_r2() */
>> + if (is_mprofile_ftrace_call(symname))
>> + continue;
>> +
>> + instruction = (u32 *)location;
>> + /* skip sibling call, same as restore_r2() */
>> + if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
>> + continue;
>> +
>> + instruction += 1;
>> + /*
>> + * Patch location + 1 back to NOP so the next
>> + * apply_relocate_add() call (reload the module) will not
>> + * fail the sanity check in restore_r2():
>> + *
>> + * if (*instruction != PPC_RAW_NOP()) {
>> + * pr_err(...);
>> + * return 0;
>> + * }
>> + */
>> + patch_instruction(instruction, ppc_inst(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 c032edcd3d95..8f997959e526 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 __write_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 apply)
Aside: I do prefer the style of one function to handle applying/clearing
of relocations. x86_64 isn't too bad, but other arches have a much
richer set of relocations that do all sorts of relative/offset/TOC/etc
gymnastics that keeping their code in one spot should be much more
maintainable.
>> {
>> 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);
How about keying off the apply bool to display "Applying" vs "Clearing".
>> @@ -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 (apply) {
>> + 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 (apply) {
>> + 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 (apply) {
>> + 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 (apply) {
>> + 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
Btw, This has been #if 0'd for so long I wonder if we should just remove it?
>> + } 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 (apply) {
>> + if (*(u64 *)loc != 0)
>> + goto invalid_relocation;
>> + val -= (u64)loc;
>> + write(loc, &val, 8);
>> + } else {
>> + write(loc, &zero, 8);
>> + }
In my branch [2] ("(song, x86_64 suggestions) livepatch: Clear
relocation targets on a module removal"), I experimented with reducing
these cases down into two steps: compute the new val and then set the
location.
Having back-to-back relocation case blocks wasn't ideal, but it does
reduce code a bit:
For step 1:
- combine the relative relocation assignment
- consider !apply to be val of 0, drop the zero variable
For step 2:
- drop the if (apply) conditional, just write the new val
For at least this arch, I think it came out OK. Summarized here for
reference:
/* Calculate value (or zero if clearing) */
if (apply) {
val = sym->st_value + rel[i].r_addend;
switch (ELF64_R_TYPE(rel[i].r_info)) {
case R_X86_64_PC32:
case R_X86_64_PLT32:
case R_X86_64_PC64:
val -= (u64)loc;
break;
}
} else {
val = 0ULL;
}
/* Apply/clear relocation value */
switch (ELF64_R_TYPE(rel[i].r_info)) {
case R_X86_64_NONE:
break;
case R_X86_64_64:
if (apply && *(u64 *)loc != 0)
goto invalid_relocation;
write(loc, &val, 8);
break;
case R_X86_64_32:
if (apply && *(u32 *)loc != 0)
goto invalid_relocation;
write(loc, &val, 4);
if (val != *(u32 *)loc)
goto overflow;
break;
[ ... etc ... ]
That said, things got hairy really fast when I tried applying a similar
pattern to ppc64le code, so maybe this model wouldn't help other arches
so much. (I haven't looked at s390x yet.)
I'm not married to this approach, but thought I'd mention it as it
helped me tease apart these long apply_relocation() functions.
>> break;
>> default:
>> pr_err("%s: Unknown rela relocation: %llu\n",
>> @@ -219,11 +241,12 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs,
>> return -ENOEXEC;
>> }
>>
>> -int apply_relocate_add(Elf64_Shdr *sechdrs,
>> - const char *strtab,
>> - unsigned int symindex,
>> - unsigned int relsec,
>> - struct module *me)
>> +static int write_relocate_add(Elf64_Shdr *sechdrs,
>> + const char *strtab,
>> + unsigned int symindex,
>> + unsigned int relsec,
>> + struct module *me,
>> + bool apply)
>> {
>> int ret;
>> bool early = me->state == MODULE_STATE_UNFORMED;
>> @@ -234,8 +257,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
>> mutex_lock(&text_mutex);
>> }
>>
>> - ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
>> - write);
>> + ret = __write_relocate_add(sechdrs, strtab, symindex, relsec, me,
>> + write, apply);
>>
>> if (!early) {
>> text_poke_sync();
>> @@ -245,6 +268,27 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
>> return ret;
>> }
>>
>> +int apply_relocate_add(Elf64_Shdr *sechdrs,
>> + const char *strtab,
>> + unsigned int symindex,
>> + unsigned int relsec,
>> + struct module *me)
>> +{
>> + return write_relocate_add(sechdrs, strtab, symindex, relsec, me, true);
>> +}
>> +
>> +#ifdef CONFIG_LIVEPATCH
>> +
>> +void clear_relocate_add(Elf64_Shdr *sechdrs,
>> + const char *strtab,
>> + unsigned int symindex,
>> + unsigned int relsec,
>> + struct module *me)
>> +{
>> + write_relocate_add(sechdrs, strtab, symindex, relsec, me, false);
>> +}
>> +#endif
>> +
>> #endif
>>
>> int module_finalize(const Elf_Ehdr *hdr,
>> diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>> index 9e09d11ffe5b..958e6da7f475 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(Elf_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 9ada0bc5247b..8cd04643f988 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -261,6 +261,41 @@ static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
>> return 0;
>> }
>>
>> +static int klp_write_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
>> + const char *shstrtab, const char *strtab,
>> + unsigned int symndx, unsigned int secndx,
>> + const char *objname, bool apply)
>> +{
>> + int cnt, ret;
>> + char sec_objname[MODULE_NAME_LEN];
>> + Elf_Shdr *sec = sechdrs + secndx;
>> +
>> + /*
>> + * Format: .klp.rela.sec_objname.section_name
>> + * See comment in klp_resolve_symbols() for an explanation
>> + * of the selected field width value.
>> + */
>> + cnt = sscanf(shstrtab + sec->sh_name, ".klp.rela.%55[^.]",
>> + sec_objname);
>> + if (cnt != 1) {
>> + pr_err("section %s has an incorrectly formatted name\n",
>> + shstrtab + sec->sh_name);
>> + return -EINVAL;
>> + }
>> +
>> + if (strcmp(objname ? objname : "vmlinux", sec_objname))
>> + return 0;
>> +
>> + ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
>> + if (ret)
>> + return ret;
>> +
>> + if (apply)
>> + return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
>> + clear_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
>> + return 0;
>> +}
>> +
>> /*
>> * At a high-level, there are two types of klp relocation sections: those which
>> * reference symbols which live in vmlinux; and those which reference symbols
>> @@ -289,31 +324,8 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
>> unsigned int symndx, unsigned int secndx,
>> const char *objname)
>> {
>> - int cnt, ret;
>> - char sec_objname[MODULE_NAME_LEN];
>> - Elf_Shdr *sec = sechdrs + secndx;
>> -
>> - /*
>> - * Format: .klp.rela.sec_objname.section_name
>> - * See comment in klp_resolve_symbols() for an explanation
>> - * of the selected field width value.
>> - */
>> - cnt = sscanf(shstrtab + sec->sh_name, ".klp.rela.%55[^.]",
>> - sec_objname);
>> - if (cnt != 1) {
>> - pr_err("section %s has an incorrectly formatted name\n",
>> - shstrtab + sec->sh_name);
>> - return -EINVAL;
>> - }
>> -
>> - if (strcmp(objname ? objname : "vmlinux", sec_objname))
>> - return 0;
>> -
>> - ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
>> - if (ret)
>> - return ret;
>> -
>> - return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
>> + return klp_write_section_relocs(pmod, sechdrs, shstrtab, strtab, symndx,
>> + secndx, objname, true);
>> }
>>
>> /*
>> @@ -762,8 +774,9 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>> func->old_sympos ? func->old_sympos : 1);
>> }
>>
>> -static int klp_apply_object_relocs(struct klp_patch *patch,
>> - struct klp_object *obj)
>> +static int klp_write_object_relocs(struct klp_patch *patch,
>> + struct klp_object *obj,
>> + bool apply)
>> {
>> int i, ret;
>> struct klp_modinfo *info = patch->mod->klp_info;
>> @@ -774,10 +787,10 @@ static int klp_apply_object_relocs(struct klp_patch *patch,
>> if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
>> continue;
>>
>> - ret = klp_apply_section_relocs(patch->mod, info->sechdrs,
>> + ret = klp_write_section_relocs(patch->mod, info->sechdrs,
>> info->secstrings,
>> patch->mod->core_kallsyms.strtab,
>> - info->symndx, i, obj->name);
>> + info->symndx, i, obj->name, apply);
>> if (ret)
>> return ret;
>> }
>> @@ -785,6 +798,18 @@ static int klp_apply_object_relocs(struct klp_patch *patch,
>> return 0;
>> }
>>
>> +static int klp_apply_object_relocs(struct klp_patch *patch,
>> + struct klp_object *obj)
>> +{
>> + return klp_write_object_relocs(patch, obj, true);
>> +}
>> +
>> +static void klp_clear_object_relocs(struct klp_patch *patch,
>> + struct klp_object *obj)
>> +{
>> + klp_write_object_relocs(patch, obj, false);
>> +}
>> +
>> /* parts of the initialization that is done only when the object is loaded */
>> static int klp_init_object_loaded(struct klp_patch *patch,
>> struct klp_object *obj)
>> @@ -1172,7 +1197,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
>> klp_unpatch_object(obj);
>>
>> klp_post_unpatch_callback(obj);
>> -
>> + klp_clear_object_relocs(patch, obj);
>> klp_free_object_loaded(obj);
>> break;
>> }
>> --
>> 2.30.2
>>
>
I will try to get around to testing s390x sometime soon and report back
on how that works out.
Thanks,
--
Joe
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7] livepatch: Clear relocation targets on a module removal
2023-01-03 22:39 ` Joe Lawrence
@ 2023-01-03 23:29 ` Song Liu
0 siblings, 0 replies; 17+ messages in thread
From: Song Liu @ 2023-01-03 23:29 UTC (permalink / raw)
To: Joe Lawrence
Cc: live-patching, jpoimboe, jikos, pmladek, Miroslav Benes, Josh Poimboeuf
On Tue, Jan 3, 2023 at 2:39 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
[...]
> >>
> >> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> >> Signed-off-by: Song Liu <song@kernel.org>
> >> Reported-by: Josh Poimboeuf <jpoimboe@redhat.com>
> >> Tested-by: Joe Lawrence <joe.lawrence@redhat.com> # x86_64, s390x, ppc64le
>
> Since there is still some work required for ppc64le and possibly s390x,
> let's strip the tested-by tag. Each version should be re-tested and
> then we can let the maintainer add it on the final version.
Sure. Will remove the tag in later version(s).
[...]
> >> +#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;
>
> Ppc64le will need to handle additional relocation types.
>
> While debugging a related issue on ppc64le regarding
> CONFIG_STRICT_MODULE_RWX [3], these were the extent of the
> klp-relocation types generated by kpatch-build and klp-convert-tree:
>
> - R_PPC64_REL24 to symbols in other .text sections
> - R_PPC64_ADDR64 to symbols thru .TOC
> - R_PPC64_REL64 to static key symbols
>
> I believe R_PPC64_ADDR64 and R_PPC64_REL64 can be simply be reset to 0.
>
> [3] https://github.com/linuxppc/issues/issues/375#issuecomment-1233698835
Hmm.. I don't quite follow why the two are related.. What's the failure like if
we don't handle R_PPC64_ADDR64 and R_PPC64_REL64?
[...]
> >> #else /*X86_64*/
> >> -static int __apply_relocate_add(Elf64_Shdr *sechdrs,
> >> +static int __write_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 apply)
>
> Aside: I do prefer the style of one function to handle applying/clearing
> of relocations. x86_64 isn't too bad, but other arches have a much
> richer set of relocations that do all sorts of relative/offset/TOC/etc
> gymnastics that keeping their code in one spot should be much more
> maintainable.
I think this pattern will be pretty hairy for ppc64le?
>
> >> {
> >> 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);
>
> How about keying off the apply bool to display "Applying" vs "Clearing".
Great catch!
>
> >> @@ -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 (apply) {
> >> + 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 (apply) {
> >> + 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 (apply) {
> >> + 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 (apply) {
> >> + 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
>
> Btw, This has been #if 0'd for so long I wonder if we should just remove it?
>
> >> + } 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 (apply) {
> >> + if (*(u64 *)loc != 0)
> >> + goto invalid_relocation;
> >> + val -= (u64)loc;
> >> + write(loc, &val, 8);
> >> + } else {
> >> + write(loc, &zero, 8);
> >> + }
>
> In my branch [2] ("(song, x86_64 suggestions) livepatch: Clear
> relocation targets on a module removal"), I experimented with reducing
> these cases down into two steps: compute the new val and then set the
> location.
>
> Having back-to-back relocation case blocks wasn't ideal, but it does
> reduce code a bit:
>
> For step 1:
> - combine the relative relocation assignment
> - consider !apply to be val of 0, drop the zero variable
>
> For step 2:
> - drop the if (apply) conditional, just write the new val
>
> For at least this arch, I think it came out OK. Summarized here for
> reference:
>
> /* Calculate value (or zero if clearing) */
> if (apply) {
> val = sym->st_value + rel[i].r_addend;
>
> switch (ELF64_R_TYPE(rel[i].r_info)) {
> case R_X86_64_PC32:
> case R_X86_64_PLT32:
> case R_X86_64_PC64:
> val -= (u64)loc;
> break;
> }
> } else {
> val = 0ULL;
> }
>
> /* Apply/clear relocation value */
> switch (ELF64_R_TYPE(rel[i].r_info)) {
> case R_X86_64_NONE:
> break;
> case R_X86_64_64:
> if (apply && *(u64 *)loc != 0)
> goto invalid_relocation;
> write(loc, &val, 8);
> break;
> case R_X86_64_32:
> if (apply && *(u32 *)loc != 0)
> goto invalid_relocation;
> write(loc, &val, 4);
> if (val != *(u32 *)loc)
> goto overflow;
> break;
> [ ... etc ... ]
This version looks good to me.
>
>
> That said, things got hairy really fast when I tried applying a similar
> pattern to ppc64le code, so maybe this model wouldn't help other arches
> so much. (I haven't looked at s390x yet.)
ppc64le seems to have totally different pattern.
>
> I'm not married to this approach, but thought I'd mention it as it
> helped me tease apart these long apply_relocation() functions.
>
> >> break;
> >> default:
> >> pr_err("%s: Unknown rela relocation: %llu\n",
> >> @@ -219,11 +241,12 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs,
> >> return -ENOEXEC;
> >> }
[...]
> >>
> >
>
> I will try to get around to testing s390x sometime soon and report back
> on how that works out.
>
Thanks!
Song
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7] livepatch: Clear relocation targets on a module removal
2022-12-14 17:40 [PATCH v7] livepatch: Clear relocation targets on a module removal Song Liu
2023-01-03 17:00 ` Song Liu
@ 2023-01-04 10:26 ` Petr Mladek
2023-01-04 17:34 ` Song Liu
2023-01-05 13:03 ` Petr Mladek
2 siblings, 1 reply; 17+ messages in thread
From: Petr Mladek @ 2023-01-04 10:26 UTC (permalink / raw)
To: Song Liu
Cc: live-patching, jpoimboe, jikos, joe.lawrence, Miroslav Benes,
Josh Poimboeuf
On Wed 2022-12-14 09:40:35, 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.
>
> 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.
>
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -739,6 +739,67 @@ 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;
Is it OK to continue?
IMHO, we should at least warn here. It means that the special elf
section contains a relocation that we are not able to clear. It will
most likely blow up when we try to load the livepatched module
again.
> + /*
> + * reverse the operations in apply_relocate_add() for case
> + * R_PPC_REL24.
> + */
> + if (sym->st_shndx != SHN_UNDEF &&
> + sym->st_shndx != SHN_LIVEPATCH)
> + continue;
Same here. IMHO, we should warn when the section contains something
that we are not able to clear.
> + /* skip mprofile and ftrace calls, same as restore_r2() */
> + if (is_mprofile_ftrace_call(symname))
> + continue;
Is this correct? restore_r2() returns "1" in this case. As a result
apply_relocate_add() returns immediately with -ENOEXEC. IMHO, we
should print a warning and return as well.
> + instruction = (u32 *)location;
> + /* skip sibling call, same as restore_r2() */
> + if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
> + continue;
Same here. restore_r2() returns '1' in this case...
> +
> + instruction += 1;
> + /*
> + * Patch location + 1 back to NOP so the next
> + * apply_relocate_add() call (reload the module) will not
> + * fail the sanity check in restore_r2():
> + *
> + * if (*instruction != PPC_RAW_NOP()) {
> + * pr_err(...);
> + * return 0;
> + * }
> + */
> + patch_instruction(instruction, ppc_inst(PPC_RAW_NOP()));
> + }
This seems incomplete. The above code reverts patch_instruction() called
from restore_r2(). But there is another patch_instruction() called in
apply_relocate_add() for case R_PPC_REL24. IMHO, we should revert this
as well.
> +}
> +#endif
IMHO, this approach is really bad. The function is not maintainable.
It will be very hard to keep it in sync with apply_relocate_add().
And all the mistakes are just a proof.
IMHO, the only sane way is to avoid the code duplication.
> #ifdef CONFIG_DYNAMIC_FTRACE
> int module_trampoline_target(struct module *mod, unsigned long addr,
> unsigned long *target)
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -261,6 +261,41 @@ static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
> return 0;
> }
>
> +static int klp_write_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
> + const char *shstrtab, const char *strtab,
> + unsigned int symndx, unsigned int secndx,
> + const char *objname, bool apply)
> +{
> + int cnt, ret;
> + char sec_objname[MODULE_NAME_LEN];
> + Elf_Shdr *sec = sechdrs + secndx;
> +
> + /*
> + * Format: .klp.rela.sec_objname.section_name
> + * See comment in klp_resolve_symbols() for an explanation
> + * of the selected field width value.
> + */
> + cnt = sscanf(shstrtab + sec->sh_name, ".klp.rela.%55[^.]",
> + sec_objname);
> + if (cnt != 1) {
> + pr_err("section %s has an incorrectly formatted name\n",
> + shstrtab + sec->sh_name);
> + return -EINVAL;
> + }
> +
> + if (strcmp(objname ? objname : "vmlinux", sec_objname))
> + return 0;
> +
> + ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
> + if (ret)
> + return ret;
We do not need to call klp_resolve_symbols() when clearing the relocations.
> +
> + if (apply)
> + return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
Please, add an empty line here.
> + clear_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
> + return 0;
> +}
> +
> /*
> * At a high-level, there are two types of klp relocation sections: those which
> * reference symbols which live in vmlinux; and those which reference symbols
Please, keep these comments above klp_write_section_relocs().
It is the function that implements these details and it is
called from more locations.
> @@ -289,31 +324,8 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
> unsigned int symndx, unsigned int secndx,
> const char *objname)
> {
> - int cnt, ret;
> - char sec_objname[MODULE_NAME_LEN];
> - Elf_Shdr *sec = sechdrs + secndx;
> -
> - /*
> - * Format: .klp.rela.sec_objname.section_name
> - * See comment in klp_resolve_symbols() for an explanation
> - * of the selected field width value.
> - */
> - cnt = sscanf(shstrtab + sec->sh_name, ".klp.rela.%55[^.]",
> - sec_objname);
> - if (cnt != 1) {
> - pr_err("section %s has an incorrectly formatted name\n",
> - shstrtab + sec->sh_name);
> - return -EINVAL;
> - }
> -
> - if (strcmp(objname ? objname : "vmlinux", sec_objname))
> - return 0;
> -
> - ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
> - if (ret)
> - return ret;
> -
> - return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
> + return klp_write_section_relocs(pmod, sechdrs, shstrtab, strtab, symndx,
> + secndx, objname, true);
> }
>
> /*
Best Regards,
Petr
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7] livepatch: Clear relocation targets on a module removal
2023-01-04 10:26 ` Petr Mladek
@ 2023-01-04 17:34 ` Song Liu
2023-01-04 23:12 ` Joe Lawrence
2023-01-05 11:19 ` Petr Mladek
0 siblings, 2 replies; 17+ messages in thread
From: Song Liu @ 2023-01-04 17:34 UTC (permalink / raw)
To: Petr Mladek
Cc: live-patching, jpoimboe, jikos, joe.lawrence, Miroslav Benes,
Josh Poimboeuf
On Wed, Jan 4, 2023 at 2:26 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Wed 2022-12-14 09:40:35, 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.
> >
> > 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.
> >
> > --- a/arch/powerpc/kernel/module_64.c
> > +++ b/arch/powerpc/kernel/module_64.c
> > @@ -739,6 +739,67 @@ 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;
>
> Is it OK to continue?
>
> IMHO, we should at least warn here. It means that the special elf
> section contains a relocation that we are not able to clear. It will
> most likely blow up when we try to load the livepatched module
> again.
>
> > + /*
> > + * reverse the operations in apply_relocate_add() for case
> > + * R_PPC_REL24.
> > + */
> > + if (sym->st_shndx != SHN_UNDEF &&
> > + sym->st_shndx != SHN_LIVEPATCH)
> > + continue;
>
> Same here. IMHO, we should warn when the section contains something
> that we are not able to clear.
>
> > + /* skip mprofile and ftrace calls, same as restore_r2() */
> > + if (is_mprofile_ftrace_call(symname))
> > + continue;
>
> Is this correct? restore_r2() returns "1" in this case. As a result
> apply_relocate_add() returns immediately with -ENOEXEC. IMHO, we
> should print a warning and return as well.
>
> > + instruction = (u32 *)location;
> > + /* skip sibling call, same as restore_r2() */
> > + if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
> > + continue;
>
> Same here. restore_r2() returns '1' in this case...
>
> > +
> > + instruction += 1;
> > + /*
> > + * Patch location + 1 back to NOP so the next
> > + * apply_relocate_add() call (reload the module) will not
> > + * fail the sanity check in restore_r2():
> > + *
> > + * if (*instruction != PPC_RAW_NOP()) {
> > + * pr_err(...);
> > + * return 0;
> > + * }
> > + */
> > + patch_instruction(instruction, ppc_inst(PPC_RAW_NOP()));
> > + }
>
> This seems incomplete. The above code reverts patch_instruction() called
> from restore_r2(). But there is another patch_instruction() called in
> apply_relocate_add() for case R_PPC_REL24. IMHO, we should revert this
> as well.
>
> > +}
> > +#endif
>
> IMHO, this approach is really bad. The function is not maintainable.
> It will be very hard to keep it in sync with apply_relocate_add().
> And all the mistakes are just a proof.
I don't really think the above are mistakes. This should be the same
as the version that passed Joe's tests. (I didn't test it myself).
>
> IMHO, the only sane way is to avoid the code duplication.
I think this falls back to the question that do we want
clear_relocate_add() to
1) undo everything by apply_relocate_add();
or
2) make sure the next apply_relocate_add() succeeds.
Current version does 2). If we want to share a lot of code
between apply_ and clear_, we need to go with 1). Do we
want something like:
/* `Everything is relative'. */
value = sym->st_value + rela[i].r_addend;
if (!apply)
value = 0;
switch (ELF64_R_TYPE(rela[i].r_info)) {
case R_PPC64_ADDR32:
/* Simply set it */
*(u32 *)location = value;
break;
case R_PPC64_ADDR64:
/* Simply set it */
*(unsigned long *)location = value;
break;
case R_PPC64_TOC:
value = apply ? my_r2(sechdrs, me) : 0;
*(unsigned long *)location = value;
break;
... (a lot more).
Actually, since R_PPC64_ADDR32 etc. don't cause
the next apply_ to fail, we can make clear_ to the same
thing as apply_ (write the same value again).
These approaches don't look better to me. But I am ok
with any of them. Please just let me know which one is
most preferable:
a. current version;
b. clear_ undo everything of apply_ (the sample code
above)
c. clear_ undo R_PPC_REL24, but _redo_ everything
of apply_ for other ELF64_R_TYPEs. (should be
clearer code than option b).
btw: undo the follow logic for R_PPC_REL24 alone is
not really easy (for me)
case R_PPC_REL24:
/* FIXME: Handle weak symbols here --RR */
if (sym->st_shndx == SHN_UNDEF ||
sym->st_shndx == SHN_LIVEPATCH) {
/* External: go via stub */
value = stub_for_addr(sechdrs, value, me,
strtab + sym->st_name);
if (!value)
return -ENOENT;
if (!restore_r2(strtab + sym->st_name,
(u32
*)location + 1, me))
return -ENOEXEC;
} else
value += local_entry_offset(sym);
/* Convert value to relative */
value -= (unsigned long)location;
if (value + 0x2000000 > 0x3ffffff || (value & 3) != 0){
pr_err("%s: REL24 %li out of range!\n",
me->name, (long int)value);
return -ENOEXEC;
}
/* Only replace bits 2 through 26 */
value = (*(uint32_t *)location & ~PPC_LI_MASK)
| PPC_LI(value);
if (patch_instruction((u32 *)location, ppc_inst(value)))
return -EFAULT;
break;
Thanks,
Song
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7] livepatch: Clear relocation targets on a module removal
2023-01-04 17:34 ` Song Liu
@ 2023-01-04 23:12 ` Joe Lawrence
2023-01-05 5:59 ` Song Liu
2023-01-05 11:19 ` Petr Mladek
1 sibling, 1 reply; 17+ messages in thread
From: Joe Lawrence @ 2023-01-04 23:12 UTC (permalink / raw)
To: Song Liu
Cc: Petr Mladek, live-patching, jpoimboe, jikos, Miroslav Benes,
Josh Poimboeuf
On Wed, Jan 04, 2023 at 09:34:25AM -0800, Song Liu wrote:
> On Wed, Jan 4, 2023 at 2:26 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Wed 2022-12-14 09:40:35, 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.
> > >
> > > 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.
> > >
> > > --- a/arch/powerpc/kernel/module_64.c
> > > +++ b/arch/powerpc/kernel/module_64.c
> > > @@ -739,6 +739,67 @@ 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;
> >
> > Is it OK to continue?
> >
> > IMHO, we should at least warn here. It means that the special elf
> > section contains a relocation that we are not able to clear. It will
> > most likely blow up when we try to load the livepatched module
> > again.
> >
> > > + /*
> > > + * reverse the operations in apply_relocate_add() for case
> > > + * R_PPC_REL24.
> > > + */
> > > + if (sym->st_shndx != SHN_UNDEF &&
> > > + sym->st_shndx != SHN_LIVEPATCH)
> > > + continue;
> >
> > Same here. IMHO, we should warn when the section contains something
> > that we are not able to clear.
> >
> > > + /* skip mprofile and ftrace calls, same as restore_r2() */
> > > + if (is_mprofile_ftrace_call(symname))
> > > + continue;
> >
> > Is this correct? restore_r2() returns "1" in this case. As a result
> > apply_relocate_add() returns immediately with -ENOEXEC. IMHO, we
> > should print a warning and return as well.
> >
> > > + instruction = (u32 *)location;
> > > + /* skip sibling call, same as restore_r2() */
> > > + if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
> > > + continue;
> >
> > Same here. restore_r2() returns '1' in this case...
> >
> > > +
> > > + instruction += 1;
> > > + /*
> > > + * Patch location + 1 back to NOP so the next
> > > + * apply_relocate_add() call (reload the module) will not
> > > + * fail the sanity check in restore_r2():
> > > + *
> > > + * if (*instruction != PPC_RAW_NOP()) {
> > > + * pr_err(...);
> > > + * return 0;
> > > + * }
> > > + */
> > > + patch_instruction(instruction, ppc_inst(PPC_RAW_NOP()));
> > > + }
> >
> > This seems incomplete. The above code reverts patch_instruction() called
> > from restore_r2(). But there is another patch_instruction() called in
> > apply_relocate_add() for case R_PPC_REL24. IMHO, we should revert this
> > as well.
> >
> > > +}
> > > +#endif
> >
> > IMHO, this approach is really bad. The function is not maintainable.
> > It will be very hard to keep it in sync with apply_relocate_add().
> > And all the mistakes are just a proof.
>
> I don't really think the above are mistakes. This should be the same
> as the version that passed Joe's tests. (I didn't test it myself).
>
> >
> > IMHO, the only sane way is to avoid the code duplication.
>
> I think this falls back to the question that do we want
> clear_relocate_add() to
> 1) undo everything by apply_relocate_add();
> or
> 2) make sure the next apply_relocate_add() succeeds.
>
This is a really good question and I think relates to your follow up
question to my earlier reply, "What's the failure like if we don't
handle R_PPC64_ADDR64 and R_PPC64_REL64?"
If the code only needs to accomplish (2), then the incoming patch simply
overwrites old relocation values. If we prefer (1), then needs to do
the full reversal on unload.
Stepping back, this feature is definitely foot-gun capable.
Kpatch-build would expect that klp-relocations would only ever be needed
in code that will patch the very same module that provides the
relocation destination -- that is, it was never intended to reference
through one of these klp-relocations unless it resolved to a live
module.
On the other hand, when writing the selftests, verifying against NULL
[1] provided 1) a quick sanity check that something was "cleared" and 2)
protected the machine against said foot-gun.
[1] https://github.com/joe-lawrence/klp-convert-tree/commit/643acbb8f4c0240030b45b64a542d126370d3e6c
> Current version does 2). If we want to share a lot of code
> between apply_ and clear_, we need to go with 1). Do we
> want something like:
>
> /* `Everything is relative'. */
> value = sym->st_value + rela[i].r_addend;
> if (!apply)
> value = 0;
>
> switch (ELF64_R_TYPE(rela[i].r_info)) {
> case R_PPC64_ADDR32:
> /* Simply set it */
> *(u32 *)location = value;
> break;
>
> case R_PPC64_ADDR64:
> /* Simply set it */
> *(unsigned long *)location = value;
> break;
>
> case R_PPC64_TOC:
> value = apply ? my_r2(sechdrs, me) : 0;
> *(unsigned long *)location = value;
> break;
> ... (a lot more).
>
> Actually, since R_PPC64_ADDR32 etc. don't cause
> the next apply_ to fail, we can make clear_ to the same
> thing as apply_ (write the same value again).
>
> These approaches don't look better to me. But I am ok
> with any of them. Please just let me know which one is
> most preferable:
>
> a. current version;
> b. clear_ undo everything of apply_ (the sample code
> above)
> c. clear_ undo R_PPC_REL24, but _redo_ everything
> of apply_ for other ELF64_R_TYPEs. (should be
> clearer code than option b).
>
This was my attempt at combining and slightly refactoring the power64
version. There is so much going on here I was tempted to split off it
into separate value assignment and write functions. Some changes I
liked, but I wasn't all too happy with the result. Also, as you
mention, completely undoing R_PPC_REL24 is less than trivial... for this
arch, there are basically three major tasks:
1) calculate the new value, including range checking
2) special constructs created by restore_r2 / create_stub
3) writing out the value
and many cases are similar, but subtly different enough to avoid easy
code consolidation.
static int write_relocate_add(Elf64_Shdr *sechdrs,
const char *strtab,
unsigned int symindex,
unsigned int relsec,
struct module *me,
bool apply)
{
unsigned int i;
Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
Elf64_Sym *sym;
unsigned long *location;
unsigned long value;
pr_debug("Applying ADD relocate section %u to %u\n", relsec,
sechdrs[relsec].sh_info);
/* First time we're called, we can fix up .TOC. */
if (!me->arch.toc_fixed) {
sym = find_dot_toc(sechdrs, strtab, symindex);
/* It's theoretically possible that a module doesn't want a
* .TOC. so don't fail it just for that. */
if (sym)
sym->st_value = my_r2(sechdrs, me);
me->arch.toc_fixed = true;
}
for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) {
/* This is where to make the change */
location = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr
+ rela[i].r_offset;
/* This is the symbol it is referring to */
sym = (Elf64_Sym *)sechdrs[symindex].sh_addr
+ ELF64_R_SYM(rela[i].r_info);
pr_debug("RELOC at %p: %li-type as %s (0x%lx) + %li\n",
location, (long)ELF64_R_TYPE(rela[i].r_info),
strtab + sym->st_name, (unsigned long)sym->st_value,
(long)rela[i].r_addend);
/* Calculate value (or zero if clearing) */
if (apply) {
/* `Everything is relative'. */
value = sym->st_value + rela[i].r_addend;
switch (ELF64_R_TYPE(rela[i].r_info)) {
case R_PPC64_TOC16:
/* Subtract TOC pointer */
value -= my_r2(sechdrs, me);
if (value + 0x8000 > 0xffff) {
pr_err("%s: bad TOC16 relocation (0x%lx)\n",
me->name, value);
return -ENOEXEC;
}
break;
case R_PPC64_TOC16_LO:
/* Subtract TOC pointer */
value -= my_r2(sechdrs, me);
value = (*((uint16_t *) location) & ~0xffff) |
(value & 0xffff);
break;
case R_PPC64_TOC16_DS:
/* Subtract TOC pointer */
value -= my_r2(sechdrs, me);
if ((value & 3) != 0 || value + 0x8000 > 0xffff) {
pr_err("%s: bad TOC16_DS relocation (0x%lx)\n",
me->name, value);
return -ENOEXEC;
}
value = (*((uint16_t *) location) & ~0xfffc) |
(value & 0xfffc);
break;
case R_PPC64_TOC16_LO_DS:
/* Subtract TOC pointer */
value -= my_r2(sechdrs, me);
if ((value & 3) != 0) {
pr_err("%s: bad TOC16_LO_DS relocation (0x%lx)\n",
me->name, value);
return -ENOEXEC;
}
value = (*((uint16_t *) location) & ~0xfffc) |
(value & 0xfffc);
break;
case R_PPC64_TOC16_HA:
/* Subtract TOC pointer */
value -= my_r2(sechdrs, me);
value = ((value + 0x8000) >> 16);
value = (*((uint16_t *) location) & ~0xffff) |
(value & 0xffff);
break;
case R_PPC_REL24:
/* FIXME: Handle weak symbols here --RR */
if (sym->st_shndx == SHN_UNDEF ||
sym->st_shndx == SHN_LIVEPATCH) {
/* External: go via stub */
value = stub_for_addr(sechdrs, value, me,
strtab + sym->st_name);
if (!value)
return -ENOENT;
if (!restore_r2(strtab + sym->st_name,
(u32 *)location + 1, me))
return -ENOEXEC;
} else
value += local_entry_offset(sym);
/* Convert value to relative */
value -= (unsigned long)location;
if (value + 0x2000000 > 0x3ffffff || (value & 3) != 0){
pr_err("%s: REL24 %li out of range!\n",
me->name, (long int)value);
return -ENOEXEC;
}
/* Only replace bits 2 through 26 */
value = (*(uint32_t *)location & ~PPC_LI_MASK) | PPC_LI(value);
break;
case R_PPC64_REL64:
/* 64 bits relative (used by features fixups) */
value -= (unsigned long)location;
break;
case R_PPC64_REL32:
/* 32 bits relative (used by relative exception tables) */
value -= (unsigned long)location;
if (value + 0x80000000 > 0xffffffff) {
pr_err("%s: REL32 %li out of range!\n",
me->name, (long int)value);
return -ENOEXEC;
}
break;
case R_PPC64_ENTRY:
/*
* Optimize ELFv2 large code model entry point if
* the TOC is within 2GB range of current location.
*/
value = my_r2(sechdrs, me) - (unsigned long)location;
if (value + 0x80008000 > 0xffffffff)
break; /* JL TODO: this should be a continue? */
break;
case R_PPC64_REL16_HA:
/* Subtract location pointer */
value -= (unsigned long)location;
value = ((value + 0x8000) >> 16);
value = (*((uint16_t *) location) & ~0xffff) |
(value & 0xffff);
break;
case R_PPC64_REL16_LO:
/* Subtract location pointer */
value -= (unsigned long)location;
value = (*((uint16_t *) location) & ~0xffff) |
(value & 0xffff);
break;
}
} else {
if (ELF64_R_TYPE(rela[i].r_info) == R_PPC_REL24) {
/* skip mprofile and ftrace calls, same as restore_r2() */
if (is_mprofile_ftrace_call(me->core_kallsyms.strtab + sym->st_name))
continue;
/* skip sibling call, same as restore_r2() */
if (!instr_is_relative_link_branch(ppc_inst(*(u32 *)location)))
continue;
/*
* Patch location + 1 back to NOP so the next
* apply_relocate_add() call (reload the module) will not
* fail the sanity check in restore_r2():
*
* if (*instruction != PPC_RAW_NOP()) {
* pr_err(...);
* return 0;
* }
*/
location = (unsigned long *)((u32 *)location + 1);
value = PPC_RAW_NOP();
} else {
value = 0UL;
}
}
/* Apply/clear relocation value */
switch (ELF64_R_TYPE(rela[i].r_info)) {
case R_PPC64_ADDR32:
case R_PPC64_REL32:
if (apply && *(u32 *)location != 0)
goto invalid_relocation;
/* Simply set it */
*(u32 *)location = value;
break;
case R_PPC64_ADDR64:
if (apply && *(unsigned long *)location != 0)
goto invalid_relocation;
/* Simply set it */
*(unsigned long *)location = value;
break;
case R_PPC64_TOC:
if (apply && *(unsigned long *)location != 0)
goto invalid_relocation;
*(unsigned long *)location = my_r2(sechdrs, me);
break;
case R_PPC64_TOC16:
case R_PPC64_TOC16_LO:
case R_PPC64_TOC16_HA:
case R_PPC64_REL16_HA:
case R_PPC64_REL16_LO:
if (apply && (*((uint16_t *) location) & 0xffff) != 0)
goto invalid_relocation;
*((uint16_t *) location) = value;
break;
case R_PPC64_TOC16_DS:
case R_PPC64_TOC16_LO_DS:
if (apply && (*((uint16_t *) location) & 0xfffc) != 0)
goto invalid_relocation;
*((uint16_t *) location) = value;
break;
case R_PPC_REL24:
/* restore_r2() already checked for invalid relocation */
if (patch_instruction((u32 *)location, ppc_inst(value)))
return -EFAULT;
break;
case R_PPC64_REL64:
if (apply && *location != 0)
goto invalid_relocation;
/* 64 bits relative (used by features fixups) */
*location = value;
break;
case R_PPC64_TOCSAVE:
/*
* Marker reloc indicates we don't have to save r2.
* That would only save us one instruction, so ignore
* it.
*/
break;
case R_PPC64_ENTRY:
/*
* Check for the large code model prolog sequence:
* ld r2, ...(r12)
* add r2, r2, r12
*/
if ((((uint32_t *)location)[0] & ~0xfffc) != PPC_RAW_LD(_R2, _R12, 0))
break;
if (((uint32_t *)location)[1] != PPC_RAW_ADD(_R2, _R2, _R12))
break;
/*
* If found, replace it with:
* addis r2, r12, (.TOC.-func)@ha
* addi r2, r2, (.TOC.-func)@l
*/
((uint32_t *)location)[0] = PPC_RAW_ADDIS(_R2, _R12, PPC_HA(value));
((uint32_t *)location)[1] = PPC_RAW_ADDI(_R2, _R2, PPC_LO(value));
break;
default:
pr_err("%s: Unknown ADD relocation: %lu\n",
me->name,
(unsigned long)ELF64_R_TYPE(rela[i].r_info));
return -ENOEXEC;
}
}
return 0;
invalid_relocation:
pr_err("ppc64le/modules: Skipping invalid relocation target, existing value is nonzero for type %d, location %p, value %lx\n",
(int)ELF64_R_TYPE(rela[i].r_info), location, value);
return -ENOEXEC;
}
> btw: undo the follow logic for R_PPC_REL24 alone is
> not really easy (for me)
>
> case R_PPC_REL24:
> /* FIXME: Handle weak symbols here --RR */
> if (sym->st_shndx == SHN_UNDEF ||
> sym->st_shndx == SHN_LIVEPATCH) {
> /* External: go via stub */
> value = stub_for_addr(sechdrs, value, me,
> strtab + sym->st_name);
> if (!value)
> return -ENOENT;
> if (!restore_r2(strtab + sym->st_name,
> (u32
> *)location + 1, me))
> return -ENOEXEC;
> } else
> value += local_entry_offset(sym);
>
> /* Convert value to relative */
> value -= (unsigned long)location;
> if (value + 0x2000000 > 0x3ffffff || (value & 3) != 0){
> pr_err("%s: REL24 %li out of range!\n",
> me->name, (long int)value);
> return -ENOEXEC;
> }
>
> /* Only replace bits 2 through 26 */
> value = (*(uint32_t *)location & ~PPC_LI_MASK)
> | PPC_LI(value);
>
> if (patch_instruction((u32 *)location, ppc_inst(value)))
> return -EFAULT;
>
> break;
>
I agree. I think the only way through it is to refactor it down to
something we do understand first :D And also settling on our
requirements as you asked about.
-- Joe
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7] livepatch: Clear relocation targets on a module removal
2023-01-04 23:12 ` Joe Lawrence
@ 2023-01-05 5:59 ` Song Liu
2023-01-05 15:05 ` Joe Lawrence
0 siblings, 1 reply; 17+ messages in thread
From: Song Liu @ 2023-01-05 5:59 UTC (permalink / raw)
To: Joe Lawrence
Cc: Petr Mladek, live-patching, jpoimboe, jikos, Miroslav Benes,
Josh Poimboeuf
On Wed, Jan 4, 2023 at 3:12 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> On Wed, Jan 04, 2023 at 09:34:25AM -0800, Song Liu wrote:
> > On Wed, Jan 4, 2023 at 2:26 AM Petr Mladek <pmladek@suse.com> wrote:
> > >
> > > On Wed 2022-12-14 09:40:35, 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.
> > > >
> > > > 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.
> > > >
> > > > --- a/arch/powerpc/kernel/module_64.c
> > > > +++ b/arch/powerpc/kernel/module_64.c
> > > > @@ -739,6 +739,67 @@ 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;
> > >
> > > Is it OK to continue?
> > >
> > > IMHO, we should at least warn here. It means that the special elf
> > > section contains a relocation that we are not able to clear. It will
> > > most likely blow up when we try to load the livepatched module
> > > again.
> > >
> > > > + /*
> > > > + * reverse the operations in apply_relocate_add() for case
> > > > + * R_PPC_REL24.
> > > > + */
> > > > + if (sym->st_shndx != SHN_UNDEF &&
> > > > + sym->st_shndx != SHN_LIVEPATCH)
> > > > + continue;
> > >
> > > Same here. IMHO, we should warn when the section contains something
> > > that we are not able to clear.
> > >
> > > > + /* skip mprofile and ftrace calls, same as restore_r2() */
> > > > + if (is_mprofile_ftrace_call(symname))
> > > > + continue;
> > >
> > > Is this correct? restore_r2() returns "1" in this case. As a result
> > > apply_relocate_add() returns immediately with -ENOEXEC. IMHO, we
> > > should print a warning and return as well.
> > >
> > > > + instruction = (u32 *)location;
> > > > + /* skip sibling call, same as restore_r2() */
> > > > + if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
> > > > + continue;
> > >
> > > Same here. restore_r2() returns '1' in this case...
> > >
> > > > +
> > > > + instruction += 1;
> > > > + /*
> > > > + * Patch location + 1 back to NOP so the next
> > > > + * apply_relocate_add() call (reload the module) will not
> > > > + * fail the sanity check in restore_r2():
> > > > + *
> > > > + * if (*instruction != PPC_RAW_NOP()) {
> > > > + * pr_err(...);
> > > > + * return 0;
> > > > + * }
> > > > + */
> > > > + patch_instruction(instruction, ppc_inst(PPC_RAW_NOP()));
> > > > + }
> > >
> > > This seems incomplete. The above code reverts patch_instruction() called
> > > from restore_r2(). But there is another patch_instruction() called in
> > > apply_relocate_add() for case R_PPC_REL24. IMHO, we should revert this
> > > as well.
> > >
> > > > +}
> > > > +#endif
> > >
> > > IMHO, this approach is really bad. The function is not maintainable.
> > > It will be very hard to keep it in sync with apply_relocate_add().
> > > And all the mistakes are just a proof.
> >
> > I don't really think the above are mistakes. This should be the same
> > as the version that passed Joe's tests. (I didn't test it myself).
> >
> > >
> > > IMHO, the only sane way is to avoid the code duplication.
> >
> > I think this falls back to the question that do we want
> > clear_relocate_add() to
> > 1) undo everything by apply_relocate_add();
> > or
> > 2) make sure the next apply_relocate_add() succeeds.
> >
>
> This is a really good question and I think relates to your follow up
> question to my earlier reply, "What's the failure like if we don't
> handle R_PPC64_ADDR64 and R_PPC64_REL64?"
>
> If the code only needs to accomplish (2), then the incoming patch simply
> overwrites old relocation values. If we prefer (1), then needs to do
> the full reversal on unload.
>
> Stepping back, this feature is definitely foot-gun capable.
> Kpatch-build would expect that klp-relocations would only ever be needed
> in code that will patch the very same module that provides the
> relocation destination -- that is, it was never intended to reference
> through one of these klp-relocations unless it resolved to a live
> module.
>
> On the other hand, when writing the selftests, verifying against NULL
> [1] provided 1) a quick sanity check that something was "cleared" and 2)
> protected the machine against said foot-gun.
>
> [1] https://github.com/joe-lawrence/klp-convert-tree/commit/643acbb8f4c0240030b45b64a542d126370d3e6c
I don't quite follow the foot-gun here. What's the failure mode?
[...]
> > These approaches don't look better to me. But I am ok
> > with any of them. Please just let me know which one is
> > most preferable:
> >
> > a. current version;
> > b. clear_ undo everything of apply_ (the sample code
> > above)
> > c. clear_ undo R_PPC_REL24, but _redo_ everything
> > of apply_ for other ELF64_R_TYPEs. (should be
> > clearer code than option b).
> >
>
> This was my attempt at combining and slightly refactoring the power64
> version. There is so much going on here I was tempted to split off it
> into separate value assignment and write functions. Some changes I
> liked, but I wasn't all too happy with the result. Also, as you
> mention, completely undoing R_PPC_REL24 is less than trivial... for this
> arch, there are basically three major tasks:
>
> 1) calculate the new value, including range checking
> 2) special constructs created by restore_r2 / create_stub
> 3) writing out the value
>
> and many cases are similar, but subtly different enough to avoid easy
> code consolidation.
Thanks for exploring this direction. I guess this part won't be perfect
anyway.
PS: While we discuss a solution for ppc64, how about we ship the
fix for other archs first? I think there are only a few small things to
be addressed.
Song
>
> static int write_relocate_add(Elf64_Shdr *sechdrs,
> const char *strtab,
> unsigned int symindex,
> unsigned int relsec,
> struct module *me,
> bool apply)
> {
> unsigned int i;
> Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
> Elf64_Sym *sym;
> unsigned long *location;
> unsigned long value;
>
[...]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7] livepatch: Clear relocation targets on a module removal
2023-01-04 17:34 ` Song Liu
2023-01-04 23:12 ` Joe Lawrence
@ 2023-01-05 11:19 ` Petr Mladek
2023-01-05 16:53 ` Song Liu
1 sibling, 1 reply; 17+ messages in thread
From: Petr Mladek @ 2023-01-05 11:19 UTC (permalink / raw)
To: Song Liu
Cc: live-patching, jpoimboe, jikos, joe.lawrence, Miroslav Benes,
Josh Poimboeuf
On Wed 2023-01-04 09:34:25, Song Liu wrote:
> On Wed, Jan 4, 2023 at 2:26 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Wed 2022-12-14 09:40:35, 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.
> > >
> > > 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.
> > >
> > > --- a/arch/powerpc/kernel/module_64.c
> > > +++ b/arch/powerpc/kernel/module_64.c
> > > @@ -739,6 +739,67 @@ 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;
> >
> > Is it OK to continue?
> >
> > IMHO, we should at least warn here. It means that the special elf
> > section contains a relocation that we are not able to clear. It will
> > most likely blow up when we try to load the livepatched module
> > again.
> >
> > > + /*
> > > + * reverse the operations in apply_relocate_add() for case
> > > + * R_PPC_REL24.
> > > + */
> > > + if (sym->st_shndx != SHN_UNDEF &&
> > > + sym->st_shndx != SHN_LIVEPATCH)
> > > + continue;
> >
> > Same here. IMHO, we should warn when the section contains something
> > that we are not able to clear.
> >
> > > + /* skip mprofile and ftrace calls, same as restore_r2() */
> > > + if (is_mprofile_ftrace_call(symname))
> > > + continue;
> >
> > Is this correct? restore_r2() returns "1" in this case. As a result
> > apply_relocate_add() returns immediately with -ENOEXEC. IMHO, we
> > should print a warning and return as well.
> >
> > > + instruction = (u32 *)location;
> > > + /* skip sibling call, same as restore_r2() */
> > > + if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
> > > + continue;
> >
> > Same here. restore_r2() returns '1' in this case...
> >
> > > +
> > > + instruction += 1;
> > > + /*
> > > + * Patch location + 1 back to NOP so the next
> > > + * apply_relocate_add() call (reload the module) will not
> > > + * fail the sanity check in restore_r2():
> > > + *
> > > + * if (*instruction != PPC_RAW_NOP()) {
> > > + * pr_err(...);
> > > + * return 0;
> > > + * }
> > > + */
> > > + patch_instruction(instruction, ppc_inst(PPC_RAW_NOP()));
> > > + }
> >
> > This seems incomplete. The above code reverts patch_instruction() called
> > from restore_r2(). But there is another patch_instruction() called in
> > apply_relocate_add() for case R_PPC_REL24. IMHO, we should revert this
> > as well.
> >
> > > +}
> > > +#endif
> >
> > IMHO, this approach is really bad. The function is not maintainable.
> > It will be very hard to keep it in sync with apply_relocate_add().
> > And all the mistakes are just a proof.
>
> I don't really think the above are mistakes. This should be the same
> as the version that passed Joe's tests. (I didn't test it myself).
I am not sure if Joe tested these situations.
Anyway, we should make it as robust as possible. If we manipulate
the addresses a wrong way then it might shoot-down the system.
If the code reaches an non-expected situation, it should at
least warn about it.
The entire livepatching code tries to be as robust as possible.
The main motivation for livepatching is to avoid reboot.
> >
> > IMHO, the only sane way is to avoid the code duplication.
>
> I think this falls back to the question that do we want
> clear_relocate_add() to
> 1) undo everything by apply_relocate_add();
> or
> 2) make sure the next apply_relocate_add() succeeds.
The ideal solution would be to add checks into apply_relocated_add().
It would make it more robust. In that case, clear_relocated_add()
would need to clear everything.
But this is not the case on powerpc and s390 at the moment.
In this case, I suggest to clear only relocations that
are checked in apply_relocated_add().
But it should be done without duplicating the code.
It would actually make sense to compute the value that was
used in apply_relocated_add() and check that we are clearing
the value. If we try to clear some other value than we
probably do something wrong.
This might actually be a solution. We could compute
the value in both situations. Then we could have
a common function for writing.
This write function would check that it replaces zero
with the value in apply_relocate_add() and that it replaces
the value with zero in clear_relocate_add().
Best Regards,
Petr
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7] livepatch: Clear relocation targets on a module removal
2022-12-14 17:40 [PATCH v7] livepatch: Clear relocation targets on a module removal Song Liu
2023-01-03 17:00 ` Song Liu
2023-01-04 10:26 ` Petr Mladek
@ 2023-01-05 13:03 ` Petr Mladek
2 siblings, 0 replies; 17+ messages in thread
From: Petr Mladek @ 2023-01-05 13:03 UTC (permalink / raw)
To: Song Liu
Cc: live-patching, jpoimboe, jikos, joe.lawrence, Miroslav Benes,
Josh Poimboeuf
On Wed 2022-12-14 09:40:35, 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.
>
> --- a/arch/x86/kernel/module.c
> +++ b/arch/x86/kernel/module.c
> @@ -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 (apply) {
> + if (*(u64 *)loc != 0)
> + goto invalid_relocation;
> + write(loc, &val, 8);
> + } else {
> + write(loc, &zero, 8);
It might make sense to check if the cleared value is the
expected one.
if (*(u64 *)loc != (u64)val)
goto invalid_relocation;
write(loc, &zero, 8);
Maybe, we could put this into a helper function or macro that
would do this operation
#define check_and_write(loc, orig_val, new_val, type) \
({ \
int err = 0; \
\
if (*(type)loc == (type)old_val) \
write(loc, &new_val, sizeof(type)); \
else \
err = -EINVAL; \
\
err; \
})
It would make it more robust. The relocation might be different
when it it redirected somewhere, for example, by ftrace.
Something might go wrong in this case.
On the other hand. I wonder if the relocation might actually
by redirected intentionally, for example, by apply_alternatives()
or apply_retpolines(). These would be hard to check.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7] livepatch: Clear relocation targets on a module removal
2023-01-05 5:59 ` Song Liu
@ 2023-01-05 15:05 ` Joe Lawrence
2023-01-05 17:11 ` Song Liu
2023-01-06 13:02 ` Miroslav Benes
0 siblings, 2 replies; 17+ messages in thread
From: Joe Lawrence @ 2023-01-05 15:05 UTC (permalink / raw)
To: Song Liu, Petr Mladek
Cc: live-patching, jpoimboe, jikos, Miroslav Benes, Josh Poimboeuf
On 1/5/23 00:59, Song Liu wrote:
> On Wed, Jan 4, 2023 at 3:12 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>>
>>
>> Stepping back, this feature is definitely foot-gun capable.
>> Kpatch-build would expect that klp-relocations would only ever be needed
>> in code that will patch the very same module that provides the
>> relocation destination -- that is, it was never intended to reference
>> through one of these klp-relocations unless it resolved to a live
>> module.
>>
>> On the other hand, when writing the selftests, verifying against NULL
>> [1] provided 1) a quick sanity check that something was "cleared" and 2)
>> protected the machine against said foot-gun.
>>
>> [1] https://github.com/joe-lawrence/klp-convert-tree/commit/643acbb8f4c0240030b45b64a542d126370d3e6c
>
> I don't quite follow the foot-gun here. What's the failure mode?
>
Kpatch-build, for better or worse, hides the potential problem. A
typical kpatch scenario would be:
1. A patch modifies module foo's function bar(), which references
symbols local to module foo
2. Kpatch-build creates a livepatch .ko with klp-relocations in the
modified bar() to foo's symbols
3. When loaded, modified bar() code that references through its
klp-relocations to module foo will only ever be active when foo is
loaded, i.e. when the original bar() redirects to the livepatch version.
However, writing source-based livepatches (like the kselftests) offers a
lot more freedom. There is no implicit guarantee from (3) that the
module is loaded. One could reference klp-relocations from anywhere in
the livepatch module.
For example, in my test_klp_convert1.c test case, I have a livepatch
module with a sysfs interface (print_debug_set()) that allows the test
bash script to force the module to references through its
klp-relocations (print_static_strings()):
...
static void print_string(const char *label, const char *str)
{
if (str)
pr_info("%s: %s\n", label, str);
}
...
static noinline void print_static_strings(void)
{
print_string("klp_string.12345", klp_string_a);
print_string("klp_string.67890", klp_string_b);
}
/* provide a sysfs handle to invoke debug functions */
static int print_debug;
static int print_debug_set(const char *val, const struct kernel_param *kp)
{
print_static_strings();
return 0;
}
static const struct kernel_param_ops print_debug_ops = {
.set = print_debug_set,
.get = param_get_int,
};
When I first wrote test_klp_convert1.c, I did not have wrappers like
print_string(), I simply referenced the symbols directly and send them
to pr_info as "%s" string formatting options.
You can probably see where this is going when I unloaded the module that
provided klp_string_a, klp_string_b, etc. and invoked the sysfs handle.
The stale relocation values point to ... somewhere we shouldn't try to
reference any more.
Perhaps I'm too paranoid about that possibility, but by actually
clearing the values in the relocations on module removal, one could
check them and try to guard against dangling pointer (dangling
relocation?) references.
> [...]
>
>>> These approaches don't look better to me. But I am ok
>>> with any of them. Please just let me know which one is
>>> most preferable:
>>>
>>> a. current version;
>>> b. clear_ undo everything of apply_ (the sample code
>>> above)
>>> c. clear_ undo R_PPC_REL24, but _redo_ everything
>>> of apply_ for other ELF64_R_TYPEs. (should be
>>> clearer code than option b).
>>>
>>
>> This was my attempt at combining and slightly refactoring the power64
>> version. There is so much going on here I was tempted to split off it
>> into separate value assignment and write functions. Some changes I
>> liked, but I wasn't all too happy with the result. Also, as you
>> mention, completely undoing R_PPC_REL24 is less than trivial... for this
>> arch, there are basically three major tasks:
>>
>> 1) calculate the new value, including range checking
>> 2) special constructs created by restore_r2 / create_stub
>> 3) writing out the value
>>
>> and many cases are similar, but subtly different enough to avoid easy
>> code consolidation.
>
> Thanks for exploring this direction. I guess this part won't be perfect
> anyway.
>
> PS: While we discuss a solution for ppc64, how about we ship the
> fix for other archs first? I think there are only a few small things to
> be addressed.
>
Yeah, the x86_64 version looks a lot simpler and closer to being done.
Though I believe that Petr would prefer a complete solution, but I'll
let him speak to that.
Alternatively, we could roll this into the klp-convert-tree as an early
patch (or separate branch), where development could continue on the
ppc64le and s390x arches as needed. I'll caution that progress is
rather slow on the entire patchset, so it may remain out of tree for
quite a while. :(
--
Joe
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7] livepatch: Clear relocation targets on a module removal
2023-01-05 11:19 ` Petr Mladek
@ 2023-01-05 16:53 ` Song Liu
2023-01-05 18:09 ` Joe Lawrence
0 siblings, 1 reply; 17+ messages in thread
From: Song Liu @ 2023-01-05 16:53 UTC (permalink / raw)
To: Petr Mladek
Cc: live-patching, jpoimboe, jikos, joe.lawrence, Miroslav Benes,
Josh Poimboeuf
On Thu, Jan 5, 2023 at 3:20 AM Petr Mladek <pmladek@suse.com> wrote:
[...]
>
> The ideal solution would be to add checks into apply_relocated_add().
> It would make it more robust. In that case, clear_relocated_add()
> would need to clear everything.
>
> But this is not the case on powerpc and s390 at the moment.
> In this case, I suggest to clear only relocations that
> are checked in apply_relocated_add().
>
> But it should be done without duplicating the code.
>
> It would actually make sense to compute the value that was
> used in apply_relocated_add() and check that we are clearing
> the value. If we try to clear some other value than we
> probably do something wrong.
>
> This might actually be a solution. We could compute
> the value in both situations. Then we could have
> a common function for writing.
>
> This write function would check that it replaces zero
> with the value in apply_relocate_add() and that it replaces
> the value with zero in clear_relocate_add().
I like this idea. But I am not quite sure whether we can do it
for all those complicated cases.
Btw: I am confused with this one:
case R_PPC64_REL16_HA:
/* Subtract location pointer */
value -= (unsigned long)location;
value = ((value + 0x8000) >> 16);
*((uint16_t *) location)
= (*((uint16_t *) location) & ~0xffff)
| (value & 0xffff);
break;
(*((uint16_t *) location) & ~0xffff) should always be zero, no?
Thanks,
Song
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7] livepatch: Clear relocation targets on a module removal
2023-01-05 15:05 ` Joe Lawrence
@ 2023-01-05 17:11 ` Song Liu
2023-01-06 13:02 ` Miroslav Benes
1 sibling, 0 replies; 17+ messages in thread
From: Song Liu @ 2023-01-05 17:11 UTC (permalink / raw)
To: Joe Lawrence
Cc: Petr Mladek, live-patching, jpoimboe, jikos, Miroslav Benes,
Josh Poimboeuf
On Thu, Jan 5, 2023 at 7:05 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> On 1/5/23 00:59, Song Liu wrote:
> > On Wed, Jan 4, 2023 at 3:12 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> >>
> >>
> >> Stepping back, this feature is definitely foot-gun capable.
> >> Kpatch-build would expect that klp-relocations would only ever be needed
> >> in code that will patch the very same module that provides the
> >> relocation destination -- that is, it was never intended to reference
> >> through one of these klp-relocations unless it resolved to a live
> >> module.
> >>
> >> On the other hand, when writing the selftests, verifying against NULL
> >> [1] provided 1) a quick sanity check that something was "cleared" and 2)
> >> protected the machine against said foot-gun.
> >>
> >> [1] https://github.com/joe-lawrence/klp-convert-tree/commit/643acbb8f4c0240030b45b64a542d126370d3e6c
> >
> > I don't quite follow the foot-gun here. What's the failure mode?
> >
>
> Kpatch-build, for better or worse, hides the potential problem. A
> typical kpatch scenario would be:
>
> 1. A patch modifies module foo's function bar(), which references
> symbols local to module foo
>
> 2. Kpatch-build creates a livepatch .ko with klp-relocations in the
> modified bar() to foo's symbols
>
> 3. When loaded, modified bar() code that references through its
> klp-relocations to module foo will only ever be active when foo is
> loaded, i.e. when the original bar() redirects to the livepatch version.
>
> However, writing source-based livepatches (like the kselftests) offers a
> lot more freedom. There is no implicit guarantee from (3) that the
> module is loaded. One could reference klp-relocations from anywhere in
> the livepatch module.
>
> For example, in my test_klp_convert1.c test case, I have a livepatch
> module with a sysfs interface (print_debug_set()) that allows the test
> bash script to force the module to references through its
> klp-relocations (print_static_strings()):
>
> ...
> static void print_string(const char *label, const char *str)
> {
> if (str)
> pr_info("%s: %s\n", label, str);
> }
> ...
> static noinline void print_static_strings(void)
> {
> print_string("klp_string.12345", klp_string_a);
> print_string("klp_string.67890", klp_string_b);
> }
>
> /* provide a sysfs handle to invoke debug functions */
> static int print_debug;
> static int print_debug_set(const char *val, const struct kernel_param *kp)
> {
> print_static_strings();
>
> return 0;
> }
> static const struct kernel_param_ops print_debug_ops = {
> .set = print_debug_set,
> .get = param_get_int,
> };
>
>
> When I first wrote test_klp_convert1.c, I did not have wrappers like
> print_string(), I simply referenced the symbols directly and send them
> to pr_info as "%s" string formatting options.
>
> You can probably see where this is going when I unloaded the module that
> provided klp_string_a, klp_string_b, etc. and invoked the sysfs handle.
> The stale relocation values point to ... somewhere we shouldn't try to
> reference any more.
Thanks for the explanation.
> Perhaps I'm too paranoid about that possibility, but by actually
> clearing the values in the relocations on module removal, one could
> check them and try to guard against dangling pointer (dangling
> relocation?) references.
I personally don't worry too much about this issue. But clearing the
relocations seems to be a good idea.
Thanks,
Song
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7] livepatch: Clear relocation targets on a module removal
2023-01-05 16:53 ` Song Liu
@ 2023-01-05 18:09 ` Joe Lawrence
0 siblings, 0 replies; 17+ messages in thread
From: Joe Lawrence @ 2023-01-05 18:09 UTC (permalink / raw)
To: Song Liu, Petr Mladek
Cc: live-patching, jpoimboe, jikos, Miroslav Benes, Josh Poimboeuf
On 1/5/23 11:53, Song Liu wrote:
> Btw: I am confused with this one:
>
> case R_PPC64_REL16_HA:
> /* Subtract location pointer */
> value -= (unsigned long)location;
> value = ((value + 0x8000) >> 16);
> *((uint16_t *) location)
> = (*((uint16_t *) location) & ~0xffff)
> | (value & 0xffff);
> break;
>
> (*((uint16_t *) location) & ~0xffff) should always be zero, no?
It looks like a lot of extra read/bit twiddling to do:
*(uint16_t *) location = value;
or am I missing a corner case that this handles?
--
Joe
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7] livepatch: Clear relocation targets on a module removal
2023-01-05 15:05 ` Joe Lawrence
2023-01-05 17:11 ` Song Liu
@ 2023-01-06 13:02 ` Miroslav Benes
2023-01-06 16:26 ` Petr Mladek
1 sibling, 1 reply; 17+ messages in thread
From: Miroslav Benes @ 2023-01-06 13:02 UTC (permalink / raw)
To: Joe Lawrence
Cc: Song Liu, Petr Mladek, live-patching, jpoimboe, jikos, Josh Poimboeuf
Hi
On Thu, 5 Jan 2023, Joe Lawrence wrote:
> On 1/5/23 00:59, Song Liu wrote:
> > On Wed, Jan 4, 2023 at 3:12 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> >>
> >>
> >> Stepping back, this feature is definitely foot-gun capable.
> >> Kpatch-build would expect that klp-relocations would only ever be needed
> >> in code that will patch the very same module that provides the
> >> relocation destination -- that is, it was never intended to reference
> >> through one of these klp-relocations unless it resolved to a live
> >> module.
> >>
> >> On the other hand, when writing the selftests, verifying against NULL
> >> [1] provided 1) a quick sanity check that something was "cleared" and 2)
> >> protected the machine against said foot-gun.
> >>
> >> [1] https://github.com/joe-lawrence/klp-convert-tree/commit/643acbb8f4c0240030b45b64a542d126370d3e6c
> >
> > I don't quite follow the foot-gun here. What's the failure mode?
> >
>
> Kpatch-build, for better or worse, hides the potential problem. A
> typical kpatch scenario would be:
>
> 1. A patch modifies module foo's function bar(), which references
> symbols local to module foo
>
> 2. Kpatch-build creates a livepatch .ko with klp-relocations in the
> modified bar() to foo's symbols
>
> 3. When loaded, modified bar() code that references through its
> klp-relocations to module foo will only ever be active when foo is
> loaded, i.e. when the original bar() redirects to the livepatch version.
>
> However, writing source-based livepatches (like the kselftests) offers a
> lot more freedom. There is no implicit guarantee from (3) that the
> module is loaded. One could reference klp-relocations from anywhere in
> the livepatch module.
Yes, on the other hand the approach you describe above seems to be the
only reasonable one in my opinion. The rest might be considered a bug.
Foot-gun as you say. I am not sure if we can do anything about it.
> > [...]
> >
> >>> These approaches don't look better to me. But I am ok
> >>> with any of them. Please just let me know which one is
> >>> most preferable:
> >>>
> >>> a. current version;
> >>> b. clear_ undo everything of apply_ (the sample code
> >>> above)
> >>> c. clear_ undo R_PPC_REL24, but _redo_ everything
> >>> of apply_ for other ELF64_R_TYPEs. (should be
> >>> clearer code than option b).
> >>>
> >>
> >> This was my attempt at combining and slightly refactoring the power64
> >> version. There is so much going on here I was tempted to split off it
> >> into separate value assignment and write functions. Some changes I
> >> liked, but I wasn't all too happy with the result. Also, as you
> >> mention, completely undoing R_PPC_REL24 is less than trivial... for this
> >> arch, there are basically three major tasks:
> >>
> >> 1) calculate the new value, including range checking
> >> 2) special constructs created by restore_r2 / create_stub
> >> 3) writing out the value
> >>
> >> and many cases are similar, but subtly different enough to avoid easy
> >> code consolidation.
> >
> > Thanks for exploring this direction. I guess this part won't be perfect
> > anyway.
> >
> > PS: While we discuss a solution for ppc64, how about we ship the
> > fix for other archs first? I think there are only a few small things to
> > be addressed.
> >
>
> Yeah, the x86_64 version looks a lot simpler and closer to being done.
> Though I believe that Petr would prefer a complete solution, but I'll
> let him speak to that.
I cannot speak for Petr, but I think it might be easier to split it given
the situation. Then we can involve arch maintainers for ppc64le because
they might have a preference with respect to a, b, c options above.
Petr, what do you think?
Miroslav
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7] livepatch: Clear relocation targets on a module removal
2023-01-06 13:02 ` Miroslav Benes
@ 2023-01-06 16:26 ` Petr Mladek
2023-01-06 16:51 ` Song Liu
0 siblings, 1 reply; 17+ messages in thread
From: Petr Mladek @ 2023-01-06 16:26 UTC (permalink / raw)
To: Miroslav Benes
Cc: Joe Lawrence, Song Liu, live-patching, jpoimboe, jikos, Josh Poimboeuf
On Fri 2023-01-06 14:02:27, Miroslav Benes wrote:
> Hi
>
> On Thu, 5 Jan 2023, Joe Lawrence wrote:
>
> > On 1/5/23 00:59, Song Liu wrote:
> > > On Wed, Jan 4, 2023 at 3:12 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> > >>
> > >>
> > >> Stepping back, this feature is definitely foot-gun capable.
> > >> Kpatch-build would expect that klp-relocations would only ever be needed
> > >> in code that will patch the very same module that provides the
> > >> relocation destination -- that is, it was never intended to reference
> > >> through one of these klp-relocations unless it resolved to a live
> > >> module.
> > >>
> > >> On the other hand, when writing the selftests, verifying against NULL
> > >> [1] provided 1) a quick sanity check that something was "cleared" and 2)
> > >> protected the machine against said foot-gun.
> > >>
> > >> [1] https://github.com/joe-lawrence/klp-convert-tree/commit/643acbb8f4c0240030b45b64a542d126370d3e6c
> > >
> > > I don't quite follow the foot-gun here. What's the failure mode?
> > >
> >
> > Kpatch-build, for better or worse, hides the potential problem. A
> > typical kpatch scenario would be:
> >
> > 1. A patch modifies module foo's function bar(), which references
> > symbols local to module foo
> >
> > 2. Kpatch-build creates a livepatch .ko with klp-relocations in the
> > modified bar() to foo's symbols
> >
> > 3. When loaded, modified bar() code that references through its
> > klp-relocations to module foo will only ever be active when foo is
> > loaded, i.e. when the original bar() redirects to the livepatch version.
> >
> > However, writing source-based livepatches (like the kselftests) offers a
> > lot more freedom. There is no implicit guarantee from (3) that the
> > module is loaded. One could reference klp-relocations from anywhere in
> > the livepatch module.
>
> Yes, on the other hand the approach you describe above seems to be the
> only reasonable one in my opinion. The rest might be considered a bug.
> Foot-gun as you say. I am not sure if we can do anything about it.
I agree that using an address from a module when the module is not
loaded is a bug. The kernel might still crash even when we clear
the addresses. But at least it can't be used to read information
from an "arbitrary" address.
It means that clearing the relocations is nice to have.
BTW: I originally understood the "foot-gun" in a more generic way.
Modyfying "elf" sections feels like a surgery. As such, it has
to be done carefully.
> > > [...]
> > >
> > >>> These approaches don't look better to me. But I am ok
> > >>> with any of them. Please just let me know which one is
> > >>> most preferable:
> > >>>
> > >>> a. current version;
> > >>> b. clear_ undo everything of apply_ (the sample code
> > >>> above)
> > >>> c. clear_ undo R_PPC_REL24, but _redo_ everything
> > >>> of apply_ for other ELF64_R_TYPEs. (should be
> > >>> clearer code than option b).
> > >>>
> > >>
> > >> This was my attempt at combining and slightly refactoring the power64
> > >> version. There is so much going on here I was tempted to split off it
> > >> into separate value assignment and write functions. Some changes I
> > >> liked, but I wasn't all too happy with the result. Also, as you
> > >> mention, completely undoing R_PPC_REL24 is less than trivial... for this
> > >> arch, there are basically three major tasks:
> > >>
> > >> 1) calculate the new value, including range checking
> > >> 2) special constructs created by restore_r2 / create_stub
> > >> 3) writing out the value
> > >>
> > >> and many cases are similar, but subtly different enough to avoid easy
> > >> code consolidation.
> > >
> > > Thanks for exploring this direction. I guess this part won't be perfect
> > > anyway.
> > >
> > > PS: While we discuss a solution for ppc64, how about we ship the
> > > fix for other archs first? I think there are only a few small things to
> > > be addressed.
> > >
> >
> > Yeah, the x86_64 version looks a lot simpler and closer to being done.
> > Though I believe that Petr would prefer a complete solution, but I'll
> > let him speak to that.
>
> I cannot speak for Petr, but I think it might be easier to split it given
> the situation. Then we can involve arch maintainers for ppc64le because
> they might have a preference with respect to a, b, c options above.
>
> Petr, what do you think?
I am fine with solving each architecture in a separate patch or
patchset. It would make it easier, especially, for arch maintainers.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7] livepatch: Clear relocation targets on a module removal
2023-01-06 16:26 ` Petr Mladek
@ 2023-01-06 16:51 ` Song Liu
0 siblings, 0 replies; 17+ messages in thread
From: Song Liu @ 2023-01-06 16:51 UTC (permalink / raw)
To: Petr Mladek
Cc: Miroslav Benes, Joe Lawrence, live-patching, jpoimboe, jikos,
Josh Poimboeuf
On Fri, Jan 6, 2023 at 8:26 AM Petr Mladek <pmladek@suse.com> wrote:
>
[...]
> > > >
> > > > PS: While we discuss a solution for ppc64, how about we ship the
> > > > fix for other archs first? I think there are only a few small things to
> > > > be addressed.
> > > >
> > >
> > > Yeah, the x86_64 version looks a lot simpler and closer to being done.
> > > Though I believe that Petr would prefer a complete solution, but I'll
> > > let him speak to that.
> >
> > I cannot speak for Petr, but I think it might be easier to split it given
> > the situation. Then we can involve arch maintainers for ppc64le because
> > they might have a preference with respect to a, b, c options above.
> >
> > Petr, what do you think?
>
> I am fine with solving each architecture in a separate patch or
> patchset. It would make it easier, especially, for arch maintainers.
I will send v8 w/o ppc changes after addressing feedback for generic
and x86 code.
Thanks,
Song
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-01-06 16:51 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-14 17:40 [PATCH v7] livepatch: Clear relocation targets on a module removal Song Liu
2023-01-03 17:00 ` Song Liu
2023-01-03 22:39 ` Joe Lawrence
2023-01-03 23:29 ` Song Liu
2023-01-04 10:26 ` Petr Mladek
2023-01-04 17:34 ` Song Liu
2023-01-04 23:12 ` Joe Lawrence
2023-01-05 5:59 ` Song Liu
2023-01-05 15:05 ` Joe Lawrence
2023-01-05 17:11 ` Song Liu
2023-01-06 13:02 ` Miroslav Benes
2023-01-06 16:26 ` Petr Mladek
2023-01-06 16:51 ` Song Liu
2023-01-05 11:19 ` Petr Mladek
2023-01-05 16:53 ` Song Liu
2023-01-05 18:09 ` Joe Lawrence
2023-01-05 13:03 ` Petr Mladek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.