+++ 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
