Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1280563 > unrolled thread

[RFC PATCH v2 4/6] livepatch: reuse module loader code to write relocations

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH v2 4/6] livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-12-01 05:30 +0100
    Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-12-01 09:50 +0100
    Re: [RFC PATCH v2 4/6] livepatch: reuse module loader code to write  relocations Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-08 19:40 +0100
      Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-12-09 20:20 +0100
        Re: livepatch: reuse module loader code to write relocations Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-10 15:30 +0100
          Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-12-10 22:40 +0100
            Re: livepatch: reuse module loader code to write relocations Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-10 22:50 +0100
              Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-12-10 23:10 +0100
    Re: [RFC PATCH v2 4/6] livepatch: reuse module loader code to write  relocations Minfei Huang <mnfhuang@gmail.com> - 2015-12-10 15:40 +0100
      Re: [RFC PATCH v2 4/6] livepatch: reuse module loader code to write  relocations Jiri Kosina <jikos@kernel.org> - 2015-12-10 21:00 +0100
        Re: [RFC PATCH v2 4/6] livepatch: reuse module loader code to write  relocations Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-10 22:20 +0100

#1280563 — [RFC PATCH v2 4/6] livepatch: reuse module loader code to write relocations

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@redhat.com>
---
 arch/x86/include/asm/livepatch.h |  2 -
 arch/x86/kernel/Makefile         |  1 -
 arch/x86/kernel/livepatch.c      | 91 --------------------------------------
 include/linux/livepatch.h        | 30 ++++++-------
 include/linux/module.h           |  6 +++
 kernel/livepatch/core.c          | 94 ++++++++++++++++++++++------------------
 6 files changed, 70 insertions(+), 154 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 d1d35cc..0000000
--- a/arch/x86/kernel/livepatch.c
+++ /dev/null
@@ -1,91 +0,0 @@
-/*
- * livepatch.c - x86-specific Kernel Live Patching Core
- *
- * Copyright (C) 2014 Seth Jennings <sjenning@redhat.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/cacheflush.h>
-#include <asm/page_types.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)
-{
-	int ret, numpages, size = 4;
-	bool readonly;
-	unsigned long val;
-	unsigned long core = (unsigned long)mod->module_core;
-	unsigned long core_size = mod->core_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;
-
-	readonly = false;
-
-#ifdef CONFIG_DEBUG_SET_MODULE_RONX
-	if (loc < core + mod->core_ro_size)
-		readonly = true;
-#endif
-
-	/* determine if the relocation spans a page boundary */
-	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
-
-	if (readonly)
-		set_memory_rw(loc & PAGE_MASK, numpages);
-
-	ret = probe_kernel_write((void *)loc, &val, size);
-
-	if (readonly)
-		set_memory_ro(loc & PAGE_MASK, numpages);
-
-	return ret;
-}
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 31db7a0..54f62a6 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -64,28 +64,21 @@ struct klp_func {
 };
 
 /**
- * struct klp_reloc - relocation structure for live patching
- * @loc:	address where the relocation will be written
- * @val:	address of the referenced symbol (optional,
- *		vmlinux	patches only)
- * @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_sec - relocation section struct for live patching
+ * @index:	Elf section index of the relocation section
+ * @name:	name of the relocation section
+ * @objname:	name of the object associated with the klp reloc section
  */
-struct klp_reloc {
-	unsigned long loc;
-	unsigned long val;
-	unsigned long type;
-	const char *name;
-	int addend;
-	int external;
+struct klp_reloc_sec {
+	unsigned int index;
+	char *name;
+	char *objname;
 };
 
 /**
  * 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
+ * @reloc_secs:	relocation sections 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 +88,7 @@ struct klp_reloc {
 struct klp_object {
 	/* external */
 	const char *name;
-	struct klp_reloc *relocs;
+	struct klp_reloc_sec *reloc_secs;
 	struct klp_func *funcs;
 
 	/* internal */
@@ -129,6 +122,9 @@ struct klp_patch {
 #define klp_for_each_func(obj, func) \
 	for (func = obj->funcs; func->old_name; func++)
 
+#define klp_for_each_reloc_sec(obj, reloc_sec) \
+	for (reloc_sec = obj->reloc_secs; reloc_sec->name; reloc_sec++)
+
 int klp_register_patch(struct klp_patch *);
 int klp_unregister_patch(struct klp_patch *);
 int klp_enable_patch(struct klp_patch *);
diff --git a/include/linux/module.h b/include/linux/module.h
index 9b46256..ea61147 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -777,9 +777,15 @@ extern int module_sysfs_initialized;
 #ifdef CONFIG_DEBUG_SET_MODULE_RONX
 extern void set_all_modules_text_rw(void);
 extern void set_all_modules_text_ro(void);
+extern void
+set_page_attributes(void *start, void *end,
+		    int (*set)(unsigned long start, int num_pages));
 #else
 static inline void set_all_modules_text_rw(void) { }
 static inline void set_all_modules_text_ro(void) { }
+static inline void
+set_page_attributes(void *start, void *end,
+		    int (*set)(unsigned long start, int num_pages)) { }
 #endif
 
 #ifdef CONFIG_GENERIC_BUG
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index db545cb..17b7278 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -28,6 +28,9 @@
 #include <linux/list.h>
 #include <linux/kallsyms.h>
 #include <linux/livepatch.h>
+#include <linux/elf.h>
+#include <asm/cacheflush.h>
+#include <linux/moduleloader.h>
 
 /**
  * struct klp_ops - structure for tracking registered ftrace ops structs
@@ -281,52 +284,48 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
 }
 
 static int klp_write_object_relocations(struct module *pmod,
-					struct klp_object *obj)
+					struct klp_object *obj,
+					struct klp_patch *patch)
 {
-	int ret;
-	struct klp_reloc *reloc;
+	int relindex, num_relas;
+	int i, ret = 0;
+	unsigned long addr;
+	char *symname;
+	struct klp_reloc_sec *reloc_sec;
+	Elf_Rela *rela;
+	Elf_Sym *sym, *symtab;
 
 	if (WARN_ON(!klp_is_object_loaded(obj)))
 		return -EINVAL;
 
-	if (WARN_ON(!obj->relocs))
-		return -EINVAL;
-
-	for (reloc = obj->relocs; reloc->name; reloc++) {
-		if (!klp_is_module(obj)) {
-
-#if defined(CONFIG_RANDOMIZE_BASE)
-			/* If KASLR has been enabled, adjust old value accordingly */
-			if (kaslr_enabled())
-				reloc->val += kaslr_offset();
-#endif
-			ret = klp_verify_vmlinux_symbol(reloc->name,
-							reloc->val);
-			if (ret)
-				return ret;
-		} else {
-			/* module, reloc->val needs to be discovered */
-			if (reloc->external)
-				ret = klp_find_external_symbol(pmod,
-							       reloc->name,
-							       &reloc->val);
-			else
-				ret = klp_find_object_symbol(obj->mod->name,
-							     reloc->name,
-							     &reloc->val);
-			if (ret)
-				return ret;
-		}
-		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
-					     reloc->val + reloc->addend);
-		if (ret) {
-			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
-			       reloc->name, reloc->val, ret);
-			return ret;
+	symtab = (void *)pmod->core_symtab;
+
+	/* For each __klp_rela section for this object */
+	klp_for_each_reloc_sec(obj, reloc_sec) {
+		relindex = reloc_sec->index;
+		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
+		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
+
+		/* For each rela in this __klp_rela section */
+		for (i = 0; i < num_relas; i++, rela++) {
+			sym = symtab + ELF_R_SYM(rela->r_info);
+			symname = pmod->core_strtab + sym->st_name;
+
+			if (sym->st_shndx == SHN_LIVEPATCH) {
+				if (sym->st_info == 'K')
+					ret = klp_find_external_symbol(pmod, symname, &addr);
+				else
+					ret = klp_find_object_symbol(obj->name, symname, &addr);
+				if (ret)
+					return ret;
+				sym->st_value = addr;
+			}
 		}
+		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
+					 pmod->index.sym, relindex, pmod);
 	}
 
