Re: module: save load_info for livepatch modules

[Miroslav Benes]

On Fri, 13 Nov 2015, Jessica Yu wrote:

> +++ 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.

Yes.

> 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.

I agree.

> 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.

It seems like the way (while looking at the code). I think we should do 
the same for strtab. It is also the parameter of apply_relocate_add and 
although it is not directly used in x86 code, it is (albeit for error 
handling) for s390 case. It corresponds to symtab. Also only the core 
symbols are preserved.

> 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?

I think it should be work. Great. I think this is the way to go. The only 
thing which needs to be solved is where all needed info should be stored. 
Whether directly in struct module (for example module->sechdrs you 
mentioned) or somewhere in our livepatching code only (klp_patch?). As we 
have already discussed it depends if such functionality could be useful 
also for someone else. I am not sure if we came to a final decision, but 
we inclined to make it general, didn't we?

Thanks,
Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/