From af1fe267c5c23c45f432a7740953d9a889f77156 Mon Sep 17 00:00:00 2001 From: Julien Thierry Date: Thu, 31 Oct 2019 07:37:52 +0000 Subject: [PATCH 1/6] create-diff-object: Avoid unnecessary parent symbol inclusion When a child symbol has changed, the parent symbol is only needed in the output object if the child symbol is unpatchable on its own. This is the case when the child symbol does not have its own profiling call. Only include unchanged parent symbols if their child has changed and the child does not have a function profiling call. Signed-off-by: Julien Thierry --- kpatch-build/create-diff-object.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/kpatch-build/create-diff-object.c b/kpatch-build/create-diff-object.c index aedd07d..243af93 100644 --- a/kpatch-build/create-diff-object.c +++ b/kpatch-build/create-diff-object.c @@ -692,7 +692,8 @@ static void kpatch_compare_sections(struct list_head *seclist) sym->status = sec->status; if (sym && sym->child && sym->status == SAME && - sym->child->sec->status == CHANGED) + sym->child->sec->status == CHANGED && + !sym->child->has_func_profiling) sym->status = CHANGED; } } @@ -1402,7 +1403,8 @@ static void kpatch_check_func_profiling_calls(struct kpatch_elf *kelf) int errs = 0; list_for_each_entry(sym, &kelf->symbols, list) { - if (sym->type != STT_FUNC || sym->status != CHANGED || sym->parent) + if (sym->type != STT_FUNC || sym->status != CHANGED || + (sym->parent && sym->parent->status == CHANGED)) continue; if (!sym->twin->has_func_profiling) { log_normal("function %s has no fentry/mcount call, unable to patch\n", From b502e5b1cc29ef9ce23d0a93d8e212de8b82d51c Mon Sep 17 00:00:00 2001 From: Julien Thierry Date: Mon, 16 Sep 2019 11:57:05 +0100 Subject: [PATCH 2/6] kpatch-build: Allow function to have multiple child functions A symbol associated to a function can be split into multiple sub-functions. Currently, kpatch only supports one child per function. Extend this to support an arbitrary number of sub-function per function. Signed-off-by: Julien Thierry --- kpatch-build/create-diff-object.c | 46 +++++++++++++++++++++++++------ kpatch-build/kpatch-elf.c | 2 ++ kpatch-build/kpatch-elf.h | 3 +- 3 files changed, 42 insertions(+), 9 deletions(-) diff --git a/kpatch-build/create-diff-object.c b/kpatch-build/create-diff-object.c index 243af93..5f33fd6 100644 --- a/kpatch-build/create-diff-object.c +++ b/kpatch-build/create-diff-object.c @@ -221,6 +221,7 @@ static void kpatch_detect_child_functions(struct kpatch_elf *kelf) list_for_each_entry(sym, &kelf->symbols, list) { char *coldstr; + struct symbol *parent; coldstr = strstr(sym->name, ".cold."); if (coldstr != NULL) { @@ -230,13 +231,14 @@ static void kpatch_detect_child_functions(struct kpatch_elf *kelf) if (!pname) ERROR("strndup"); - sym->parent = find_symbol_by_name(&kelf->symbols, pname); + parent = find_symbol_by_name(&kelf->symbols, pname); free(pname); - if (!sym->parent) + if (!parent) ERROR("failed to find parent function for %s", sym->name); - sym->parent->child = sym; + list_add_tail(&sym->subfunction_node, &parent->children); + sym->parent = parent; } } } @@ -659,6 +661,26 @@ static int kpatch_line_macro_change_only(struct section *sec) } #endif +/* + * Child functions with "*.cold" names don't have _fentry_ calls, but "*.part", + * often do. In the later case, it is not necessary to include the parent + * in the output object when the child function has changed. + */ +static bool kpatch_changed_child_needs_parent_profiling(struct symbol *sym) +{ + struct symbol *child; + + list_for_each_entry(child, &sym->children, subfunction_node) { + if (child->has_func_profiling) + continue; + if (child->sec->status == CHANGED || + kpatch_changed_child_needs_parent_profiling(child)) + return true; + } + + return false; +} + static void kpatch_compare_sections(struct list_head *seclist) { struct section *sec; @@ -691,9 +713,8 @@ static void kpatch_compare_sections(struct list_head *seclist) if (sym && sym->status != CHANGED) sym->status = sec->status; - if (sym && sym->child && sym->status == SAME && - sym->child->sec->status == CHANGED && - !sym->child->has_func_profiling) + if (sym && sym->status == SAME && + kpatch_changed_child_needs_parent_profiling(sym)) sym->status = CHANGED; } } @@ -2364,6 +2385,16 @@ static void kpatch_mark_ignored_sections_same(struct kpatch_elf *kelf) sym->status = SAME; } +static void kpatch_mark_ignored_children_same(struct symbol *sym) +{ + struct symbol *child; + + list_for_each_entry(child, &sym->children, subfunction_node) { + child->status = SAME; + kpatch_mark_ignored_children_same(child); + } +} + static void kpatch_mark_ignored_functions_same(struct kpatch_elf *kelf) { struct section *sec; @@ -2385,8 +2416,7 @@ static void kpatch_mark_ignored_functions_same(struct kpatch_elf *kelf) rela->sym->status = SAME; rela->sym->sec->status = SAME; - if (rela->sym->child) - rela->sym->child->status = SAME; + kpatch_mark_ignored_children_same(rela->sym); if (rela->sym->sec->secsym) rela->sym->sec->secsym->status = SAME; diff --git a/kpatch-build/kpatch-elf.c b/kpatch-build/kpatch-elf.c index c6af59e..377dc7f 100644 --- a/kpatch-build/kpatch-elf.c +++ b/kpatch-build/kpatch-elf.c @@ -277,6 +277,8 @@ void kpatch_create_symbol_list(struct kpatch_elf *kelf) while (symbols_nr--) { ALLOC_LINK(sym, &kelf->symbols); + INIT_LIST_HEAD(&sym->children); + sym->index = index; if (!gelf_getsym(symtab->data, index, &sym->sym)) ERROR("gelf_getsym"); diff --git a/kpatch-build/kpatch-elf.h b/kpatch-build/kpatch-elf.h index 3a0dc4b..d2bb454 100644 --- a/kpatch-build/kpatch-elf.h +++ b/kpatch-build/kpatch-elf.h @@ -71,7 +71,8 @@ struct symbol { struct list_head list; struct symbol *twin; struct symbol *parent; - struct symbol *child; + struct list_head children; + struct list_head subfunction_node; struct section *sec; GElf_Sym sym; char *name; From 42128ff78caf8f4e6f72d2849637302e022ae0bd Mon Sep 17 00:00:00 2001 From: Julien Thierry Date: Mon, 16 Sep 2019 13:30:02 +0100 Subject: [PATCH 3/6] kpatch-build: Include .part. symbols as child function Consider symbols containing .part. in their names as sub-function of the symbols they are derived from (if such symbol still exists in the object file). Signed-off-by: Julien Thierry --- kpatch-build/create-diff-object.c | 57 +++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 18 deletions(-) diff --git a/kpatch-build/create-diff-object.c b/kpatch-build/create-diff-object.c index 5f33fd6..b8423c9 100644 --- a/kpatch-build/create-diff-object.c +++ b/kpatch-build/create-diff-object.c @@ -209,9 +209,28 @@ static void kpatch_bundle_symbols(struct kpatch_elf *kelf) } } +static struct symbol *kpatch_lookup_parent(struct kpatch_elf *kelf, + const char *symname, + const char *child_suffix) +{ + struct symbol *parent; + char *pname; + + pname = strndup(symname, child_suffix - symname); + if (!pname) + ERROR("strndup"); + + parent = find_symbol_by_name(&kelf->symbols, pname); + free(pname); + + return parent; +} + /* * During optimization gcc may move unlikely execution branches into *.cold - * subfunctions. kpatch_detect_child_functions detects such subfunctions and + * subfunctions. Some functions can also be split into multiple *.part + * functions. + * kpatch_detect_child_functions detects such subfunctions and * crossreferences them with their parent functions through parent/child * pointers. */ @@ -220,26 +239,28 @@ static void kpatch_detect_child_functions(struct kpatch_elf *kelf) struct symbol *sym; list_for_each_entry(sym, &kelf->symbols, list) { - char *coldstr; - struct symbol *parent; + char *childstr; - coldstr = strstr(sym->name, ".cold."); - if (coldstr != NULL) { - char *pname; + if (sym->type != STT_FUNC) + continue; - pname = strndup(sym->name, coldstr - sym->name); - if (!pname) - ERROR("strndup"); - - parent = find_symbol_by_name(&kelf->symbols, pname); - free(pname); - - if (!parent) - ERROR("failed to find parent function for %s", sym->name); - - list_add_tail(&sym->subfunction_node, &parent->children); - sym->parent = parent; + childstr = strstr(sym->name, ".cold."); + if (childstr) { + sym->parent = kpatch_lookup_parent(kelf, sym->name, + childstr); + if (!sym->parent) + ERROR("failed to find parent function for %s", + sym->name); + } else { + childstr = strstr(sym->name, ".part."); + if (!childstr) + continue; + sym->parent = kpatch_lookup_parent(kelf, sym->name, + childstr); } + + if (sym->parent) + list_add_tail(&sym->subfunction_node, &sym->parent->children); } } From b548ba153fd35ee24222c7a0b91d0db8530fa45c Mon Sep 17 00:00:00 2001 From: Julien Thierry Date: Mon, 16 Sep 2019 14:11:15 +0100 Subject: [PATCH 4/6] kpatch-build: Look for local static variables in child functions A symbol in the original object might get split in several sub-functions in the patched object, which can themselves be bundled (and use a separate rela section). References to local static variables from the original function, might have been moved in one of the sub-functions in the patched object. Look for references to local static variables in the rela section of child symbols in the patched object. Signed-off-by: Julien Thierry --- kpatch-build/create-diff-object.c | 140 ++++++++++++++++++++++++------ 1 file changed, 115 insertions(+), 25 deletions(-) diff --git a/kpatch-build/create-diff-object.c b/kpatch-build/create-diff-object.c index b8423c9..fcef50a 100644 --- a/kpatch-build/create-diff-object.c +++ b/kpatch-build/create-diff-object.c @@ -757,6 +757,13 @@ static int kpatch_subsection_changed(struct section *sec1, struct section *sec2) return kpatch_subsection_type(sec1) != kpatch_subsection_type(sec2); } +static struct symbol *kpatch_get_correlated_parent(struct symbol *sym) +{ + while (sym->parent && !sym->parent->twin) + sym = sym->parent; + return sym->parent; +} + static void kpatch_compare_correlated_symbol(struct symbol *sym) { struct symbol *sym1 = sym, *sym2 = sym->twin; @@ -1008,21 +1015,13 @@ static char *kpatch_section_function_name(struct section *sec) return sec->sym ? sec->sym->name : sec->name; } -/* - * Given a static local variable symbol and a rela section which references it - * in the base object, find a corresponding usage of a similarly named symbol - * in the patched object. - */ -static struct symbol *kpatch_find_static_twin(struct section *sec, - struct symbol *sym) +static struct symbol *kpatch_find_uncorrelated_rela(struct section *rela_sec, + struct symbol *sym) { struct rela *rela, *rela_toc; - if (!sec->twin) - return NULL; - /* find the patched object's corresponding variable */ - list_for_each_entry(rela, &sec->twin->relas, list) { + list_for_each_entry(rela, &rela_sec->relas, list) { struct symbol *patched_sym; rela_toc = toc_rela(rela); @@ -1048,6 +1047,67 @@ static struct symbol *kpatch_find_static_twin(struct section *sec, return NULL; } +static struct symbol *kpatch_find_static_twin_in_children(struct symbol *parent, + struct symbol *sym) +{ + struct symbol *child; + + list_for_each_entry(child, &parent->children, subfunction_node) { + struct symbol *res; + + /* Only look in children whose rela section differ from the parent's */ + if (child->sec->rela == parent->sec->rela || !child->sec->rela) + continue; + + res = kpatch_find_uncorrelated_rela(child->sec->rela, sym); + /* Need to go deeper */ + if (!res) + res = kpatch_find_static_twin_in_children(child, sym); + if (res != NULL) + return res; + } + + return NULL; +} + +/* + * Given a static local variable symbol and a rela section which references it + * in the base object, find a corresponding usage of a similarly named symbol + * in the patched object. + */ +static struct symbol *kpatch_find_static_twin(struct section *sec, + struct symbol *sym) +{ + struct symbol *res; + + if (!sec->twin && sec->base->sym) { + struct symbol *parent = NULL; + + /* + * The static twin might have been in a .part. symbol in the + * original object that got removed in the patched object. + */ + parent = kpatch_get_correlated_parent(sec->base->sym); + if (parent) + sec = parent->sec->rela; + + } + + if (!sec->twin) + return NULL; + + res = kpatch_find_uncorrelated_rela(sec->twin, sym); + if (res != NULL) + return res; + + /* Look if reference might have moved to child functions' sections */ + if (sec->twin->base->sym) + return kpatch_find_static_twin_in_children(sec->twin->base->sym, + sym); + + return NULL; +} + static int kpatch_is_normal_static_local(struct symbol *sym) { if (sym->type != STT_OBJECT || sym->bind != STB_LOCAL) @@ -1062,6 +1122,35 @@ static int kpatch_is_normal_static_local(struct symbol *sym) return 1; } +static struct rela *kpatch_find_static_twin_ref(struct section *rela_sec, struct symbol *sym) +{ + struct rela *rela; + + list_for_each_entry(rela, &rela_sec->relas, list) { + if (rela->sym == sym->twin) + return rela; + } + + /* Reference to static variable might have moved to child function section */ + if (rela_sec->base->sym) { + struct symbol *parent = rela_sec->base->sym; + struct symbol *child; + + list_for_each_entry(child, &parent->children, subfunction_node) { + /* Only look in children whose rela section differ from the parent's */ + if (child->sec->rela == parent->sec->rela || + !child->sec->rela) + continue; + + rela = kpatch_find_static_twin_ref(child->sec->rela, sym); + if (rela) + return rela; + } + } + + return NULL; +} + /* * gcc renames static local variables by appending a period and a number. For * example, __foo could be renamed to __foo.31452. Unfortunately this number @@ -1091,8 +1180,8 @@ static void kpatch_correlate_static_local_variables(struct kpatch_elf *base, { struct symbol *sym, *patched_sym; struct section *sec; - struct rela *rela, *rela2; - int bundled, patched_bundled, found; + struct rela *rela; + int bundled, patched_bundled; /* * First undo the correlations for all static locals. Two static @@ -1192,28 +1281,29 @@ static void kpatch_correlate_static_local_variables(struct kpatch_elf *base, continue; list_for_each_entry(rela, &sec->relas, list) { + struct section *target_sec = sec; sym = rela->sym; if (!kpatch_is_normal_static_local(sym)) continue; - if (!sym->twin || !sec->twin) - DIFF_FATAL("reference to static local variable %s in %s was removed", - sym->name, - kpatch_section_function_name(sec)); + if (!sec->twin && sec->base->sym) { + struct symbol *parent = NULL; - found = 0; - list_for_each_entry(rela2, &sec->twin->relas, list) { - if (rela2->sym == sym->twin) { - found = 1; - break; - } + parent = kpatch_get_correlated_parent(sec->base->sym); + if (parent) + target_sec = parent->sec->rela; } - if (!found) + if (!sym->twin || !target_sec->twin) + DIFF_FATAL("reference to static local variable %s in %s was removed", + sym->name, + kpatch_section_function_name(target_sec)); + + if (!kpatch_find_static_twin_ref(target_sec->twin, sym)) DIFF_FATAL("static local %s has been correlated with %s, but patched %s is missing a reference to it", sym->name, sym->twin->name, - kpatch_section_function_name(sec->twin)); + kpatch_section_function_name(target_sec->twin)); } } From 2265ce6fc26cad1723b08b9f0d9ec6ddc25f01d4 Mon Sep 17 00:00:00 2001 From: Julien Thierry Date: Mon, 9 Mar 2020 14:29:45 +0000 Subject: [PATCH 5/6] test/unit: Add unit test for static local moving sections Signed-off-by: Julien Thierry --- test/unit/objs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/objs b/test/unit/objs index 6bd3e93..126104c 160000 --- a/test/unit/objs +++ b/test/unit/objs @@ -1 +1 @@ -Subproject commit 6bd3e931ef29967a0b5bb7f7b4e4dc999e38baea +Subproject commit 126104cae25a649a1013a7f1533906d0f49a98e7 From 24fc731ab2db9aa6859235d56e4a1b62d622240e Mon Sep 17 00:00:00 2001 From: Julien Thierry Date: Mon, 30 Mar 2020 14:21:40 +0100 Subject: [PATCH 6/6] test/integration: Reenable test Reenable previously failing test gcc-static-local-var-5. Signed-off-by: Julien Thierry --- ...ic-local-var-5.patch.disabled => gcc-static-local-var-5.patch} | 0 ...ic-local-var-5.patch.disabled => gcc-static-local-var-5.patch} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename test/integration/rhel-8.0/{gcc-static-local-var-5.patch.disabled => gcc-static-local-var-5.patch} (100%) rename test/integration/rhel-8.1/{gcc-static-local-var-5.patch.disabled => gcc-static-local-var-5.patch} (100%) diff --git a/test/integration/rhel-8.0/gcc-static-local-var-5.patch.disabled b/test/integration/rhel-8.0/gcc-static-local-var-5.patch similarity index 100% rename from test/integration/rhel-8.0/gcc-static-local-var-5.patch.disabled rename to test/integration/rhel-8.0/gcc-static-local-var-5.patch diff --git a/test/integration/rhel-8.1/gcc-static-local-var-5.patch.disabled b/test/integration/rhel-8.1/gcc-static-local-var-5.patch similarity index 100% rename from test/integration/rhel-8.1/gcc-static-local-var-5.patch.disabled rename to test/integration/rhel-8.1/gcc-static-local-var-5.patch