add Linux kernel symbol namespace support

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<string> 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 <maennich@google.com>
Signed-off-by: Giuliano Procida <gprocida@google.com>

diff --git a/include/abg-ir.h b/include/abg-ir.h
index 5b7649f4d3850fcffc8c3b9236269d9868cb789a..86a6ce271822d54ae709aee2742a429e85b482dd 100644 (file)
--- a/include/abg-ir.h
+++ b/include/abg-ir.h
@@ -928,6 +928,7 @@ private:
             visibility         vi,
             bool               is_in_ksymtab = false,
             const abg_compat::optional<uint64_t>&      crc = {},
+            const abg_compat::optional<std::string>&   ns = {},
             bool               is_suppressed = false);
 
   elf_symbol(const elf_symbol&);
@@ -953,6 +954,7 @@ public:
         visibility         vi,
         bool               is_in_ksymtab = false,
         const abg_compat::optional<uint64_t>&          crc = {},
+        const abg_compat::optional<std::string>&       ns = {},
         bool               is_suppressed = false);
 
   const environment*
@@ -1030,6 +1032,12 @@ public:
   void
   set_crc(const abg_compat::optional<uint64_t>& crc);
 
+  const abg_compat::optional<std::string>&
+  get_namespace() const;
+
+  void
+  set_namespace(const abg_compat::optional<std::string>& ns);
+
   bool
   is_suppressed() const;
 

diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc
index 3e2d32086743a07e9d4e8d5265c7e8c1e4095d58..273f496d3b2d2ea2f1b4fce52fade57245f0c5a5 100644 (file)
--- 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 <typename function_or_var_decl_sptr>
+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<const function_decl_diff*>(diff))
+    return namespace_changed(d->first_function_decl(),
+                            d->second_function_decl());
+  if (const var_diff* d = dynamic_cast<const var_diff*>(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 9a89808eb5067b7ef9b84aa6079e4dba76b2503d..0ba595a8405e86046aca563b54d4c7daadfbabc1 100644 (file)
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -1725,6 +1725,7 @@ struct elf_symbol::priv
   bool                 is_common_;
   bool                 is_in_ksymtab_;
   abg_compat::optional<uint64_t>       crc_;
+  abg_compat::optional<std::string>    namespace_;
   bool                 is_suppressed_;
   elf_symbol_wptr      main_symbol_;
   elf_symbol_wptr      next_alias_;
@@ -1742,6 +1743,7 @@ struct elf_symbol::priv
       is_common_(false),
       is_in_ksymtab_(false),
       crc_(),
+      namespace_(),
       is_suppressed_(false)
   {}
 
@@ -1757,6 +1759,7 @@ struct elf_symbol::priv
        elf_symbol::visibility    vi,
        bool                      is_in_ksymtab,
        const abg_compat::optional<uint64_t>&   crc,
+       const abg_compat::optional<std::string>&        ns,
        bool                      is_suppressed)
     : env_(e),
       index_(i),
@@ -1770,6 +1773,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_)
@@ -1815,6 +1819,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,
@@ -1827,6 +1833,7 @@ elf_symbol::elf_symbol(const environment* e,
                       visibility         vi,
                       bool               is_in_ksymtab,
                       const abg_compat::optional<uint64_t>&    crc,
+                      const abg_compat::optional<std::string>& ns,
                       bool               is_suppressed)
   : priv_(new priv(e,
                   i,
@@ -1840,6 +1847,7 @@ elf_symbol::elf_symbol(const environment* e,
                   vi,
                   is_in_ksymtab,
                   crc,
+                  ns,
                   is_suppressed))
 {}
 
@@ -1883,6 +1891,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
@@ -1898,10 +1908,11 @@ elf_symbol::create(const environment* e,
                   visibility         vi,
                   bool               is_in_ksymtab,
                   const abg_compat::optional<uint64_t>&        crc,
+                  const abg_compat::optional<std::string>&     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;
 }
@@ -1923,7 +1934,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.
@@ -2143,6 +2155,20 @@ void
 elf_symbol::set_crc(const abg_compat::optional<uint64_t>& crc)
 {priv_->crc_ = crc;}
 
+/// Getter of the 'namespace' property.
+///
+/// @return the namespace for Linux Kernel symbols, if any
+const abg_compat::optional<std::string>&
+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<std::string>& 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 53b8398da7a835304845a27d0160f624762d60b1..dd7814e397c4159bb84dcc4e5e3281632dc94e61 100644 (file)
--- 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 5b53fdaab5080c57b66b6304c1d452fe5b278046..2ecf80160822c2d21f97a8067b9b82b3a8810576 100644 (file)
--- 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<std::string>& ns1 = symbol1->get_namespace();
+  const abg_compat::optional<std::string>& 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 441fb7e4455bcd6ba786874e5b595d266b13f012..606c96a2aba92d6b1f2192024ce5cab27f83eaf6 100644 (file)
--- 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<const char *>(data->d_buf);
+      strings_size = data->d_size;
+    }
+
   const bool is_kernel = elf_helpers::is_linux_kernel(elf_handle);
   std::unordered_set<std::string> exported_kernel_symbols;
   std::unordered_map<std::string, uint64_t> crc_values;
