Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1266222 > unrolled thread
| Started by | Jessica Yu <jeyu@redhat.com> |
|---|---|
| First post | 2015-11-10 05:50 +0100 |
| Last post | 2015-11-11 17:30 +0100 |
| Articles | 18 on this page of 58 — 6 participants |
Back to article view | Back to linux.kernel
[RFC PATCH 0/5] Arch-independent livepatch Jessica Yu <jeyu@redhat.com> - 2015-11-10 05:50 +0100
[RFC PATCH 2/5] module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-10 05:50 +0100
Re: [RFC PATCH 2/5] module: save load_info for livepatch modules Minfei Huang <mnfhuang@gmail.com> - 2015-11-11 09:10 +0100
Re: [RFC PATCH 2/5] module: save load_info for livepatch modules Miroslav Benes <mbenes@suse.cz> - 2015-11-11 15:20 +0100
Re: module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-12 06:40 +0100
Re: module: save load_info for livepatch modules Petr Mladek <pmladek@suse.com> - 2015-11-12 11:30 +0100
Re: module: save load_info for livepatch modules Miroslav Benes <mbenes@suse.cz> - 2015-11-12 14:30 +0100
Re: module: save load_info for livepatch modules Petr Mladek <pmladek@suse.com> - 2015-11-12 16:10 +0100
Re: module: save load_info for livepatch modules Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 18:10 +0100
Re: module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-12 23:20 +0100
Re: module: save load_info for livepatch modules Miroslav Benes <mbenes@suse.cz> - 2015-11-13 13:30 +0100
Re: module: save load_info for livepatch modules Miroslav Benes <mbenes@suse.cz> - 2015-11-13 13:50 +0100
Re: module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-14 01:40 +0100
Re: module: save load_info for livepatch modules Miroslav Benes <mbenes@suse.cz> - 2015-11-13 14:00 +0100
Re: module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-14 03:20 +0100
Re: module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-13 01:30 +0100
Re: [RFC PATCH 2/5] module: save load_info for livepatch modules Petr Mladek <pmladek@suse.com> - 2015-11-11 15:40 +0100
Re: module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-12 05:50 +0100
Re: module: save load_info for livepatch modules Petr Mladek <pmladek@suse.com> - 2015-11-12 11:10 +0100
Re: module: save load_info for livepatch modules Miroslav Benes <mbenes@suse.cz> - 2015-11-12 15:20 +0100
Re: module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-13 07:40 +0100
Re: module: save load_info for livepatch modules Miroslav Benes <mbenes@suse.cz> - 2015-11-13 14:10 +0100
Re: module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-13 09:30 +0100
Re: [RFC PATCH 2/5] module: save load_info for livepatch modules Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 18:20 +0100
Re: [RFC PATCH 2/5] module: save load_info for livepatch modules Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 18:30 +0100
[RFC PATCH 1/5] elf: add livepatch-specific elf constants Jessica Yu <jeyu@redhat.com> - 2015-11-10 05:50 +0100
Re: [RFC PATCH 1/5] elf: add livepatch-specific elf constants Petr Mladek <pmladek@suse.com> - 2015-11-11 15:00 +0100
Re: [RFC PATCH 1/5] elf: add livepatch-specific elf constants Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 16:40 +0100
Re: [RFC PATCH 1/5] elf: add livepatch-specific elf constants Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 16:50 +0100
Re: elf: add livepatch-specific elf constants Jessica Yu <jeyu@redhat.com> - 2015-11-13 08:00 +0100
[RFC PATCH 4/5] samples: livepatch: init reloc list and mark as klp module Jessica Yu <jeyu@redhat.com> - 2015-11-10 05:50 +0100
Re: [RFC PATCH 4/5] samples: livepatch: init reloc list and mark as klp module Jiri Slaby <jslaby@suse.cz> - 2015-11-10 09:20 +0100
Re: [RFC PATCH 4/5] samples: livepatch: init reloc list and mark as klp module Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-10 15:00 +0100
Re: samples: livepatch: init reloc list and mark as klp module Jessica Yu <jeyu@redhat.com> - 2015-11-10 19:40 +0100
Re: [RFC PATCH 4/5] samples: livepatch: init reloc list and mark as klp module Petr Mladek <pmladek@suse.com> - 2015-11-11 16:50 +0100
Re: samples: livepatch: init reloc list and mark as klp module Jessica Yu <jeyu@redhat.com> - 2015-11-12 07:10 +0100
Re: samples: livepatch: init reloc list and mark as klp module Miroslav Benes <mbenes@suse.cz> - 2015-11-12 11:50 +0100
[RFC PATCH 5/5] livepatch: x86: remove unused relocation code Jessica Yu <jeyu@redhat.com> - 2015-11-10 05:50 +0100
Re: [RFC PATCH 5/5] livepatch: x86: remove unused relocation code Petr Mladek <pmladek@suse.com> - 2015-11-11 16:50 +0100
Re: [RFC PATCH 5/5] livepatch: x86: remove unused relocation code Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 19:10 +0100
[RFC PATCH 3/5] livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-11-10 05:50 +0100
Re: [RFC PATCH 3/5] livepatch: reuse module loader code to write relocations Jiri Slaby <jslaby@suse.cz> - 2015-11-10 09:20 +0100
Re: [RFC PATCH 3/5] livepatch: reuse module loader code to write relocations Miroslav Benes <mbenes@suse.cz> - 2015-11-11 15:40 +0100
Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-11-11 21:10 +0100
Re: livepatch: reuse module loader code to write relocations Miroslav Benes <mbenes@suse.cz> - 2015-11-12 16:30 +0100
Re: livepatch: reuse module loader code to write relocations Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 18:50 +0100
Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-11-12 21:30 +0100
Re: livepatch: reuse module loader code to write relocations Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 21:40 +0100
Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-11-13 08:20 +0100
Re: livepatch: reuse module loader code to write relocations Miroslav Benes <mbenes@suse.cz> - 2015-11-13 15:00 +0100
Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-11-12 20:20 +0100
Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-11-12 21:40 +0100
Re: [RFC PATCH 3/5] livepatch: reuse module loader code to write relocations Petr Mladek <pmladek@suse.com> - 2015-11-11 16:30 +0100
Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-11-11 19:30 +0100
Re: livepatch: reuse module loader code to write relocations Petr Mladek <pmladek@suse.com> - 2015-11-12 10:20 +0100
Re: [RFC PATCH 3/5] livepatch: reuse module loader code to write relocations Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 19:00 +0100
Re: [RFC PATCH 0/5] Arch-independent livepatch Miroslav Benes <mbenes@suse.cz> - 2015-11-11 15:10 +0100
Re: [RFC PATCH 0/5] Arch-independent livepatch Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-11 17:30 +0100
Page 3 of 3 — ← Prev page 1 2 [3]
| From | Jessica Yu <jeyu@redhat.com> |
|---|---|
| Date | 2015-11-10 05:50 +0100 |
| Subject | [RFC PATCH 3/5] livepatch: reuse module loader code to write relocations |
| Message-ID | <qt92y-kt-15@gated-at.bofh.it> |
| In reply to | #1266222 |
Reuse module loader code to write relocations, thereby eliminating the
need for architecture specific code in livepatch. Namely, we reuse
apply_relocate_add() in the module loader to write relocs 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.
Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
include/linux/livepatch.h | 11 ++++--
include/linux/module.h | 6 ++++
kernel/livepatch/core.c | 89 +++++++++++++++++++++++++++++------------------
3 files changed, 70 insertions(+), 36 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 31db7a0..601e892 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -85,7 +85,7 @@ struct klp_reloc {
/**
* 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 +95,7 @@ struct klp_reloc {
struct klp_object {
/* external */
const char *name;
- struct klp_reloc *relocs;
+ struct list_head reloc_secs;
struct klp_func *funcs;
/* internal */
@@ -129,6 +129,13 @@ struct klp_patch {
#define klp_for_each_func(obj, func) \
for (func = obj->funcs; func->old_name; func++)
+struct klp_reloc_sec {
+ unsigned int index;
+ char *name;
+ char *objname;
+ struct list_head list;
+};
+
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 c8680b1..3c34eb8 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -793,9 +793,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 087a8c7..26c419f 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -28,6 +28,8 @@
#include <linux/list.h>
#include <linux/kallsyms.h>
#include <linux/livepatch.h>
+#include <linux/elf.h>
+#include <asm/cacheflush.h>
/**
* struct klp_ops - structure for tracking registered ftrace ops structs
@@ -281,46 +283,54 @@ 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;
+ unsigned int bind;
+ char *symname;
+ struct klp_reloc_sec *reloc_sec;
+ struct load_info *info;
+ Elf_Rela *rela;
+ Elf_Sym *sym, *symtab;
+ Elf_Shdr *symsect;
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)) {
- 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;
+ info = pmod->info;
+ symsect = info->sechdrs + info->index.sym;
+ symtab = (void *)info->hdr + symsect->sh_offset;
+
+ /* For each __klp_rela section for this object */
+ list_for_each_entry(reloc_sec, &obj->reloc_secs, list) {
+ relindex = reloc_sec->index;
+ num_relas = info->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
+ rela = (Elf_Rela *) info->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 = info->strtab + sym->st_name;
+ bind = ELF_ST_BIND(sym->st_info);
+
+ if (sym->st_shndx == SHN_LIVEPATCH) {
+ if (bind == STB_LIVEPATCH_EXT)
+ 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(info->sechdrs, info->strtab,
+ info->index.sym, relindex, pmod);
}
- return 0;
+ return ret;
}
static void notrace klp_ftrace_handler(unsigned long ip,
@@ -741,12 +751,23 @@ 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);
+ pmod = patch->mod;
+
+ if (!list_empty(&obj->reloc_secs)) {
+ 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) {
--
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] | [prev] | [next] | [standalone]
| From | Jiri Slaby <jslaby@suse.cz> |
|---|---|
| Date | 2015-11-10 09:20 +0100 |
| Subject | Re: [RFC PATCH 3/5] livepatch: reuse module loader code to write relocations |
| Message-ID | <qtcjM-2xd-15@gated-at.bofh.it> |
| In reply to | #1266230 |
On 11/10/2015, 05:45 AM, Jessica Yu wrote:
> @@ -95,7 +95,7 @@ struct klp_reloc {
> struct klp_object {
> /* external */
> const char *name;
> - struct klp_reloc *relocs;
> + struct list_head reloc_secs;
Too early to proper review the code, but you seem you never call
INIT_LIST_HEAD for this guy.
thanks,
--
js
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]
| From | Miroslav Benes <mbenes@suse.cz> |
|---|---|
| Date | 2015-11-11 15:40 +0100 |
| Subject | Re: [RFC PATCH 3/5] livepatch: reuse module loader code to write relocations |
| Message-ID | <qtEJ5-3Zk-33@gated-at.bofh.it> |
| In reply to | #1266230 |
On Mon, 9 Nov 2015, Jessica Yu wrote:
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 31db7a0..601e892 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -85,7 +85,7 @@ struct klp_reloc {
> /**
> * 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 +95,7 @@ struct klp_reloc {
> struct klp_object {
> /* external */
> const char *name;
> - struct klp_reloc *relocs;
> + struct list_head reloc_secs;
> struct klp_func *funcs;
So I guess we don't need klp_reloc anymore. If true, we should really
start thinking about proper documentation because there are going to be
plenty of assumptions about a patch module and we need to have it written
somewhere. Especially how the relocation sections look like.
> /* internal */
> @@ -129,6 +129,13 @@ struct klp_patch {
> #define klp_for_each_func(obj, func) \
> for (func = obj->funcs; func->old_name; func++)
>
> +struct klp_reloc_sec {
> + unsigned int index;
> + char *name;
> + char *objname;
> + struct list_head list;
> +};
Description of the structure and its members is missing.
> diff --git a/include/linux/module.h b/include/linux/module.h
> index c8680b1..3c34eb8 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -793,9 +793,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
This would be solved after Rusty's and Josh's patches get merged, right?
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 087a8c7..26c419f 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -28,6 +28,8 @@
> #include <linux/list.h>
> #include <linux/kallsyms.h>
> #include <linux/livepatch.h>
> +#include <linux/elf.h>
> +#include <asm/cacheflush.h>
>
> /**
> * struct klp_ops - structure for tracking registered ftrace ops structs
> @@ -281,46 +283,54 @@ 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;
> + unsigned int bind;
> + char *symname;
> + struct klp_reloc_sec *reloc_sec;
> + struct load_info *info;
> + Elf_Rela *rela;
> + Elf_Sym *sym, *symtab;
> + Elf_Shdr *symsect;
>
> 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)) {
> - 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;
> + info = pmod->info;
> + symsect = info->sechdrs + info->index.sym;
> + symtab = (void *)info->hdr + symsect->sh_offset;
> +
> + /* For each __klp_rela section for this object */
> + list_for_each_entry(reloc_sec, &obj->reloc_secs, list) {
> + relindex = reloc_sec->index;
> + num_relas = info->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> + rela = (Elf_Rela *) info->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 = info->strtab + sym->st_name;
> + bind = ELF_ST_BIND(sym->st_info);
> +
> + if (sym->st_shndx == SHN_LIVEPATCH) {
> + if (bind == STB_LIVEPATCH_EXT)
> + 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(info->sechdrs, info->strtab,
> + info->index.sym, relindex, pmod);
> }
>
> - return 0;
> + return ret;
> }
Looking at this... do we even need reloc_secs in klp_object? Question is
whether we need more than one dynrela section for an object. If not then
the binding between klp_reloc_sec and an object is the only relevant thing
in the structure, be it index or objname. So we can replace the
list of structures with just the index in klp_object, or get rid of it
completely and rely on the name of dynrela section be something like
__klp_rela_{objname}.
You see, we go through elf sections here which were preserved by module
loader. We even have relevant sections marked with SHF_RELA_LIVEPATCH. So
maybe all the stuff around klp_reloc_sec is not necessary.
Thoughts?
> @@ -741,12 +751,23 @@ 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);
> + pmod = patch->mod;
> +
> + if (!list_empty(&obj->reloc_secs)) {
> + 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);
> }
And this would get solved with different patches as well. I think the
calls to set_page_attributes() should be hidden in
klp_write_object_relocations() as it is in Josh's patch IIRC.
Thanks,
Miroslav
--
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]
| From | Jessica Yu <jeyu@redhat.com> |
|---|---|
| Date | 2015-11-11 21:10 +0100 |
| Subject | Re: livepatch: reuse module loader code to write relocations |
| Message-ID | <qtJSq-7to-13@gated-at.bofh.it> |
| In reply to | #1267225 |
+++ Miroslav Benes [11/11/15 15:30 +0100]:
>On Mon, 9 Nov 2015, Jessica Yu wrote:
>
>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> index 31db7a0..601e892 100644
>> --- a/include/linux/livepatch.h
>> +++ b/include/linux/livepatch.h
>> @@ -85,7 +85,7 @@ struct klp_reloc {
>> /**
>> * 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 +95,7 @@ struct klp_reloc {
>> struct klp_object {
>> /* external */
>> const char *name;
>> - struct klp_reloc *relocs;
>> + struct list_head reloc_secs;
>> struct klp_func *funcs;
>
>So I guess we don't need klp_reloc anymore.
Yes, that's correct. I am noticing just now that I forgot to remove
the klp_reloc struct definition from livepatch.h. That change will be
reflected in v2...
>If true, we should really
>start thinking about proper documentation because there are going to be
>plenty of assumptions about a patch module and we need to have it written
>somewhere. Especially how the relocation sections look like.
Agreed. As a first step the patch module format can perhaps be
documented somewhere. Perhaps it's time we create
Documentation/livepatch/? :-)
>> /* internal */
>> @@ -129,6 +129,13 @@ struct klp_patch {
>> #define klp_for_each_func(obj, func) \
>> for (func = obj->funcs; func->old_name; func++)
>>
>> +struct klp_reloc_sec {
>> + unsigned int index;
>> + char *name;
>> + char *objname;
>> + struct list_head list;
>> +};
>
>Description of the structure and its members is missing.
>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index c8680b1..3c34eb8 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -793,9 +793,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
>
>This would be solved after Rusty's and Josh's patches get merged, right?
Yes, correct. When Josh and Rusty's patches get merged, I'll base
subsequent versions of this patchset on them.
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index 087a8c7..26c419f 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -28,6 +28,8 @@
>> #include <linux/list.h>
>> #include <linux/kallsyms.h>
>> #include <linux/livepatch.h>
>> +#include <linux/elf.h>
>> +#include <asm/cacheflush.h>
>>
>> /**
>> * struct klp_ops - structure for tracking registered ftrace ops structs
>> @@ -281,46 +283,54 @@ 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;
>> + unsigned int bind;
>> + char *symname;
>> + struct klp_reloc_sec *reloc_sec;
>> + struct load_info *info;
>> + Elf_Rela *rela;
>> + Elf_Sym *sym, *symtab;
>> + Elf_Shdr *symsect;
>>
>> 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)) {
>> - 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;
>> + info = pmod->info;
>> + symsect = info->sechdrs + info->index.sym;
>> + symtab = (void *)info->hdr + symsect->sh_offset;
>> +
>> + /* For each __klp_rela section for this object */
>> + list_for_each_entry(reloc_sec, &obj->reloc_secs, list) {
>> + relindex = reloc_sec->index;
>> + num_relas = info->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
>> + rela = (Elf_Rela *) info->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 = info->strtab + sym->st_name;
>> + bind = ELF_ST_BIND(sym->st_info);
>> +
>> + if (sym->st_shndx == SHN_LIVEPATCH) {
>> + if (bind == STB_LIVEPATCH_EXT)
>> + 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(info->sechdrs, info->strtab,
>> + info->index.sym, relindex, pmod);
>> }
>>
>> - return 0;
>> + return ret;
>> }
>
>Looking at this... do we even need reloc_secs in klp_object? Question is
>whether we need more than one dynrela section for an object. If not then
>the binding between klp_reloc_sec and an object is the only relevant thing
>in the structure, be it index or objname. So we can replace the
>list of structures with just the index in klp_object, or get rid of it
>completely and rely on the name of dynrela section be something like
>__klp_rela_{objname}.
Hm, you bring up a good point. I think theoretically yes, it is
possible to just have one klp_reloc_sec for each object and therefore
a list is not required (I have not checked yet how difficult it would
be to implement this on the kpatch-build side of things). However,
considering the final format of the patch module, I think it is
semantically clearer to leave it as a list, and for each object to
possibly have more than one __klp_rela section.
For example, say we are patching two functions in ext4. In my
resulting kpatch module I will have two __klp_rela_ext4 sections, and
they might look like this when we run readelf --sections:
[34] __klp_rela_ext4.text.ext4_attr_store RELA ...
[35] __klp_rela_ext4.text.ext4_attr_show RELA ...
Then these two klp rela sections end up as two elements in the
reloc_secs list for the ext4 patch object. I think this way, we can
better tell which rela is being applied to what function. Might be
easier to understand what's happening from the developer's point of
view.
>You see, we go through elf sections here which were preserved by module
>loader. We even have relevant sections marked with SHF_RELA_LIVEPATCH. So
>maybe all the stuff around klp_reloc_sec is not necessary.
>
>Thoughts?
Ah, so this is where descriptive comments and documentation might have
been useful :-) So I think we will still need to keep the
klp_reloc_sec struct to help the patch module initialize. Though the
name and objname fields aren't used in this patchset, they are used in
the kpatch patch module code [1], where we iterate through each elf
section, find the ones marked with SHF_RELA_LIVEPATCH, set the
klp_reloc_sec's objname (which we find from the "name" field,
formatted as __klp_rela_{objname}.text..). Once we have the objname
set, we can then find the object to attach the reloc_sec to (i.e. add
it to its list of reloc_secs).
Hope that clears some things up.
[1] https://github.com/flaming-toast/kpatch/blob/no_dynrela_redux/kmod/patch/livepatch-patch-hook.c#L213
>> @@ -741,12 +751,23 @@ 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);
>> + pmod = patch->mod;
>> +
>> + if (!list_empty(&obj->reloc_secs)) {
>> + 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);
>> }
>
>And this would get solved with different patches as well. I think the
>calls to set_page_attributes() should be hidden in
>klp_write_object_relocations() as it is in Josh's patch IIRC.
Yes, when those patches are merged I will switch over to Josh's
functions for setting/unsetting memory ro.
Thanks,
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]
| From | Miroslav Benes <mbenes@suse.cz> |
|---|---|
| Date | 2015-11-12 16:30 +0100 |
| Subject | Re: livepatch: reuse module loader code to write relocations |
| Message-ID | <qu1Z0-2aO-13@gated-at.bofh.it> |
| In reply to | #1267423 |
On Wed, 11 Nov 2015, Jessica Yu wrote:
> +++ Miroslav Benes [11/11/15 15:30 +0100]:
> > On Mon, 9 Nov 2015, Jessica Yu wrote:
> >
> > So I guess we don't need klp_reloc anymore.
>
> Yes, that's correct. I am noticing just now that I forgot to remove
> the klp_reloc struct definition from livepatch.h. That change will be
> reflected in v2...
>
> > If true, we should really
> > start thinking about proper documentation because there are going to be
> > plenty of assumptions about a patch module and we need to have it written
> > somewhere. Especially how the relocation sections look like.
>
> Agreed. As a first step the patch module format can perhaps be
> documented somewhere. Perhaps it's time we create
> Documentation/livepatch/? :-)
Yes, I think so.
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index 087a8c7..26c419f 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -28,6 +28,8 @@
> > > #include <linux/list.h>
> > > #include <linux/kallsyms.h>
> > > #include <linux/livepatch.h>
> > > +#include <linux/elf.h>
> > > +#include <asm/cacheflush.h>
> > >
> > > /**
> > > * struct klp_ops - structure for tracking registered ftrace ops structs
> > > @@ -281,46 +283,54 @@ 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;
> > > + unsigned int bind;
> > > + char *symname;
> > > + struct klp_reloc_sec *reloc_sec;
> > > + struct load_info *info;
> > > + Elf_Rela *rela;
> > > + Elf_Sym *sym, *symtab;
> > > + Elf_Shdr *symsect;
> > >
> > > 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)) {
> > > - 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;
> > > + info = pmod->info;
> > > + symsect = info->sechdrs + info->index.sym;
> > > + symtab = (void *)info->hdr + symsect->sh_offset;
> > > +
> > > + /* For each __klp_rela section for this object */
> > > + list_for_each_entry(reloc_sec, &obj->reloc_secs, list) {
> > > + relindex = reloc_sec->index;
> > > + num_relas = info->sechdrs[relindex].sh_size /
> > > sizeof(Elf_Rela);
> > > + rela = (Elf_Rela *) info->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 = info->strtab + sym->st_name;
> > > + bind = ELF_ST_BIND(sym->st_info);
> > > +
> > > + if (sym->st_shndx == SHN_LIVEPATCH) {
> > > + if (bind == STB_LIVEPATCH_EXT)
> > > + 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(info->sechdrs, info->strtab,
> > > + info->index.sym, relindex, pmod);
> > > }
> > >
> > > - return 0;
> > > + return ret;
> > > }
> >
> > Looking at this... do we even need reloc_secs in klp_object? Question is
> > whether we need more than one dynrela section for an object. If not then
> > the binding between klp_reloc_sec and an object is the only relevant thing
> > in the structure, be it index or objname. So we can replace the
> > list of structures with just the index in klp_object, or get rid of it
> > completely and rely on the name of dynrela section be something like
> > __klp_rela_{objname}.
>
> Hm, you bring up a good point. I think theoretically yes, it is
> possible to just have one klp_reloc_sec for each object and therefore
> a list is not required (I have not checked yet how difficult it would
> be to implement this on the kpatch-build side of things). However,
> considering the final format of the patch module, I think it is
> semantically clearer to leave it as a list, and for each object to
> possibly have more than one __klp_rela section.
>
> For example, say we are patching two functions in ext4. In my
> resulting kpatch module I will have two __klp_rela_ext4 sections, and
> they might look like this when we run readelf --sections:
>
> [34] __klp_rela_ext4.text.ext4_attr_store RELA ...
> [35] __klp_rela_ext4.text.ext4_attr_show RELA ...
>
> Then these two klp rela sections end up as two elements in the
> reloc_secs list for the ext4 patch object. I think this way, we can
> better tell which rela is being applied to what function. Might be
> easier to understand what's happening from the developer's point of
> view.
>
> > You see, we go through elf sections here which were preserved by module
> > loader. We even have relevant sections marked with SHF_RELA_LIVEPATCH. So
> > maybe all the stuff around klp_reloc_sec is not necessary.
> >
> > Thoughts?
>
> Ah, so this is where descriptive comments and documentation might have
> been useful :-) So I think we will still need to keep the
> klp_reloc_sec struct to help the patch module initialize. Though the
> name and objname fields aren't used in this patchset, they are used in
> the kpatch patch module code [1], where we iterate through each elf
> section, find the ones marked with SHF_RELA_LIVEPATCH, set the
> klp_reloc_sec's objname (which we find from the "name" field,
> formatted as __klp_rela_{objname}.text..). Once we have the objname
> set, we can then find the object to attach the reloc_sec to (i.e. add
> it to its list of reloc_secs).
>
> Hope that clears some things up.
Ok, I'll try to explain myself and it is gonna be long. I'll try to
describe how we deal with dynrelas in klp today, how you use it in kpatch
(and this is the place where my knowledge can be wrong or obsolete), what
you propose and what I'd like to propose.
1. First let's look on what we have now.
We have a patch module in which there is a section with all dynrelas
needed to be resolved (it was like this in kpatch some time ago and maybe
it is different now so just have a patience, I'll get to it). In the init
function of the module kpatch builds all the relevant info from dynrela
section. It goes through it, creates an array of klp_reloc for each object
and includes each dynrela record with an objname to the array of
klp_object with that objname. Later when we need to apply relocations for
patched object we just go through the list (array) of its dynrelas in
klp_object and call our arch-specific klp_write_module_reloc().
Now this was probably changed in kpatch and you do not have one dynrela
section but one dynrela section for each patched function. Is that
correct? (and can you tell us what the reason for the change was? It might
be crucial because I might be missing something.). Which leads us to your
proposal...
2. So we have several dynrela section for one patched object. During init
function in a patch module kpatch once again builds needed structures from
these sections. Now they are called klp_reloc_sec and contain different
kind of info. There is no val, loc and such, only name of the symbol,
objname and index to dynrela section in ELF. So when you need to write
relocations for the patched object you go through all relevant dynrela
sections (because they are stored in the klp_object), all dynrela records
in each section and you resolve the undefined symbols. All needed info is
stored in ELF. Then you just call apply_relocate_add().
3. I propose to go one step further. I think we don't need klp_reloc_sec
if there is only one dynrela section for patched object (and I currently
cannot see why this is not possible. It is possible even with one dynrela
section for whole patch module, that is for all patched objects.).
In my proposal there would be nothing done in init function of the patched
module (albeit some optimization mentioned later). When you call
klp_write_object_relocations you would go through all ELF section and find
the relevant one for the object (it is marked with SHF_RELA_LIVEPATCH and
objname is in the name of the section. It is the same thing you do in 2.
in the init function.). Then you go through all dynrela records in the
section, you do the same things which you do in the proposed patch above,
and call apply_relocate_add.
Now it would be crazy to go through all sections each time
klp_write_object_relocations is called (maybe it does not even matter but
nevertheless). So klp_object could have an index to its ELF dynrela
section. This index can be retrieved in the init function the same way you
do all the stuff with klp_reloc_sec.
If you insisted on more than one dynrela section for a patched object we
could have an array of indices there. Or whatever.
It is almost the same as your proposal but in my opinion somewhat nicer.
We just use the info stored in ELF directly without unnecessary middle
layer (== klp_reloc_sec).
Does it make sense? I hope it does. Would it work?
Miroslav
--
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]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-11-12 18:50 +0100 |
| Subject | Re: livepatch: reuse module loader code to write relocations |
| Message-ID | <qu4at-3v0-13@gated-at.bofh.it> |
| In reply to | #1268029 |
On Thu, Nov 12, 2015 at 04:27:01PM +0100, Miroslav Benes wrote:
> On Wed, 11 Nov 2015, Jessica Yu wrote:
>
> > +++ Miroslav Benes [11/11/15 15:30 +0100]:
> > > On Mon, 9 Nov 2015, Jessica Yu wrote:
> > >
> > > So I guess we don't need klp_reloc anymore.
> >
> > Yes, that's correct. I am noticing just now that I forgot to remove
> > the klp_reloc struct definition from livepatch.h. That change will be
> > reflected in v2...
> >
> > > If true, we should really
> > > start thinking about proper documentation because there are going to be
> > > plenty of assumptions about a patch module and we need to have it written
> > > somewhere. Especially how the relocation sections look like.
> >
> > Agreed. As a first step the patch module format can perhaps be
> > documented somewhere. Perhaps it's time we create
> > Documentation/livepatch/? :-)
>
> Yes, I think so.
>
> > > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > > index 087a8c7..26c419f 100644
> > > > --- a/kernel/livepatch/core.c
> > > > +++ b/kernel/livepatch/core.c
> > > > @@ -28,6 +28,8 @@
> > > > #include <linux/list.h>
> > > > #include <linux/kallsyms.h>
> > > > #include <linux/livepatch.h>
> > > > +#include <linux/elf.h>
> > > > +#include <asm/cacheflush.h>
> > > >
> > > > /**
> > > > * struct klp_ops - structure for tracking registered ftrace ops structs
> > > > @@ -281,46 +283,54 @@ 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;
> > > > + unsigned int bind;
> > > > + char *symname;
> > > > + struct klp_reloc_sec *reloc_sec;
> > > > + struct load_info *info;
> > > > + Elf_Rela *rela;
> > > > + Elf_Sym *sym, *symtab;
> > > > + Elf_Shdr *symsect;
> > > >
> > > > 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)) {
> > > > - 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;
> > > > + info = pmod->info;
> > > > + symsect = info->sechdrs + info->index.sym;
> > > > + symtab = (void *)info->hdr + symsect->sh_offset;
> > > > +
> > > > + /* For each __klp_rela section for this object */
> > > > + list_for_each_entry(reloc_sec, &obj->reloc_secs, list) {
> > > > + relindex = reloc_sec->index;
> > > > + num_relas = info->sechdrs[relindex].sh_size /
> > > > sizeof(Elf_Rela);
> > > > + rela = (Elf_Rela *) info->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 = info->strtab + sym->st_name;
> > > > + bind = ELF_ST_BIND(sym->st_info);
> > > > +
> > > > + if (sym->st_shndx == SHN_LIVEPATCH) {
> > > > + if (bind == STB_LIVEPATCH_EXT)
> > > > + 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(info->sechdrs, info->strtab,
> > > > + info->index.sym, relindex, pmod);
> > > > }
> > > >
> > > > - return 0;
> > > > + return ret;
> > > > }
> > >
> > > Looking at this... do we even need reloc_secs in klp_object? Question is
> > > whether we need more than one dynrela section for an object. If not then
> > > the binding between klp_reloc_sec and an object is the only relevant thing
> > > in the structure, be it index or objname. So we can replace the
> > > list of structures with just the index in klp_object, or get rid of it
> > > completely and rely on the name of dynrela section be something like
> > > __klp_rela_{objname}.
> >
> > Hm, you bring up a good point. I think theoretically yes, it is
> > possible to just have one klp_reloc_sec for each object and therefore
> > a list is not required (I have not checked yet how difficult it would
> > be to implement this on the kpatch-build side of things). However,
> > considering the final format of the patch module, I think it is
> > semantically clearer to leave it as a list, and for each object to
> > possibly have more than one __klp_rela section.
> >
> > For example, say we are patching two functions in ext4. In my
> > resulting kpatch module I will have two __klp_rela_ext4 sections, and
> > they might look like this when we run readelf --sections:
> >
> > [34] __klp_rela_ext4.text.ext4_attr_store RELA ...
> > [35] __klp_rela_ext4.text.ext4_attr_show RELA ...
> >
> > Then these two klp rela sections end up as two elements in the
> > reloc_secs list for the ext4 patch object. I think this way, we can
> > better tell which rela is being applied to what function. Might be
> > easier to understand what's happening from the developer's point of
> > view.
> >
> > > You see, we go through elf sections here which were preserved by module
> > > loader. We even have relevant sections marked with SHF_RELA_LIVEPATCH. So
> > > maybe all the stuff around klp_reloc_sec is not necessary.
> > >
> > > Thoughts?
> >
> > Ah, so this is where descriptive comments and documentation might have
> > been useful :-) So I think we will still need to keep the
> > klp_reloc_sec struct to help the patch module initialize. Though the
> > name and objname fields aren't used in this patchset, they are used in
> > the kpatch patch module code [1], where we iterate through each elf
> > section, find the ones marked with SHF_RELA_LIVEPATCH, set the
> > klp_reloc_sec's objname (which we find from the "name" field,
> > formatted as __klp_rela_{objname}.text..). Once we have the objname
> > set, we can then find the object to attach the reloc_sec to (i.e. add
> > it to its list of reloc_secs).
> >
> > Hope that clears some things up.
>
> Ok, I'll try to explain myself and it is gonna be long. I'll try to
> describe how we deal with dynrelas in klp today, how you use it in kpatch
> (and this is the place where my knowledge can be wrong or obsolete), what
> you propose and what I'd like to propose.
>
> 1. First let's look on what we have now.
>
> We have a patch module in which there is a section with all dynrelas
> needed to be resolved (it was like this in kpatch some time ago and maybe
> it is different now so just have a patience, I'll get to it). In the init
> function of the module kpatch builds all the relevant info from dynrela
> section. It goes through it, creates an array of klp_reloc for each object
> and includes each dynrela record with an objname to the array of
> klp_object with that objname. Later when we need to apply relocations for
> patched object we just go through the list (array) of its dynrelas in
> klp_object and call our arch-specific klp_write_module_reloc().
>
> Now this was probably changed in kpatch and you do not have one dynrela
> section but one dynrela section for each patched function. Is that
> correct? (and can you tell us what the reason for the change was? It might
> be crucial because I might be missing something.). Which leads us to your
> proposal...
>
> 2. So we have several dynrela section for one patched object. During init
> function in a patch module kpatch once again builds needed structures from
> these sections. Now they are called klp_reloc_sec and contain different
> kind of info. There is no val, loc and such, only name of the symbol,
> objname and index to dynrela section in ELF. So when you need to write
> relocations for the patched object you go through all relevant dynrela
> sections (because they are stored in the klp_object), all dynrela records
> in each section and you resolve the undefined symbols. All needed info is
> stored in ELF. Then you just call apply_relocate_add().
>
> 3. I propose to go one step further. I think we don't need klp_reloc_sec
> if there is only one dynrela section for patched object (and I currently
> cannot see why this is not possible. It is possible even with one dynrela
> section for whole patch module, that is for all patched objects.).
I think I agree that we don't need klp_reloc_sec, and that klp rela
sections can presumably be discovered by iterating over the sections.
But I don't think it matters much whether we have one klp rela section
per object, or multiple rela sections per object. Either way, can't we
find them when we iterate over the sections?
For example, for one rela section per object, it could be named:
__klp_rela.objname
Or for multiple rela sections per object, they could be named:
__klp_rela.objname.func1
__klp_rela.objname.func2
Either way, when iterating over the sections, we could just look for
"__klp_rela.objname".
All that said, I think I would vote for one rela section per object,
just because it seems simpler.
>
> In my proposal there would be nothing done in init function of the patched
> module (albeit some optimization mentioned later). When you call
> klp_write_object_relocations you would go through all ELF section and find
> the relevant one for the object (it is marked with SHF_RELA_LIVEPATCH and
> objname is in the name of the section. It is the same thing you do in 2.
> in the init function.). Then you go through all dynrela records in the
> section, you do the same things which you do in the proposed patch above,
> and call apply_relocate_add.
>
> Now it would be crazy to go through all sections each time
> klp_write_object_relocations is called (maybe it does not even matter but
Why would that be crazy? To me it seems perfectly logical :-) It
doesn't seem like a very expensive operation to me.
> nevertheless). So klp_object could have an index to its ELF dynrela
> section. This index can be retrieved in the init function the same way you
> do all the stuff with klp_reloc_sec.
>
> If you insisted on more than one dynrela section for a patched object we
> could have an array of indices there. Or whatever.
>
> It is almost the same as your proposal but in my opinion somewhat nicer.
> We just use the info stored in ELF directly without unnecessary middle
> layer (== klp_reloc_sec).
>
> Does it make sense? I hope it does. Would it work?
--
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]
| From | Jessica Yu <jeyu@redhat.com> |
|---|---|
| Date | 2015-11-12 21:30 +0100 |
| Subject | Re: livepatch: reuse module loader code to write relocations |
| Message-ID | <qu6Fk-5dX-5@gated-at.bofh.it> |
| In reply to | #1268141 |
+++ Josh Poimboeuf [12/11/15 11:40 -0600]:
>On Thu, Nov 12, 2015 at 04:27:01PM +0100, Miroslav Benes wrote:
>> On Wed, 11 Nov 2015, Jessica Yu wrote:
>>
>> > +++ Miroslav Benes [11/11/15 15:30 +0100]:
>> > > On Mon, 9 Nov 2015, Jessica Yu wrote:
>> > >
>> > > So I guess we don't need klp_reloc anymore.
>> >
>> > Yes, that's correct. I am noticing just now that I forgot to remove
>> > the klp_reloc struct definition from livepatch.h. That change will be
>> > reflected in v2...
>> >
>> > > If true, we should really
>> > > start thinking about proper documentation because there are going to be
>> > > plenty of assumptions about a patch module and we need to have it written
>> > > somewhere. Especially how the relocation sections look like.
>> >
>> > Agreed. As a first step the patch module format can perhaps be
>> > documented somewhere. Perhaps it's time we create
>> > Documentation/livepatch/? :-)
>>
>> Yes, I think so.
>>
>> > > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> > > > index 087a8c7..26c419f 100644
>> > > > --- a/kernel/livepatch/core.c
>> > > > +++ b/kernel/livepatch/core.c
>> > > > @@ -28,6 +28,8 @@
>> > > > #include <linux/list.h>
>> > > > #include <linux/kallsyms.h>
>> > > > #include <linux/livepatch.h>
>> > > > +#include <linux/elf.h>
>> > > > +#include <asm/cacheflush.h>
>> > > >
>> > > > /**
>> > > > * struct klp_ops - structure for tracking registered ftrace ops structs
>> > > > @@ -281,46 +283,54 @@ 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;
>> > > > + unsigned int bind;
>> > > > + char *symname;
>> > > > + struct klp_reloc_sec *reloc_sec;
>> > > > + struct load_info *info;
>> > > > + Elf_Rela *rela;
>> > > > + Elf_Sym *sym, *symtab;
>> > > > + Elf_Shdr *symsect;
>> > > >
>> > > > 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)) {
>> > > > - 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;
>> > > > + info = pmod->info;
>> > > > + symsect = info->sechdrs + info->index.sym;
>> > > > + symtab = (void *)info->hdr + symsect->sh_offset;
>> > > > +
>> > > > + /* For each __klp_rela section for this object */
>> > > > + list_for_each_entry(reloc_sec, &obj->reloc_secs, list) {
>> > > > + relindex = reloc_sec->index;
>> > > > + num_relas = info->sechdrs[relindex].sh_size /
>> > > > sizeof(Elf_Rela);
>> > > > + rela = (Elf_Rela *) info->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 = info->strtab + sym->st_name;
>> > > > + bind = ELF_ST_BIND(sym->st_info);
>> > > > +
>> > > > + if (sym->st_shndx == SHN_LIVEPATCH) {
>> > > > + if (bind == STB_LIVEPATCH_EXT)
>> > > > + 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(info->sechdrs, info->strtab,
>> > > > + info->index.sym, relindex, pmod);
>> > > > }
>> > > >
>> > > > - return 0;
>> > > > + return ret;
>> > > > }
>> > >
>> > > Looking at this... do we even need reloc_secs in klp_object? Question is
>> > > whether we need more than one dynrela section for an object. If not then
>> > > the binding between klp_reloc_sec and an object is the only relevant thing
>> > > in the structure, be it index or objname. So we can replace the
>> > > list of structures with just the index in klp_object, or get rid of it
>> > > completely and rely on the name of dynrela section be something like
>> > > __klp_rela_{objname}.
>> >
>> > Hm, you bring up a good point. I think theoretically yes, it is
>> > possible to just have one klp_reloc_sec for each object and therefore
>> > a list is not required (I have not checked yet how difficult it would
>> > be to implement this on the kpatch-build side of things). However,
>> > considering the final format of the patch module, I think it is
>> > semantically clearer to leave it as a list, and for each object to
>> > possibly have more than one __klp_rela section.
>> >
>> > For example, say we are patching two functions in ext4. In my
>> > resulting kpatch module I will have two __klp_rela_ext4 sections, and
>> > they might look like this when we run readelf --sections:
>> >
>> > [34] __klp_rela_ext4.text.ext4_attr_store RELA ...
>> > [35] __klp_rela_ext4.text.ext4_attr_show RELA ...
>> >
>> > Then these two klp rela sections end up as two elements in the
>> > reloc_secs list for the ext4 patch object. I think this way, we can
>> > better tell which rela is being applied to what function. Might be
>> > easier to understand what's happening from the developer's point of
>> > view.
>> >
>> > > You see, we go through elf sections here which were preserved by module
>> > > loader. We even have relevant sections marked with SHF_RELA_LIVEPATCH. So
>> > > maybe all the stuff around klp_reloc_sec is not necessary.
>> > >
>> > > Thoughts?
>> >
>> > Ah, so this is where descriptive comments and documentation might have
>> > been useful :-) So I think we will still need to keep the
>> > klp_reloc_sec struct to help the patch module initialize. Though the
>> > name and objname fields aren't used in this patchset, they are used in
>> > the kpatch patch module code [1], where we iterate through each elf
>> > section, find the ones marked with SHF_RELA_LIVEPATCH, set the
>> > klp_reloc_sec's objname (which we find from the "name" field,
>> > formatted as __klp_rela_{objname}.text..). Once we have the objname
>> > set, we can then find the object to attach the reloc_sec to (i.e. add
>> > it to its list of reloc_secs).
>> >
>> > Hope that clears some things up.
>>
>> Ok, I'll try to explain myself and it is gonna be long. I'll try to
>> describe how we deal with dynrelas in klp today, how you use it in kpatch
>> (and this is the place where my knowledge can be wrong or obsolete), what
>> you propose and what I'd like to propose.
>>
>> 1. First let's look on what we have now.
>>
>> We have a patch module in which there is a section with all dynrelas
>> needed to be resolved (it was like this in kpatch some time ago and maybe
>> it is different now so just have a patience, I'll get to it). In the init
>> function of the module kpatch builds all the relevant info from dynrela
>> section. It goes through it, creates an array of klp_reloc for each object
>> and includes each dynrela record with an objname to the array of
>> klp_object with that objname. Later when we need to apply relocations for
>> patched object we just go through the list (array) of its dynrelas in
>> klp_object and call our arch-specific klp_write_module_reloc().
>>
>> Now this was probably changed in kpatch and you do not have one dynrela
>> section but one dynrela section for each patched function. Is that
>> correct? (and can you tell us what the reason for the change was? It might
>> be crucial because I might be missing something.). Which leads us to your
>> proposal...
>>
>> 2. So we have several dynrela section for one patched object. During init
>> function in a patch module kpatch once again builds needed structures from
>> these sections. Now they are called klp_reloc_sec and contain different
>> kind of info. There is no val, loc and such, only name of the symbol,
>> objname and index to dynrela section in ELF. So when you need to write
>> relocations for the patched object you go through all relevant dynrela
>> sections (because they are stored in the klp_object), all dynrela records
>> in each section and you resolve the undefined symbols. All needed info is
>> stored in ELF. Then you just call apply_relocate_add().
>>
>> 3. I propose to go one step further. I think we don't need klp_reloc_sec
>> if there is only one dynrela section for patched object (and I currently
>> cannot see why this is not possible. It is possible even with one dynrela
>> section for whole patch module, that is for all patched objects.).
>
>I think I agree that we don't need klp_reloc_sec, and that klp rela
>sections can presumably be discovered by iterating over the sections.
>
>But I don't think it matters much whether we have one klp rela section
>per object, or multiple rela sections per object. Either way, can't we
>find them when we iterate over the sections?
>
>For example, for one rela section per object, it could be named:
>
>__klp_rela.objname
>
>Or for multiple rela sections per object, they could be named:
>
>__klp_rela.objname.func1
>__klp_rela.objname.func2
>
>Either way, when iterating over the sections, we could just look for
>"__klp_rela.objname".
>
>All that said, I think I would vote for one rela section per object,
>just because it seems simpler.
Looking into this more, I think we do need one __klp_rela section per
function being patched. Each rela section is linked to the section to
which the relocations apply via the rela section's sh_info field. In
SHT_RELA sections, the sh_info field contains the section index to
which the relocs apply. We cannot have one single combined rela
section per object as the call to apply_relocate_add() simply won't
work, because we would have relocs that apply to different functions
(and hence different sections).
So I guess instead of a single field in klp_object specifying the
__klp_rela section index, we could probably just have an array of
section indices.
>>
>> In my proposal there would be nothing done in init function of the patched
>> module (albeit some optimization mentioned later). When you call
>> klp_write_object_relocations you would go through all ELF section and find
>> the relevant one for the object (it is marked with SHF_RELA_LIVEPATCH and
>> objname is in the name of the section. It is the same thing you do in 2.
>> in the init function.). Then you go through all dynrela records in the
>> section, you do the same things which you do in the proposed patch above,
>> and call apply_relocate_add.
>>
>> Now it would be crazy to go through all sections each time
>> klp_write_object_relocations is called (maybe it does not even matter but
>
>Why would that be crazy? To me it seems perfectly logical :-) It
>doesn't seem like a very expensive operation to me.
>
>> nevertheless). So klp_object could have an index to its ELF dynrela
>> section. This index can be retrieved in the init function the same way you
>> do all the stuff with klp_reloc_sec.
>>
>> If you insisted on more than one dynrela section for a patched object we
>> could have an array of indices there. Or whatever.
>>
>> It is almost the same as your proposal but in my opinion somewhat nicer.
>> We just use the info stored in ELF directly without unnecessary middle
>> layer (== klp_reloc_sec).
>>
>> Does it make sense? I hope it does. Would it work?
>
>--
>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]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-11-12 21:40 +0100 |
| Subject | Re: livepatch: reuse module loader code to write relocations |
| Message-ID | <qu6P1-5ho-35@gated-at.bofh.it> |
| In reply to | #1268246 |
On Thu, Nov 12, 2015 at 03:22:44PM -0500, Jessica Yu wrote: > Looking into this more, I think we do need one __klp_rela section per > function being patched. Each rela section is linked to the section to > which the relocations apply via the rela section's sh_info field. In > SHT_RELA sections, the sh_info field contains the section index to > which the relocs apply. We cannot have one single combined rela > section per object as the call to apply_relocate_add() simply won't > work, because we would have relocs that apply to different functions > (and hence different sections). > > So I guess instead of a single field in klp_object specifying the > __klp_rela section index, we could probably just have an array of > section indices. Ok, makes sense, sounds like we need multiple klp relas per object. I still don't quite understand the benefit of caching the klp_rela section indices. What problem does it solve? It seems simpler to just iterate over all the sections in klp_write_object_relocations(). -- 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]
| From | Jessica Yu <jeyu@redhat.com> |
|---|---|
| Date | 2015-11-13 08:20 +0100 |
| Subject | Re: livepatch: reuse module loader code to write relocations |
| Message-ID | <qugOl-3k9-1@gated-at.bofh.it> |
| In reply to | #1268255 |
+++ Josh Poimboeuf [12/11/15 14:32 -0600]: >On Thu, Nov 12, 2015 at 03:22:44PM -0500, Jessica Yu wrote: >> Looking into this more, I think we do need one __klp_rela section per >> function being patched. Each rela section is linked to the section to >> which the relocations apply via the rela section's sh_info field. In >> SHT_RELA sections, the sh_info field contains the section index to >> which the relocs apply. We cannot have one single combined rela >> section per object as the call to apply_relocate_add() simply won't >> work, because we would have relocs that apply to different functions >> (and hence different sections). >> >> So I guess instead of a single field in klp_object specifying the >> __klp_rela section index, we could probably just have an array of >> section indices. > >Ok, makes sense, sounds like we need multiple klp relas per object. > >I still don't quite understand the benefit of caching the klp_rela >section indices. What problem does it solve? It seems simpler to just >iterate over all the sections in klp_write_object_relocations(). I was trying to accomodate Miroslav's dislike of klp_reloc_sec :-) But now that we are sure that we need multiple __klp_rela sections per object, I think an array of indices might still be somewhat complicated (we need to figure out its size and everything, a list here would be easier). I think it might be fine to leave things the way they are here. 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]
| From | Miroslav Benes <mbenes@suse.cz> |
|---|---|
| Date | 2015-11-13 15:00 +0100 |
| Subject | Re: livepatch: reuse module loader code to write relocations |
| Message-ID | <qun3z-77o-145@gated-at.bofh.it> |
| In reply to | #1268255 |
On Thu, 12 Nov 2015, Josh Poimboeuf wrote: > On Thu, Nov 12, 2015 at 03:22:44PM -0500, Jessica Yu wrote: > > Looking into this more, I think we do need one __klp_rela section per > > function being patched. Each rela section is linked to the section to > > which the relocations apply via the rela section's sh_info field. In > > SHT_RELA sections, the sh_info field contains the section index to > > which the relocs apply. We cannot have one single combined rela > > section per object as the call to apply_relocate_add() simply won't > > work, because we would have relocs that apply to different functions > > (and hence different sections). > > > > So I guess instead of a single field in klp_object specifying the > > __klp_rela section index, we could probably just have an array of > > section indices. > > Ok, makes sense, sounds like we need multiple klp relas per object. Ok, it seems so. > I still don't quite understand the benefit of caching the klp_rela > section indices. What problem does it solve? It seems simpler to just > iterate over all the sections in klp_write_object_relocations(). It was just my need to be efficient and I think it would have made sense with only one dynrela section per object. An array of indices is "ugly" so I am all for iteration over all the sections in klp_write_object_relocations(). Miroslav -- 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]
| From | Jessica Yu <jeyu@redhat.com> |
|---|---|
| Date | 2015-11-12 20:20 +0100 |
| Subject | Re: livepatch: reuse module loader code to write relocations |
| Message-ID | <qu5zA-4wv-27@gated-at.bofh.it> |
| In reply to | #1268029 |
+++ Miroslav Benes [12/11/15 16:27 +0100]:
>On Wed, 11 Nov 2015, Jessica Yu wrote:
>
>> +++ Miroslav Benes [11/11/15 15:30 +0100]:
>> > On Mon, 9 Nov 2015, Jessica Yu wrote:
>> >
>> > So I guess we don't need klp_reloc anymore.
>>
>> Yes, that's correct. I am noticing just now that I forgot to remove
>> the klp_reloc struct definition from livepatch.h. That change will be
>> reflected in v2...
>>
>> > If true, we should really
>> > start thinking about proper documentation because there are going to be
>> > plenty of assumptions about a patch module and we need to have it written
>> > somewhere. Especially how the relocation sections look like.
>>
>> Agreed. As a first step the patch module format can perhaps be
>> documented somewhere. Perhaps it's time we create
>> Documentation/livepatch/? :-)
>
>Yes, I think so.
>
>> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> > > index 087a8c7..26c419f 100644
>> > > --- a/kernel/livepatch/core.c
>> > > +++ b/kernel/livepatch/core.c
>> > > @@ -28,6 +28,8 @@
>> > > #include <linux/list.h>
>> > > #include <linux/kallsyms.h>
>> > > #include <linux/livepatch.h>
>> > > +#include <linux/elf.h>
>> > > +#include <asm/cacheflush.h>
>> > >
>> > > /**
>> > > * struct klp_ops - structure for tracking registered ftrace ops structs
>> > > @@ -281,46 +283,54 @@ 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;
>> > > + unsigned int bind;
>> > > + char *symname;
>> > > + struct klp_reloc_sec *reloc_sec;
>> > > + struct load_info *info;
>> > > + Elf_Rela *rela;
>> > > + Elf_Sym *sym, *symtab;
>> > > + Elf_Shdr *symsect;
>> > >
>> > > 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)) {
>> > > - 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;
>> > > + info = pmod->info;
>> > > + symsect = info->sechdrs + info->index.sym;
>> > > + symtab = (void *)info->hdr + symsect->sh_offset;
>> > > +
>> > > + /* For each __klp_rela section for this object */
>> > > + list_for_each_entry(reloc_sec, &obj->reloc_secs, list) {
>> > > + relindex = reloc_sec->index;
>> > > + num_relas = info->sechdrs[relindex].sh_size /
>> > > sizeof(Elf_Rela);
>> > > + rela = (Elf_Rela *) info->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 = info->strtab + sym->st_name;
>> > > + bind = ELF_ST_BIND(sym->st_info);
>> > > +
>> > > + if (sym->st_shndx == SHN_LIVEPATCH) {
>> > > + if (bind == STB_LIVEPATCH_EXT)
>> > > + 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(info->sechdrs, info->strtab,
>> > > + info->index.sym, relindex, pmod);
>> > > }
>> > >
>> > > - return 0;
>> > > + return ret;
>> > > }
>> >
>> > Looking at this... do we even need reloc_secs in klp_object? Question is
>> > whether we need more than one dynrela section for an object. If not then
>> > the binding between klp_reloc_sec and an object is the only relevant thing
>> > in the structure, be it index or objname. So we can replace the
>> > list of structures with just the index in klp_object, or get rid of it
>> > completely and rely on the name of dynrela section be something like
>> > __klp_rela_{objname}.
>>
>> Hm, you bring up a good point. I think theoretically yes, it is
>> possible to just have one klp_reloc_sec for each object and therefore
>> a list is not required (I have not checked yet how difficult it would
>> be to implement this on the kpatch-build side of things). However,
>> considering the final format of the patch module, I think it is
>> semantically clearer to leave it as a list, and for each object to
>> possibly have more than one __klp_rela section.
>>
>> For example, say we are patching two functions in ext4. In my
>> resulting kpatch module I will have two __klp_rela_ext4 sections, and
>> they might look like this when we run readelf --sections:
>>
>> [34] __klp_rela_ext4.text.ext4_attr_store RELA ...
>> [35] __klp_rela_ext4.text.ext4_attr_show RELA ...
>>
>> Then these two klp rela sections end up as two elements in the
>> reloc_secs list for the ext4 patch object. I think this way, we can
>> better tell which rela is being applied to what function. Might be
>> easier to understand what's happening from the developer's point of
>> view.
>>
>> > You see, we go through elf sections here which were preserved by module
>> > loader. We even have relevant sections marked with SHF_RELA_LIVEPATCH. So
>> > maybe all the stuff around klp_reloc_sec is not necessary.
>> >
>> > Thoughts?
>>
>> Ah, so this is where descriptive comments and documentation might have
>> been useful :-) So I think we will still need to keep the
>> klp_reloc_sec struct to help the patch module initialize. Though the
>> name and objname fields aren't used in this patchset, they are used in
>> the kpatch patch module code [1], where we iterate through each elf
>> section, find the ones marked with SHF_RELA_LIVEPATCH, set the
>> klp_reloc_sec's objname (which we find from the "name" field,
>> formatted as __klp_rela_{objname}.text..). Once we have the objname
>> set, we can then find the object to attach the reloc_sec to (i.e. add
>> it to its list of reloc_secs).
>>
>> Hope that clears some things up.
>
>Ok, I'll try to explain myself and it is gonna be long. I'll try to
>describe how we deal with dynrelas in klp today, how you use it in kpatch
>(and this is the place where my knowledge can be wrong or obsolete), what
>you propose and what I'd like to propose.
>
>1. First let's look on what we have now.
>
>We have a patch module in which there is a section with all dynrelas
>needed to be resolved (it was like this in kpatch some time ago and maybe
>it is different now so just have a patience, I'll get to it). In the init
>function of the module kpatch builds all the relevant info from dynrela
>section. It goes through it, creates an array of klp_reloc for each object
>and includes each dynrela record with an objname to the array of
>klp_object with that objname. Later when we need to apply relocations for
>patched object we just go through the list (array) of its dynrelas in
>klp_object and call our arch-specific klp_write_module_reloc().
Sounds correct to me.
>Now this was probably changed in kpatch and you do not have one dynrela
>section but one dynrela section for each patched function. Is that
>correct? (and can you tell us what the reason for the change was? It might
>be crucial because I might be missing something.). Which leads us to your
>proposal...
Your original assumption was correct; current kpatch has one big
.kpatch.dynrelas section, and each dynrela entry within that single
section gets sorted to the correct object as you described above. The
one dynrela section per patched function only came with this patchset,
but for no particular reason other than to make the kpatch-build code
for generating the patch module slightly less complicated. But I
haven't checked how big of a change it would be to do
one-dynrela-section per object, perhaps (and I hope) it will be an
easy change.
>2. So we have several dynrela section for one patched object. During init
>function in a patch module kpatch once again builds needed structures from
>these sections. Now they are called klp_reloc_sec and contain different
>kind of info. There is no val, loc and such, only name of the symbol,
>objname and index to dynrela section in ELF. So when you need to write
>relocations for the patched object you go through all relevant dynrela
>sections (because they are stored in the klp_object), all dynrela records
>in each section and you resolve the undefined symbols. All needed info is
>stored in ELF. Then you just call apply_relocate_add().
>
>3. I propose to go one step further. I think we don't need klp_reloc_sec
>if there is only one dynrela section for patched object (and I currently
>cannot see why this is not possible. It is possible even with one dynrela
>section for whole patch module, that is for all patched objects.).
I think the furthest we can go in terms of simplifying klp rela secs
is to have one __klp_rela section per object. We can't smush all the
__klp_rela_objname sections into one big __klp_rela section since we
could be patching some objects that won't be loaded yet, and
apply_relocate_add() needs to work with real SHT_RELA sections + their
corresponding section indices (i.e., we cannot call
apply_relocate_add() with that single, combined klp rela section).
So, I think I would be OK with one __klp_rela section per object.
>In my proposal there would be nothing done in init function of the patched
>module (albeit some optimization mentioned later). When you call
>klp_write_object_relocations you would go through all ELF section and find
>the relevant one for the object (it is marked with SHF_RELA_LIVEPATCH and
>objname is in the name of the section. It is the same thing you do in 2.
>in the init function.). Then you go through all dynrela records in the
>section, you do the same things which you do in the proposed patch above,
>and call apply_relocate_add.
I'm not sure I like having klp_write_object_relocations() repeatedly
perform a loop through all the elf sections when we can pre-process
this information in the patch module's init function, and "cache" the
relevant klp section indices before passing things off to
klp_write_object_relocations(). So how about this: we do the
__klp_rela sec sorting in the init function of the patched module. The
patch module would iterate through its own elf sections, and when it
encounters a __klp_rela section, it looks at its objname, find the
corresponding klp_object, and save the section index of the __klp_rela
section in that klp_object. Then in klp_write_object_relocations, we
just have to look at the saved section index for the corresponding
object and access the klp rela section through that index, do the same
processing in this patch and call apply_relocate_add().
>Now it would be crazy to go through all sections each time
>klp_write_object_relocations is called (maybe it does not even matter but
>nevertheless). So klp_object could have an index to its ELF dynrela
>section. This index can be retrieved in the init function the same way you
>do all the stuff with klp_reloc_sec.
Ah, exactly what I said above :-D
>If you insisted on more than one dynrela section for a patched object we
>could have an array of indices there. Or whatever.
>
>It is almost the same as your proposal but in my opinion somewhat nicer.
>We just use the info stored in ELF directly without unnecessary middle
>layer (== klp_reloc_sec).
>
>Does it make sense? I hope it does. Would it work?
It does make sense, and I think we can make it work without
klp_reloc_sec. Thanks for the explanations.
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]
| From | Jessica Yu <jeyu@redhat.com> |
|---|---|
| Date | 2015-11-12 21:40 +0100 |
| Subject | Re: livepatch: reuse module loader code to write relocations |
| Message-ID | <qu6P0-5ho-13@gated-at.bofh.it> |
| In reply to | #1268223 |
+++ Jessica Yu [12/11/15 14:14 -0500]:
>+++ Miroslav Benes [12/11/15 16:27 +0100]:
>>On Wed, 11 Nov 2015, Jessica Yu wrote:
>>
>>>+++ Miroslav Benes [11/11/15 15:30 +0100]:
>>>> On Mon, 9 Nov 2015, Jessica Yu wrote:
>>>>
>>>> So I guess we don't need klp_reloc anymore.
>>>
>>>Yes, that's correct. I am noticing just now that I forgot to remove
>>>the klp_reloc struct definition from livepatch.h. That change will be
>>>reflected in v2...
>>>
>>>> If true, we should really
>>>> start thinking about proper documentation because there are going to be
>>>> plenty of assumptions about a patch module and we need to have it written
>>>> somewhere. Especially how the relocation sections look like.
>>>
>>>Agreed. As a first step the patch module format can perhaps be
>>>documented somewhere. Perhaps it's time we create
>>>Documentation/livepatch/? :-)
>>
>>Yes, I think so.
>>
>>>> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>>>> > index 087a8c7..26c419f 100644
>>>> > --- a/kernel/livepatch/core.c
>>>> > +++ b/kernel/livepatch/core.c
>>>> > @@ -28,6 +28,8 @@
>>>> > #include <linux/list.h>
>>>> > #include <linux/kallsyms.h>
>>>> > #include <linux/livepatch.h>
>>>> > +#include <linux/elf.h>
>>>> > +#include <asm/cacheflush.h>
>>>> >
>>>> > /**
>>>> > * struct klp_ops - structure for tracking registered ftrace ops structs
>>>> > @@ -281,46 +283,54 @@ 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;
>>>> > + unsigned int bind;
>>>> > + char *symname;
>>>> > + struct klp_reloc_sec *reloc_sec;
>>>> > + struct load_info *info;
>>>> > + Elf_Rela *rela;
>>>> > + Elf_Sym *sym, *symtab;
>>>> > + Elf_Shdr *symsect;
>>>> >
>>>> > 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)) {
>>>> > - 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;
>>>> > + info = pmod->info;
>>>> > + symsect = info->sechdrs + info->index.sym;
>>>> > + symtab = (void *)info->hdr + symsect->sh_offset;
>>>> > +
>>>> > + /* For each __klp_rela section for this object */
>>>> > + list_for_each_entry(reloc_sec, &obj->reloc_secs, list) {
>>>> > + relindex = reloc_sec->index;
>>>> > + num_relas = info->sechdrs[relindex].sh_size /
>>>> > sizeof(Elf_Rela);
>>>> > + rela = (Elf_Rela *) info->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 = info->strtab + sym->st_name;
>>>> > + bind = ELF_ST_BIND(sym->st_info);
>>>> > +
>>>> > + if (sym->st_shndx == SHN_LIVEPATCH) {
>>>> > + if (bind == STB_LIVEPATCH_EXT)
>>>> > + 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(info->sechdrs, info->strtab,
>>>> > + info->index.sym, relindex, pmod);
>>>> > }
>>>> >
>>>> > - return 0;
>>>> > + return ret;
>>>> > }
>>>>
>>>> Looking at this... do we even need reloc_secs in klp_object? Question is
>>>> whether we need more than one dynrela section for an object. If not then
>>>> the binding between klp_reloc_sec and an object is the only relevant thing
>>>> in the structure, be it index or objname. So we can replace the
>>>> list of structures with just the index in klp_object, or get rid of it
>>>> completely and rely on the name of dynrela section be something like
>>>> __klp_rela_{objname}.
>>>
>>>Hm, you bring up a good point. I think theoretically yes, it is
>>>possible to just have one klp_reloc_sec for each object and therefore
>>>a list is not required (I have not checked yet how difficult it would
>>>be to implement this on the kpatch-build side of things). However,
>>>considering the final format of the patch module, I think it is
>>>semantically clearer to leave it as a list, and for each object to
>>>possibly have more than one __klp_rela section.
>>>
>>>For example, say we are patching two functions in ext4. In my
>>>resulting kpatch module I will have two __klp_rela_ext4 sections, and
>>>they might look like this when we run readelf --sections:
>>>
>>>[34] __klp_rela_ext4.text.ext4_attr_store RELA ...
>>>[35] __klp_rela_ext4.text.ext4_attr_show RELA ...
>>>
>>>Then these two klp rela sections end up as two elements in the
>>>reloc_secs list for the ext4 patch object. I think this way, we can
>>>better tell which rela is being applied to what function. Might be
>>>easier to understand what's happening from the developer's point of
>>>view.
>>>
>>>> You see, we go through elf sections here which were preserved by module
>>>> loader. We even have relevant sections marked with SHF_RELA_LIVEPATCH. So
>>>> maybe all the stuff around klp_reloc_sec is not necessary.
>>>>
>>>> Thoughts?
>>>
>>>Ah, so this is where descriptive comments and documentation might have
>>>been useful :-) So I think we will still need to keep the
>>>klp_reloc_sec struct to help the patch module initialize. Though the
>>>name and objname fields aren't used in this patchset, they are used in
>>>the kpatch patch module code [1], where we iterate through each elf
>>>section, find the ones marked with SHF_RELA_LIVEPATCH, set the
>>>klp_reloc_sec's objname (which we find from the "name" field,
>>>formatted as __klp_rela_{objname}.text..). Once we have the objname
>>>set, we can then find the object to attach the reloc_sec to (i.e. add
>>>it to its list of reloc_secs).
>>>
>>>Hope that clears some things up.
>>
>>Ok, I'll try to explain myself and it is gonna be long. I'll try to
>>describe how we deal with dynrelas in klp today, how you use it in kpatch
>>(and this is the place where my knowledge can be wrong or obsolete), what
>>you propose and what I'd like to propose.
>>
>>1. First let's look on what we have now.
>>
>>We have a patch module in which there is a section with all dynrelas
>>needed to be resolved (it was like this in kpatch some time ago and maybe
>>it is different now so just have a patience, I'll get to it). In the init
>>function of the module kpatch builds all the relevant info from dynrela
>>section. It goes through it, creates an array of klp_reloc for each object
>>and includes each dynrela record with an objname to the array of
>>klp_object with that objname. Later when we need to apply relocations for
>>patched object we just go through the list (array) of its dynrelas in
>>klp_object and call our arch-specific klp_write_module_reloc().
>
>Sounds correct to me.
>
>>Now this was probably changed in kpatch and you do not have one dynrela
>>section but one dynrela section for each patched function. Is that
>>correct? (and can you tell us what the reason for the change was? It might
>>be crucial because I might be missing something.). Which leads us to your
>>proposal...
>
>Your original assumption was correct; current kpatch has one big
>.kpatch.dynrelas section, and each dynrela entry within that single
>section gets sorted to the correct object as you described above. The
>one dynrela section per patched function only came with this patchset,
>but for no particular reason other than to make the kpatch-build code
>for generating the patch module slightly less complicated. But I
>haven't checked how big of a change it would be to do
>one-dynrela-section per object, perhaps (and I hope) it will be an
>easy change.
>
>>2. So we have several dynrela section for one patched object. During init
>>function in a patch module kpatch once again builds needed structures from
>>these sections. Now they are called klp_reloc_sec and contain different
>>kind of info. There is no val, loc and such, only name of the symbol,
>>objname and index to dynrela section in ELF. So when you need to write
>>relocations for the patched object you go through all relevant dynrela
>>sections (because they are stored in the klp_object), all dynrela records
>>in each section and you resolve the undefined symbols. All needed info is
>>stored in ELF. Then you just call apply_relocate_add().
>>
>>3. I propose to go one step further. I think we don't need klp_reloc_sec
>>if there is only one dynrela section for patched object (and I currently
>>cannot see why this is not possible. It is possible even with one dynrela
>>section for whole patch module, that is for all patched objects.).
>
>I think the furthest we can go in terms of simplifying klp rela secs
>is to have one __klp_rela section per object. We can't smush all the
>__klp_rela_objname sections into one big __klp_rela section since we
>could be patching some objects that won't be loaded yet, and
>apply_relocate_add() needs to work with real SHT_RELA sections + their
>corresponding section indices (i.e., we cannot call
>apply_relocate_add() with that single, combined klp rela section).
>
>So, I think I would be OK with one __klp_rela section per object.
I just found a problem with this one __klp_rela section per object
approach, so I have to retract what I said above. I no longer think it
works, and the reason has to do with how apply_relocate_add() uses the
sh_info field of each relocation section to figure out to which
section the reloc section applies. Each relocation section references
the section to which it modifies, so this is a 1-1 correspondance we
cannot break. Thus we cannot simply combine all relocations into a
single __klp_rela section, because they would apply to multiple
different sections. See my reply to Josh.
This isn't a big problem anyway, we can just use an array of section
indices and still get rid of klp_reloc_sec. Plus the current patchset
already expects/implements multiple __klp_rela sections per object.
>>In my proposal there would be nothing done in init function of the patched
>>module (albeit some optimization mentioned later). When you call
>>klp_write_object_relocations you would go through all ELF section and find
>>the relevant one for the object (it is marked with SHF_RELA_LIVEPATCH and
>>objname is in the name of the section. It is the same thing you do in 2.
>>in the init function.). Then you go through all dynrela records in the
>>section, you do the same things which you do in the proposed patch above,
>>and call apply_relocate_add.
>
>I'm not sure I like having klp_write_object_relocations() repeatedly
>perform a loop through all the elf sections when we can pre-process
>this information in the patch module's init function, and "cache" the
>relevant klp section indices before passing things off to
>klp_write_object_relocations(). So how about this: we do the
>__klp_rela sec sorting in the init function of the patched module. The
>patch module would iterate through its own elf sections, and when it
>encounters a __klp_rela section, it looks at its objname, find the
>corresponding klp_object, and save the section index of the __klp_rela
>section in that klp_object. Then in klp_write_object_relocations, we
>just have to look at the saved section index for the corresponding
>object and access the klp rela section through that index, do the same
>processing in this patch and call apply_relocate_add().
s/index/indices/, so instead of saving a single section index, just
keep track of all the indices of the __klp_rela_objname sections in
each klp_object.
>>Now it would be crazy to go through all sections each time
>>klp_write_object_relocations is called (maybe it does not even matter but
>>nevertheless). So klp_object could have an index to its ELF dynrela
>>section. This index can be retrieved in the init function the same way you
>>do all the stuff with klp_reloc_sec.
>
>Ah, exactly what I said above :-D
>
>>If you insisted on more than one dynrela section for a patched object we
>>could have an array of indices there. Or whatever.
>>
>>It is almost the same as your proposal but in my opinion somewhat nicer.
>>We just use the info stored in ELF directly without unnecessary middle
>>layer (== klp_reloc_sec).
>>
>>Does it make sense? I hope it does. Would it work?
>
>It does make sense, and I think we can make it work without
>klp_reloc_sec. Thanks for the explanations.
--
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]
| From | Petr Mladek <pmladek@suse.com> |
|---|---|
| Date | 2015-11-11 16:30 +0100 |
| Subject | Re: [RFC PATCH 3/5] livepatch: reuse module loader code to write relocations |
| Message-ID | <qtFvs-4xR-11@gated-at.bofh.it> |
| In reply to | #1266230 |
On Mon 2015-11-09 23:45:53, Jessica Yu wrote:
> Reuse module loader code to write relocations, thereby eliminating the
> need for architecture specific code in livepatch. Namely, we reuse
> apply_relocate_add() in the module loader to write relocs 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.
>
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
> include/linux/livepatch.h | 11 ++++--
> include/linux/module.h | 6 ++++
> kernel/livepatch/core.c | 89 +++++++++++++++++++++++++++++------------------
> 3 files changed, 70 insertions(+), 36 deletions(-)
>
> index 087a8c7..26c419f 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -28,6 +28,8 @@
> #include <linux/list.h>
> #include <linux/kallsyms.h>
> #include <linux/livepatch.h>
> +#include <linux/elf.h>
> +#include <asm/cacheflush.h>
>
> /**
> * struct klp_ops - structure for tracking registered ftrace ops structs
> @@ -281,46 +283,54 @@ 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)
> {
[...]
> +
> + /* For each __klp_rela section for this object */
> + list_for_each_entry(reloc_sec, &obj->reloc_secs, list) {
Who, when, and how will define reloc_secs, please?
I guess that it is just an optimization. It helps to avoid going
through all elf sections of all livepatch modules. Therefore, I think
that we might fill this when the livepatch module is loaded. But
I do not see the code for this.
Best Regards,
Petr
--
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]
| From | Jessica Yu <jeyu@redhat.com> |
|---|---|
| Date | 2015-11-11 19:30 +0100 |
| Subject | Re: livepatch: reuse module loader code to write relocations |
| Message-ID | <qtIjE-6mP-25@gated-at.bofh.it> |
| In reply to | #1267245 |
+++ Petr Mladek [11/11/15 16:22 +0100]:
>On Mon 2015-11-09 23:45:53, Jessica Yu wrote:
>> Reuse module loader code to write relocations, thereby eliminating the
>> need for architecture specific code in livepatch. Namely, we reuse
>> apply_relocate_add() in the module loader to write relocs 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.
>>
>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>> ---
>> include/linux/livepatch.h | 11 ++++--
>> include/linux/module.h | 6 ++++
>> kernel/livepatch/core.c | 89 +++++++++++++++++++++++++++++------------------
>> 3 files changed, 70 insertions(+), 36 deletions(-)
>>
>> index 087a8c7..26c419f 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -28,6 +28,8 @@
>> #include <linux/list.h>
>> #include <linux/kallsyms.h>
>> #include <linux/livepatch.h>
>> +#include <linux/elf.h>
>> +#include <asm/cacheflush.h>
>>
>> /**
>> * struct klp_ops - structure for tracking registered ftrace ops structs
>> @@ -281,46 +283,54 @@ 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)
>> {
>[...]
>
>> +
>> + /* For each __klp_rela section for this object */
>> + list_for_each_entry(reloc_sec, &obj->reloc_secs, list) {
>
>Who, when, and how will define reloc_secs, please?
>
>I guess that it is just an optimization. It helps to avoid going
>through all elf sections of all livepatch modules. Therefore, I think
>that we might fill this when the livepatch module is loaded. But
>I do not see the code for this.
Thanks for bringing this up, I admit that from this patchset it is
unclear where and how the reloc_secs list is built.
Basically, the patch module code is expected to build the reloc_secs
list for each object that is being patched. For example in kpatch, the
patch module generates this list in patch_init(). Like you guessed, it
does go through all the elf sections of the patch module to find the
reloc sections marked SHF_RELA_LIVEPATCH. We are able to access these
sections through module->info, which is set up for livepatch modules
before the module loader calls do_init_module() (and hence before
patch_init(), See patch 2/5).
See below for an example of how the reloc_secs list might be built:
https://github.com/flaming-toast/kpatch/blob/no_dynrela_redux/kmod/patch/livepatch-patch-hook.c#L213
Once the patch module has built this list, it is good to go for use in
livepatch core. All livepatch has to do then is to iterate though the
list, resolve any outstanding symbols, and call apply_relocate_add(),
as shown in this patch.
Miroslav mentioned in another email that we should start thinking
about including documentation about this, including the expected patch
module format. So perhaps v2 should include some documentation about
this whole process somewhere.
Please let me know if anything else is unclear.
Thanks,
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]
| From | Petr Mladek <pmladek@suse.com> |
|---|---|
| Date | 2015-11-12 10:20 +0100 |
| Subject | Re: livepatch: reuse module loader code to write relocations |
| Message-ID | <qtWcW-71a-3@gated-at.bofh.it> |
| In reply to | #1267372 |
On Wed 2015-11-11 13:27:18, Jessica Yu wrote: > Basically, the patch module code is expected to build the reloc_secs > list for each object that is being patched. For example in kpatch, the > patch module generates this list in patch_init(). Like you guessed, it > does go through all the elf sections of the patch module to find the > reloc sections marked SHF_RELA_LIVEPATCH. We are able to access these > sections through module->info, which is set up for livepatch modules > before the module loader calls do_init_module() (and hence before > patch_init(), See patch 2/5). > See below for an example of how the reloc_secs list might be built: https://github.com/flaming-toast/kpatch/blob/no_dynrela_redux/kmod/patch/livepatch-patch-hook.c#L213 Thanks a lot for the link and explanation. I think that it would be nice to incorporate this into the patchset. I would put it into a separate function, e.g. klp_reloc_secs_init(). It can be called either from klp_register_patch() or from klp_init_patch(). > Once the patch module has built this list, it is good to go for use in > livepatch core. All livepatch has to do then is to iterate though the > list, resolve any outstanding symbols, and call apply_relocate_add(), > as shown in this patch. > > Miroslav mentioned in another email that we should start thinking > about including documentation about this, including the expected patch > module format. So perhaps v2 should include some documentation about > this whole process somewhere. That would be cool. Thank you, Petr -- 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]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-11-12 19:00 +0100 |
| Subject | Re: [RFC PATCH 3/5] livepatch: reuse module loader code to write relocations |
| Message-ID | <qu4kb-3yL-33@gated-at.bofh.it> |
| In reply to | #1266230 |
On Mon, Nov 09, 2015 at 11:45:53PM -0500, Jessica Yu wrote:
> Reuse module loader code to write relocations, thereby eliminating the
> need for architecture specific code in livepatch. Namely, we reuse
> apply_relocate_add() in the module loader to write relocs 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.
>
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
> include/linux/livepatch.h | 11 ++++--
> include/linux/module.h | 6 ++++
> kernel/livepatch/core.c | 89 +++++++++++++++++++++++++++++------------------
> 3 files changed, 70 insertions(+), 36 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 31db7a0..601e892 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -85,7 +85,7 @@ struct klp_reloc {
> /**
> * 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 +95,7 @@ struct klp_reloc {
> struct klp_object {
> /* external */
> const char *name;
> - struct klp_reloc *relocs;
> + struct list_head reloc_secs;
> struct klp_func *funcs;
>
> /* internal */
> @@ -129,6 +129,13 @@ struct klp_patch {
> #define klp_for_each_func(obj, func) \
> for (func = obj->funcs; func->old_name; func++)
>
> +struct klp_reloc_sec {
> + unsigned int index;
> + char *name;
> + char *objname;
> + struct list_head list;
> +};
> +
> 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 c8680b1..3c34eb8 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -793,9 +793,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 087a8c7..26c419f 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -28,6 +28,8 @@
> #include <linux/list.h>
> #include <linux/kallsyms.h>
> #include <linux/livepatch.h>
> +#include <linux/elf.h>
> +#include <asm/cacheflush.h>
>
> /**
> * struct klp_ops - structure for tracking registered ftrace ops structs
> @@ -281,46 +283,54 @@ 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;
> + unsigned int bind;
> + char *symname;
> + struct klp_reloc_sec *reloc_sec;
> + struct load_info *info;
> + Elf_Rela *rela;
> + Elf_Sym *sym, *symtab;
> + Elf_Shdr *symsect;
>
> 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)) {
> - 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;
> + info = pmod->info;
> + symsect = info->sechdrs + info->index.sym;
> + symtab = (void *)info->hdr + symsect->sh_offset;
> +
> + /* For each __klp_rela section for this object */
> + list_for_each_entry(reloc_sec, &obj->reloc_secs, list) {
> + relindex = reloc_sec->index;
> + num_relas = info->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> + rela = (Elf_Rela *) info->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 = info->strtab + sym->st_name;
> + bind = ELF_ST_BIND(sym->st_info);
> +
> + if (sym->st_shndx == SHN_LIVEPATCH) {
> + if (bind == STB_LIVEPATCH_EXT)
> + 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;
> + }
This is missing an important piece, I think. There's no way to
disambiguate duplicate symbols. Before, there was reloc->val, which
contained the symbol's address. There's nothing like that here.
I think we could use sym->st_value for that purpose. After Chris's
patch set gets merged, it could contain the sympos.
> }
> + ret = apply_relocate_add(info->sechdrs, info->strtab,
> + info->index.sym, relindex, pmod);
> }
>
> - return 0;
> + return ret;
> }
>
> static void notrace klp_ftrace_handler(unsigned long ip,
> @@ -741,12 +751,23 @@ 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);
> + pmod = patch->mod;
> +
> + if (!list_empty(&obj->reloc_secs)) {
> + 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) {
> --
> 2.4.3
>
--
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]
| From | Miroslav Benes <mbenes@suse.cz> |
|---|---|
| Date | 2015-11-11 15:10 +0100 |
| Message-ID | <qtEg3-3OD-15@gated-at.bofh.it> |
| In reply to | #1266222 |
On Mon, 9 Nov 2015, Jessica Yu wrote: > This patchset removes livepatch's need for architecture-specific relocation > code by leveraging existing code in the module loader to perform > arch-dependent work. Specifically, instead of duplicating code and > re-implementing what the apply_relocate_add() function in the module loader > already does in livepatch's klp_write_module_reloc(), we reuse > apply_relocate_add() to write relocations. The hope is that this will make > livepatch more easily portable to other architectures and greatly reduce > the amount of arch-specific code required to port livepatch to a particular > architecture. Hi, thanks for the patch set. I started going through it but it is gonna take some time. Nevertheless I've already found few things which I need to clarify. See respective patches. > Background: Why does livepatch need to write its own relocations? > == > A typical livepatch module contains patched versions of functions that can > reference non-exported global symbols and non-included local symbols. > Relocations referencing these types of symbols cannot be left in as-is > since the kernel module loader cannot resolve them and will therefore > reject the livepatch module. Furthermore, we cannot apply relocations that > affect modules not loaded yet at run time (e.g. a patch to a driver). The > current kpatch build system therefore solves this problem by embedding > special "dynrela" (dynamic reloc) sections in the resulting patch module > elf output. Using these dynrela sections, livepatch can correctly resolve > symbols while taking into account its scope and what module the symbol > belongs to, and then manually apply the dynamic relocations. I'll only add that we solve the problem with kallsyms calls in kGraft. It can get really cumbersome from time to time, so this work would simplify our effort as well. Miroslav -- 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]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-11-11 17:30 +0100 |
| Message-ID | <qtGrx-592-35@gated-at.bofh.it> |
| In reply to | #1267207 |
On Wed, Nov 11, 2015 at 03:00:44PM +0100, Miroslav Benes wrote: > > Background: Why does livepatch need to write its own relocations? > > == > > A typical livepatch module contains patched versions of functions that can > > reference non-exported global symbols and non-included local symbols. > > Relocations referencing these types of symbols cannot be left in as-is > > since the kernel module loader cannot resolve them and will therefore > > reject the livepatch module. Furthermore, we cannot apply relocations that > > affect modules not loaded yet at run time (e.g. a patch to a driver). The > > current kpatch build system therefore solves this problem by embedding > > special "dynrela" (dynamic reloc) sections in the resulting patch module > > elf output. Using these dynrela sections, livepatch can correctly resolve > > symbols while taking into account its scope and what module the symbol > > belongs to, and then manually apply the dynamic relocations. > > I'll only add that we solve the problem with kallsyms calls in kGraft. It > can get really cumbersome from time to time, so this work would simplify > our effort as well. I haven't yet reviewed the code in-depth, but overall I think this patch set is a good idea. It simplifies livepatch itself as well as the patch creation process (for both kGraft and kpatch-build), so it's a win-win. -- 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]
Page 3 of 3 — ← Prev page 1 2 [3]
Back to top | Article view | linux.kernel
csiph-web