On Thu, 16 Jul 2015, Jessica Yu wrote: > +++ Miroslav Benes [15/07/15 15:13 +0200]: > > On Tue, 14 Jul 2015, Jessica Yu wrote: > > > > > +++ Miroslav Benes [14/07/15 16:36 +0200]: > > > > Ah ok, so there was an misunderstanding. I thought of something slightly > > > > else. Sorry about that. > > > > > > > > To make it clear and avoid further confusion, you want to add one call > > > to > > > > simplify_symbols() (called from load_module()) to SHN_UNDEF processing. > > > > This would handle all those unresolved symbols and thus there would be > > > no > > > > need to do it quite expensively in userspace (that is your > > > > create-diff-object for example). Is that correct? > > > > > > Right, we want to add a bit of extra logic in simplify_symbols() to > > > handle unresolved symbols (for example, to resolve non-exported local > > > symbols in another module, that find_symbol() would otherwise be > > > unable to resolve.) In addition, we let the module loader handle all > > > relocations for us and there would be no need to duplicate > > > the relocation code we have in klp_write_module_reloc(), since all > > > that architecture-specific relocation code already exists as part of the > > > module loader (e.g., see apply_relocate_add() in > > > arch/x86/kernel/module.c for instance, we'd be leveraging and reusing > > > existing > > > code instead of rewriting basically what's already there). But I agree > > > with your concerns, it makes me a bit uneasy to have to touch other > > > subsystems to accomplish this, regardless of size of change. > > > As I mentioned previously, the original motivation for this was to be able > > > to > > > eventually support livepatch on s390, and one of the roadblocks > > > encountered was writing s390-type relocs. Leveraging existing reloc > > > code in the module loader meant that some of the issues we've > > > encountered with certain PLT/GOT-type relocations would be solved. > > > > First, thanks a lot for great explanation. The benefits are clear and I > > find them appealing. Still concerned a lot, so let's obtain Rusty's > > opinion before discussing further. > > > > > > What are exactly the problems with s390x > > > > relocation types (I still haven't looked into them :/)? > > > > > > So let me take a few steps back and try to explain some of the issues with > > > s390 > > > relocs we've encountered. Maybe we can figure out a better solution > > > with more people thinking about the problem :-) > > > > > > So let's look at relocation type R_390_PLT32DBL for example. If we look > > > at apply_rela() in arch/s390/kernel/module.c, we can see how these > > > relocations are typically applied. The module loader lazily fills in > > > PLT entries, so we see that apply_rela() first checks if the PLT entry for > > > the symbol that the rela is associated with has already been filled > > > in. If not, it encodes several instructions + the value of the symbol > > > into that symbol's PLT entry. After initialization of the PLT entry, > > > then, we can finally perform the calculation according to the s390x > > > abi: (L + A - P) >> 1, where L is the *address* of the PLT entry in > > > memory, A is the addend, and P is the location where the reloc is to > > > be applied. According to the ABI, it is expected that execution / control > > > be > > > transferred to entries in the PLT. > > > > Oh, I knew I would regret to look into arch/s390/kernel/module.c again :) > > > > > So if we encounter such reloc types in livepatch, the question became > > > how do we support this functionality in klp_write_module_reloc()? > > > This problem was also exacerbated by the fact that these PLT/GOT > > > tables are freed once a module is loaded. Even if we *did* have access > > > to these tables, we would need a symbol's index to find the offset in > > > the PLT, which we currently don't have access to in livepatch. We could > > > maybe > > > construct > > > pseudo-plt entries managed by livepatch but that can become ugly, imo. > > > > Yes, that would be ugly. The other solution could be to hold on to PLT/GOT > > tables in arch specific part of struct module, if the module is livepatch > > module (and livepatching is enabled). It still is an interference to > > module.c, but not so big in my opinion. With the tables preserved we could > > process relas in klp_write_module_reloc(). > > > I made a mistake in my earlier email; I think we do have access to > these tables. And we definitely don't want to manage our own tables in > livepatch :P > > To access a module's plt table for example, we would > access mod->module_core + mod->arch.plt_offset. > > To access a specific symbol's PLT entry, we would use: > mod->module_core + mod->arch.plt_offset + symbol's plt_offset into the > table. > > A symbol's plt offset is saved in a mod_arch_syminfo struct, and there is an > array of > ptrs to mod_arch_syminfo structs, ordered by symbol index, located > starting at mod->arch.syminfo. > > To get a specific symbol's mod_arch_syminfo struct, we do: mod->arch.syminfo + > symbol index. > > From this we grab the plt_offset and access the symbol's plt entry with it. > We can see how all of this works in the apply_rela() function in > arch/s390/kernel/module.c :-) [the mail is getting long but leaving it as it is as it contains useful info] And I made a mistake too. Of course we have access to all these information as long as we are hooked somewhere in load_module. So... 1. We can switch from COMING notifier to hook in a load_module(). This is certainly possible (we do it in kGraft that way). We would have all the information need to correctly process dynrelas in klp_write_module_reloc(), because arch specific things are freed at the end of load_module(). It would allow us to reject loading a patch module if the patching fails somehow. BUT this does not work for the to-be-patched modules loaded afterwards. It is only a variant of your approach (which offloads the work to kernel module loader) but we still do it ourselves in klp. EDIT: The hook in load_module() is not even necessary. COMING notifiers are called before arch specific info is freed (if I am not wrong) so we have all the info in a notifier even now (I did not test it though). 2. We can change the kernel module loader behaviour to not to free arch specific info if the module is a livepatch module (as I proposed above). This would allow us to process also the not-loaded-yet modules later in the notifier. Is that correct? (I have a feeling I still mix several independent situations to one, so correct me if I am wrong) > We still have 3 problems though. > 1) mod->arch.syminfo, which contains all the symbol plt+got offsets, > gets freed at the end of module initialization.We need this information to > persist. > We could probably not free it then if we see the module is a livepatch > module... > > 2) How do we obtain a symbol's index to be able to index into > mod->arch.syminfo? We currently don't have a way to access this from > inside livepatch code. I think this is probably easy to solve though, > on kpatch-build's side we can just do this in userspace and stuff it in our > dynrela sections. Yes, that is hopefully possible. > 3) This last one is a tougher problem. So, I think this is all doable from > klp_write_module_reloc *as long as* the module loader allocates > those PLT/GOT entries for us.... > We want plt/got entries to be allocated *even* for symbols we can't resolve > yet because the to-be-patched module hasn't loaded yet. The current > behavior is that the module loader will reject the patch module if it > cannot resolve a symbol (i.e. target module not loaded yet!) and > therefore no PLT/GOT entry will be allocated! I think in the RFC > patchset I sent, one fix might be to say it is *not* an error if > klp_resolve_symbol_wait() couldn't resolve an address, under the > promise that we will resolve the symbol ourselves in our > klp_module_notify + klp_write_module_reloc call chain. Then the module > loader will be happy and allocate those PLT/GOT entries for us. I thought about this yesterday when I proposed the preservation of PLT/GOT tables. It is certainly possible but somewhat dangerous. If we did not resolve the symbols ourselves and they would be called somehow, we would have a big problem. But on the other hand we do the similar thing even now, don't we? You create dynrelas in create-diff-object for all relevant symbols. Then only those dynrelas for already loaded objects (vmlinux and loaded modules) are processed in the kernel. The rest is left unresolved and eventually process when the module is loaded. Thus livepatching depends on correctly prepared livepatch module even now (dynrelas, kallsyms calls to resolve used symbols and such). > > > I then thought if we could offload the brunt of our relocation work to the > > > module loader, we > > > would not need to worry about klp_write_module_reloc() anymore, and > > > supporting such relocation types on s390 would become easier. > > > Hopefully, the problem I described above helps you understand where we > > > are coming from. I am by no means an s390/x expert, so please let me > > > know if I understood something wrong, or if I completely missed > > > something. > > > > I still don't know how to solve the problem with the delayed modules (not > > loaded when the patch is applied) :/ > > I don't know what the best solution is either, it is a hard problem! > See my other email on this topic. I think we might have to go with > some hybrid approach that combines these two approaches (i.e. we might need to > keep the klp module notifier stuff). Yes, that is possible, but it sounds complicated and it could introduce some unwanted scenarios. Thanks, Miroslav -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