-	return 0;
+	return ret;
 }
 
 static void notrace klp_ftrace_handler(unsigned long ip,
@@ -747,13 +746,22 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 				  struct klp_object *obj)
 {
 	struct klp_func *func;
+	struct module *pmod;
 	int ret;
 
-	if (obj->relocs) {
-		ret = klp_write_object_relocations(patch->mod, obj);
-		if (ret)
-			return ret;
-	}
+	pmod = patch->mod;
+
+	set_page_attributes(pmod->module_core,
+			    pmod->module_core + pmod->core_text_size,
+			    set_memory_rw);
+
+	ret = klp_write_object_relocations(pmod, obj, patch);
+	if (ret)
+		return ret;
+
+	set_page_attributes(pmod->module_core,
+			    pmod->module_core + pmod->core_text_size,
+			    set_memory_ro);
 
 	klp_for_each_func(obj, func) {
 		ret = klp_find_verify_func_addr(obj, func);
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1280653 — Re: livepatch: reuse module loader code to write relocations

+++ Jessica Yu [30/11/15 23:21 -0500]:
>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@redhat.com>
>---
> arch/x86/include/asm/livepatch.h |  2 -
> arch/x86/kernel/Makefile         |  1 -
> arch/x86/kernel/livepatch.c      | 91 --------------------------------------
> include/linux/livepatch.h        | 30 ++++++-------
> include/linux/module.h           |  6 +++
> kernel/livepatch/core.c          | 94 ++++++++++++++++++++++------------------
> 6 files changed, 70 insertions(+), 154 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 d1d35cc..0000000
>--- a/arch/x86/kernel/livepatch.c
>+++ /dev/null
>@@ -1,91 +0,0 @@
>-/*
>- * livepatch.c - x86-specific Kernel Live Patching Core
>- *
>- * Copyright (C) 2014 Seth Jennings <sjenning@redhat.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/cacheflush.h>
>-#include <asm/page_types.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)
>-{
>-	int ret, numpages, size = 4;
>-	bool readonly;
>-	unsigned long val;
>-	unsigned long core = (unsigned long)mod->module_core;
>-	unsigned long core_size = mod->core_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;
>-
>-	readonly = false;
>-
>-#ifdef CONFIG_DEBUG_SET_MODULE_RONX
>-	if (loc < core + mod->core_ro_size)
>-		readonly = true;
>-#endif
>-
>-	/* determine if the relocation spans a page boundary */
>-	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
>-
>-	if (readonly)
>-		set_memory_rw(loc & PAGE_MASK, numpages);
>-
>-	ret = probe_kernel_write((void *)loc, &val, size);
>-
>-	if (readonly)
>-		set_memory_ro(loc & PAGE_MASK, numpages);
>-
>-	return ret;
>-}
>diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>index 31db7a0..54f62a6 100644
>--- a/include/linux/livepatch.h
>+++ b/include/linux/livepatch.h
>@@ -64,28 +64,21 @@ struct klp_func {
> };
>
> /**
>- * struct klp_reloc - relocation structure for live patching
>- * @loc:	address where the relocation will be written
>- * @val:	address of the referenced symbol (optional,
>- *		vmlinux	patches only)
>- * @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_sec - relocation section struct for live patching
>+ * @index:	Elf section index of the relocation section
>+ * @name:	name of the relocation section
>+ * @objname:	name of the object associated with the klp reloc section
>  */
>-struct klp_reloc {
>-	unsigned long loc;
>-	unsigned long val;
>-	unsigned long type;
>-	const char *name;
>-	int addend;
>-	int external;
>+struct klp_reloc_sec {
>+	unsigned int index;
>+	char *name;
>+	char *objname;
> };
>
> /**
>  * 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
>+ * @reloc_secs:	relocation sections 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 +88,7 @@ struct klp_reloc {
> struct klp_object {
> 	/* external */
> 	const char *name;
>-	struct klp_reloc *relocs;
>+	struct klp_reloc_sec *reloc_secs;
> 	struct klp_func *funcs;
>
> 	/* internal */
>@@ -129,6 +122,9 @@ struct klp_patch {
> #define klp_for_each_func(obj, func) \
> 	for (func = obj->funcs; func->old_name; func++)
>
>+#define klp_for_each_reloc_sec(obj, reloc_sec) \
>+	for (reloc_sec = obj->reloc_secs; reloc_sec->name; reloc_sec++)
>+
> int klp_register_patch(struct klp_patch *);
> int klp_unregister_patch(struct klp_patch *);
> int klp_enable_patch(struct klp_patch *);
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 9b46256..ea61147 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -777,9 +777,15 @@ extern int module_sysfs_initialized;
> #ifdef CONFIG_DEBUG_SET_MODULE_RONX
> extern void set_all_modules_text_rw(void);
> extern void set_all_modules_text_ro(void);
>+extern void
>+set_page_attributes(void *start, void *end,
>+		    int (*set)(unsigned long start, int num_pages));
> #else
> static inline void set_all_modules_text_rw(void) { }
> static inline void set_all_modules_text_ro(void) { }
>+static inline void
>+set_page_attributes(void *start, void *end,
>+		    int (*set)(unsigned long start, int num_pages)) { }
> #endif
>
> #ifdef CONFIG_GENERIC_BUG
>diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>index db545cb..17b7278 100644
>--- a/kernel/livepatch/core.c
>+++ b/kernel/livepatch/core.c
>@@ -28,6 +28,9 @@
> #include <linux/list.h>
> #include <linux/kallsyms.h>
> #include <linux/livepatch.h>
>+#include <linux/elf.h>
>+#include <asm/cacheflush.h>
>+#include <linux/moduleloader.h>
>
> /**
>  * struct klp_ops - structure for tracking registered ftrace ops structs
>@@ -281,52 +284,48 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
> }
>
> static int klp_write_object_relocations(struct module *pmod,
>-					struct klp_object *obj)
>+					struct klp_object *obj,
>+					struct klp_patch *patch)
> {
>-	int ret;
>-	struct klp_reloc *reloc;
>+	int relindex, num_relas;
>+	int i, ret = 0;
>+	unsigned long addr;
>+	char *symname;
>+	struct klp_reloc_sec *reloc_sec;
>+	Elf_Rela *rela;
>+	Elf_Sym *sym, *symtab;
>
> 	if (WARN_ON(!klp_is_object_loaded(obj)))
> 		return -EINVAL;
>
>-	if (WARN_ON(!obj->relocs))
>-		return -EINVAL;
>-
>-	for (reloc = obj->relocs; reloc->name; reloc++) {
>-		if (!klp_is_module(obj)) {
>-
>-#if defined(CONFIG_RANDOMIZE_BASE)
>-			/* If KASLR has been enabled, adjust old value accordingly */
>-			if (kaslr_enabled())
>-				reloc->val += kaslr_offset();
>-#endif
>-			ret = klp_verify_vmlinux_symbol(reloc->name,
>-							reloc->val);
>-			if (ret)
>-				return ret;
>-		} else {
>-			/* module, reloc->val needs to be discovered */
>-			if (reloc->external)
>-				ret = klp_find_external_symbol(pmod,
>-							       reloc->name,
>-							       &reloc->val);
>-			else
>-				ret = klp_find_object_symbol(obj->mod->name,
>-							     reloc->name,
>-							     &reloc->val);
>-			if (ret)
>-				return ret;
>-		}
>-		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
>-					     reloc->val + reloc->addend);
>-		if (ret) {
>-			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
>-			       reloc->name, reloc->val, ret);
>-			return ret;
>+	symtab = (void *)pmod->core_symtab;
>+
>+	/* For each __klp_rela section for this object */
>+	klp_for_each_reloc_sec(obj, reloc_sec) {
>+		relindex = reloc_sec->index;
>+		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
>+		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
>+
>+		/* For each rela in this __klp_rela section */
>+		for (i = 0; i < num_relas; i++, rela++) {
>+			sym = symtab + ELF_R_SYM(rela->r_info);
>+			symname = pmod->core_strtab + sym->st_name;
>+
>+			if (sym->st_shndx == SHN_LIVEPATCH) {
>+				if (sym->st_info == 'K')
>+					ret = klp_find_external_symbol(pmod, symname, &addr);
>+				else
>+					ret = klp_find_object_symbol(obj->name, symname, &addr);
>+				if (ret)
>+					return ret;
>+				sym->st_value = addr;

I'll note here that this patchset has the same problem that current
livepatch has in that it will be unable to handle duplicate symbols
within the same object (i.e. sysfs will be unable to create entries
with the same name and livepatch errors out). After the Chris's sympos
patchset gets merged I should be able to rebase this patchset on top
of it to resolve the duplicate symbol issue.

>+			}
> 		}
>+		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
>+					 pmod->index.sym, relindex, pmod);
> 	}
>
>-	return 0;
>+	return ret;
> }
>
> static void notrace klp_ftrace_handler(unsigned long ip,
>@@ -747,13 +746,22 @@ static int klp_init_object_loaded(struct klp_patch *patch,
> 				  struct klp_object *obj)
> {
> 	struct klp_func *func;
>+	struct module *pmod;
> 	int ret;
>
>-	if (obj->relocs) {
>-		ret = klp_write_object_relocations(patch->mod, obj);
>-		if (ret)
>-			return ret;
>-	}
>+	pmod = patch->mod;
>+
>+	set_page_attributes(pmod->module_core,
>+			    pmod->module_core + pmod->core_text_size,
>+			    set_memory_rw);

Note #2: When Josh's module cleanup patches get merged, I'll swap out these
set_page_attribute() calls for (un)set_module_core_ro_nx().

Jessica
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286750 — Re: [RFC PATCH v2 4/6] livepatch: reuse module loader code to write relocations

On Mon, Nov 30, 2015 at 11:21:17PM -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@redhat.com>
> ---
>  arch/x86/include/asm/livepatch.h |  2 -
>  arch/x86/kernel/Makefile         |  1 -
>  arch/x86/kernel/livepatch.c      | 91 --------------------------------------
>  include/linux/livepatch.h        | 30 ++++++-------
>  include/linux/module.h           |  6 +++
>  kernel/livepatch/core.c          | 94 ++++++++++++++++++++++------------------
>  6 files changed, 70 insertions(+), 154 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 d1d35cc..0000000
> --- a/arch/x86/kernel/livepatch.c
> +++ /dev/null
> @@ -1,91 +0,0 @@
> -/*
> - * livepatch.c - x86-specific Kernel Live Patching Core
> - *
> - * Copyright (C) 2014 Seth Jennings <sjenning@redhat.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/cacheflush.h>
> -#include <asm/page_types.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)
> -{
> -	int ret, numpages, size = 4;
> -	bool readonly;
> -	unsigned long val;
> -	unsigned long core = (unsigned long)mod->module_core;
> -	unsigned long core_size = mod->core_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;
> -
> -	readonly = false;
> -
> -#ifdef CONFIG_DEBUG_SET_MODULE_RONX
> -	if (loc < core + mod->core_ro_size)
> -		readonly = true;
> -#endif
> -
> -	/* determine if the relocation spans a page boundary */
> -	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
> -
> -	if (readonly)
> -		set_memory_rw(loc & PAGE_MASK, numpages);
> -
> -	ret = probe_kernel_write((void *)loc, &val, size);
> -
> -	if (readonly)
> -		set_memory_ro(loc & PAGE_MASK, numpages);
> -
> -	return ret;
> -}
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 31db7a0..54f62a6 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -64,28 +64,21 @@ struct klp_func {
>  };
>  
>  /**
> - * struct klp_reloc - relocation structure for live patching
> - * @loc:	address where the relocation will be written
> - * @val:	address of the referenced symbol (optional,
> - *		vmlinux	patches only)
> - * @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_sec - relocation section struct for live patching
> + * @index:	Elf section index of the relocation section
> + * @name:	name of the relocation section
> + * @objname:	name of the object associated with the klp reloc section
>   */
> -struct klp_reloc {
> -	unsigned long loc;
> -	unsigned long val;
> -	unsigned long type;
> -	const char *name;
> -	int addend;
> -	int external;
> +struct klp_reloc_sec {
> +	unsigned int index;
> +	char *name;
> +	char *objname;
>  };
>  
>  /**
>   * 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
> + * @reloc_secs:	relocation sections 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 +88,7 @@ struct klp_reloc {
>  struct klp_object {
>  	/* external */
>  	const char *name;
> -	struct klp_reloc *relocs;
> +	struct klp_reloc_sec *reloc_secs;

There was a lot of discussion for v1, so I'm not sure, but I thought we
ended up deciding to get rid of the klp_reloc_sec struct?  Instead I
think the symbol table can be walked directly looking for klp rela
sections?

The benefits of doing so would be to make the interface simpler -- no
need to "cache" the section metadata when it's already easily and
quickly available in the module struct elf section headers.

>  	struct klp_func *funcs;
>  
>  	/* internal */
> @@ -129,6 +122,9 @@ struct klp_patch {
>  #define klp_for_each_func(obj, func) \
>  	for (func = obj->funcs; func->old_name; func++)
>  
> +#define klp_for_each_reloc_sec(obj, reloc_sec) \
> +	for (reloc_sec = obj->reloc_secs; reloc_sec->name; reloc_sec++)
> +
>  int klp_register_patch(struct klp_patch *);
>  int klp_unregister_patch(struct klp_patch *);
>  int klp_enable_patch(struct klp_patch *);
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 9b46256..ea61147 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -777,9 +777,15 @@ extern int module_sysfs_initialized;
>  #ifdef CONFIG_DEBUG_SET_MODULE_RONX
>  extern void set_all_modules_text_rw(void);
>  extern void set_all_modules_text_ro(void);
> +extern void
> +set_page_attributes(void *start, void *end,
> +		    int (*set)(unsigned long start, int num_pages));
>  #else
>  static inline void set_all_modules_text_rw(void) { }
>  static inline void set_all_modules_text_ro(void) { }
> +static inline void
> +set_page_attributes(void *start, void *end,
> +		    int (*set)(unsigned long start, int num_pages)) { }
>  #endif
>  
>  #ifdef CONFIG_GENERIC_BUG
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index db545cb..17b7278 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -28,6 +28,9 @@
>  #include <linux/list.h>
>  #include <linux/kallsyms.h>
>  #include <linux/livepatch.h>
> +#include <linux/elf.h>
> +#include <asm/cacheflush.h>
> +#include <linux/moduleloader.h>
>  
>  /**
>   * struct klp_ops - structure for tracking registered ftrace ops structs
> @@ -281,52 +284,48 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
>  }
>  
>  static int klp_write_object_relocations(struct module *pmod,
> -					struct klp_object *obj)
> +					struct klp_object *obj,
> +					struct klp_patch *patch)
>  {
> -	int ret;
> -	struct klp_reloc *reloc;
> +	int relindex, num_relas;
> +	int i, ret = 0;
> +	unsigned long addr;
> +	char *symname;
> +	struct klp_reloc_sec *reloc_sec;
> +	Elf_Rela *rela;
> +	Elf_Sym *sym, *symtab;
>  
>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>  		return -EINVAL;
>  
> -	if (WARN_ON(!obj->relocs))
> -		return -EINVAL;
> -
> -	for (reloc = obj->relocs; reloc->name; reloc++) {
> -		if (!klp_is_module(obj)) {
> -
> -#if defined(CONFIG_RANDOMIZE_BASE)
> -			/* If KASLR has been enabled, adjust old value accordingly */
> -			if (kaslr_enabled())
> -				reloc->val += kaslr_offset();
> -#endif
> -			ret = klp_verify_vmlinux_symbol(reloc->name,
> -							reloc->val);
> -			if (ret)
> -				return ret;
> -		} else {
> -			/* module, reloc->val needs to be discovered */
> -			if (reloc->external)
> -				ret = klp_find_external_symbol(pmod,
> -							       reloc->name,
> -							       &reloc->val);
> -			else
> -				ret = klp_find_object_symbol(obj->mod->name,
> -							     reloc->name,
> -							     &reloc->val);
> -			if (ret)
> -				return ret;
> -		}
> -		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
> -					     reloc->val + reloc->addend);
> -		if (ret) {
> -			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
> -			       reloc->name, reloc->val, ret);
> -			return ret;
> +	symtab = (void *)pmod->core_symtab;
> +
> +	/* For each __klp_rela section for this object */
> +	klp_for_each_reloc_sec(obj, reloc_sec) {
> +		relindex = reloc_sec->index;
> +		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> +		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> +
> +		/* For each rela in this __klp_rela section */
> +		for (i = 0; i < num_relas; i++, rela++) {
> +			sym = symtab + ELF_R_SYM(rela->r_info);
> +			symname = pmod->core_strtab + sym->st_name;
> +
> +			if (sym->st_shndx == SHN_LIVEPATCH) {
> +				if (sym->st_info == 'K')
> +					ret = klp_find_external_symbol(pmod, symname, &addr);
> +				else
> +					ret = klp_find_object_symbol(obj->name, symname, &addr);
> +				if (ret)
> +					return ret;
> +				sym->st_value = addr;

So I think you're also working on removing the 'external' stuff.  Any
idea how this code will handle that?  Specifically I'm wondering how the
objname and sympos of the rela's symbol will be specified.  Maybe it can
be encoded somehow in one of the symbol fields (st_value)?

> +			}
>  		}
> +		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
> +					 pmod->index.sym, relindex, pmod);
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static void notrace klp_ftrace_handler(unsigned long ip,
> @@ -747,13 +746,22 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>  				  struct klp_object *obj)
>  {
>  	struct klp_func *func;
> +	struct module *pmod;
>  	int ret;
>  
> -	if (obj->relocs) {
> -		ret = klp_write_object_relocations(patch->mod, obj);
> -		if (ret)
> -			return ret;
> -	}
> +	pmod = patch->mod;
> +
> +	set_page_attributes(pmod->module_core,
> +			    pmod->module_core + pmod->core_text_size,
> +			    set_memory_rw);
> +
> +	ret = klp_write_object_relocations(pmod, obj, patch);
> +	if (ret)
> +		return ret;
> +
> +	set_page_attributes(pmod->module_core,
> +			    pmod->module_core + pmod->core_text_size,
> +			    set_memory_ro);
>  
>  	klp_for_each_func(obj, func) {
>  		ret = klp_find_verify_func_addr(obj, func);
> -- 
> 2.4.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1287755 — Re: livepatch: reuse module loader code to write relocations

+++ Josh Poimboeuf [08/12/15 12:38 -0600]:
>On Mon, Nov 30, 2015 at 11:21:17PM -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@redhat.com>
>> ---
>>  arch/x86/include/asm/livepatch.h |  2 -
>>  arch/x86/kernel/Makefile         |  1 -
>>  arch/x86/kernel/livepatch.c      | 91 --------------------------------------
>>  include/linux/livepatch.h        | 30 ++++++-------
>>  include/linux/module.h           |  6 +++
>>  kernel/livepatch/core.c          | 94 ++++++++++++++++++++++------------------
>>  6 files changed, 70 insertions(+), 154 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 d1d35cc..0000000
>> --- a/arch/x86/kernel/livepatch.c
>> +++ /dev/null
>> @@ -1,91 +0,0 @@
>> -/*
>> - * livepatch.c - x86-specific Kernel Live Patching Core
>> - *
>> - * Copyright (C) 2014 Seth Jennings <sjenning@redhat.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/cacheflush.h>
>> -#include <asm/page_types.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)
>> -{
>> -	int ret, numpages, size = 4;
>> -	bool readonly;
>> -	unsigned long val;
>> -	unsigned long core = (unsigned long)mod->module_core;
>> -	unsigned long core_size = mod->core_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;
>> -
>> -	readonly = false;
>> -
>> -#ifdef CONFIG_DEBUG_SET_MODULE_RONX
>> -	if (loc < core + mod->core_ro_size)
>> -		readonly = true;
>> -#endif
>> -
>> -	/* determine if the relocation spans a page boundary */
>> -	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
>> -
>> -	if (readonly)
>> -		set_memory_rw(loc & PAGE_MASK, numpages);
>> -
>> -	ret = probe_kernel_write((void *)loc, &val, size);
>> -
>> -	if (readonly)
>> -		set_memory_ro(loc & PAGE_MASK, numpages);
>> -
>> -	return ret;
>> -}
>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> index 31db7a0..54f62a6 100644
>> --- a/include/linux/livepatch.h
>> +++ b/include/linux/livepatch.h
>> @@ -64,28 +64,21 @@ struct klp_func {
>>  };
>>
>>  /**
>> - * struct klp_reloc - relocation structure for live patching
>> - * @loc:	address where the relocation will be written
>> - * @val:	address of the referenced symbol (optional,
>> - *		vmlinux	patches only)
>> - * @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_sec - relocation section struct for live patching
>> + * @index:	Elf section index of the relocation section
>> + * @name:	name of the relocation section
>> + * @objname:	name of the object associated with the klp reloc section
>>   */
>> -struct klp_reloc {
>> -	unsigned long loc;
>> -	unsigned long val;
>> -	unsigned long type;
>> -	const char *name;
>> -	int addend;
>> -	int external;
>> +struct klp_reloc_sec {
>> +	unsigned int index;
>> +	char *name;
>> +	char *objname;
>>  };
>>
>>  /**
>>   * 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
>> + * @reloc_secs:	relocation sections 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 +88,7 @@ struct klp_reloc {
>>  struct klp_object {
>>  	/* external */
>>  	const char *name;
>> -	struct klp_reloc *relocs;
>> +	struct klp_reloc_sec *reloc_secs;
>
>There was a lot of discussion for v1, so I'm not sure, but I thought we
>ended up deciding to get rid of the klp_reloc_sec struct?  Instead I
>think the symbol table can be walked directly looking for klp rela
>sections?
>
>The benefits of doing so would be to make the interface simpler -- no
>need to "cache" the section metadata when it's already easily and
>quickly available in the module struct elf section headers.

Ah, I might have interpreted the conclusions of that last discussion
incorrectly.

In that case, I think I can just get rid of my klp_for_each_reloc_sec
macro as well as the lreloc scaffolding code from kpatch. The only
potentially "ugly" part of this change is that I'll have to move the
string parsing stuff here to extract the objname of the __klp_rela
section (which may not actually look that bad, we'll see how that
turns out).

>>  	struct klp_func *funcs;
>>
>>  	/* internal */
>> @@ -129,6 +122,9 @@ struct klp_patch {
>>  #define klp_for_each_func(obj, func) \
>>  	for (func = obj->funcs; func->old_name; func++)
>>
>> +#define klp_for_each_reloc_sec(obj, reloc_sec) \
>> +	for (reloc_sec = obj->reloc_secs; reloc_sec->name; reloc_sec++)
>> +
>>  int klp_register_patch(struct klp_patch *);
>>  int klp_unregister_patch(struct klp_patch *);
>>  int klp_enable_patch(struct klp_patch *);
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index 9b46256..ea61147 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -777,9 +777,15 @@ extern int module_sysfs_initialized;
>>  #ifdef CONFIG_DEBUG_SET_MODULE_RONX
>>  extern void set_all_modules_text_rw(void);
>>  extern void set_all_modules_text_ro(void);
>> +extern void
>> +set_page_attributes(void *start, void *end,
>> +		    int (*set)(unsigned long start, int num_pages));
>>  #else
>>  static inline void set_all_modules_text_rw(void) { }
>>  static inline void set_all_modules_text_ro(void) { }
>> +static inline void
>> +set_page_attributes(void *start, void *end,
>> +		    int (*set)(unsigned long start, int num_pages)) { }
>>  #endif
>>
>>  #ifdef CONFIG_GENERIC_BUG
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index db545cb..17b7278 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -28,6 +28,9 @@
>>  #include <linux/list.h>
>>  #include <linux/kallsyms.h>
>>  #include <linux/livepatch.h>
>> +#include <linux/elf.h>
>> +#include <asm/cacheflush.h>
>> +#include <linux/moduleloader.h>
>>
>>  /**
>>   * struct klp_ops - structure for tracking registered ftrace ops structs
>> @@ -281,52 +284,48 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
>>  }
>>
>>  static int klp_write_object_relocations(struct module *pmod,
>> -					struct klp_object *obj)
>> +					struct klp_object *obj,
>> +					struct klp_patch *patch)
>>  {
>> -	int ret;
>> -	struct klp_reloc *reloc;
>> +	int relindex, num_relas;
>> +	int i, ret = 0;
>> +	unsigned long addr;
>> +	char *symname;
>> +	struct klp_reloc_sec *reloc_sec;
>> +	Elf_Rela *rela;
>> +	Elf_Sym *sym, *symtab;
>>
>>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>>  		return -EINVAL;
>>
>> -	if (WARN_ON(!obj->relocs))
>> -		return -EINVAL;
>> -
>> -	for (reloc = obj->relocs; reloc->name; reloc++) {
>> -		if (!klp_is_module(obj)) {
>> -
>> -#if defined(CONFIG_RANDOMIZE_BASE)
>> -			/* If KASLR has been enabled, adjust old value accordingly */
>> -			if (kaslr_enabled())
>> -				reloc->val += kaslr_offset();
>> -#endif
>> -			ret = klp_verify_vmlinux_symbol(reloc->name,
>> -							reloc->val);
>> -			if (ret)
>> -				return ret;
>> -		} else {
>> -			/* module, reloc->val needs to be discovered */
>> -			if (reloc->external)
>> -				ret = klp_find_external_symbol(pmod,
>> -							       reloc->name,
>> -							       &reloc->val);
>> -			else
>> -				ret = klp_find_object_symbol(obj->mod->name,
>> -							     reloc->name,
>> -							     &reloc->val);
>> -			if (ret)
>> -				return ret;
>> -		}
>> -		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
>> -					     reloc->val + reloc->addend);
>> -		if (ret) {
>> -			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
>> -			       reloc->name, reloc->val, ret);
>> -			return ret;
>> +	symtab = (void *)pmod->core_symtab;
>> +
>> +	/* For each __klp_rela section for this object */
>> +	klp_for_each_reloc_sec(obj, reloc_sec) {
>> +		relindex = reloc_sec->index;
>> +		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
>> +		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
>> +
>> +		/* For each rela in this __klp_rela section */
>> +		for (i = 0; i < num_relas; i++, rela++) {
>> +			sym = symtab + ELF_R_SYM(rela->r_info);
>> +			symname = pmod->core_strtab + sym->st_name;
>> +
>> +			if (sym->st_shndx == SHN_LIVEPATCH) {
>> +				if (sym->st_info == 'K')
>> +					ret = klp_find_external_symbol(pmod, symname, &addr);
>> +				else
>> +					ret = klp_find_object_symbol(obj->name, symname, &addr);
>> +				if (ret)
>> +					return ret;
>> +				sym->st_value = addr;
>
>So I think you're also working on removing the 'external' stuff.  Any
>idea how this code will handle that?  Specifically I'm wondering how the
>objname and sympos of the rela's symbol will be specified.  Maybe it can
>be encoded somehow in one of the symbol fields (st_value)?

Thanks for bringing this up. I think we can just encode the symbol's
position in kallsyms in the symbol's st_other field. It isn't used
anywhere and has size char, which is plenty of bits to represent the
small ints.

For objname, the simplest solution might be to append ".klp.objname"
to the symbol name, and extract out this suffix when resolving the
symbol. Another way might be to have st_value contain the index into
the strtab (or .kpatch.strings) that contains the objname. Then we'd
access the objname just like how we access the symbol's name (strtab +
sym->st_name). After we find the objname we can then overwrite
st_value with the real address. I think this second method is cleaner.
Thoughts?

>> +			}
>>  		}
>> +		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
>> +					 pmod->index.sym, relindex, pmod);
>>  	}
>>
>> -	return 0;
>> +	return ret;
>>  }
>>
>>  static void notrace klp_ftrace_handler(unsigned long ip,
>> @@ -747,13 +746,22 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>>  				  struct klp_object *obj)
>>  {
>>  	struct klp_func *func;
>> +	struct module *pmod;
>>  	int ret;
>>
>> -	if (obj->relocs) {
>> -		ret = klp_write_object_relocations(patch->mod, obj);
>> -		if (ret)
>> -			return ret;
>> -	}
>> +	pmod = patch->mod;
>> +
>> +	set_page_attributes(pmod->module_core,
>> +			    pmod->module_core + pmod->core_text_size,
>> +			    set_memory_rw);
>> +
>> +	ret = klp_write_object_relocations(pmod, obj, patch);
>> +	if (ret)
>> +		return ret;
>> +
>> +	set_page_attributes(pmod->module_core,
>> +			    pmod->module_core + pmod->core_text_size,
>> +			    set_memory_ro);
>>
>>  	klp_for_each_func(obj, func) {
>>  		ret = klp_find_verify_func_addr(obj, func);
>> --
>> 2.4.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe live-patching" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>-- 
>Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288531 — Re: livepatch: reuse module loader code to write relocations

On Wed, Dec 09, 2015 at 02:10:14PM -0500, Jessica Yu wrote:
> +++ Josh Poimboeuf [08/12/15 12:38 -0600]:
> >>+	/* For each __klp_rela section for this object */
> >>+	klp_for_each_reloc_sec(obj, reloc_sec) {
> >>+		relindex = reloc_sec->index;
> >>+		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> >>+		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> >>+
> >>+		/* For each rela in this __klp_rela section */
> >>+		for (i = 0; i < num_relas; i++, rela++) {
> >>+			sym = symtab + ELF_R_SYM(rela->r_info);
> >>+			symname = pmod->core_strtab + sym->st_name;
> >>+
> >>+			if (sym->st_shndx == SHN_LIVEPATCH) {
> >>+				if (sym->st_info == 'K')
> >>+					ret = klp_find_external_symbol(pmod, symname, &addr);
> >>+				else
> >>+					ret = klp_find_object_symbol(obj->name, symname, &addr);
> >>+				if (ret)
> >>+					return ret;
> >>+				sym->st_value = addr;
> >
> >So I think you're also working on removing the 'external' stuff.  Any
> >idea how this code will handle that?  Specifically I'm wondering how the
> >objname and sympos of the rela's symbol will be specified.  Maybe it can
> >be encoded somehow in one of the symbol fields (st_value)?
> 
> Thanks for bringing this up. I think we can just encode the symbol's
> position in kallsyms in the symbol's st_other field. It isn't used
> anywhere and has size char, which is plenty of bits to represent the
> small ints.

st_other does seem to at least have some trivial usage in the kernel,
see print_absolute_symbols() and sym_visibility() in
arch/x86/tools/relocs.c.  Two of the bits are used to specify the
"visibility" of a symbol.  Also readelf shows a "Vis" column in the
symbol table.

> For objname, the simplest solution might be to append ".klp.objname"
> to the symbol name, and extract out this suffix when resolving the
> symbol. Another way might be to have st_value contain the index into
> the strtab (or .kpatch.strings) that contains the objname. Then we'd
> access the objname just like how we access the symbol's name (strtab +
> sym->st_name). After we find the objname we can then overwrite
> st_value with the real address. I think this second method is cleaner.
> Thoughts?

Yeah, I guess there are a lot of possibilities for ways to encode it.

Personally I think it would be nice if the encoding were something that
could easily be seen when dumping the symbol table with readelf.  So,
for example, the objname could be encoded in the symbol name (as you
suggested), and the sympos could be in st_value.

If we do that, it'd probably be good to keep the naming consistent with
the '__klp_rela_objname.symname' thing.  So something like
'_klp_sym_objname.symname'.

But... would there be any side effects associated with renaming it?  For
example, would it cause problems with the s390 PLT?

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288942 — Re: livepatch: reuse module loader code to write relocations

+++ Josh Poimboeuf [10/12/15 08:28 -0600]:
>On Wed, Dec 09, 2015 at 02:10:14PM -0500, Jessica Yu wrote:
>> +++ Josh Poimboeuf [08/12/15 12:38 -0600]:
>> >>+	/* For each __klp_rela section for this object */
>> >>+	klp_for_each_reloc_sec(obj, reloc_sec) {
>> >>+		relindex = reloc_sec->index;
>> >>+		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
>> >>+		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
>> >>+
>> >>+		/* For each rela in this __klp_rela section */
>> >>+		for (i = 0; i < num_relas; i++, rela++) {
>> >>+			sym = symtab + ELF_R_SYM(rela->r_info);
>> >>+			symname = pmod->core_strtab + sym->st_name;
>> >>+
>> >>+			if (sym->st_shndx == SHN_LIVEPATCH) {
>> >>+				if (sym->st_info == 'K')
>> >>+					ret = klp_find_external_symbol(pmod, symname, &addr);
>> >>+				else
>> >>+					ret = klp_find_object_symbol(obj->name, symname, &addr);
>> >>+				if (ret)
>> >>+					return ret;
>> >>+				sym->st_value = addr;
>> >
>> >So I think you're also working on removing the 'external' stuff.  Any
>> >idea how this code will handle that?  Specifically I'm wondering how the
>> >objname and sympos of the rela's symbol will be specified.  Maybe it can
>> >be encoded somehow in one of the symbol fields (st_value)?
>>
>> Thanks for bringing this up. I think we can just encode the symbol's
>> position in kallsyms in the symbol's st_other field. It isn't used
>> anywhere and has size char, which is plenty of bits to represent the
>> small ints.
>
>st_other does seem to at least have some trivial usage in the kernel,
>see print_absolute_symbols() and sym_visibility() in
>arch/x86/tools/relocs.c.  Two of the bits are used to specify the
>"visibility" of a symbol.  Also readelf shows a "Vis" column in the
>symbol table.

Yeah, for x86 it looks like st_other is used only for SHN_ABS symbols
in print_absolute_symbols(). Technically SHN_LIVEPATCH symbols
shouldn't be affected in this case...but despite its sparse usage in the
kernel it does look like using st_other to encode sympos is out of the
question as its meaning is architecture specific..

>> For objname, the simplest solution might be to append ".klp.objname"
>> to the symbol name, and extract out this suffix when resolving the
>> symbol. Another way might be to have st_value contain the index into
>> the strtab (or .kpatch.strings) that contains the objname. Then we'd
>> access the objname just like how we access the symbol's name (strtab +
>> sym->st_name). After we find the objname we can then overwrite
>> st_value with the real address. I think this second method is cleaner.
>> Thoughts?
>
>Yeah, I guess there are a lot of possibilities for ways to encode it.
>
>Personally I think it would be nice if the encoding were something that
>could easily be seen when dumping the symbol table with readelf.  So,
>for example, the objname could be encoded in the symbol name (as you
>suggested), and the sympos could be in st_value.

Sure, that should be doable. So the new process might look like this:

For every livepatch symbol referenced by a rela..

1) Save the sympos encoded in st_value

2) Save the sym_objname that is encoded in the symbol's name with the
'klp' suffix (Just to clarify: the sym_objname specifies the object
in which the symbol lives, and recall that we need this to remove the
need for the "external" flag)

3) Resolve the symbol by using its name (without the klp suffix),
sympos, and sym_objname

4) Set st_value to the found address

>If we do that, it'd probably be good to keep the naming consistent with
>the '__klp_rela_objname.symname' thing.  So something like
>'_klp_sym_objname.symname'.

How about 'symname.klp.objname', and renaming the klp reloc sections
to '.klp.objname.rela.section_name'? Special symbol suffixes and
section names seem to always use '.', so maybe this would look better?
:-) But we can keep the underscores if people like that more. Both
naming methods would work, it is only a matter of preference.

>But... would there be any side effects associated with renaming it?  For
>example, would it cause problems with the s390 PLT?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288955 — Re: livepatch: reuse module loader code to write relocations

On Thu, Dec 10, 2015 at 04:33:29PM -0500, Jessica Yu wrote:
> +++ Josh Poimboeuf [10/12/15 08:28 -0600]:
> >On Wed, Dec 09, 2015 at 02:10:14PM -0500, Jessica Yu wrote:
> >>+++ Josh Poimboeuf [08/12/15 12:38 -0600]:
> >>>>+	/* For each __klp_rela section for this object */
> >>>>+	klp_for_each_reloc_sec(obj, reloc_sec) {
> >>>>+		relindex = reloc_sec->index;
> >>>>+		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> >>>>+		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> >>>>+
> >>>>+		/* For each rela in this __klp_rela section */
> >>>>+		for (i = 0; i < num_relas; i++, rela++) {
> >>>>+			sym = symtab + ELF_R_SYM(rela->r_info);
> >>>>+			symname = pmod->core_strtab + sym->st_name;
> >>>>+
> >>>>+			if (sym->st_shndx == SHN_LIVEPATCH) {
> >>>>+				if (sym->st_info == 'K')
> >>>>+					ret = klp_find_external_symbol(pmod, symname, &addr);
> >>>>+				else
> >>>>+					ret = klp_find_object_symbol(obj->name, symname, &addr);
> >>>>+				if (ret)
> >>>>+					return ret;
> >>>>+				sym->st_value = addr;
> >>>
> >>>So I think you're also working on removing the 'external' stuff.  Any
> >>>idea how this code will handle that?  Specifically I'm wondering how the
> >>>objname and sympos of the rela's symbol will be specified.  Maybe it can
> >>>be encoded somehow in one of the symbol fields (st_value)?
> >>
> >>Thanks for bringing this up. I think we can just encode the symbol's
> >>position in kallsyms in the symbol's st_other field. It isn't used
> >>anywhere and has size char, which is plenty of bits to represent the
> >>small ints.
> >
> >st_other does seem to at least have some trivial usage in the kernel,
> >see print_absolute_symbols() and sym_visibility() in
> >arch/x86/tools/relocs.c.  Two of the bits are used to specify the
> >"visibility" of a symbol.  Also readelf shows a "Vis" column in the
> >symbol table.
> 
> Yeah, for x86 it looks like st_other is used only for SHN_ABS symbols
> in print_absolute_symbols(). Technically SHN_LIVEPATCH symbols
> shouldn't be affected in this case...but despite its sparse usage in the
> kernel it does look like using st_other to encode sympos is out of the
> question as its meaning is architecture specific..
> 
> >>For objname, the simplest solution might be to append ".klp.objname"
> >>to the symbol name, and extract out this suffix when resolving the
> >>symbol. Another way might be to have st_value contain the index into
> >>the strtab (or .kpatch.strings) that contains the objname. Then we'd
> >>access the objname just like how we access the symbol's name (strtab +
> >>sym->st_name). After we find the objname we can then overwrite
> >>st_value with the real address. I think this second method is cleaner.
> >>Thoughts?
> >
> >Yeah, I guess there are a lot of possibilities for ways to encode it.
> >
> >Personally I think it would be nice if the encoding were something that
> >could easily be seen when dumping the symbol table with readelf.  So,
> >for example, the objname could be encoded in the symbol name (as you
> >suggested), and the sympos could be in st_value.
> 
> Sure, that should be doable. So the new process might look like this:
> 
> For every livepatch symbol referenced by a rela..
> 
> 1) Save the sympos encoded in st_value
> 
> 2) Save the sym_objname that is encoded in the symbol's name with the
> 'klp' suffix (Just to clarify: the sym_objname specifies the object
> in which the symbol lives, and recall that we need this to remove the
> need for the "external" flag)
> 
> 3) Resolve the symbol by using its name (without the klp suffix),
> sympos, and sym_objname
> 
> 4) Set st_value to the found address

