Message-ID: <20160114050730.GE980@packer-debian-8-amd64.digitalocean.com>
Date:	Thu, 14 Jan 2016 00:07:30 -0500
From:	Jessica Yu <jeyu@...hat.com>
To:	Josh Poimboeuf <jpoimboe@...hat.com>
Cc:	Rusty Russell <rusty@...tcorp.com.au>,
	Seth Jennings <sjenning@...hat.com>,
	Jiri Kosina <jikos@...nel.org>,
	Vojtech Pavlik <vojtech@...e.com>,
	Jonathan Corbet <corbet@....net>,
	Miroslav Benes <mbenes@...e.cz>, linux-api@...r.kernel.org,
	live-patching@...r.kernel.org, x86@...nel.org,
	linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
	linux-doc@...r.kernel.org
Subject: Re: livepatch: reuse module loader code to write relocations

+++ Josh Poimboeuf [11/01/16 21:05 -0600]:
>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 <jeyu@...hat.com>
>> >>---
>> >> 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 <sjenning@...hat.com>
>> >>- * 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 <http://www.gnu.org/licenses/>.
>> >>- */
>> >>-
>> >>-#include <linux/module.h>
>> >>-#include <linux/uaccess.h>
>> >>-#include <asm/elf.h>
>> >>-#include <asm/livepatch.h>
>> >>-
>> >>-/**
>> >>- * 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:
>> >
>> > https://lkml.kernel.org/g/20151210213328.GA6553@packer-debian-8-amd64.digitalocean.com
>> >
>> >Because:
>> >
>> >- st_value is easily viewable in readelf
>> >- st_other has some arch-specific uses
>> >
>> >And another reason not previously discussed:
>> >
>> >- st_other is an unsigned char, which limits sympos to values < 64
>>
>> I originally wanted to encode the symbol position in st_value, but
>> I've discovered that since st_value is overwritten once the
>> symbol is (first) resolved, we no longer have the symbol position if we
>> need to resolve the symbols again (so we wouldn't be able to resolve
>> them). This could happen when we patch a module that loads and
>> unloads more than once for example.
>
>Ah, good point.
>
>> I chose st_other since it isn't touched in s390x kernel code nor in
>> x86 kernel code (it does get used in the x86 userspace reloc tool in
>> arch/x86/tools, which is why I left the first two bits alone for
>> ELF_ST_VISIBILITY, and the rest had undefined usage). However this
>> isn't the best approach and I'm afraid of stepping on other arch's
>> toes in future patches. Perhaps there is another field where we can
>> stuff the sympos in? st_size for instance doesn't seem to be touched
>> anywhere in the module loader. I don't know if I'd want to stuff
>> sympos in the sym name, there's enough going on there..
>
>I think st_other still isn't going to work because of possible arch
>conflicts and because of its small size (6 bits) otherwise.
>
>st_size could work.  It doesn't _seem_ to be used by the module code,
>though it's hard to confirm that for all arches.  But it makes me a
>little nervous to override that field.  For example, might it confuse
>some user-space tools out there, or the linker?
>
>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 ',<sympos>' to be consistent with the sysfs entries:
>
>  .klp.sym.vmlinux.symname,1

Sure, as long as commas aren't allowed in symbol names, that will work. :-)

Powered by blists - more mailing lists