Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752990AbbKLUfv (ORCPT ); Thu, 12 Nov 2015 15:35:51 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42403 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751035AbbKLUft (ORCPT ); Thu, 12 Nov 2015 15:35:49 -0500 Date: Thu, 12 Nov 2015 15:35:45 -0500 From: Jessica Yu To: Miroslav Benes Cc: Rusty Russell , Josh Poimboeuf , Seth Jennings , Jiri Kosina , Vojtech Pavlik , linux-api@vger.kernel.org, live-patching@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org Subject: Re: livepatch: reuse module loader code to write relocations Message-ID: <20151112203543.GD5841@packer-debian-8-amd64.digitalocean.com> References: <1447130755-17383-1-git-send-email-jeyu@redhat.com> <1447130755-17383-4-git-send-email-jeyu@redhat.com> <20151111200732.GB30025@packer-debian-8-amd64.digitalocean.com> <20151112191458.GB5841@packer-debian-8-amd64.digitalocean.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20151112191458.GB5841@packer-debian-8-amd64.digitalocean.com> X-OS: Linux eisen.io 3.16.0-4-amd64 x86_64 User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 13737 Lines: 302 +++ 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 >>>> > #include >>>> > #include >>>> > +#include >>>> > +#include >>>> > >>>> > /** >>>> > * 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/