On Sun, Jul 19, 2015 at 10:49:12PM -0400, Jessica Yu wrote: > +++ Josh Poimboeuf [19/07/15 21:01 -0500]: > >On Thu, Jul 16, 2015 at 07:14:44PM -0400, Jessica Yu wrote: > >>+++ Josh Poimboeuf [16/07/15 11:22 -0500]: > >>>The kernel module loader would automatically resolve > >>>some_exported_vmlinux_sym and do the .rela.text relocation. > >>> > >>>Then, when the patch module registers with klp, klp_init_object_loaded() > >>>would be called for vmlinux. It would resolve > >>>some_unexported_vmlinux_sym and would then call apply_relocate_add(), > >>>passing it the symbol indices for __klp_rela_vmlinux.rela.text and > >>>__klp_symtab_vmlinux. > >>> > >>>The same would happen for the module with some_module_sym, > >>>__klp_rela_modname.rela.text and __klp_symtab_modname. This would work > >>>whether the module is already loaded, or if it were loaded later and > >>>patched via the klp module notifier. > >>> > >>>It's not as simple as the original proposal, but IMO it's much simpler > >>>than having to do arch-specific dynrelas, and it allows us to keep > >>>delayed module patching. > >>> > >>>I probably missed some important details, but it at least seems like a > >>>promising idea. Thoughts? > >>> > >>Ah, that is pretty clever! :-) I think this general approach would work on > >>x86. But...I think I see a few problems if we try calling > >>apply_relocate_add() on s390x.. > >> > >>Recall that on s390x, arch-specific structs carrying each symbol's plt > >>and got offsets are allocated for every symbol in the module's symtab. > >>This is done in module_frob_arch_sections() in > >>arch/s390/kernel/module.c > >> > >>If we remove and split up the symbols into klp managed symbol tables and > >>rela secs, these symbols won't get plt/got offsets assigned to them, > >>or even get entries allocated for them, because those symbols won't be > >>in the module's main symbol table. > >> > >>If we break these expectations, calling apply_relocate_add() from within klp > >>on s390 won't work unfortunately :( there's too much reliance on > >>those structures being there internally. > >> > >>But wait, if the module loader can't allocate these entries for us, why > >>don't we allocate our own? There are problems with this solution too. > >>This solution would then be architecture-dependent, and if we want to > >>call apply_relocate_add(), we might have to rewrite where the module > >>expects its plt table to be (i.e. change module->arch.plt_offset) as > >>well as where it expects its syminfo table (the one containing all the > >>symbol plt+got offsets and stuff) to be, and that isn't good. > >> > >>I hate to point out problems without offering an alternative solution, > >>but at the moment I'm not sure if there is a way around this if we > >>want livepatch to be arch-independent. So still thinking... > > > >Ok. How about this variation on my last idea: > > > >1. Instead of having a per-object symbol table, leave all the symbols in > > the real symbol table. Any unexported or object symbols would be of > > a reserved type -- I'm not sure if it would be SHN_UNDEF or something > > else -- let's call it SHN_LIVEPATCH. > > > >2. Change simplify_symbols() to ignore any symbols of type > > SHN_LIVEPATCH. > > > >3. Have one or more custom klp sections which provide metadata about the > > SHN_LIVEPATCH symbols and how to resolve them. > > > >4. For s390, persist the me->arch.syminfo struct, as you and Miroslav > > previously mentioned. For all arches, also persist the symbol table > > (either in kernel module or in klp). Having both of these will allow > > apply_relocate_add() to be called after module init (I think?). > > > >Then the flow would look like: > > > >1. kernel module loader resolves exported symbols > >2. For each object, klp_init_object_loaded() resolves the object's > > SHN_LIVEPATCH symbols in the real symbol table > >3. For each object, klp_init_object_loaded() calls apply_relocate_add() > > with the per-object rela table and the real symbol table > > > > This is good! I think we're very close. :-) I'll offer just a few tweaks. > > Ok, so we keep all syms in the original symbol table, and we go with your > suggestion of creating separate __klp_rela_objname.rela.* sections for > module/unexported syms. I think these __klp_rela_* sections should be marked > with type SHT_RELA, since the s390 module loader loops through every SHT_RELA > section and for every sym referenced by a PLT/GOT-type rela, assigns their > plt/got offsets in in check_rela() in module_frob_arch_sections() (in > arch/s390/kernel/module.c). > > But one tiny problem with marking our klp rela sections with SHT_RELA is that we > don't want the module loader to apply them immediately in apply_relocations(), > so maybe we can add a small if statement in apply_relocations() to check if its > a __klp_rela section, and don't apply the relas in that section if it is. Or > maybe create a special section flag SHF_LIVEPATCH and if that flag is set, > ignore it. Reasonable? Ah, I knew I must have missed something. That sounds good. > As for symbol resolution, I initially thought that we should have the module > loader skip the symbol if its from another object and we couldn't resolve it, > but I like your solution better where we can specially mark the sym with > SHN_LIVEPATCH or something. We can create another special __klp section that > just records the indices of all our SHN_LIVEPATCH syms and other useful > metadata, and we use this info in klp_init_object_loaded() to resolve remaining > syms. > > If that all sounds good, I'll provide another v2 patchset reflecting these > changes. :-) Sounds good to me! -- Josh -- 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
