Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1268246
| From | Jessica Yu <jeyu@redhat.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: livepatch: reuse module loader code to write relocations |
| Date | 2015-11-12 21:30 +0100 |
| Message-ID | <qu6Fk-5dX-5@gated-at.bofh.it> (permalink) |
| References | (1 earlier) <qt92y-kt-15@gated-at.bofh.it> <qtEJ5-3Zk-33@gated-at.bofh.it> <qtJSq-7to-13@gated-at.bofh.it> <qu1Z0-2aO-13@gated-at.bofh.it> <qu4at-3v0-13@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
+++ 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/
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[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
csiph-web