lkml.org 
[lkml]   [2015]   [Nov]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: livepatch: reuse module loader code to write relocations
    +++ 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


    \
     
     \ /
      Last update: 2015-11-11 21:21    [W:3.425 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site