Sounds right to me.

> >If we do that, it'd probably be good to keep the naming consistent with
> >the '__klp_rela_objname.symname' thing.  So something like
> >'_klp_sym_objname.symname'.
> 
> How about 'symname.klp.objname', and renaming the klp reloc sections
> to '.klp.objname.rela.section_name'? Special symbol suffixes and
> section names seem to always use '.', so maybe this would look better?
> :-) But we can keep the underscores if people like that more. Both
> naming methods would work, it is only a matter of preference.

It's your patches, so I'd say you get to pick ;-)  My only request would
be some consistency between the symbol names and the rela section names.

> >But... would there be any side effects associated with renaming it?  For
> >example, would it cause problems with the s390 PLT?

Just to verify, did you see this question? :-)

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288966 — Re: livepatch: reuse module loader code to write relocations

+++ Josh Poimboeuf [10/12/15 15:41 -0600]:
>On Thu, Dec 10, 2015 at 04:33:29PM -0500, Jessica Yu wrote:
>> +++ Josh Poimboeuf [10/12/15 08:28 -0600]:
>> >On Wed, Dec 09, 2015 at 02:10:14PM -0500, Jessica Yu wrote:
>> >>+++ Josh Poimboeuf [08/12/15 12:38 -0600]:
>> >>>>+	/* For each __klp_rela section for this object */
>> >>>>+	klp_for_each_reloc_sec(obj, reloc_sec) {
>> >>>>+		relindex = reloc_sec->index;
>> >>>>+		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
>> >>>>+		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
>> >>>>+
>> >>>>+		/* For each rela in this __klp_rela section */
>> >>>>+		for (i = 0; i < num_relas; i++, rela++) {
>> >>>>+			sym = symtab + ELF_R_SYM(rela->r_info);
>> >>>>+			symname = pmod->core_strtab + sym->st_name;
>> >>>>+
>> >>>>+			if (sym->st_shndx == SHN_LIVEPATCH) {
>> >>>>+				if (sym->st_info == 'K')
>> >>>>+					ret = klp_find_external_symbol(pmod, symname, &addr);
>> >>>>+				else
>> >>>>+					ret = klp_find_object_symbol(obj->name, symname, &addr);
>> >>>>+				if (ret)
>> >>>>+					return ret;
>> >>>>+				sym->st_value = addr;
>> >>>
>> >>>So I think you're also working on removing the 'external' stuff.  Any
>> >>>idea how this code will handle that?  Specifically I'm wondering how the
>> >>>objname and sympos of the rela's symbol will be specified.  Maybe it can
>> >>>be encoded somehow in one of the symbol fields (st_value)?
>> >>
>> >>Thanks for bringing this up. I think we can just encode the symbol's
>> >>position in kallsyms in the symbol's st_other field. It isn't used
>> >>anywhere and has size char, which is plenty of bits to represent the
>> >>small ints.
>> >
>> >st_other does seem to at least have some trivial usage in the kernel,
>> >see print_absolute_symbols() and sym_visibility() in
>> >arch/x86/tools/relocs.c.  Two of the bits are used to specify the
>> >"visibility" of a symbol.  Also readelf shows a "Vis" column in the
>> >symbol table.
>>
>> Yeah, for x86 it looks like st_other is used only for SHN_ABS symbols
>> in print_absolute_symbols(). Technically SHN_LIVEPATCH symbols
>> shouldn't be affected in this case...but despite its sparse usage in the
>> kernel it does look like using st_other to encode sympos is out of the
>> question as its meaning is architecture specific..
>>
>> >>For objname, the simplest solution might be to append ".klp.objname"
>> >>to the symbol name, and extract out this suffix when resolving the
>> >>symbol. Another way might be to have st_value contain the index into
>> >>the strtab (or .kpatch.strings) that contains the objname. Then we'd
>> >>access the objname just like how we access the symbol's name (strtab +
>> >>sym->st_name). After we find the objname we can then overwrite
>> >>st_value with the real address. I think this second method is cleaner.
>> >>Thoughts?
>> >
>> >Yeah, I guess there are a lot of possibilities for ways to encode it.
>> >
>> >Personally I think it would be nice if the encoding were something that
>> >could easily be seen when dumping the symbol table with readelf.  So,
>> >for example, the objname could be encoded in the symbol name (as you
>> >suggested), and the sympos could be in st_value.
>>
>> Sure, that should be doable. So the new process might look like this:
>>
>> For every livepatch symbol referenced by a rela..
>>
>> 1) Save the sympos encoded in st_value
>>
>> 2) Save the sym_objname that is encoded in the symbol's name with the
>> 'klp' suffix (Just to clarify: the sym_objname specifies the object
>> in which the symbol lives, and recall that we need this to remove the
>> need for the "external" flag)
>>
>> 3) Resolve the symbol by using its name (without the klp suffix),
>> sympos, and sym_objname
>>
>> 4) Set st_value to the found address
>
>Sounds right to me.
>
>> >If we do that, it'd probably be good to keep the naming consistent with
>> >the '__klp_rela_objname.symname' thing.  So something like
>> >'_klp_sym_objname.symname'.
>>
>> How about 'symname.klp.objname', and renaming the klp reloc sections
>> to '.klp.objname.rela.section_name'? Special symbol suffixes and
>> section names seem to always use '.', so maybe this would look better?
>> :-) But we can keep the underscores if people like that more. Both
>> naming methods would work, it is only a matter of preference.
>
>It's your patches, so I'd say you get to pick ;-)  My only request would
>be some consistency between the symbol names and the rela section names.
>
>> >But... would there be any side effects associated with renaming it?  For
>> >example, would it cause problems with the s390 PLT?
>
>Just to verify, did you see this question? :-)

