+++ Miroslav Benes [12/01/16 17:40 +0100]:
Hi Jessica,
I walked through the series and it looks really nice. Others have already
pointed out the issues I also found, so only few minor things below.
First thing, could you copy&paste the information and reasoning from the
cover letter to the changelogs where appropriate? It is very detailed and
it would be a pity to lost it.
Thanks Miroslav! I'll do that.
On Fri, 8 Jan 2016, Jessica Yu wrote:
diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
index 19c099a..7312e25 100644
--- a/arch/x86/include/asm/livepatch.h
+++ b/arch/x86/include/asm/livepatch.h
@@ -33,8 +33,6 @@ static inline int klp_check_compiler_support(void)
#endif
return 0;
}
-int klp_write_module_reloc(struct module *mod, unsigned long type,
- unsigned long loc, unsigned long value);
You left klp_write_module_reloc() in arch/s390/include/asm/livepatch.h I'm
afraid. Anyway it would be really great if you managed to test the series
on s390 somehow. Just to know that all the roadblocks are really gone.
Ah, thanks for catching that. I will also try testing the patchset on
s390x and report back.
-/*
- * external symbols are located outside the parent object (where the parent
- * object is either vmlinux or the kmod being patched).
- */
-static int klp_find_external_symbol(struct module *pmod, const char *name,
- unsigned long *addr)
+static int klp_resolve_symbols(Elf_Shdr *relsec, struct module *pmod)
{
- const struct kernel_symbol *sym;
+ int i, len, ret = 0;
+ Elf_Rela *relas;
+ Elf_Sym *sym;
+ char *symname, *sym_objname;
- /* first, check if it's an exported symbol */
- preempt_disable();
- sym = find_symbol(name, NULL, NULL, true, true);
- if (sym) {
- *addr = sym->value;
- preempt_enable();
- return 0;
+ relas = (Elf_Rela *) relsec->sh_addr;
+ /* For each rela in this .klp.rel. section */
+ for (i = 0; i < relsec->sh_size / sizeof(Elf_Rela); i++) {
+ sym = pmod->core_symtab + ELF_R_SYM(relas[i].r_info);
+ symname = pmod->core_strtab + sym->st_name;
Maybe it would be better to use pmod->symtab and pmod->strtab everywhere.
It should be the same, but core_* versions are only helpers used in
load_module and friends. There is even a comment in
include/linux/module.h.
/*
* We keep the symbol and string tables for kallsyms.
* The core_* fields below are temporary, loader-only (they
* could really be discarded after module init).
*/
We should respect that.
I admit I'm a bit confused by the comment, I can't seem to find where
core_symtab and core_strtab are purportedly discarded after module
init (perhaps I'm missing something?). IMO it sounds more like it's
describing mod->symtab and mod->strtab instead, because these are in
module init memory and are freed later. In any case, my reason for using
core_symtab is that the original symbol table (mod->symtab) is marked
with INIT_OFFSET_MASK in layout_symtab() (see kernel/module.c), and is
therefore in init memory. This memory is freed near the end of
do_init_module() with do_free_init(). Since core_symtab is in module core
memory, for livepatch modules I simply used core_symtab to hold a
full copy of the symbol table instead of the slimmed down version that
it was originally intended to hold.
Alternatively, we can tweak layout_symtab() to *not* mark the symtab
with INIT_OFFSET_MASK and put it in core memory instead. I think
either way will work, but maybe it is cleaner to do it this way
instead.
Jessica
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
