On Wed, 16 Mar 2016, Jessica Yu wrote: [...] > +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. > + char objname[MODULE_NAME_LEN]; > +}; [...] > +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? > + if (cnt != 3) > + return -EINVAL; > + > + /* klp_find_object_symbol() treats a NULL objname as vmlinux */ > + vmlinux = !strcmp(bufs.objname, "vmlinux"); > + ret = klp_find_object_symbol(vmlinux ? NULL : bufs.objname, > + bufs.symname, sympos, > + (unsigned long *) &sym->st_value); > + if (ret) > + return ret; > } > - preempt_enable(); > > - /* > - * Check if it's in another .o within the patch module. This also > - * checks that the external symbol is unique. > - */ > - return klp_find_object_symbol(pmod->name, name, 0, addr); > + return 0; > } > > static int klp_write_object_relocations(struct module *pmod, > struct klp_object *obj) > { > - int ret = 0; > - unsigned long val; > - struct klp_reloc *reloc; > + int i, cnt, ret = 0; > + const char *objname, *secname; > + struct klp_buf bufs = {0}; > + Elf_Shdr *sec; > > if (WARN_ON(!klp_is_object_loaded(obj))) > return -EINVAL; > > - if (WARN_ON(!obj->relocs)) > - return -EINVAL; > + objname = klp_is_module(obj) ? obj->name : "vmlinux"; > > module_disable_ro(pmod); > + /* For each klp relocation section */ > + for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) { > + sec = pmod->klp_info->sechdrs + i; > + if (!(sec->sh_flags & SHF_RELA_LIVEPATCH)) > + continue; > > - for (reloc = obj->relocs; reloc->name; reloc++) { > - /* discover the address of the referenced symbol */ > - if (reloc->external) { > - if (reloc->sympos > 0) { > - pr_err("non-zero sympos for external reloc symbol '%s' is not supported\n", > - reloc->name); > - ret = -EINVAL; > - goto out; > - } > - ret = klp_find_external_symbol(pmod, reloc->name, &val); > - } else > - ret = klp_find_object_symbol(obj->name, > - reloc->name, > - reloc->sympos, > - &val); > - if (ret) > - goto out; > + klp_clear_buf(&bufs); > > - ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc, > - val + reloc->addend); > - if (ret) { > - pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n", > - reloc->name, val, ret); > - goto out; > + /* Check if this klp relocation section belongs to obj */ > + secname = pmod->klp_info->secstrings + sec->sh_name; > + cnt = sscanf(secname, ".klp.rela.%64[^.]", bufs.objname); Same here. Otherwise it looks really good (which applies for the whole series), so after fixing these nits you can add my Reviewed-by: Miroslav Benes <mbenes@xxxxxxx> Cheers, Miroslav -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html