Re: [PATCH v5 4/6] livepatch: reuse module loader code to write relocations

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@suse.cz>

Cheers,
Miroslav