Ah, sorry I missed this! So I looked through the module loader code
for x86, s390, arm (and just grepped in a few other archs) and the
symbol's name (st_name) is only ever used in printing error or debug
messages, or not used at all. As long as livepatch correctly resolves
the symbol, and st_value is set, apply_relocate_add() doesn't really
care about the name.

s390 plt/got information (struct mod_arch_syminfo) for each symbol is
stored in an array in mod->arch.syminfo. Every symbol that is in the
module's symbol table will get an entry, but not every symbol will
have their plt/got offsets filled in. Every symbol referenced by a
PLT/GOT rela will have those offsets filled in. Since our livepatch
symbols are real symbols, with indexes and entries in the symbol
table, they will get their own s390 mod_arch_syminfo struct, and
should any PLT rela reference them, a PLT entry as well, which is
good. Tldr, names of symbols don't affect s390 plt/got handling, so we
are safe.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288536 — Re: [RFC PATCH v2 4/6] livepatch: reuse module loader code to write relocations

On 11/30/15 at 11:21P, Jessica Yu wrote:
> +	klp_for_each_reloc_sec(obj, reloc_sec) {
> +		relindex = reloc_sec->index;
> +		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> +		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> +
> +		/* For each rela in this __klp_rela section */
> +		for (i = 0; i < num_relas; i++, rela++) {
> +			sym = symtab + ELF_R_SYM(rela->r_info);
> +			symname = pmod->core_strtab + sym->st_name;
> +
> +			if (sym->st_shndx == SHN_LIVEPATCH) {
> +				if (sym->st_info == 'K')
> +					ret = klp_find_external_symbol(pmod, symname, &addr);
> +				else
> +					ret = klp_find_object_symbol(obj->name, symname, &addr);
> +				if (ret)
> +					return ret;
> +				sym->st_value = addr;
> +			}
>  		}
> +		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
> +					 pmod->index.sym, relindex, pmod);

