Re: livepatch: reuse module loader code to write relocations
From: Jessica Yu
Date: Thu Jan 14 2016 - 00:07:43 EST
+++ 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@xxxxxxxxxx>
>>---
>> 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@xxxxxxxxxx>
>>- * 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>
>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. :-)