Re: module: save load_info for livepatch modules

[Jessica Yu <jeyu@redhat.com>]

+++ Miroslav Benes [13/11/15 13:46 +0100]:
>On Fri, 13 Nov 2015, Miroslav Benes wrote:
>
>> As for load_info, I don't have a strong opinion whether to keep it for all
>> modules or for livepatch modules only.
>
>I have. We cannot keep it, even for livepatch modules...
>
>In info->hdr there is a temporary copy of the whole module (see
>init_module syscall and the first parts of load_module). In load_module
>a final struct module * is created with parts of info->hdr copied (I'll
>get to that later). So if we saved info->hdr for later purposes we would
>just have two copies of the same module in the memory. The original one
>with !SHF_ALLOC sections and everything in vmalloc area, and the new
>final copy with SHF_ALLOC sections only. This is not good.
>
>If this is correct (and I think it is after some staring into the code) we
>need to do something different. We should build the info we need for
>delayed relocations from the final copy (or refactor the existing
>module code).
>
>The second problem... dynrela sections need to be marked with SHF_ALLOC
>flag, right? Perhaps it would be better not to do it and copy also
>SHF_RELA_LIVEPATCH sections. It is equivalent but not hidden somewhere
>else (in userspace "kpatch-build" tool).

Hm, OK. I understand your concern about leaving a redundant copy of
the module in memory and I agree that we need to do better. I think I
have a solution.

I'm looking at exactly what components we need to make the calls to
apply_relocate_add() work. It's quite simple, I think we only need to
keep the following:

1. A copy of the module's elf section headers i.e. info->sechdrs.
This should be easy to copy. memcpy [info->hdr->e_shnum *
sizeof(Elf_Shdr)] bytes from info->sechdrs. We can maybe put
this in a new field called module->sechdrs.

2. A copy of each __klp_rela section.
If we don't keep info, the current code will discard/not copy the rela
sections over to module core memory since they are !SHF_ALLOC. In
kpatch-build, it is very easy to simply |= the SHF_ALLOC flag with
each __klp_rela section and they will automatically get copied over to
module core memory, and their sh_addr's automatically get reassigned
as well. Thus the klp rela sections will be accessible at
sechdrs[index_of_klpsec].sh_addr. I think this is the easiest solution.

3. A copy of the symbol table. 
Notice that module already has a "symtab" field. In kernels configured
with CONFIG_KALLSYMS, it points to a trimmed down symtab (the
mod->core_symtab) in module core memory. This symtab is not normally
complete; only "core" symbols are kept in it. See add_kallsyms()
(called in post_relocations()) for how core symbols are copied into
this symtab. Then, after the symbols have been copied, module->symtab
is reassigned to point to this core_symtab in do_init_module(). Since
CONFIG_LIVEPATCH requires CONFIG_KALLSYMS, I think we can assume that
mod->symtab will be pointing to mod->core_symtab at the end of the
module load process, since mod->symtab gets assigned to core_symtab in
do_init_module() if CONFIG_KALLSYMS is set.

So for livepatch, what we can do is make sure every symbol in a
livepatch module gets copied into this core symtab. It is important we
keep every symbol since apply_relocate_add() will be using the
original symbol indices. We can implement this by adding a check in
add_kallsyms() to see if we're dealing with a livepatch module. If
yes, just copy all the symbols over.

Then, we will also update Elf_Shdr corresponding to the symbol table
section (sechdrs[symindex].sh_addr) to make sure its sh_addr points to
mod->symtab, so apply_relocate_add() will be able to use it.

4. A copy of mod_arch_specific
I think we discussed this in another email somewhere, but we need to
keep a copy if this somewhere as well. 

So to summarize, keep a copy of sechdrs in module->sechdrs, keep a
copy of mod_arch_specific, mark klp rela sections with SHF_ALLOC,
re-use module->symtab by making sure every symbol gets considered a
"core" symbol and gets copied over. And of course any memory we
allocate (sechdrs, arch stuff) we will free in perhaps free_module()
somewhere.

I haven't implemented it yet but I think it will work, and we don't
need to keep load_info in this scheme. What do you think?

Thanks,
Jessica