It is more appropiate to test the ret.

Thanks
Minfei

>  	}
>  
> -	return 0;
> +	return ret;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288887 — Re: [RFC PATCH v2 4/6] livepatch: reuse module loader code to write relocations

On Thu, 10 Dec 2015, Minfei Huang wrote:

> > +	klp_for_each_reloc_sec(obj, reloc_sec) {
> > +		relindex = reloc_sec->index;
> > +		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> > +		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> > +
> > +		/* For each rela in this __klp_rela section */
> > +		for (i = 0; i < num_relas; i++, rela++) {
> > +			sym = symtab + ELF_R_SYM(rela->r_info);
> > +			symname = pmod->core_strtab + sym->st_name;
> > +
> > +			if (sym->st_shndx == SHN_LIVEPATCH) {
> > +				if (sym->st_info == 'K')
> > +					ret = klp_find_external_symbol(pmod, symname, &addr);
> > +				else
> > +					ret = klp_find_object_symbol(obj->name, symname, &addr);
> > +				if (ret)
> > +					return ret;
> > +				sym->st_value = addr;
> > +			}
> >  		}
> > +		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
> > +					 pmod->index.sym, relindex, pmod);
> 
> It is more appropiate to test the ret.

And if error is encountered ... then you'd propose to do what?

The code is correct as is; the return value is propagated properly to 
caller, and all the existing callers have proper error handling.

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288934 — Re: [RFC PATCH v2 4/6] livepatch: reuse module loader code to write relocations

