fix review comments

Signed-off-by: Seth Jennings <sjenning@redhat.com>

.gitignore

@@ -7,7 +7,6 @@
 *.swp
 .tmp_versions
 Module.symvers
-kpatch-build/lookup.o
 kpatch-build/lookup
 kpatch-build/create-diff-object
 man/kpatch.1.gz

kmod/core/core.c

@@ -472,13 +472,13 @@ int kpatch_register(struct kpatch_module *kpmod, bool replace)
 				       dynrela->type, dynrela->dest,
 				       dynrela->src, i);
 				ret = -EINVAL;
-				goto err_up;
+				goto err_put;
 		}
 		set_memory_rw((unsigned long)loc & PAGE_MASK, 1);
 		ret = probe_kernel_write(loc, &val, size);
 		set_memory_ro((unsigned long)loc & PAGE_MASK, 1);
 		if (ret)
-			goto err_up;
+			goto err_put;
 	}
 
 	for (i = 0; i < num_funcs; i++) {
@@ -597,6 +597,7 @@ err_unregister:
 	kpatch_num_registered--;
 err_rollback:
 	kpatch_remove_funcs_from_filter(funcs, num_funcs);
+err_put:
 	module_put(kpmod->mod);
 err_up:
 	up(&kpatch_mutex);

kpatch-build/Makefile

@@ -7,9 +7,6 @@ TARGETS = create-diff-object
 
 all: $(TARGETS)
 
-%: %.c
-	$(CC) $(CFLAGS) $^ -o $@ $(LDFLAGS)
-
 create-diff-object: create-diff-object.c list.h lookup.c lookup.h
 	$(CC) $(CFLAGS) create-diff-object.c lookup.c -o $@ $(LDFLAGS)
 

kpatch-build/create-diff-object.c

@@ -1366,7 +1366,7 @@ void kpatch_create_patches_sections(struct kpatch_elf *kelf,
 	/* allocate section resources */
 	ALLOC_LINK(sec, &kelf->sections);
 	size = nr * sizeof(*patches);
-	patches = malloc(nr * sizeof(*patches));
+	patches = malloc(size);
 	if (!patches)
 		ERROR("malloc");
 	sec->name = ".kpatch.patches";
@@ -1410,7 +1410,7 @@ void kpatch_create_patches_sections(struct kpatch_elf *kelf,
 		find_section_by_name(&kelf->sections, ".symtab")->index;
 	relasec->sh.sh_info = sec->index;
 
-	/* populate text section */
+	/* populate sections */
 	index = 0;
 	list_for_each_entry(sym, &kelf->symbols, list) {
 		if (sym->type == STT_FUNC && sym->sec->include) {
@@ -1433,7 +1433,11 @@ void kpatch_create_patches_sections(struct kpatch_elf *kelf,
 			patches[index].old_size = result.size;
 			patches[index].new_size = sym->sym.st_size;
 
-			/* add entry in rela list */
+			/*
+			 * Add a relocation that will populate
+			 * the patches[index].new_addr field at
+			 * module load time.
+			 */
 			ALLOC_LINK(rela, &relasec->relas);
 			rela->sym = sym;
 			rela->type = R_X86_64_64;
@@ -1480,7 +1484,7 @@ void kpatch_create_dynamic_rela_sections(struct kpatch_elf *kelf,
 	/* allocate section resources */
 	ALLOC_LINK(sec, &kelf->sections);
 	size = nr * sizeof(*dynrelas);
-	dynrelas = malloc(nr * sizeof(*dynrelas));
+	dynrelas = malloc(size);
 	if (!dynrelas)
 		ERROR("malloc");
 	sec->name = ".kpatch.dynrelas";
@@ -1524,7 +1528,7 @@ void kpatch_create_dynamic_rela_sections(struct kpatch_elf *kelf,
 		find_section_by_name(&kelf->sections, ".symtab")->index;
 	relasec->sh.sh_info = sec->index;
 
-	/* populate text section (reuse sec for iterator here) */
+	/* populate sections (reuse sec for iterator here) */
 	index = 0;
 	list_for_each_entry(sec, &kelf->sections, list) {
 		if (!is_rela_section(sec))
@@ -1575,7 +1579,12 @@ void kpatch_create_dynamic_rela_sections(struct kpatch_elf *kelf,
 		ERROR("size mismatch in dynrelas sections");
 }
 
-void kpatch_strip_non_included_syms(struct lookup_table *table,
+/*
+ * This function strips out symbols that were referenced by changed rela
+ * sections, but the rela entries that referenced them were converted to
+ * dynrelas and are no longer needed.
+ */
+void kpatch_strip_unneeded_syms(struct lookup_table *table,
                                     struct kpatch_elf *kelf)
 {
 	struct symbol *sym, *safe;
@@ -1610,7 +1619,7 @@ void kpatch_rebuild_rela_section_data(struct section *sec)
 	relas = malloc(size);
 	if (!relas)
 		ERROR("malloc");
-	
+
 	sec->data->d_buf = relas;
 	sec->data->d_size = size;
 	/* d_type remains ELF_T_RELA */
@@ -1666,9 +1675,6 @@ void kpatch_write_output_elf(struct kpatch_elf *kelf, Elf *elf, char *outfile)
 
 	/* add changed sections */
 	list_for_each_entry(sec, &kelf->sections, list) {
-		if (is_rela_section(sec))
-			kpatch_rebuild_rela_section_data(sec);
-
 		scn = elf_newscn(elfout);
 		if (!scn)
 			ERROR("elf_newscn");
@@ -1756,6 +1762,7 @@ int main(int argc, char *argv[])
 	struct arguments arguments;
 	int num_changed;
 	struct lookup_table *vmlinux;
+	struct section *sec;
 	struct symbol *sym;
 	char *hint;
 
@@ -1825,7 +1832,7 @@ int main(int argc, char *argv[])
 	}
 	kpatch_create_patches_sections(kelf_out, vmlinux, hint);
 	kpatch_create_dynamic_rela_sections(kelf_out, vmlinux, hint);
-	kpatch_strip_non_included_syms(vmlinux, kelf_out);
+	kpatch_strip_unneeded_syms(vmlinux, kelf_out);
 
 	kpatch_create_shstrtab(kelf_out);
 	kpatch_create_strtab(kelf_out);
@@ -1834,6 +1841,12 @@ int main(int argc, char *argv[])
 
 	if (arguments.inventory)
 		kpatch_write_inventory_file(kelf_out, outfile);
+
+	/* rebuild the rela section data buffers from their relas lists */
+	list_for_each_entry(sec, &kelf_out->sections, list)
+		if (is_rela_section(sec))
+			kpatch_rebuild_rela_section_data(sec);
+
 	kpatch_write_output_elf(kelf_out, kelf_patched->elf, outfile);
 
 	return 0;

kpatch-build/lookup.c

@@ -88,7 +88,7 @@ struct lookup_table *lookup_open(char *path)
 		name = elf_strptr(elf, shstrndx, sh.sh_name);
 		if (!name)
 			ERROR("elf_strptr scn");
-			
+
 		if (!strcmp(name, ".symtab"))
 			break;
 	}
@@ -140,7 +140,7 @@ void lookup_close(struct lookup_table *table)
 int lookup_local_symbol(struct lookup_table *table, char *name, char *hint,
                         struct lookup_result *result)
 {
-	struct symbol *sym;
+	struct symbol *sym, *match = NULL;
 	int i;
 	char *curfile = NULL;
 
@@ -151,18 +151,24 @@ int lookup_local_symbol(struct lookup_table *table, char *name, char *hint,
 				curfile = sym->name;
 				continue; /* begin hint file symbols */
 			} else if (curfile)
-				break; /* end hint file symbols */
+				curfile = NULL; /* end hint file symbols */
 		}
 		if (!curfile)
 			continue;
 		if (sym->bind == STB_LOCAL && !strcmp(sym->name, name)) {
-			result->value = sym->value;
-			result->size = sym->size;
-			return 0;
+			if (match)
+				/* dup file+symbol, unresolvable ambiguity */
+				return 1;
+			match = sym;
 		}
 	}
 
-	return 1;
+	if (!match)
+		return 1;
+
+	result->value = match->value;
+	result->size = match->size;
+	return 0;
 }
 
 int lookup_global_symbol(struct lookup_table *table, char *name,
@@ -203,7 +209,7 @@ static void find_this(struct lookup_table *table, char *sym, char *hint)
 {
 	struct lookup_result result;
 
-	if (hint) 
+	if (hint)
 		lookup_local_symbol(table, sym, hint, &result);
 	else
 		lookup_global_symbol(table, sym, &result);