On Sun, 19 Jul 2015, 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? > > 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. So if I get all of this correctly it would almost be the same as the current situation in klp, but we'll process all the unresolved unexported symbols/"dynrelas" ourselves and apply them via apply_relocate_add() from the kernel module loader. No need for klp_write_module_reloc() and arch-specific code there. The advantage compared to the original proposal in this thread is that there would be almost no interventions to kernel module loader (apart from not freeing of arch-specific info in struct module for livepatch module). Yup, this could work. My only worry is whether we can extend ELF spec (that is kernel ABI) without objections from someone (SHF_LIVEPATCH, SHN_LIVEPATCH). > If that all sounds good, I'll provide another v2 patchset reflecting these > changes. :-) Great. I am looking forward to seeing that. 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
