From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964808AbcALJMU (ORCPT ); Tue, 12 Jan 2016 04:12:20 -0500 Received: from mx2.suse.de ([195.135.220.15]:35184 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933393AbcALJMK (ORCPT ); Tue, 12 Jan 2016 04:12:10 -0500 Date: Tue, 12 Jan 2016 10:12:07 +0100 From: Petr Mladek To: Josh Poimboeuf Cc: Jessica Yu , Rusty Russell , Seth Jennings , Jiri Kosina , Vojtech Pavlik , Jonathan Corbet , Miroslav Benes , linux-api@vger.kernel.org, live-patching@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org, linux-s390@vger.kernel.org, linux-doc@vger.kernel.org Subject: Re: livepatch: reuse module loader code to write relocations Message-ID: <20160112091207.GM731@pathway.suse.cz> References: <1452281304-28618-1-git-send-email-jeyu@redhat.com> <1452281304-28618-5-git-send-email-jeyu@redhat.com> <20160111213349.GB17874@treble.redhat.com> <20160111223513.GA10796@packer-debian-8-amd64.digitalocean.com> <20160112030552.GC17874@treble.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160112030552.GC17874@treble.redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon 2016-01-11 21:05:52, Josh Poimboeuf wrote: > On Mon, Jan 11, 2016 at 05:35:13PM -0500, Jessica Yu wrote: > > +++ Josh Poimboeuf [11/01/16 15:33 -0600]: > > >On Fri, Jan 08, 2016 at 02:28:22PM -0500, Jessica Yu wrote: > > >>Reuse module loader code to write relocations, thereby eliminating the need > > >>for architecture specific relocation code in livepatch. Namely, we reuse > > >>apply_relocate_add() in the module loader to write relocations instead of > > >>duplicating functionality in livepatch's klp_write_module_reloc(). To apply > > >>relocation sections, remaining SHN_LIVEPATCH symbols referenced by relocs > > >>are resolved and then apply_relocate_add() is called to apply those > > >>relocations. > > >> > > >>In addition, remove x86 livepatch relocation code. It is no longer needed > > >>since symbol resolution and relocation work have been offloaded to module > > >>loader. > > >> > > >>Signed-off-by: Jessica Yu > > >>--- > > >> arch/x86/include/asm/livepatch.h | 2 - > > >> arch/x86/kernel/Makefile | 1 - > > >> arch/x86/kernel/livepatch.c | 70 --------------------------- > > >> include/linux/livepatch.h | 33 +++++-------- > > >> kernel/livepatch/core.c | 101 +++++++++++++++++++-------------------- > > >> 5 files changed, 62 insertions(+), 145 deletions(-) > > >> delete mode 100644 arch/x86/kernel/livepatch.c > > >> > > >>diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h > > >>index 19c099a..7312e25 100644 > > >>--- a/arch/x86/include/asm/livepatch.h > > >>+++ b/arch/x86/include/asm/livepatch.h > > >>@@ -33,8 +33,6 @@ static inline int klp_check_compiler_support(void) > > >> #endif > > >> return 0; > > >> } > > >>-int klp_write_module_reloc(struct module *mod, unsigned long type, > > >>- unsigned long loc, unsigned long value); > > >> > > >> static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip) > > >> { > > >>diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile > > >>index b1b78ff..c5e9a5c 100644 > > >>--- a/arch/x86/kernel/Makefile > > >>+++ b/arch/x86/kernel/Makefile > > >>@@ -67,7 +67,6 @@ obj-$(CONFIG_X86_MPPARSE) += mpparse.o > > >> obj-y += apic/ > > >> obj-$(CONFIG_X86_REBOOTFIXUPS) += reboot_fixups_32.o > > >> obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o > > >>-obj-$(CONFIG_LIVEPATCH) += livepatch.o > > >> obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o > > >> obj-$(CONFIG_FTRACE_SYSCALLS) += ftrace.o > > >> obj-$(CONFIG_X86_TSC) += trace_clock.o > > >>diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c > > >>deleted file mode 100644 > > >>index 92fc1a5..0000000 > > >>--- a/arch/x86/kernel/livepatch.c > > >>+++ /dev/null > > >>@@ -1,70 +0,0 @@ > > >>-/* > > >>- * livepatch.c - x86-specific Kernel Live Patching Core > > >>- * > > >>- * Copyright (C) 2014 Seth Jennings > > >>- * Copyright (C) 2014 SUSE > > >>- * > > >>- * This program is free software; you can redistribute it and/or > > >>- * modify it under the terms of the GNU General Public License > > >>- * as published by the Free Software Foundation; either version 2 > > >>- * of the License, or (at your option) any later version. > > >>- * > > >>- * This program is distributed in the hope that it will be useful, > > >>- * but WITHOUT ANY WARRANTY; without even the implied warranty of > > >>- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > >>- * GNU General Public License for more details. > > >>- * > > >>- * You should have received a copy of the GNU General Public License > > >>- * along with this program; if not, see . > > >>- */ > > >>- > > >>-#include > > >>-#include > > >>-#include > > >>-#include > > >>- > > >>-/** > > >>- * klp_write_module_reloc() - write a relocation in a module > > >>- * @mod: module in which the section to be modified is found > > >>- * @type: ELF relocation type (see asm/elf.h) > > >>- * @loc: address that the relocation should be written to > > >>- * @value: relocation value (sym address + addend) > > >>- * > > >>- * This function writes a relocation to the specified location for > > >>- * a particular module. > > >>- */ > > >>-int klp_write_module_reloc(struct module *mod, unsigned long type, > > >>- unsigned long loc, unsigned long value) > > >>-{ > > >>- size_t size = 4; > > >>- unsigned long val; > > >>- unsigned long core = (unsigned long)mod->core_layout.base; > > >>- unsigned long core_size = mod->core_layout.size; > > >>- > > >>- switch (type) { > > >>- case R_X86_64_NONE: > > >>- return 0; > > >>- case R_X86_64_64: > > >>- val = value; > > >>- size = 8; > > >>- break; > > >>- case R_X86_64_32: > > >>- val = (u32)value; > > >>- break; > > >>- case R_X86_64_32S: > > >>- val = (s32)value; > > >>- break; > > >>- case R_X86_64_PC32: > > >>- val = (u32)(value - loc); > > >>- break; > > >>- default: > > >>- /* unsupported relocation type */ > > >>- return -EINVAL; > > >>- } > > >>- > > >>- if (loc < core || loc >= core + core_size) > > >>- /* loc does not point to any symbol inside the module */ > > >>- return -EINVAL; > > >>- > > >>- return probe_kernel_write((void *)loc, &val, size); > > >>-} > > >>diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > > >>index a882865..2f12ce7 100644 > > >>--- a/include/linux/livepatch.h > > >>+++ b/include/linux/livepatch.h > > >>@@ -65,27 +65,8 @@ struct klp_func { > > >> }; > > >> > > >> /** > > >>- * struct klp_reloc - relocation structure for live patching > > >>- * @loc: address where the relocation will be written > > >>- * @sympos: position in kallsyms to disambiguate symbols (optional) > > >>- * @type: ELF relocation type > > >>- * @name: name of the referenced symbol (for lookup/verification) > > >>- * @addend: offset from the referenced symbol > > >>- * @external: symbol is either exported or within the live patch module itself > > >>- */ > > >>-struct klp_reloc { > > >>- unsigned long loc; > > >>- unsigned long sympos; > > >>- unsigned long type; > > >>- const char *name; > > >>- int addend; > > >>- int external; > > >>-}; > > >>- > > >>-/** > > >> * struct klp_object - kernel object structure for live patching > > >> * @name: module name (or NULL for vmlinux) > > >>- * @relocs: relocation entries to be applied at load time > > >> * @funcs: function entries for functions to be patched in the object > > >> * @kobj: kobject for sysfs resources > > >> * @mod: kernel module associated with the patched object > > >>@@ -95,7 +76,6 @@ struct klp_reloc { > > >> struct klp_object { > > >> /* external */ > > >> const char *name; > > >>- struct klp_reloc *relocs; > > >> struct klp_func *funcs; > > >> > > >> /* internal */ > > >>@@ -123,6 +103,19 @@ struct klp_patch { > > >> enum klp_state state; > > >> }; > > >> > > >>+/* > > >>+ * Livepatch-specific symbols and relocation > > >>+ * sections are prefixed with a tag: > > >>+ * .klp.rel. for relocation sections > > >>+ * .klp.sym. for livepatch symbols > > >>+ */ > > >>+#define KLP_TAG_LEN 9 > > >>+/* > > >>+ * Livepatch-specific bits for specifying symbol > > >>+ * positions in the Elf_Sym st_other field > > >>+ */ > > >>+#define KLP_SYMPOS(o) (o >> 2) & 0xff > > >>+ > > >Can st_value be used instead? I think we ended up deciding that would > > >be better: > > Maybe encoding it in the sym name is the safest bet. Why not, we've > already got a bunch of other stuff there anyway :-) One idea would be > to append it with ',' to be consistent with the sysfs entries: > > .klp.sym.vmlinux.symname,1 This looks like the most safe solution to me as well. Best Regards, Petr From mboxrd@z Thu Jan 1 00:00:00 1970 From: Petr Mladek Subject: Re: livepatch: reuse module loader code to write relocations Date: Tue, 12 Jan 2016 10:12:07 +0100 Message-ID: <20160112091207.GM731@pathway.suse.cz> References: <1452281304-28618-1-git-send-email-jeyu@redhat.com> <1452281304-28618-5-git-send-email-jeyu@redhat.com> <20160111213349.GB17874@treble.redhat.com> <20160111223513.GA10796@packer-debian-8-amd64.digitalocean.com> <20160112030552.GC17874@treble.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20160112030552.GC17874-8wJ5/zUtDR0XGNroddHbYwC/G2K4zDHf@public.gmane.org> Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Josh Poimboeuf Cc: Jessica Yu , Rusty Russell , Seth Jennings , Jiri Kosina , Vojtech Pavlik , Jonathan Corbet , Miroslav Benes , linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, live-patching-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-s390-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-api@vger.kernel.org On Mon 2016-01-11 21:05:52, Josh Poimboeuf wrote: > On Mon, Jan 11, 2016 at 05:35:13PM -0500, Jessica Yu wrote: > > +++ Josh Poimboeuf [11/01/16 15:33 -0600]: > > >On Fri, Jan 08, 2016 at 02:28:22PM -0500, Jessica Yu wrote: > > >>Reuse module loader code to write relocations, thereby eliminating the need > > >>for architecture specific relocation code in livepatch. Namely, we reuse > > >>apply_relocate_add() in the module loader to write relocations instead of > > >>duplicating functionality in livepatch's klp_write_module_reloc(). To apply > > >>relocation sections, remaining SHN_LIVEPATCH symbols referenced by relocs > > >>are resolved and then apply_relocate_add() is called to apply those > > >>relocations. > > >> > > >>In addition, remove x86 livepatch relocation code. It is no longer needed > > >>since symbol resolution and relocation work have been offloaded to module > > >>loader. > > >> > > >>Signed-off-by: Jessica Yu > > >>--- > > >> arch/x86/include/asm/livepatch.h | 2 - > > >> arch/x86/kernel/Makefile | 1 - > > >> arch/x86/kernel/livepatch.c | 70 --------------------------- > > >> include/linux/livepatch.h | 33 +++++-------- > > >> kernel/livepatch/core.c | 101 +++++++++++++++++++-------------------- > > >> 5 files changed, 62 insertions(+), 145 deletions(-) > > >> delete mode 100644 arch/x86/kernel/livepatch.c > > >> > > >>diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h > > >>index 19c099a..7312e25 100644 > > >>--- a/arch/x86/include/asm/livepatch.h > > >>+++ b/arch/x86/include/asm/livepatch.h > > >>@@ -33,8 +33,6 @@ static inline int klp_check_compiler_support(void) > > >> #endif > > >> return 0; > > >> } > > >>-int klp_write_module_reloc(struct module *mod, unsigned long type, > > >>- unsigned long loc, unsigned long value); > > >> > > >> static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip) > > >> { > > >>diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile > > >>index b1b78ff..c5e9a5c 100644 > > >>--- a/arch/x86/kernel/Makefile > > >>+++ b/arch/x86/kernel/Makefile > > >>@@ -67,7 +67,6 @@ obj-$(CONFIG_X86_MPPARSE) += mpparse.o > > >> obj-y += apic/ > > >> obj-$(CONFIG_X86_REBOOTFIXUPS) += reboot_fixups_32.o > > >> obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o > > >>-obj-$(CONFIG_LIVEPATCH) += livepatch.o > > >> obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o > > >> obj-$(CONFIG_FTRACE_SYSCALLS) += ftrace.o > > >> obj-$(CONFIG_X86_TSC) += trace_clock.o > > >>diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c > > >>deleted file mode 100644 > > >>index 92fc1a5..0000000 > > >>--- a/arch/x86/kernel/livepatch.c > > >>+++ /dev/null > > >>@@ -1,70 +0,0 @@ > > >>-/* > > >>- * livepatch.c - x86-specific Kernel Live Patching Core > > >>- * > > >>- * Copyright (C) 2014 Seth Jennings > > >>- * Copyright (C) 2014 SUSE > > >>- * > > >>- * This program is free software; you can redistribute it and/or > > >>- * modify it under the terms of the GNU General Public License > > >>- * as published by the Free Software Foundation; either version 2 > > >>- * of the License, or (at your option) any later version. > > >>- * > > >>- * This program is distributed in the hope that it will be useful, > > >>- * but WITHOUT ANY WARRANTY; without even the implied warranty of > > >>- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > >>- * GNU General Public License for more details. > > >>- * > > >>- * You should have received a copy of the GNU General Public License > > >>- * along with this program; if not, see . > > >>- */ > > >>- > > >>-#include > > >>-#include > > >>-#include > > >>-#include > > >>- > > >>-/** > > >>- * klp_write_module_reloc() - write a relocation in a module > > >>- * @mod: module in which the section to be modified is found > > >>- * @type: ELF relocation type (see asm/elf.h) > > >>- * @loc: address that the relocation should be written to > > >>- * @value: relocation value (sym address + addend) > > >>- * > > >>- * This function writes a relocation to the specified location for > > >>- * a particular module. > > >>- */ > > >>-int klp_write_module_reloc(struct module *mod, unsigned long type, > > >>- unsigned long loc, unsigned long value) > > >>-{ > > >>- size_t size = 4; > > >>- unsigned long val; > > >>- unsigned long core = (unsigned long)mod->core_layout.base; > > >>- unsigned long core_size = mod->core_layout.size; > > >>- > > >>- switch (type) { > > >>- case R_X86_64_NONE: > > >>- return 0; > > >>- case R_X86_64_64: > > >>- val = value; > > >>- size = 8; > > >>- break; > > >>- case R_X86_64_32: > > >>- val = (u32)value; > > >>- break; > > >>- case R_X86_64_32S: > > >>- val = (s32)value; > > >>- break; > > >>- case R_X86_64_PC32: > > >>- val = (u32)(value - loc); > > >>- break; > > >>- default: > > >>- /* unsupported relocation type */ > > >>- return -EINVAL; > > >>- } > > >>- > > >>- if (loc < core || loc >= core + core_size) > > >>- /* loc does not point to any symbol inside the module */ > > >>- return -EINVAL; > > >>- > > >>- return probe_kernel_write((void *)loc, &val, size); > > >>-} > > >>diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > > >>index a882865..2f12ce7 100644 > > >>--- a/include/linux/livepatch.h > > >>+++ b/include/linux/livepatch.h > > >>@@ -65,27 +65,8 @@ struct klp_func { > > >> }; > > >> > > >> /** > > >>- * struct klp_reloc - relocation structure for live patching > > >>- * @loc: address where the relocation will be written > > >>- * @sympos: position in kallsyms to disambiguate symbols (optional) > > >>- * @type: ELF relocation type > > >>- * @name: name of the referenced symbol (for lookup/verification) > > >>- * @addend: offset from the referenced symbol > > >>- * @external: symbol is either exported or within the live patch module itself > > >>- */ > > >>-struct klp_reloc { > > >>- unsigned long loc; > > >>- unsigned long sympos; > > >>- unsigned long type; > > >>- const char *name; > > >>- int addend; > > >>- int external; > > >>-}; > > >>- > > >>-/** > > >> * struct klp_object - kernel object structure for live patching > > >> * @name: module name (or NULL for vmlinux) > > >>- * @relocs: relocation entries to be applied at load time > > >> * @funcs: function entries for functions to be patched in the object > > >> * @kobj: kobject for sysfs resources > > >> * @mod: kernel module associated with the patched object > > >>@@ -95,7 +76,6 @@ struct klp_reloc { > > >> struct klp_object { > > >> /* external */ > > >> const char *name; > > >>- struct klp_reloc *relocs; > > >> struct klp_func *funcs; > > >> > > >> /* internal */ > > >>@@ -123,6 +103,19 @@ struct klp_patch { > > >> enum klp_state state; > > >> }; > > >> > > >>+/* > > >>+ * Livepatch-specific symbols and relocation > > >>+ * sections are prefixed with a tag: > > >>+ * .klp.rel. for relocation sections > > >>+ * .klp.sym. for livepatch symbols > > >>+ */ > > >>+#define KLP_TAG_LEN 9 > > >>+/* > > >>+ * Livepatch-specific bits for specifying symbol > > >>+ * positions in the Elf_Sym st_other field > > >>+ */ > > >>+#define KLP_SYMPOS(o) (o >> 2) & 0xff > > >>+ > > >Can st_value be used instead? I think we ended up deciding that would > > >be better: > > Maybe encoding it in the sym name is the safest bet. Why not, we've > already got a bunch of other stuff there anyway :-) One idea would be > to append it with ',' to be consistent with the sysfs entries: > > .klp.sym.vmlinux.symname,1 This looks like the most safe solution to me as well. Best Regards, Petr