Re: livepatch: reuse module loader code to write relocations

[Jessica Yu <jeyu@redhat.com>]

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