From 3aa5abb8072c2e198a3e5dbb84502823d830b8cc Mon Sep 17 00:00:00 2001 From: Artem Savkov Date: Fri, 11 May 2018 11:51:00 +0200 Subject: [PATCH] kpatch-build: use symbol table instead of kobject create-diff-object doesn't really need the full kernel object file as input. All it requires is a symbol table. Switch to using "eu-readelf -s"'s output instead of object files. This will enable us to cover more cases in unit tests. Signed-off-by: Josh Poimboeuf Signed-off-by: Artem Savkov --- .travis.yml | 2 +- kpatch-build/create-diff-object.c | 50 ++++------ kpatch-build/kpatch-build | 28 ++++-- kpatch-build/lookup.c | 159 +++++++++++++----------------- kpatch-build/lookup.h | 2 +- test/unit/Makefile.include | 18 +++- 6 files changed, 124 insertions(+), 135 deletions(-) diff --git a/.travis.yml b/.travis.yml index 052cef0..1c1fda6 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,7 +1,7 @@ language: c before_install: - sudo apt-get -qq update - - sudo apt-get install -y libelf-dev linux-headers-$(uname -r) shellcheck + - sudo apt-get install -y libelf-dev linux-headers-$(uname -r) shellcheck elfutils script: - make diff --git a/kpatch-build/create-diff-object.c b/kpatch-build/create-diff-object.c index 8a9d756..bd55104 100644 --- a/kpatch-build/create-diff-object.c +++ b/kpatch-build/create-diff-object.c @@ -2984,11 +2984,11 @@ static void kpatch_build_strings_section_data(struct kpatch_elf *kelf) } struct arguments { - char *args[6]; + char *args[7]; int debug; }; -static char args_doc[] = "original.o patched.o kernel-object output.o Module.symvers patch-module-name"; +static char args_doc[] = "original.o patched.o parent-name parent-symtab Module.symvers patch-module-name output.o"; static struct argp_option options[] = { {"debug", 'd', NULL, 0, "Show debug output" }, @@ -3007,13 +3007,13 @@ static error_t parse_opt (int key, char *arg, struct argp_state *state) arguments->debug = 1; break; case ARGP_KEY_ARG: - if (state->arg_num >= 6) + if (state->arg_num >= 7) /* Too many arguments. */ argp_usage (state); arguments->args[state->arg_num] = arg; break; case ARGP_KEY_END: - if (state->arg_num < 6) + if (state->arg_num < 7) /* Not enough arguments. */ argp_usage (state); break; @@ -3033,8 +3033,8 @@ int main(int argc, char *argv[]) struct lookup_table *lookup; struct section *sec, *symtab; struct symbol *sym; - char *hint = NULL, *objname, *pos, *orig_obj, *patched_obj; - char *parent_kobj, *mod_symvers, *patch_name, *output_obj; + char *hint = NULL, *orig_obj, *patched_obj, *parent_name; + char *parent_symtab, *mod_symvers, *patch_name, *output_obj; struct sym_compare_type *base_locals; arguments.debug = 0; @@ -3044,12 +3044,13 @@ int main(int argc, char *argv[]) elf_version(EV_CURRENT); - orig_obj = arguments.args[0]; - patched_obj = arguments.args[1]; - parent_kobj = arguments.args[2]; - output_obj = arguments.args[3]; - mod_symvers = arguments.args[4]; - patch_name = arguments.args[5]; + orig_obj = arguments.args[0]; + patched_obj = arguments.args[1]; + parent_name = arguments.args[2]; + parent_symtab = arguments.args[3]; + mod_symvers = arguments.args[4]; + patch_name = arguments.args[5]; + output_obj = arguments.args[6]; childobj = basename(orig_obj); @@ -3074,7 +3075,7 @@ int main(int argc, char *argv[]) /* create symbol lookup table */ base_locals = kpatch_elf_locals(kelf_base); - lookup = lookup_open(parent_kobj, mod_symvers, hint, base_locals); + lookup = lookup_open(parent_symtab, mod_symvers, hint, base_locals); free(base_locals); kpatch_mark_grouped_sections(kelf_patched); @@ -3132,27 +3133,12 @@ int main(int argc, char *argv[]) */ kpatch_elf_teardown(kelf_patched); - /* extract module name (destructive to arguments.modulefile) */ - objname = basename(parent_kobj); - if (!strncmp(objname, "vmlinux-", 8)) - objname = "vmlinux"; - else { - pos = strchr(objname,'.'); - if (pos) { - /* kernel module */ - *pos = '\0'; - pos = objname; - while ((pos = strchr(pos, '-'))) - *pos++ = '_'; - } - } - /* create strings, patches, and dynrelas sections */ kpatch_create_strings_elements(kelf_out); - kpatch_create_patches_sections(kelf_out, lookup, objname); - kpatch_create_intermediate_sections(kelf_out, lookup, objname, patch_name); - kpatch_create_kpatch_arch_section(kelf_out, objname); - kpatch_create_callbacks_objname_rela(kelf_out, objname); + kpatch_create_patches_sections(kelf_out, lookup, parent_name); + kpatch_create_intermediate_sections(kelf_out, lookup, parent_name, patch_name); + kpatch_create_kpatch_arch_section(kelf_out, parent_name); + kpatch_create_callbacks_objname_rela(kelf_out, parent_name); kpatch_build_strings_section_data(kelf_out); kpatch_create_mcount_sections(kelf_out); diff --git a/kpatch-build/kpatch-build b/kpatch-build/kpatch-build index 24b7368..cf44789 100755 --- a/kpatch-build/kpatch-build +++ b/kpatch-build/kpatch-build @@ -773,16 +773,26 @@ for i in $FILES; do mkdir -p "output/$(dirname "$i")" cd "$SRCDIR" || die find_kobj "$i" - if [[ "$KOBJFILE" = vmlinux ]]; then - KOBJFILE="$VMLINUX" - else - KOBJFILE="$TEMPDIR/module/$KOBJFILE" - fi cd "$TEMPDIR" || die if [[ -e "orig/$i" ]]; then - # create-diff-object orig.o patched.o kernel-object output.o Module.symvers patch-mod-name - "$TOOLSDIR"/create-diff-object "orig/$i" "patched/$i" "$KOBJFILE" \ - "output/$i" "$SRCDIR/Module.symvers" "${MODNAME//-/_}" 2>&1 | logger 1 + if [[ "$KOBJFILE" = vmlinux ]]; then + KOBJFILE_NAME=vmlinux + KOBJFILE_PATH="$VMLINUX" + SYMTAB="${TEMPDIR}/${KOBJFILE_NAME}.symtab" + else + KOBJFILE_NAME=$(basename "${KOBJFILE%.ko}") + KOBJFILE_NAME="${KOBJFILE_NAME/-/_}" + KOBJFILE_PATH="${TEMPDIR}/module/$KOBJFILE" + SYMTAB="${KOBJFILE_PATH}.symtab" + fi + + eu-readelf -s "$KOBJFILE_PATH" > "$SYMTAB" + + # create-diff-object orig.o patched.o parent-name parent-symtab + # Module.symvers patch-mod-name output.o + "$TOOLSDIR"/create-diff-object "orig/$i" "patched/$i" "$KOBJFILE_NAME" \ + "$SYMTAB" "$SRCDIR/Module.symvers" "${MODNAME//-/_}" \ + "output/$i" 2>&1 | logger 1 check_pipe_status create-diff-object # create-diff-object returns 3 if no functional change is found [[ "$rc" -eq 0 ]] || [[ "$rc" -eq 3 ]] || ERROR="$((ERROR + 1))" @@ -808,7 +818,7 @@ fi echo -n "Patched objects:" for i in $(echo "${objnames[@]}" | tr ' ' '\n' | sort -u | tr '\n' ' ') do - echo -n " $(basename "$i")" + echo -n " $i" done echo diff --git a/kpatch-build/lookup.c b/kpatch-build/lookup.c index e4b2dca..b7dc6d9 100644 --- a/kpatch-build/lookup.c +++ b/kpatch-build/lookup.c @@ -43,7 +43,7 @@ struct object_symbol { unsigned long value; unsigned long size; char *name; - int type, bind, skip; + int type, bind; }; struct export_symbol { @@ -174,86 +174,6 @@ static void find_local_syms(struct lookup_table *table, char *hint, ERROR("find_local_syms for %s: found_none", hint); } -static void obj_read(struct lookup_table *table, char *path) -{ - Elf *elf; - int fd, i, len; - Elf_Scn *scn; - GElf_Shdr sh; - GElf_Sym sym; - Elf_Data *data; - char *name; - struct object_symbol *mysym; - size_t shstrndx; - - if ((fd = open(path, O_RDONLY, 0)) < 0) - ERROR("open"); - - elf_version(EV_CURRENT); - - elf = elf_begin(fd, ELF_C_READ_MMAP, NULL); - if (!elf) { - printf("%s\n", elf_errmsg(-1)); - ERROR("elf_begin"); - } - - if (elf_getshdrstrndx(elf, &shstrndx)) - ERROR("elf_getshdrstrndx"); - - scn = NULL; - while ((scn = elf_nextscn(elf, scn))) { - if (!gelf_getshdr(scn, &sh)) - ERROR("gelf_getshdr"); - - name = elf_strptr(elf, shstrndx, sh.sh_name); - if (!name) - ERROR("elf_strptr scn"); - - if (!strcmp(name, ".symtab")) - break; - } - - if (!scn) - ERROR(".symtab section not found"); - - data = elf_getdata(scn, NULL); - if (!data) - ERROR("elf_getdata"); - - len = sh.sh_size / sh.sh_entsize; - - table->obj_syms = malloc(len * sizeof(*table->obj_syms)); - if (!table->obj_syms) - ERROR("malloc table.obj_syms"); - memset(table->obj_syms, 0, len * sizeof(*table->obj_syms)); - table->obj_nr = len; - - for_each_obj_symbol(i, mysym, table) { - if (!gelf_getsym(data, i, &sym)) - ERROR("gelf_getsym"); - - if (sym.st_shndx == SHN_UNDEF) { - mysym->skip = 1; - continue; - } - - name = elf_strptr(elf, sh.sh_link, sym.st_name); - if(!name) - ERROR("elf_strptr sym"); - - mysym->value = sym.st_value; - mysym->size = sym.st_size; - mysym->type = GELF_ST_TYPE(sym.st_info); - mysym->bind = GELF_ST_BIND(sym.st_info); - mysym->name = strdup(name); - if (!mysym->name) - ERROR("strdup"); - } - - close(fd); - elf_end(elf); -} - /* Strip the path and replace '-' with '_' */ static char *make_modname(char *modname) { @@ -272,6 +192,72 @@ static char *make_modname(char *modname) return basename(modname); } +static void symtab_read(struct lookup_table *table, char *path) +{ + FILE *file; + long unsigned int value, size; + unsigned int i = 0; + char line[256], name[256], type[16], bind[16]; + + if ((file = fopen(path, "r")) == NULL) + ERROR("fopen"); + + while (fgets(line, 256, file)) { + if (sscanf(line, "%*s %lx %lu %s %s %*s %*s %s\n", + &value, &size, type, bind, name) == 5 && + strcmp(bind, "SECTION")) { + table->obj_nr++; + } + } + + table->obj_syms = malloc(table->obj_nr * sizeof(*table->obj_syms)); + if (!table->obj_syms) + ERROR("malloc table.obj_syms"); + memset(table->obj_syms, 0, table->obj_nr * sizeof(*table->obj_syms)); + + rewind(file); + + while (fgets(line, 256, file)) { + if (sscanf(line, "%*s %lx %lu %s %s %*s %*s %s\n", + &value, &size, type, bind, name) != 5 || + !strcmp(bind, "SECTION")) { + continue; + } + table->obj_syms[i].value = value; + table->obj_syms[i].size = size; + table->obj_syms[i].name = strdup(name); + + if (!strcmp(bind, "LOCAL")) { + table->obj_syms[i].bind = STB_LOCAL; + } else if (!strcmp(bind, "GLOBAL")) { + table->obj_syms[i].bind = STB_GLOBAL; + } else if (!strcmp(bind, "WEAK")) { + table->obj_syms[i].bind = STB_WEAK; + } else { + ERROR("unknown symbol bind %s", bind); + } + + if (!strcmp(type, "NOTYPE")) { + table->obj_syms[i].type = STT_NOTYPE; + } else if (!strcmp(type, "OBJECT")) { + table->obj_syms[i].type = STT_OBJECT; + } else if (!strcmp(type, "FUNC")) { + table->obj_syms[i].type = STT_FUNC; + } else if (!strcmp(type, "FILE")) { + table->obj_syms[i].type = STT_FILE; + } else { + ERROR("unknown symbol type %s", type); + } + + table->obj_syms[i].name = strdup(name); + if (!table->obj_syms[i].name) + ERROR("strdup"); + i++; + } + + fclose(file); +} + static void symvers_read(struct lookup_table *table, char *path) { FILE *file; @@ -279,7 +265,7 @@ static void symvers_read(struct lookup_table *table, char *path) char name[256], mod[256], export[256]; char *objname, *symname; - if ((file = fopen(path, "r")) < 0) + if ((file = fopen(path, "r")) == NULL) ERROR("fopen"); while (fscanf(file, "%x %s %s %s\n", @@ -314,7 +300,7 @@ static void symvers_read(struct lookup_table *table, char *path) fclose(file); } -struct lookup_table *lookup_open(char *obj_path, char *symvers_path, +struct lookup_table *lookup_open(char *symtab_path, char *symvers_path, char *hint, struct sym_compare_type *locals) { struct lookup_table *table; @@ -324,7 +310,7 @@ struct lookup_table *lookup_open(char *obj_path, char *symvers_path, ERROR("malloc table"); memset(table, 0, sizeof(*table)); - obj_read(table, obj_path); + symtab_read(table, symtab_path); symvers_read(table, symvers_path); find_local_syms(table, hint, locals); @@ -350,9 +336,6 @@ int lookup_local_symbol(struct lookup_table *table, char *name, memset(result, 0, sizeof(*result)); for_each_obj_symbol(i, sym, table) { - if (sym->skip) - continue; - if (sym->bind == STB_LOCAL && !strcmp(sym->name, name)) pos++; @@ -390,7 +373,7 @@ int lookup_global_symbol(struct lookup_table *table, char *name, memset(result, 0, sizeof(*result)); for_each_obj_symbol(i, sym, table) { - if (!sym->skip && (sym->bind == STB_GLOBAL || sym->bind == STB_WEAK) && + if ((sym->bind == STB_GLOBAL || sym->bind == STB_WEAK) && !strcmp(sym->name, name)) { result->value = sym->value; result->size = sym->size; diff --git a/kpatch-build/lookup.h b/kpatch-build/lookup.h index 327a9d5..420d0f0 100644 --- a/kpatch-build/lookup.h +++ b/kpatch-build/lookup.h @@ -14,7 +14,7 @@ struct sym_compare_type { int type; }; -struct lookup_table *lookup_open(char *obj_path, char *symvers_path, +struct lookup_table *lookup_open(char *symtab_path, char *symvers_path, char *hint, struct sym_compare_type *locals); void lookup_close(struct lookup_table *table); int lookup_local_symbol(struct lookup_table *table, char *name, diff --git a/test/unit/Makefile.include b/test/unit/Makefile.include index 4f44e1b..75b550f 100644 --- a/test/unit/Makefile.include +++ b/test/unit/Makefile.include @@ -4,6 +4,8 @@ EXT_FAIL ?= PATCHED.FAIL.o EXT_TEST ?= test EXT_OUTPUT ?= OUTPUT.o EXT_TEST_OUTPUT ?= test.out +EXT_SYMTAB ?= symtab +EXT_SYMVERS ?= symvers TNAME = $(@:.$(EXT_OUTPUT)=) ifndef VERBOSE @@ -21,6 +23,8 @@ TEST_ENV = KPATCH_TEST_LIBRARY=$(TEST_LIBRARY) TARGETS = $(patsubst %.$(EXT_ORIG),%.$(EXT_OUTPUT),$(wildcard *.$(EXT_ORIG))) TEST_TARGETS = $(patsubst %.$(EXT_TEST),%.$(EXT_TEST_OUTPUT),$(wildcard *.$(EXT_TEST))) +SYMVERS_FILE = $(if $(wildcard $(TNAME).$(EXT_SYMVERS)),$(TNAME).$(EXT_SYMVERS),/dev/null) + define check_stripped = $(if $(shell readelf --debug-dump $(1)), $(error $(1) is not properly stripped, use 'strip --strip-debug --keep-file-symbols $(1)' to fix this), @@ -31,30 +35,36 @@ define check_all = $(call check_stripped,$(1)) endef + all: $(TARGETS) $(TEST_TARGETS) clean: rm -f *.$(EXT_TEST_OUTPUT) *.$(EXT_OUTPUT) +%.$(EXT_SYMTAB): + eu-readelf -s $(patsubst %.$(EXT_SYMTAB),%.$(EXT_ORIG),$(@)) >$@ + %.$(EXT_TEST_OUTPUT): %.$(EXT_OUTPUT) %.$(EXT_TEST) $(TEST_LIBRARY) @echo "TEST $(@:.$(EXT_TEST_OUTPUT)=)" $(TEST_ENV) bash $(@:.$(EXT_TEST_OUTPUT)=.$(EXT_TEST)) $< # Don't rely on script creating this @touch $@ -%.$(EXT_OUTPUT): %.$(EXT_ORIG) %.$(EXT_PATCHED) $(CDO) +%.$(EXT_OUTPUT): %.$(EXT_ORIG) %.$(EXT_PATCHED) %.$(EXT_SYMTAB) $(CDO) @echo "BUILD $(TNAME)" $(call check_all,$(TNAME).$(EXT_ORIG)) $(call check_all,$(TNAME).$(EXT_PATCHED)) $(CDO_ENV) $(CDO) $(TNAME).$(EXT_ORIG) $(TNAME).$(EXT_PATCHED) \ - $(TNAME).$(EXT_ORIG) $@ /dev/null test_$(TNAME) $(MUTE_PASS) + vmlinux $(TNAME).$(EXT_SYMTAB) $(SYMVERS_FILE) \ + test_$(TNAME) $@ $(MUTE_PASS) -%.$(EXT_OUTPUT): %.$(EXT_ORIG) %.$(EXT_FAIL) $(CDO) +%.$(EXT_OUTPUT): %.$(EXT_ORIG) %.$(EXT_FAIL) %.$(EXT_SYMTAB) $(CDO) @echo "BUILD $(TNAME)-FAIL" $(call check_all,$(TNAME).$(EXT_ORIG)) $(call check_all,$(TNAME).$(EXT_FAIL)) ! $(CDO_ENV) $(CDO) $(TNAME).$(EXT_ORIG) $(TNAME).$(EXT_FAIL) \ - $(TNAME).$(EXT_ORIG) $@ /dev/null test_$(TNAME) $(MUTE_FAIL) + vmlinux $(TNAME).$(EXT_SYMTAB) $(SYMVERS_FILE) \ + test_$(TNAME) $@ $(MUTE_FAIL) # Expecting to fail, thus create output file manually so we won't rerun the # test without clean @touch $@