Re: module: save load_info for livepatch modules

+++ Miroslav Benes [13/11/15 13:46 +0100]:

On Fri, 13 Nov 2015, Miroslav Benes wrote:

quoted

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