+  std::unordered_map<std::string, std::string> 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 9bfea5d679ea89e6d72f9e196d369ef7729b5812..04f26edfbe9e7ea0a486c4fb695735b9c98e5e50 100644 (file)
--- a/src/abg-writer.cc
+++ b/src/abg-writer.cc
@@ -3198,6 +3198,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 ffd6bf26a06fcd47194894464258b36bff9062f1..3a7f6f67082c38f98140a46a9a2fb268f35b0fda 100644 (file)
--- 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 (file)
index 0000000..5a9f5cd
--- /dev/null
+++ b/tests/data/test-abidiff/test-namespace-0.xml
@@ -0,0 +1,15 @@
+<abi-corpus path='test.o' architecture='elf-amd-x86_64'>
+  <elf-variable-symbols>
+    <elf-symbol name='v1' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
+    <elf-symbol name='v2' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes' namespace='this'/>
+    <elf-symbol name='v3' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes' namespace='that'/>
+    <elf-symbol name='v4' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes' namespace=''/>
+  </elf-variable-symbols>
+  <abi-instr version='1.0' address-size='64' path='test.c' comp-dir-path='/tmp' language='LANG_C89'>
+    <type-decl name='int' size-in-bits='32' id='type-id-1'/>
+    <var-decl name='v1' type-id='type-id-1' mangled-name='v1' visibility='default' filepath='test.c' line='1' column='1' elf-symbol-id='v1'/>
+    <var-decl name='v2' type-id='type-id-1' mangled-name='v2' visibility='default' filepath='test.c' line='2' column='1' elf-symbol-id='v2'/>
+    <var-decl name='v3' type-id='type-id-1' mangled-name='v3' visibility='default' filepath='test.c' line='3' column='1' elf-symbol-id='v3'/>
+    <var-decl name='v4' type-id='type-id-1' mangled-name='v4' visibility='default' filepath='test.c' line='4' column='1' elf-symbol-id='v4'/>
+  </abi-instr>
+</abi-corpus>

diff --git a/tests/data/test-abidiff/test-namespace-1.xml b/tests/data/test-abidiff/test-namespace-1.xml
new file mode 100644 (file)
index 0000000..9814844
--- /dev/null
+++ b/tests/data/test-abidiff/test-namespace-1.xml
@@ -0,0 +1,15 @@
+<abi-corpus path='test.o' architecture='elf-amd-x86_64'>
+  <elf-variable-symbols>
+    <elf-symbol name='v1' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes' namespace='this'/>
+    <elf-symbol name='v2' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes' namespace='that'/>
+    <elf-symbol name='v3' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes' namespace=''/>
+    <elf-symbol name='v4' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
+  </elf-variable-symbols>
+  <abi-instr version='1.0' address-size='64' path='test.c' comp-dir-path='/tmp' language='LANG_C89'>
+    <type-decl name='int' size-in-bits='32' id='type-id-1'/>
+    <var-decl name='v1' type-id='type-id-1' mangled-name='v1' visibility='default' filepath='test.c' line='1' column='1' elf-symbol-id='v1'/>
+    <var-decl name='v2' type-id='type-id-1' mangled-name='v2' visibility='default' filepath='test.c' line='2' column='1' elf-symbol-id='v2'/>
+    <var-decl name='v3' type-id='type-id-1' mangled-name='v3' visibility='default' filepath='test.c' line='3' column='1' elf-symbol-id='v3'/>
+    <var-decl name='v4' type-id='type-id-1' mangled-name='v4' visibility='default' filepath='test.c' line='4' column='1' elf-symbol-id='v4'/>
+  </abi-instr>
+</abi-corpus>

diff --git a/tests/data/test-abidiff/test-namespace-report.txt b/tests/data/test-abidiff/test-namespace-report.txt
new file mode 100644 (file)
index 0000000..d2c421e
--- /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 63153b1dab4628336450d6c6230433ff1b031e71..951b6d251d3d33c8c52a09930f3724da9b00e3e0 100644 (file)
--- 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",