From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <39UinYggKCkAirtqekfciqqing.eqonkdcdkicknuqwtegyctg.qti@flex--gprocida.bounces.google.com> Received: from mail-ed1-x549.google.com (mail-ed1-x549.google.com [IPv6:2a00:1450:4864:20::549]) by sourceware.org (Postfix) with ESMTPS id CD0353851AA3 for ; Mon, 13 Jun 2022 14:25:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CD0353851AA3 Received: by mail-ed1-x549.google.com with SMTP id j4-20020aa7ca44000000b0042dd12a7bc5so4042867edt.13 for ; Mon, 13 Jun 2022 07:25:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:in-reply-to:message-id:mime-version :references:subject:from:to:cc; bh=LhQvJZ9KFNAkK1LMP4HS0tftAi2b7lyoBp07+7pRc44=; b=dOYKXhbw2s82VhjzTb/LOLfKoj8tW2Sew2+CKPrLVeoQgEitwACYSdSrLnrFWReurz YSBMmkBZA40sJvGD3hHQGNasO0RJf4c2Zb/YbZrm//irRyPZJc4ugj1uQHF9AQ6L2H83 9eVVkmlnBKfJc+PbGtRc00LlFvZRUJ2rmeCrTpzPib9jz58pi18Pcpp5dge8+G4AHqIk tx0bTLiurN7g8tyry02b2TeJ8McywhcpbGsTykK8zxh04FS6XV3V3RhZBEydm6PVeAMm qgl2petlBpw6jvA2ND7/NeIm1EnOhBLFBdrC/higog+EpMgrW/cxo3VTckhxxddG0kKW iLig== X-Gm-Message-State: AOAM532Of6B4y8ZXsmbE2CvduvLyv7N+sCUz6BPjnZy68/NAvWtvPtC6 ftHboo52WcBCSFvtGBrCS9QGfd+/PYpwpE732u8oLjWsRWn1Mh8IpyXKIDqWUfJEyxFgieZ1vAK Ow5EDFGaMSAE+Isv1l84+zuIgGwzJosHorSwdHWj+IVH3Vpm3ADRWdP4UiffeS5DFup4t06Q= X-Google-Smtp-Source: ABdhPJwvGSWPGPijiIdc/iImTSrLp2r5AsRRWtK4xT7m0NKtYCy/V8RXdXtb9BuEGhZzJUNmuY2yAA8Z86wRuw== X-Received: from tef.lon.corp.google.com ([2a00:79e0:d:209:8336:7478:639d:964a]) (user=gprocida job=sendgmr) by 2002:a05:6402:348e:b0:42e:2e1a:817c with SMTP id v14-20020a056402348e00b0042e2e1a817cmr59364603edc.23.1655130357506; Mon, 13 Jun 2022 07:25:57 -0700 (PDT) Date: Mon, 13 Jun 2022 15:25:33 +0100 In-Reply-To: <20220321160221.1372398-1-gprocida@google.com> Message-Id: <20220613142533.3676501-5-gprocida@google.com> Mime-Version: 1.0 References: <20220321160221.1372398-1-gprocida@google.com> X-Mailer: git-send-email 2.36.1.476.g0c4daa206d-goog Subject: [PATCH v5 4/4] add Linux kernel symbol namespace support From: Giuliano Procida To: libabigail@sourceware.org Cc: dodji@seketeli.org, kernel-team@android.com, gprocida@google.com, maennich@google.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-21.4 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE, USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Jun 2022 14:26:02 -0000 Bug 28954 - add Linux Kernel symbol namespace support Each Linux kernel symbol can be exported to a specified named namespace or left in the global (nameless) namespace. One complexity is that the symbol values which identify a string in the __ksymtab_strings section must be interpretated differently for vmlinux and .ko loadable modules as the former has a fixed load address but the latter are relocatable. For vmlinux, the section base address needs to be subtracted to obtain a section-relative offset. The global namespace is explicitly represented as the empty string, at least when it comes to the value of __kstrtabns_FOO symbols, but the common interpretation is that such symbols lack an export namespace. I would rather not have to make use of "empty implies missing" in many places, so the code here represents namespace as optional and only the symtab reader cares about empty strings in __ksymtab_strings. * include/abg-ir.h (elf_symbol::elf_symbol): Add ns argument. (elf_symbol::create): Add ns argument. (elf_symbol::get_namespace): Declare new function. (elf_symbol::set_namespace): Declare new function. and set_namespace. * src/abg-comp-filter.cc (namespace_changed): Define new helper functions. (categorize_harmful_diff_node): Also call namespace_changed(). * src/abg-ir.cc (elf_symbol::priv): Add namespace_ member. (elf_symbol::priv::priv): Add namespace_ to initialisers. (elf_symbol::elf_symbol): Take new ns argument and pass it to priv constructor. (elf_symbol::create): Take new ns argument and pass it to elf_symbol constructor. (elf_symbol::get_namespace): Define new function. (elf_symbol::set_namespace): Define new function. * src/abg-reader.cc (build_elf_symbol): If namespace attribute is present, set symbol namespace. * src/abg-reporter-priv.cc (maybe_report_diff_for_symbol): If symbol namespaces differ, report this. * src/abg-symtab-reader.cc (symtab::load): Get ELF header to distinguish vmlinux from .ko. Try to get __ksymtab_strings metadata and data. Use these to look up __kstrtabns_FOO namespace entries. Set symbol namespace where found. * src/abg-writer.cc (write_elf_symbol): Emit namespace attribute, if symbol has a namespace. * tests/data/Makefile.am: Add new test files. * tests/data/test-abidiff/test-namespace-0.xml: New test file. * tests/data/test-abidiff/test-namespace-1.xml: Likewise * tests/data/test-abidiff/test-namespace-report.txt: Likewise. * tests/test-abidiff.cc: Add new test case. Reviewed-by: Matthias Maennich Signed-off-by: Giuliano Procida --- include/abg-ir.h | 8 +++ src/abg-comp-filter.cc | 40 +++++++++++- src/abg-ir.cc | 30 ++++++++- src/abg-reader.cc | 7 ++ src/abg-reporter-priv.cc | 18 ++++++ src/abg-symtab-reader.cc | 64 +++++++++++++++++++ src/abg-writer.cc | 3 + tests/data/Makefile.am | 3 + tests/data/test-abidiff/test-namespace-0.xml | 15 +++++ tests/data/test-abidiff/test-namespace-1.xml | 15 +++++ .../test-abidiff/test-namespace-report.txt | 17 +++++ tests/test-abidiff.cc | 6 ++ 12 files changed, 223 insertions(+), 3 deletions(-) create mode 100644 tests/data/test-abidiff/test-namespace-0.xml create mode 100644 tests/data/test-abidiff/test-namespace-1.xml create mode 100644 tests/data/test-abidiff/test-namespace-report.txt diff --git a/include/abg-ir.h b/include/abg-ir.h index b05a8c6f..7c558828 100644 --- a/include/abg-ir.h +++ b/include/abg-ir.h @@ -922,6 +922,7 @@ private: visibility vi, bool is_in_ksymtab = false, const abg_compat::optional& crc = {}, + const abg_compat::optional& ns = {}, bool is_suppressed = false); elf_symbol(const elf_symbol&); @@ -947,6 +948,7 @@ public: visibility vi, bool is_in_ksymtab = false, const abg_compat::optional& crc = {}, + const abg_compat::optional& ns = {}, bool is_suppressed = false); const environment* @@ -1024,6 +1026,12 @@ public: void set_crc(const abg_compat::optional& crc); + const abg_compat::optional& + get_namespace() const; + + void + set_namespace(const abg_compat::optional& ns); + bool is_suppressed() const; diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc index 31590284..22da5244 100644 --- a/src/abg-comp-filter.cc +++ b/src/abg-comp-filter.cc @@ -254,6 +254,43 @@ crc_changed(const diff* diff) return false; } +/// Test if there was a function or variable namespace change. +/// +/// @param f the first function or variable to consider. +/// +/// @param s the second function or variable to consider. +/// +/// @return true if the test is positive, false otherwise. +template +static bool +namespace_changed(const function_or_var_decl_sptr& f, + const function_or_var_decl_sptr& s) +{ + const auto& symbol_f = f->get_symbol(); + const auto& symbol_s = s->get_symbol(); + if (!symbol_f || !symbol_s) + return false; + return symbol_f->get_namespace() != symbol_s->get_namespace(); +} + +/// Test if the current diff tree node carries a namespace change in +/// either a function or a variable. +/// +/// @param diff the diff tree node to consider. +/// +/// @return true if the test is positive, false otherwise. +static bool +namespace_changed(const diff* diff) +{ + if (const function_decl_diff* d = + dynamic_cast(diff)) + return namespace_changed(d->first_function_decl(), + d->second_function_decl()); + if (const var_diff* d = dynamic_cast(diff)) + return namespace_changed(d->first_var(), d->second_var()); + return false; +} + /// Test if there was a function name change, but there there was no /// change in name of the underlying symbol. IOW, if the name of a /// function changed, but the symbol of the new function is equal to @@ -1781,7 +1818,8 @@ categorize_harmful_diff_node(diff *d, bool pre) || non_static_data_member_added_or_removed(d) || base_classes_added_or_removed(d) || has_harmful_enum_change(d) - || crc_changed(d))) + || crc_changed(d) + || namespace_changed(d))) category |= SIZE_OR_OFFSET_CHANGE_CATEGORY; if (has_virtual_mem_fn_change(d)) diff --git a/src/abg-ir.cc b/src/abg-ir.cc index 34ae486f..a271bfde 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -1728,6 +1728,7 @@ struct elf_symbol::priv bool is_common_; bool is_in_ksymtab_; abg_compat::optional crc_; + abg_compat::optional namespace_; bool is_suppressed_; elf_symbol_wptr main_symbol_; elf_symbol_wptr next_alias_; @@ -1745,6 +1746,7 @@ struct elf_symbol::priv is_common_(false), is_in_ksymtab_(false), crc_(), + namespace_(), is_suppressed_(false) {} @@ -1760,6 +1762,7 @@ struct elf_symbol::priv elf_symbol::visibility vi, bool is_in_ksymtab, const abg_compat::optional& crc, + const abg_compat::optional& ns, bool is_suppressed) : env_(e), index_(i), @@ -1773,6 +1776,7 @@ struct elf_symbol::priv is_common_(c), is_in_ksymtab_(is_in_ksymtab), crc_(crc), + namespace_(ns), is_suppressed_(is_suppressed) { if (!is_common_) @@ -1818,6 +1822,8 @@ elf_symbol::elf_symbol() /// @param vi the visibility of the symbol. /// /// @param crc the CRC (modversions) value of Linux Kernel symbols +/// +/// @param ns the namespace of Linux Kernel symbols, if any elf_symbol::elf_symbol(const environment* e, size_t i, size_t s, @@ -1830,6 +1836,7 @@ elf_symbol::elf_symbol(const environment* e, visibility vi, bool is_in_ksymtab, const abg_compat::optional& crc, + const abg_compat::optional& ns, bool is_suppressed) : priv_(new priv(e, i, @@ -1843,6 +1850,7 @@ elf_symbol::elf_symbol(const environment* e, vi, is_in_ksymtab, crc, + ns, is_suppressed)) {} @@ -1886,6 +1894,8 @@ elf_symbol::create() /// /// @param crc the CRC (modversions) value of Linux Kernel symbols /// +/// @param ns the namespace of Linux Kernel symbols, if any +/// /// @return a (smart) pointer to a newly created instance of @ref /// elf_symbol. elf_symbol_sptr @@ -1901,10 +1911,11 @@ elf_symbol::create(const environment* e, visibility vi, bool is_in_ksymtab, const abg_compat::optional& crc, + const abg_compat::optional& ns, bool is_suppressed) { elf_symbol_sptr sym(new elf_symbol(e, i, s, n, t, b, d, c, ve, vi, - is_in_ksymtab, crc, is_suppressed)); + is_in_ksymtab, crc, ns, is_suppressed)); sym->priv_->main_symbol_ = sym; return sym; } @@ -1926,7 +1937,8 @@ textually_equals(const elf_symbol&l, && l.is_defined() == r.is_defined() && l.is_common_symbol() == r.is_common_symbol() && l.get_version() == r.get_version() - && l.get_crc() == r.get_crc()); + && l.get_crc() == r.get_crc() + && l.get_namespace() == r.get_namespace()); if (equals && l.is_variable()) // These are variable symbols. Let's compare their symbol size. @@ -2146,6 +2158,20 @@ void elf_symbol::set_crc(const abg_compat::optional& crc) {priv_->crc_ = crc;} +/// Getter of the 'namespace' property. +/// +/// @return the namespace for Linux Kernel symbols, if any +const abg_compat::optional& +elf_symbol::get_namespace() const +{return priv_->namespace_;} + +/// Setter of the 'namespace' property. +/// +/// @param ns the new namespace for Linux Kernel symbols, if any +void +elf_symbol::set_namespace(const abg_compat::optional& ns) +{priv_->namespace_ = ns;} + /// Getter for the 'is-suppressed' property. /// /// @return true iff the current symbol has been suppressed by a diff --git a/src/abg-reader.cc b/src/abg-reader.cc index 7a9ad1f9..f9e420f1 100644 --- a/src/abg-reader.cc +++ b/src/abg-reader.cc @@ -3265,6 +3265,13 @@ build_elf_symbol(read_context& ctxt, const xmlNodePtr node, if (xml_char_sptr s = XML_NODE_GET_ATTRIBUTE(node, "crc")) e->set_crc(strtoull(CHAR_STR(s), NULL, 0)); + if (xml_char_sptr s = XML_NODE_GET_ATTRIBUTE(node, "namespace")) + { + std::string ns; + xml::xml_char_sptr_to_string(s, ns); + e->set_namespace(ns); + } + return e; } diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc index d000620b..fcfc9dce 100644 --- a/src/abg-reporter-priv.cc +++ b/src/abg-reporter-priv.cc @@ -1168,6 +1168,24 @@ maybe_report_diff_for_symbol(const elf_symbol_sptr& symbol1, out << std::noshowbase << std::dec << "\n"; } + + const abg_compat::optional& ns1 = symbol1->get_namespace(); + const abg_compat::optional& ns2 = symbol2->get_namespace(); + if (ns1 != ns2) + { + const std::string none = "(none)"; + out << indent << "namespace changed from "; + if (ns1.has_value()) + out << "'" << ns1.value() << "'"; + else + out << none; + out << " to "; + if (ns2.has_value()) + out << "'" << ns2.value() << "'"; + else + out << none; + out << "\n"; + } } /// For a given symbol, emit a string made of its name and version. diff --git a/src/abg-symtab-reader.cc b/src/abg-symtab-reader.cc index 0c2f8ccc..e20ee2a7 100644 --- a/src/abg-symtab-reader.cc +++ b/src/abg-symtab-reader.cc @@ -204,6 +204,13 @@ symtab::load_(Elf* elf_handle, ir::environment* env, symbol_predicate is_suppressed) { + GElf_Ehdr ehdr_mem; + GElf_Ehdr* header = gelf_getehdr(elf_handle, &ehdr_mem); + if (!header) + { + std::cerr << "Could not get ELF header: Skipping symtab load.\n"; + return false; + } Elf_Scn* symtab_section = elf_helpers::find_symbol_table_section(elf_handle); if (!symtab_section) @@ -232,9 +239,34 @@ symtab::load_(Elf* elf_handle, return false; } + // The __kstrtab_strings sections is basically an ELF strtab but does not + // support elf_strptr lookups. A single call to elf_getdata gives a handle to + // washed section data. + // + // The value of a __kstrtabns_FOO (or other similar) symbol is an address + // within the __kstrtab_strings section. To look up the string value, we need + // to translate from vmlinux load address to section offset by subtracting the + // base address of the section. This adjustment is not needed for loadable + // modules which are relocatable and so identifiable by ELF type ET_REL. + Elf_Scn* strings_section = elf_helpers::find_ksymtab_strings_section(elf_handle); + size_t strings_offset = 0; + const char* strings_data = nullptr; + size_t strings_size = 0; + if (strings_section) + { + GElf_Shdr strings_sheader; + gelf_getshdr(strings_section, &strings_sheader); + strings_offset = header->e_type == ET_REL ? 0 : strings_sheader.sh_addr; + Elf_Data* data = elf_getdata(strings_section, nullptr); + ABG_ASSERT(data->d_off == 0); + strings_data = reinterpret_cast(data->d_buf); + strings_size = data->d_size; + } + const bool is_kernel = elf_helpers::is_linux_kernel(elf_handle); std::unordered_set exported_kernel_symbols; std::unordered_map crc_values; + std::unordered_map namespaces; for (size_t i = 0; i < number_syms; ++i) { @@ -285,6 +317,27 @@ symtab::load_(Elf* elf_handle, ABG_ASSERT(crc_values.emplace(name.substr(6), sym->st_value).second); continue; } + if (strings_section && is_kernel && name.rfind("__kstrtabns_", 0) == 0) + { + // This symbol lives in the __ksymtab_strings section but st_value may + // be a vmlinux load address so we need to subtract the offset before + // looking it up in that section. + const size_t value = sym->st_value; + const size_t offset = value - strings_offset; + // check offset + ABG_ASSERT(offset < strings_size); + // find the terminating NULL + const char* first = strings_data + offset; + const char* last = strings_data + strings_size; + const char* limit = std::find(first, last, 0); + // check NULL found + ABG_ASSERT(limit < last); + // interpret the empty namespace name as no namespace name + if (first < limit) + ABG_ASSERT(namespaces.emplace( + name.substr(12), std::string(first, limit - first)).second); + continue; + } // filter out uninteresting entries and only keep functions/variables for // now. The rest might be interesting in the future though. @@ -374,6 +427,17 @@ symtab::load_(Elf* elf_handle, symbol->set_crc(crc_entry.second); } + // Now add the namespaces + for (const auto& namespace_entry : namespaces) + { + const auto r = name_symbol_map_.find(namespace_entry.first); + if (r == name_symbol_map_.end()) + continue; + + for (const auto& symbol : r->second) + symbol->set_namespace(namespace_entry.second); + } + // sort the symbols for deterministic output std::sort(symbols_.begin(), symbols_.end(), symbol_sort); diff --git a/src/abg-writer.cc b/src/abg-writer.cc index 23598ef0..90db34be 100644 --- a/src/abg-writer.cc +++ b/src/abg-writer.cc @@ -3075,6 +3075,9 @@ write_elf_symbol(const elf_symbol_sptr& sym, << std::hex << std::showbase << sym->get_crc().value() << std::dec << std::noshowbase << "'"; + if (sym->get_namespace().has_value()) + o << " namespace='" << sym->get_namespace().value() << "'"; + o << "/>\n"; return true; diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index be342684..c3112075 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -93,6 +93,9 @@ test-abidiff/test-crc-2.xml \ test-abidiff/test-crc-report-0-1.txt \ test-abidiff/test-crc-report-1-0.txt \ test-abidiff/test-crc-report-1-2.txt \ +test-abidiff/test-namespace-0.xml \ +test-abidiff/test-namespace-1.xml \ +test-abidiff/test-namespace-report.txt \ test-abidiff/test-PR27985-report.txt \ test-abidiff/test-PR27985-v0.c \ test-abidiff/test-PR27985-v0.o \ diff --git a/tests/data/test-abidiff/test-namespace-0.xml b/tests/data/test-abidiff/test-namespace-0.xml new file mode 100644 index 00000000..5a9f5cd2 --- /dev/null +++ b/tests/data/test-abidiff/test-namespace-0.xml @@ -0,0 +1,15 @@ + + + + + + + + + + + + + + + diff --git a/tests/data/test-abidiff/test-namespace-1.xml b/tests/data/test-abidiff/test-namespace-1.xml new file mode 100644 index 00000000..9814844b --- /dev/null +++ b/tests/data/test-abidiff/test-namespace-1.xml @@ -0,0 +1,15 @@ + + + + + + + + + + + + + + + diff --git a/tests/data/test-abidiff/test-namespace-report.txt b/tests/data/test-abidiff/test-namespace-report.txt new file mode 100644 index 00000000..d2c421ed --- /dev/null +++ b/tests/data/test-abidiff/test-namespace-report.txt @@ -0,0 +1,17 @@ +Functions changes summary: 0 Removed, 0 Changed, 0 Added function +Variables changes summary: 0 Removed, 4 Changed, 0 Added variables + +4 Changed variables: + + [C] 'int v1' was changed: + namespace changed from (none) to 'this' + + [C] 'int v2' was changed: + namespace changed from 'this' to 'that' + + [C] 'int v3' was changed: + namespace changed from 'that' to '' + + [C] 'int v4' was changed: + namespace changed from '' to (none) + diff --git a/tests/test-abidiff.cc b/tests/test-abidiff.cc index a02bbe3d..93e44d6a 100644 --- a/tests/test-abidiff.cc +++ b/tests/test-abidiff.cc @@ -128,6 +128,12 @@ static InOutSpec specs[] = "data/test-abidiff/test-crc-report-1-2.txt", "output/test-abidiff/test-crc-report-1-2.txt" }, + { + "data/test-abidiff/test-namespace-0.xml", + "data/test-abidiff/test-namespace-1.xml", + "data/test-abidiff/test-namespace-report.txt", + "output/test-abidiff/test-namespace-report-0-1.txt" + }, { "data/test-abidiff/test-PR27616-v0.xml", "data/test-abidiff/test-PR27616-v1.xml", -- 2.36.1.476.g0c4daa206d-goog