On Thu, Dec 10, 2015 at 08:56:27PM +0100, Jiri Kosina wrote:
> On Thu, 10 Dec 2015, Minfei Huang wrote:
> 
> > > +	klp_for_each_reloc_sec(obj, reloc_sec) {
> > > +		relindex = reloc_sec->index;
> > > +		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> > > +		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> > > +
> > > +		/* For each rela in this __klp_rela section */
> > > +		for (i = 0; i < num_relas; i++, rela++) {
> > > +			sym = symtab + ELF_R_SYM(rela->r_info);
> > > +			symname = pmod->core_strtab + sym->st_name;
> > > +
> > > +			if (sym->st_shndx == SHN_LIVEPATCH) {
> > > +				if (sym->st_info == 'K')
> > > +					ret = klp_find_external_symbol(pmod, symname, &addr);
> > > +				else
> > > +					ret = klp_find_object_symbol(obj->name, symname, &addr);
> > > +				if (ret)
> > > +					return ret;
> > > +				sym->st_value = addr;
> > > +			}
> > >  		}
> > > +		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
> > > +					 pmod->index.sym, relindex, pmod);
> > 
> > It is more appropiate to test the ret.
> 
> And if error is encountered ... then you'd propose to do what?
> 
> The code is correct as is; the return value is propagated properly to 
> caller, and all the existing callers have proper error handling.

apply_relocate_add() is called from inside a loop, so if there's an
error, it should return immediately instead of continuing the next
iteration.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web