Messages in this thread | | | Date | Thu, 10 Dec 2015 17:07:03 -0500 | From | Jessica Yu <> | Subject | Re: livepatch: reuse module loader code to write relocations |
| |
+++ Josh Poimboeuf [10/12/15 15:41 -0600]: >On Thu, Dec 10, 2015 at 04:33:29PM -0500, Jessica Yu wrote: >> +++ Josh Poimboeuf [10/12/15 08:28 -0600]: >> >On Wed, Dec 09, 2015 at 02:10:14PM -0500, Jessica Yu wrote: >> >>+++ Josh Poimboeuf [08/12/15 12:38 -0600]: >> >>>>+ /* For each __klp_rela section for this object */ >> >>>>+ klp_for_each_reloc_sec(obj, reloc_sec) { >> >>>>+ relindex = reloc_sec->index; >> >>>>+ num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela); >> >>>>+ rela = (Elf_Rela *) pmod->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 = pmod->core_strtab + sym->st_name; >> >>>>+ >> >>>>+ if (sym->st_shndx == SHN_LIVEPATCH) { >> >>>>+ if (sym->st_info == 'K') >> >>>>+ 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; >> >>> >> >>>So I think you're also working on removing the 'external' stuff. Any >> >>>idea how this code will handle that? Specifically I'm wondering how the >> >>>objname and sympos of the rela's symbol will be specified. Maybe it can >> >>>be encoded somehow in one of the symbol fields (st_value)? >> >> >> >>Thanks for bringing this up. I think we can just encode the symbol's >> >>position in kallsyms in the symbol's st_other field. It isn't used >> >>anywhere and has size char, which is plenty of bits to represent the >> >>small ints. >> > >> >st_other does seem to at least have some trivial usage in the kernel, >> >see print_absolute_symbols() and sym_visibility() in >> >arch/x86/tools/relocs.c. Two of the bits are used to specify the >> >"visibility" of a symbol. Also readelf shows a "Vis" column in the >> >symbol table. >> >> Yeah, for x86 it looks like st_other is used only for SHN_ABS symbols >> in print_absolute_symbols(). Technically SHN_LIVEPATCH symbols >> shouldn't be affected in this case...but despite its sparse usage in the >> kernel it does look like using st_other to encode sympos is out of the >> question as its meaning is architecture specific.. >> >> >>For objname, the simplest solution might be to append ".klp.objname" >> >>to the symbol name, and extract out this suffix when resolving the >> >>symbol. Another way might be to have st_value contain the index into >> >>the strtab (or .kpatch.strings) that contains the objname. Then we'd >> >>access the objname just like how we access the symbol's name (strtab + >> >>sym->st_name). After we find the objname we can then overwrite >> >>st_value with the real address. I think this second method is cleaner. >> >>Thoughts? >> > >> >Yeah, I guess there are a lot of possibilities for ways to encode it. >> > >> >Personally I think it would be nice if the encoding were something that >> >could easily be seen when dumping the symbol table with readelf. So, >> >for example, the objname could be encoded in the symbol name (as you >> >suggested), and the sympos could be in st_value. >> >> Sure, that should be doable. So the new process might look like this: >> >> For every livepatch symbol referenced by a rela.. >> >> 1) Save the sympos encoded in st_value >> >> 2) Save the sym_objname that is encoded in the symbol's name with the >> 'klp' suffix (Just to clarify: the sym_objname specifies the object >> in which the symbol lives, and recall that we need this to remove the >> need for the "external" flag) >> >> 3) Resolve the symbol by using its name (without the klp suffix), >> sympos, and sym_objname >> >> 4) Set st_value to the found address > >Sounds right to me. > >> >If we do that, it'd probably be good to keep the naming consistent with >> >the '__klp_rela_objname.symname' thing. So something like >> >'_klp_sym_objname.symname'. >> >> How about 'symname.klp.objname', and renaming the klp reloc sections >> to '.klp.objname.rela.section_name'? Special symbol suffixes and >> section names seem to always use '.', so maybe this would look better? >> :-) But we can keep the underscores if people like that more. Both >> naming methods would work, it is only a matter of preference. > >It's your patches, so I'd say you get to pick ;-) My only request would >be some consistency between the symbol names and the rela section names. > >> >But... would there be any side effects associated with renaming it? For >> >example, would it cause problems with the s390 PLT? > >Just to verify, did you see this question? :-)
Ah, sorry I missed this! So I looked through the module loader code for x86, s390, arm (and just grepped in a few other archs) and the symbol's name (st_name) is only ever used in printing error or debug messages, or not used at all. As long as livepatch correctly resolves the symbol, and st_value is set, apply_relocate_add() doesn't really care about the name.
s390 plt/got information (struct mod_arch_syminfo) for each symbol is stored in an array in mod->arch.syminfo. Every symbol that is in the module's symbol table will get an entry, but not every symbol will have their plt/got offsets filled in. Every symbol referenced by a PLT/GOT rela will have those offsets filled in. Since our livepatch symbols are real symbols, with indexes and entries in the symbol table, they will get their own s390 mod_arch_syminfo struct, and should any PLT rela reference them, a PLT entry as well, which is good. Tldr, names of symbols don't affect s390 plt/got handling, so we are safe.
| |