Re: livepatch: reuse module loader code to write relocations
From: Josh Poimboeuf <hidden>
Date: 2016-03-21 19:24:08
Also in:
linux-s390, lkml
On Mon, Mar 21, 2016 at 03:18:32PM -0400, Jessica Yu wrote:
quoted hunk ↗ jump to hunk
+++ Miroslav Benes [21/03/16 14:55 +0100]:quoted
On Wed, 16 Mar 2016, Jessica Yu wrote: [...]quoted
+struct klp_buf { + char symname[KSYM_SYMBOL_LEN];I think it is better to make this KSYM_NAME_LEN. KSYM_SYMBOL_LEN looks like something different and KSYM_NAME_LEN is 128 which you reference below.Ack, I did mean to use KSYM_NAME_LEN, thanks.quoted
quoted
+ char objname[MODULE_NAME_LEN]; +};[...]quoted
+static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod) +{ + int i, cnt, vmlinux, ret; + struct klp_buf bufs = {0}; + Elf_Rela *relas; + Elf_Sym *sym; + char *symname; + unsigned long sympos; + + relas = (Elf_Rela *) relasec->sh_addr; + /* For each rela in this klp relocation section */ + for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) { + sym = pmod->core_kallsyms.symtab + ELF_R_SYM(relas[i].r_info); + if (sym->st_shndx != SHN_LIVEPATCH) + return -EINVAL; + + klp_clear_buf(&bufs); + + /* Format: .klp.sym.objname.symbol_name,sympos */ + symname = pmod->core_kallsyms.strtab + sym->st_name; + cnt = sscanf(symname, ".klp.sym.%64[^.].%128[^,],%lu", + bufs.objname, bufs.symname, &sympos);It would be really nice to change actual values for their macro definitions, but this would be a mess which is not worth it. Anyway shouldn't those width modifiers be %63 and %127 to make a room for \0?Yes, this is a concern and I'm not sure what the best way to fix it is. If both MODULE_NAME_LEN and KSYM_NAME_LEN were straight up constants, then I think Josh's stringify approach would have worked perfectly. However since MODULE_NAME_LEN translates to an expression (64 - sizeof(unsigned long)), which the preprocessor cannot evaluate, we will need another approach. Building the format strings at run time might be messier than we'd like. Alternatively we could just go the simple route and simply be a bit more aggressive on the upper bound for the format width; though the size of long varies on different architectures, afaik the max size it could ever be on any arch is 8 bytes, so perhaps 64 - 8 = 56 (then - 1 to make room for \0) might be an appropriate field width. This would deserve a comment as well.
I think something like that would be good, along with: BUILD_BUG_ON(MODULE_NAME_LEN < 56); and a comment explaining why. -- Josh