[PATCH 0/4] libdwfl: make dwfl_addrmodule work for Linux kernel modules

[PATCH 0/4] libdwfl: make dwfl_addrmodule work for Linux kernel modules

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Hello,

I recently encountered a bug that dwfl_addrmodule doesn't work correctly
for Linux kernel modules. This is because each section of a kernel
module is allocated independently, so sections from different kernel
modules may be intermixed. For example:

# cd /sys/modules
# cat ext4/sections/.init.text
0xffffffffc0f0f000
# cat ext4/sections/.bss
0xffffffffc1303e80
# cat kvm/sections/.init.text
0xffffffffc0f06000
# cat kvm/sections/.bss
0xffffffffc10d2340

This confuses dwfl_addrmodule/dwfl_addrsegment, which builds a lookup
table based on mod->low_addr and mod->high_addr. For relocatable files,
we should be using the addresses of each section, instead.

Patch 4 makes this change, but it needs some preparation. Patch 1 allows
us to distinguish between unloaded sections and sections loaded at zero.
This is necessary so that dwfl_addrmodule doesn't map, e.g., 0x123 to a
module with an unloaded section of size 0x200. Because indexing every
section creates many more lookup entries than we previously had, patch 3
separates the module lookup table from the dwfl_report_segment lookup
table. Finally, patch 2 is the patch I sent yesterday, included in this
series because it would conflict with the later patches.

If these patches are the wrong way to go about this, please consider
this a bug report. I'd be happy to test alternative fixes.

Thanks!

Omar Sandoval (4):
  libdwfl: return error from __libdwfl_relocate_value for unloaded
    sections
  libdwfl: remove broken coalescing logic in dwfl_report_segment
  libdwfl: store module lookup table separately from segments
  libdwfl: use sections of relocatable files for dwfl_addrmodule

 .gitignore                             |   1 +
 libdwfl/ChangeLog                      |  30 +++++
 libdwfl/derelocate.c                   |  24 +---
 libdwfl/dwfl_addrmodule.c              | 106 ++++++++++++++-
 libdwfl/dwfl_getmodules.c              |  14 +-
 libdwfl/dwfl_module.c                  |  11 +-
 libdwfl/dwfl_module_getsym.c           |   3 +-
 libdwfl/libdwfl.h                      |  20 +--
 libdwfl/libdwflP.h                     |  42 ++++--
 libdwfl/link_map.c                     |   7 +-
 libdwfl/relocate.c                     |   9 +-
 libdwfl/segment.c                      | 178 +++----------------------
 tests/ChangeLog                        |   5 +
 tests/Makefile.am                      |   6 +-
 tests/dwfl-report-segment-contiguous.c |  82 ++++++++++++
 15 files changed, 305 insertions(+), 233 deletions(-)
 create mode 100644 tests/dwfl-report-segment-contiguous.c

-- 
2.24.0

[PATCH 2/4] libdwfl: remove broken coalescing logic in dwfl_report_segment

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

dwfl_report_segment has some logic that detects when a segment is
contiguous with the previously reported segment, in which case it's
supposed to coalesce them. However, in this case, it actually returns
without updating the segment array at all. As far as I can tell, this
has always been broken. It appears that no one uses the coalescing logic
anyways, as they pass IDENT as NULL. Let's just get rid of the logic and
add a test case.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 .gitignore                             |  1 +
 libdwfl/ChangeLog                      |  4 ++
 libdwfl/libdwfl.h                      | 20 ++-----
 libdwfl/libdwflP.h                     |  7 +--
 libdwfl/segment.c                      | 35 +++++------
 tests/ChangeLog                        |  5 ++
 tests/Makefile.am                      |  6 +-
 tests/dwfl-report-segment-contiguous.c | 82 ++++++++++++++++++++++++++
 8 files changed, 115 insertions(+), 45 deletions(-)
 create mode 100644 tests/dwfl-report-segment-contiguous.c

diff --git a/.gitignore b/.gitignore
index 72f22855..43fb7275 100644
--- a/.gitignore
+++ b/.gitignore
@@ -112,6 +112,7 @@ Makefile.in
 /tests/dwfl-bug-report
 /tests/dwfl-proc-attach
 /tests/dwfl-report-elf-align
+/tests/dwfl-report-segment-contiguous
 /tests/dwfllines
 /tests/dwflmodtest
 /tests/dwflsyms
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index b00ac8d6..09a4a473 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,10 +1,14 @@
 2019-12-11  Omar Sandoval  <osandov@fb.com>
 
 	* libdwflP.h: Add new NOT_LOADED DWFL_ERROR.
+	(Dwfl_Module): Remove coalescing state.
+	Rename lookup_tail_ndx to next_segndx.
 	* relocate.c (__libdwfl_relocate_value): Return
 	DWFL_E_NOT_LOADED if section is not loaded.
 	(relocate): Handle DWFL_E_NOT_LOADED.
 	* dwfl_module_getsym: Ignore DWFL_E_NOT_LOADED.
+	* segment.c (dwfl_report_segment): Remove coalescing logic.
+	* libdwfl.h (dwfl_report_segment): Document that IDENT is now ignored.
 
 2019-12-05  Mark Wielaard  <mark@klomp.org>
 
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index a0c1d357..d5fa06d4 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -111,7 +111,7 @@ extern void dwfl_report_begin (Dwfl *dwfl);
 
 /* Report that segment NDX begins at PHDR->p_vaddr + BIAS.
    If NDX is < 0, the value succeeding the last call's NDX
-   is used instead (zero on the first call).
+   is used instead (zero on the first call).  IDENT is ignored.
 
    If nonzero, the smallest PHDR->p_align value seen sets the
    effective page size for the address space DWFL describes.
@@ -120,21 +120,9 @@ extern void dwfl_report_begin (Dwfl *dwfl);
 
    Returns -1 for errors, or NDX (or its assigned replacement) on success.
 
-   When NDX is the value succeeding the last call's NDX (or is implicitly
-   so as above), IDENT is nonnull and matches the value in the last call,
-   and the PHDR and BIAS values reflect a segment that would be contiguous,
-   in both memory and file, with the last segment reported, then this
-   segment may be coalesced internally with preceding segments.  When given
-   an address inside this segment, dwfl_addrsegment may return the NDX of a
-   preceding contiguous segment.  To prevent coalesced segments, always
-   pass a null pointer for IDENT.
-
-   The values passed are not stored (except to track coalescence).
-   The only information that can be extracted from DWFL later is the
-   mapping of an address to a segment index that starts at or below
-   it.  Reporting segments at all is optional.  Its only benefit to
-   the caller is to offer this quick lookup via dwfl_addrsegment,
-   or use other segment-based calls.  */
+   Reporting segments at all is optional.  Its only benefit to the caller is to
+   offer this quick lookup via dwfl_addrsegment, or use other segment-based
+   calls.  */
 extern int dwfl_report_segment (Dwfl *dwfl, int ndx,
 				const GElf_Phdr *phdr, GElf_Addr bias,
 				const void *ident);
diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
index 6c10eddc..d21471b1 100644
--- a/libdwfl/libdwflP.h
+++ b/libdwfl/libdwflP.h
@@ -133,12 +133,7 @@ struct Dwfl
   GElf_Addr *lookup_addr;	/* Start address of segment.  */
   Dwfl_Module **lookup_module;	/* Module associated with segment, or null.  */
   int *lookup_segndx;		/* User segment index, or -1.  */
-
-  /* Cache from last dwfl_report_segment call.  */
-  const void *lookup_tail_ident;
-  GElf_Off lookup_tail_vaddr;
-  GElf_Off lookup_tail_offset;
-  int lookup_tail_ndx;
+  int next_segndx;
 
   struct Dwfl_User_Core *user_core;
 };
diff --git a/libdwfl/segment.c b/libdwfl/segment.c
index d9599a7f..f6a3e84e 100644
--- a/libdwfl/segment.c
+++ b/libdwfl/segment.c
@@ -287,11 +287,15 @@ int
 dwfl_report_segment (Dwfl *dwfl, int ndx, const GElf_Phdr *phdr, GElf_Addr bias,
 		     const void *ident)
 {
+  /* This was previously used for coalescing segments, but it was buggy since
+     day one.  We don't use it anymore.  */
+  (void)ident;
+
   if (dwfl == NULL)
     return -1;
 
   if (ndx < 0)
-    ndx = dwfl->lookup_tail_ndx;
+    ndx = dwfl->next_segndx;
 
   if (phdr->p_align > 1 && (dwfl->segment_align <= 1 ||
 			    phdr->p_align < dwfl->segment_align))
@@ -307,30 +311,19 @@ dwfl_report_segment (Dwfl *dwfl, int ndx, const GElf_Phdr *phdr, GElf_Addr bias,
   GElf_Addr end = __libdwfl_segment_end (dwfl,
 					 bias + phdr->p_vaddr + phdr->p_memsz);
 
-  /* Coalesce into the last one if contiguous and matching.  */
-  if (ndx != dwfl->lookup_tail_ndx
-      || ident == NULL
-      || ident != dwfl->lookup_tail_ident
-      || start != dwfl->lookup_tail_vaddr
-      || phdr->p_offset != dwfl->lookup_tail_offset)
-    {
-      /* Normally just appending keeps us sorted.  */
+  /* Normally just appending keeps us sorted.  */
 
-      size_t i = dwfl->lookup_elts;
-      while (i > 0 && unlikely (start < dwfl->lookup_addr[i - 1]))
-	--i;
+  size_t i = dwfl->lookup_elts;
+  while (i > 0 && unlikely (start < dwfl->lookup_addr[i - 1]))
+    --i;
 
-      if (unlikely (insert (dwfl, i, start, end, ndx)))
-	{
-	  __libdwfl_seterrno (DWFL_E_NOMEM);
-	  return -1;
-	}
+  if (unlikely (insert (dwfl, i, start, end, ndx)))
+    {
+      __libdwfl_seterrno (DWFL_E_NOMEM);
+      return -1;
     }
 
-  dwfl->lookup_tail_ident = ident;
-  dwfl->lookup_tail_vaddr = end;
-  dwfl->lookup_tail_offset = end - bias - phdr->p_vaddr + phdr->p_offset;
-  dwfl->lookup_tail_ndx = ndx + 1;
+  dwfl->next_segndx = ndx + 1;
 
   return ndx;
 }
diff --git a/tests/ChangeLog b/tests/ChangeLog
index ce67ce55..30bd8256 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@
+2019-12-11  Omar Sandoval  <osandov@fb.com>
+
+	* dwfl-report-segment-coalesce.c: New test.
+	* Makefile.am: Add dwfl-report-segment-coalesce
+
 2019-12-06  Mark Wielaard  <mark@klomp.org>
 
 	* run-debuginfod-find.sh: Force -Wl,--build-id.
diff --git a/tests/Makefile.am b/tests/Makefile.am
index eab4ae6f..2c3ac299 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -50,7 +50,8 @@ check_PROGRAMS = arextract arsymtest newfile saridx scnnames sectiondump \
 		  test-flag-nobits dwarf-getstring rerequest_tag \
 		  alldts typeiter typeiter2 low_high_pc \
 		  test-elf_cntl_gelf_getshdr dwflsyms dwfllines \
-		  dwfl-report-elf-align varlocs backtrace backtrace-child \
+		  dwfl-report-elf-align dwfl-report-segment-contiguous \
+		  varlocs backtrace backtrace-child \
 		  backtrace-data backtrace-dwarf debuglink debugaltlink \
 		  buildid deleted deleted-lib.so aggregate_size peel_type \
 		  vdsosyms \
@@ -113,7 +114,7 @@ TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh newfile test-nlist \
 	run-native-test.sh run-bug1-test.sh \
 	run-debuglink.sh run-debugaltlink.sh run-buildid.sh \
 	dwfl-bug-addr-overflow run-addrname-test.sh \
-	dwfl-bug-fd-leak dwfl-bug-report \
+	dwfl-bug-fd-leak dwfl-bug-report dwfl-report-segment-contiguous \
 	run-dwfl-bug-offline-rel.sh run-dwfl-addr-sect.sh \
 	run-disasm-x86.sh run-disasm-x86-64.sh \
 	run-early-offscn.sh run-dwarf-getmacros.sh run-dwarf-ranges.sh \
@@ -589,6 +590,7 @@ test_elf_cntl_gelf_getshdr_LDADD = $(libelf)
 dwflsyms_LDADD = $(libdw) $(libelf) $(argp_LDADD)
 dwfllines_LDADD = $(libdw) $(libelf) $(argp_LDADD)
 dwfl_report_elf_align_LDADD = $(libdw)
+dwfl_report_segment_contiguous_LDADD = $(libdw) $(libebl) $(libelf)
 varlocs_LDADD = $(libdw) $(libelf) $(argp_LDADD)
 backtrace_LDADD = $(libdw) $(libelf) $(argp_LDADD)
 # backtrace-child-biarch also uses those *_CFLAGS and *_LDLAGS variables:
diff --git a/tests/dwfl-report-segment-contiguous.c b/tests/dwfl-report-segment-contiguous.c
new file mode 100644
index 00000000..61e6bfee
--- /dev/null
+++ b/tests/dwfl-report-segment-contiguous.c
@@ -0,0 +1,82 @@
+/* Test bug in dwfl_report_segment() coalescing.
+   Copyright (C) 2019 Facebook
+   This file is part of elfutils.
+
+   This file is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   elfutils is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <config.h>
+#include <assert.h>
+#include <inttypes.h>
+#include <stdio.h>
+#include <stdio_ext.h>
+#include <locale.h>
+#include ELFUTILS_HEADER(dwfl)
+
+
+static const Dwfl_Callbacks offline_callbacks =
+  {
+    .find_debuginfo = INTUSE(dwfl_standard_find_debuginfo),
+    .section_address = INTUSE(dwfl_offline_section_address),
+  };
+
+
+int
+main (void)
+{
+  /* We use no threads here which can interfere with handling a stream.  */
+  (void) __fsetlocking (stdout, FSETLOCKING_BYCALLER);
+
+  /* Set locale.  */
+  (void) setlocale (LC_ALL, "");
+
+  Dwfl *dwfl = dwfl_begin (&offline_callbacks);
+  assert (dwfl != NULL);
+
+  GElf_Phdr phdr1 =
+    {
+      .p_type = PT_LOAD,
+      .p_flags = PF_R,
+      .p_offset = 0xf00,
+      .p_vaddr = 0xf00,
+      .p_filesz = 0x100,
+      .p_memsz = 0x100,
+      .p_align = 4,
+    };
+
+  int ndx = dwfl_report_segment (dwfl, 1, &phdr1, 0, dwfl);
+  assert(ndx == 1);
+
+  ndx = dwfl_addrsegment (dwfl, 0xf00, NULL);
+  assert(ndx == 1);
+
+  GElf_Phdr phdr2 =
+    {
+      .p_type = PT_LOAD,
+      .p_flags = PF_R | PF_W,
+      .p_offset = 0x1000,
+      .p_vaddr = 0x1000,
+      .p_filesz = 0x100,
+      .p_memsz = 0x100,
+      .p_align = 4,
+    };
+  ndx = dwfl_report_segment (dwfl, 2, &phdr2, 0, dwfl);
+  assert(ndx == 2);
+
+  ndx = dwfl_addrsegment (dwfl, 0x1000, NULL);
+  assert(ndx == 1 || ndx == 2);
+
+  dwfl_end (dwfl);
+
+  return 0;
+}
-- 
2.24.0

[PATCH 4/4] libdwfl: use sections of relocatable files for dwfl_addrmodule

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

dwfl_addrmodule matches a module if the address lies within low_addr and
high_addr. This is incorrect for relocatable files, particularly kernel
modules: sections of different modules can be intermingled within the
same range of addresses. Instead, we should index each section
independently.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 libdwfl/ChangeLog         |  6 ++++++
 libdwfl/derelocate.c      | 24 ++++++------------------
 libdwfl/dwfl_addrmodule.c | 25 +++++++++++++++++++++----
 libdwfl/libdwflP.h        | 17 +++++++++++++++++
 4 files changed, 50 insertions(+), 22 deletions(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 8dc20961..d55aeb47 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -4,6 +4,8 @@
 	(Dwfl_Module): Remove coalescing state.
 	Rename lookup_tail_ndx to next_segndx.
 	Rename segment member to lookup.
+	(struct dwfl_relocation): Move from derelocate.c
+	(__libdwfl_cache_sections): Add.
 	(Dwfl): Replace module lookup table with new definition.
 	* relocate.c (__libdwfl_relocate_value): Return
 	DWFL_E_NOT_LOADED if section is not loaded.
@@ -14,6 +16,7 @@
 	(dwfl_addrsegment): Use dwfl_addrmodule to get module.
 	* libdwfl.h (dwfl_report_segment): Document that IDENT is now ignored.
 	* dwfl_addrmodule.c: Add new module lookup table implementation.
+	Index sections for relocatable files instead of full address range.
 	(dwfl_addrmodule): Use new module lookup table instead of
 	dwfl_addrsegment.
 	* dwfl_getmodules.c (dwfl_getmodules): Use new module lookup table.
@@ -21,6 +24,9 @@
 	(use): Likewise.
 	* link_map.c: Use dwfl_addrmodule instead of dwfl_addrsegment when
 	segment index is not needed.
+	* derelocate.c (struct dwfl_relocation): Move to libdwflP.h.
+	(cache_sections): Rename to __libdwfl_cache_sections and make
+	non-static.
 
 2019-12-05  Mark Wielaard  <mark@klomp.org>
 
diff --git a/libdwfl/derelocate.c b/libdwfl/derelocate.c
index 2f80b20f..238aa6e2 100644
--- a/libdwfl/derelocate.c
+++ b/libdwfl/derelocate.c
@@ -32,19 +32,6 @@
 
 #include "libdwflP.h"
 
-struct dwfl_relocation
-{
-  size_t count;
-  struct
-  {
-    Elf_Scn *scn;
-    Elf_Scn *relocs;
-    const char *name;
-    GElf_Addr start, end;
-  } refs[0];
-};
-
-
 struct secref
 {
   struct secref *next;
@@ -76,8 +63,9 @@ compare_secrefs (const void *a, const void *b)
   return elf_ndxscn ((*p1)->scn) - elf_ndxscn ((*p2)->scn);
 }
 
-static int
-cache_sections (Dwfl_Module *mod)
+int
+internal_function
+__libdwfl_cache_sections (Dwfl_Module *mod)
 {
   if (likely (mod->reloc_info != NULL))
     return mod->reloc_info->count;
@@ -242,7 +230,7 @@ dwfl_module_relocations (Dwfl_Module *mod)
   switch (mod->e_type)
     {
     case ET_REL:
-      return cache_sections (mod);
+      return __libdwfl_cache_sections (mod);
 
     case ET_DYN:
       return 1;
@@ -278,7 +266,7 @@ dwfl_module_relocation_info (Dwfl_Module *mod, unsigned int idx,
       return NULL;
     }
 
-  if (cache_sections (mod) < 0)
+  if (__libdwfl_cache_sections (mod) < 0)
     return NULL;
 
   struct dwfl_relocation *sections = mod->reloc_info;
@@ -330,7 +318,7 @@ check_module (Dwfl_Module *mod)
 static int
 find_section (Dwfl_Module *mod, Dwarf_Addr *addr)
 {
-  if (cache_sections (mod) < 0)
+  if (__libdwfl_cache_sections (mod) < 0)
     return -1;
 
   struct dwfl_relocation *sections = mod->reloc_info;
diff --git a/libdwfl/dwfl_addrmodule.c b/libdwfl/dwfl_addrmodule.c
index 21db4883..1eb45317 100644
--- a/libdwfl/dwfl_addrmodule.c
+++ b/libdwfl/dwfl_addrmodule.c
@@ -74,10 +74,27 @@ create_lookup_module (Dwfl *dwfl)
   for (Dwfl_Module *mod = dwfl->modulelist; mod != NULL; mod = mod->next)
     if (! mod->gc)
       {
-	GElf_Addr start = __libdwfl_segment_start(dwfl, mod->low_addr);
-	GElf_Addr end = __libdwfl_segment_end(dwfl, mod->high_addr);
-	if (append_lookup_module(dwfl, mod, start, end))
-	  return true;
+	Dwarf_Addr bias;
+	if (mod->dwfl->callbacks->find_elf
+	    && dwfl_module_getdwarf(mod, &bias)
+	    && mod->e_type == ET_REL)
+	  {
+	    if (__libdwfl_cache_sections (mod) < 0)
+	      return true;
+
+	    struct dwfl_relocation *sections = mod->reloc_info;
+	    for (size_t i = 0; i < sections->count; i++)
+	      if (append_lookup_module(dwfl, mod, sections->refs[i].start,
+				       sections->refs[i].end))
+		return true;
+	  }
+	else
+	  {
+	    GElf_Addr start = __libdwfl_segment_start(dwfl, mod->low_addr);
+	    GElf_Addr end = __libdwfl_segment_end(dwfl, mod->high_addr);
+	    if (append_lookup_module(dwfl, mod, start, end))
+	      return true;
+	  }
       }
 
   qsort (dwfl->lookup_module, dwfl->lookup_module_elts,
diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
index 6661cc4c..2589eb66 100644
--- a/libdwfl/libdwflP.h
+++ b/libdwfl/libdwflP.h
@@ -169,6 +169,18 @@ struct dwfl_file
   GElf_Addr address_sync;
 };
 
+struct dwfl_relocation
+{
+  size_t count;
+  struct
+  {
+    Elf_Scn *scn;
+    Elf_Scn *relocs;
+    const char *name;
+    GElf_Addr start, end;
+  } refs[0];
+};
+
 struct Dwfl_Module
 {
   Dwfl *dwfl;
@@ -486,6 +498,11 @@ extern void __libdwfl_getelf (Dwfl_Module *mod) internal_function;
 extern Dwfl_Error __libdwfl_relocate (Dwfl_Module *mod, Elf *file, bool debug)
   internal_function;
 
+/* Cache SHF_ALLOC sections in MOD->reloc_info.  Returns the number of sections
+   on success or -1 on error.  */
+extern int __libdwfl_cache_sections (Dwfl_Module *mod)
+  internal_function;
+
 /* Find the section index in mod->main.elf that contains the given
    *ADDR.  Adjusts *ADDR to be section relative on success, returns
    SHN_UNDEF on failure.  */
-- 
2.24.0

[PATCH 1/4] libdwfl: return error from __libdwfl_relocate_value for unloaded sections

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Currently, __libdwfl_relocate_value doesn't distinguish between unloaded
sections and sections loaded at address zero. This has a few
consequences:

* relocate.c attempts relocation on unloaded sections when we don't have
  anything meaningful to relocate against.
* derelocate.c matches addresses which happen to be less than the
  sh_size of an unloaded section, which can lead to confusing results
  from dwfl_module_relocate_address and __libdwfl_find_section_ndx.
* find_elf_build_id returns an invalid non-zero address if the build ID
  note is not loaded.

Let's return a new error, DWFL_E_NOT_LOADED, from
__libdwfl_relocate_value if the section is not loaded that callers can
handle appropriately.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 libdwfl/ChangeLog            | 8 ++++++++
 libdwfl/dwfl_module_getsym.c | 3 ++-
 libdwfl/libdwflP.h           | 3 ++-
 libdwfl/relocate.c           | 9 ++++++---
 4 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index b6b427d4..b00ac8d6 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,11 @@
+2019-12-11  Omar Sandoval  <osandov@fb.com>
+
+	* libdwflP.h: Add new NOT_LOADED DWFL_ERROR.
+	* relocate.c (__libdwfl_relocate_value): Return
+	DWFL_E_NOT_LOADED if section is not loaded.
+	(relocate): Handle DWFL_E_NOT_LOADED.
+	* dwfl_module_getsym: Ignore DWFL_E_NOT_LOADED.
+
 2019-12-05  Mark Wielaard  <mark@klomp.org>
 
 	* linux-kernel-modules.c (find_kernel_elf): Also try to find
diff --git a/libdwfl/dwfl_module_getsym.c b/libdwfl/dwfl_module_getsym.c
index 8de9a3eb..d75588b2 100644
--- a/libdwfl/dwfl_module_getsym.c
+++ b/libdwfl/dwfl_module_getsym.c
@@ -165,7 +165,8 @@ __libdwfl_getsym (Dwfl_Module *mod, int ndx, GElf_Sym *sym, GElf_Addr *addr,
 	  Dwfl_Error result = __libdwfl_relocate_value (mod, elf,
 							&symshstrndx,
 							shndx, &st_value);
-	  if (unlikely (result != DWFL_E_NOERROR))
+	  if (unlikely (result != DWFL_E_NOERROR
+			&& result != DWFL_E_NOT_LOADED))
 	    {
 	      __libdwfl_seterrno (result);
 	      return NULL;
diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
index f631f946..6c10eddc 100644
--- a/libdwfl/libdwflP.h
+++ b/libdwfl/libdwflP.h
@@ -90,7 +90,8 @@ typedef struct Dwfl_Process Dwfl_Process;
   DWFL_ERROR (NO_ATTACH_STATE, N_("Dwfl has no attached state"))	      \
   DWFL_ERROR (NO_UNWIND, N_("Unwinding not supported for this architecture")) \
   DWFL_ERROR (INVALID_ARGUMENT, N_("Invalid argument"))			      \
-  DWFL_ERROR (NO_CORE_FILE, N_("Not an ET_CORE ELF file"))
+  DWFL_ERROR (NO_CORE_FILE, N_("Not an ET_CORE ELF file"))		      \
+  DWFL_ERROR (NOT_LOADED, N_("Not loaded"))
 
 #define DWFL_ERROR(name, text) DWFL_E_##name,
 typedef enum { DWFL_ERRORS DWFL_E_NUM } Dwfl_Error;
diff --git a/libdwfl/relocate.c b/libdwfl/relocate.c
index 88b5211d..5c9c08f3 100644
--- a/libdwfl/relocate.c
+++ b/libdwfl/relocate.c
@@ -73,9 +73,8 @@ __libdwfl_relocate_value (Dwfl_Module *mod, Elf *elf, size_t *shstrndx,
 	return CBFAIL;
 
       if (refshdr->sh_addr == (Dwarf_Addr) -1l)
-	/* The callback indicated this section wasn't really loaded but we
-	   don't really care.  */
-	refshdr->sh_addr = 0;	/* Make no adjustment below.  */
+	/* The callback indicated this section wasn't loaded.  */
+	return DWFL_E_NOT_LOADED;
 
       /* Update the in-core file's section header to show the final
 	 load address (or unloadedness).  This serves as a cache,
@@ -361,6 +360,8 @@ relocate (Dwfl_Module * const mod,
 	GElf_Word shndx;
 	Dwfl_Error error = relocate_getsym (mod, relocated, reloc_symtab,
 					    symndx, &sym, &shndx);
+	if (error == DWFL_E_NOT_LOADED)
+	  return DWFL_E_NOERROR;
 	if (unlikely (error != DWFL_E_NOERROR))
 	  return error;
 
@@ -368,6 +369,8 @@ relocate (Dwfl_Module * const mod,
 	  {
 	    /* Maybe we can figure it out anyway.  */
 	    error = resolve_symbol (mod, reloc_symtab, &sym, shndx);
+	    if (error == DWFL_E_NOT_LOADED)
+	      return DWFL_E_NOERROR;
 	    if (error != DWFL_E_NOERROR
 		&& !(error == DWFL_E_RELUNDEF && shndx == SHN_COMMON))
 	      return error;
-- 
2.24.0

[PATCH 3/4] libdwfl: store module lookup table separately from segments

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Currently, the address ranges for segments reported with
dwfl_report_segment and modules are stored in the same sorted array.
This requires complicated logic in reify_segments for splitting up
existing segments and inserting into the table, which can have quadratic
runtime in the worst case. Simplify it by adding a separate table for
the module list, which we can manage much more simply. This will be
especially important for the next change.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 libdwfl/ChangeLog         |  14 +++-
 libdwfl/dwfl_addrmodule.c |  89 +++++++++++++++++++++++-
 libdwfl/dwfl_getmodules.c |  14 ++--
 libdwfl/dwfl_module.c     |  11 +--
 libdwfl/libdwflP.h        |  17 ++++-
 libdwfl/link_map.c        |   7 +-
 libdwfl/segment.c         | 143 +-------------------------------------
 7 files changed, 128 insertions(+), 167 deletions(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 09a4a473..8dc20961 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -3,12 +3,24 @@
 	* libdwflP.h: Add new NOT_LOADED DWFL_ERROR.
 	(Dwfl_Module): Remove coalescing state.
 	Rename lookup_tail_ndx to next_segndx.
+	Rename segment member to lookup.
+	(Dwfl): Replace module lookup table with new definition.
 	* relocate.c (__libdwfl_relocate_value): Return
 	DWFL_E_NOT_LOADED if section is not loaded.
 	(relocate): Handle DWFL_E_NOT_LOADED.
 	* dwfl_module_getsym: Ignore DWFL_E_NOT_LOADED.
-	* segment.c (dwfl_report_segment): Remove coalescing logic.
+	* segment.c: Remove old module lookup table implementation.
+	(dwfl_report_segment): Remove coalescing logic.
+	(dwfl_addrsegment): Use dwfl_addrmodule to get module.
 	* libdwfl.h (dwfl_report_segment): Document that IDENT is now ignored.
+	* dwfl_addrmodule.c: Add new module lookup table implementation.
+	(dwfl_addrmodule): Use new module lookup table instead of
+	dwfl_addrsegment.
+	* dwfl_getmodules.c (dwfl_getmodules): Use new module lookup table.
+	* dwfl_module.c (dwfl_report_begin): Clear new module lookup table.
+	(use): Likewise.
+	* link_map.c: Use dwfl_addrmodule instead of dwfl_addrsegment when
+	segment index is not needed.
 
 2019-12-05  Mark Wielaard  <mark@klomp.org>
 
diff --git a/libdwfl/dwfl_addrmodule.c b/libdwfl/dwfl_addrmodule.c
index abf1ff48..21db4883 100644
--- a/libdwfl/dwfl_addrmodule.c
+++ b/libdwfl/dwfl_addrmodule.c
@@ -32,11 +32,94 @@
 
 #include "libdwflP.h"
 
+static bool append_lookup_module (Dwfl *dwfl, Dwfl_Module *mod, GElf_Addr start,
+				  GElf_Addr end)
+{
+  if (dwfl->lookup_module_elts >= dwfl->lookup_module_alloc)
+    {
+      size_t n = dwfl->lookup_module_alloc;
+      n = n == 0 ? 16 : n * 2;
+      struct dwfl_module_segment *tmp;
+      tmp = realloc (dwfl->lookup_module, n * sizeof (tmp[0]));
+      if (tmp == NULL)
+	{
+	  __libdwfl_seterrno (DWFL_E_NOMEM);
+	  return true;
+	}
+      dwfl->lookup_module = tmp;
+      dwfl->lookup_module_alloc = n;
+    }
+  size_t i = dwfl->lookup_module_elts++;
+  dwfl->lookup_module[i].start = start;
+  dwfl->lookup_module[i].end = end;
+  dwfl->lookup_module[i].mod = mod;
+  return false;
+}
+
+static int compare_dwfl_module_segment (const void *a, const void *b)
+{
+  const struct dwfl_module_segment *seg1 = a;
+  const struct dwfl_module_segment *seg2 = b;
+  if (seg1->start < seg2->start)
+    return -1;
+  else if (seg1->start > seg2->start)
+    return 1;
+  else
+    return 0;
+}
+
+static bool
+create_lookup_module (Dwfl *dwfl)
+{
+  for (Dwfl_Module *mod = dwfl->modulelist; mod != NULL; mod = mod->next)
+    if (! mod->gc)
+      {
+	GElf_Addr start = __libdwfl_segment_start(dwfl, mod->low_addr);
+	GElf_Addr end = __libdwfl_segment_end(dwfl, mod->high_addr);
+	if (append_lookup_module(dwfl, mod, start, end))
+	  return true;
+      }
+
+  qsort (dwfl->lookup_module, dwfl->lookup_module_elts,
+	 sizeof (dwfl->lookup_module[0]), compare_dwfl_module_segment);
+  for (size_t i = 0; i < dwfl->lookup_module_elts; i++)
+    {
+      dwfl->lookup_module[i].mod->lookup = i;
+      /* If the upper boundary of the segment isn't part of the next segment,
+	 treat it as part of the segment.  */
+      if (i == dwfl->lookup_module_elts - 1
+	  || dwfl->lookup_module[i].end < dwfl->lookup_module[i + 1].start)
+	dwfl->lookup_module[i].end++;
+    }
+  return false;
+}
+
+static int search_dwfl_module_segment (const void *key, const void *elt)
+{
+  Dwarf_Addr address = *(Dwarf_Addr *)key;
+  const struct dwfl_module_segment *seg = elt;
+  if (address < seg->start)
+    return -1;
+  else if (address >= seg->end)
+    return 1;
+  else
+    return 0;
+}
+
 Dwfl_Module *
 dwfl_addrmodule (Dwfl *dwfl, Dwarf_Addr address)
 {
-  Dwfl_Module *mod;
-  (void) INTUSE(dwfl_addrsegment) (dwfl, address, &mod);
-  return mod;
+  if (unlikely (dwfl == NULL)
+      || (unlikely (dwfl->lookup_module_elts == 0)
+	  && unlikely (create_lookup_module (dwfl))))
+    return NULL;
+
+  struct dwfl_module_segment *seg = bsearch (&address, dwfl->lookup_module,
+					     dwfl->lookup_module_elts,
+					     sizeof (dwfl->lookup_module[0]),
+					     search_dwfl_module_segment);
+  if (seg == NULL)
+    return NULL;
+  return seg->mod;
 }
 INTDEF (dwfl_addrmodule)
diff --git a/libdwfl/dwfl_getmodules.c b/libdwfl/dwfl_getmodules.c
index 243cb04d..807fe855 100644
--- a/libdwfl/dwfl_getmodules.c
+++ b/libdwfl/dwfl_getmodules.c
@@ -61,17 +61,17 @@ dwfl_getmodules (Dwfl *dwfl,
 	else
 	  m = m->next;
     }
-  else if (((offset & 3) == 2) && likely (dwfl->lookup_module != NULL))
+  else if (((offset & 3) == 2) && likely (dwfl->lookup_module_elts > 0))
     {
       offset >>= 2;
 
-      if ((size_t) offset - 1 == dwfl->lookup_elts)
+      if ((size_t) offset - 1 == dwfl->lookup_module_elts)
 	return 0;
 
-      if (unlikely ((size_t) offset - 1 > dwfl->lookup_elts))
+      if (unlikely ((size_t) offset - 1 > dwfl->lookup_module_elts))
 	return -1;
 
-      m = dwfl->lookup_module[offset - 1];
+      m = dwfl->lookup_module[offset - 1].mod;
       if (unlikely (m == NULL))
 	return -1;
     }
@@ -87,9 +87,9 @@ dwfl_getmodules (Dwfl *dwfl,
       ++offset;
       m = m->next;
       if (ok != DWARF_CB_OK)
-	return ((dwfl->lookup_module == NULL) ? ((offset << 2) | 1)
-		: (((m == NULL ? (ptrdiff_t) dwfl->lookup_elts + 1
-		     : m->segment + 1) << 2) | 2));
+	return ((dwfl->lookup_module_elts == 0) ? ((offset << 2) | 1)
+		: (((m == NULL ? (ptrdiff_t) dwfl->lookup_module_elts + 1
+		     : m->lookup + 1) << 2) | 2));
     }
   return 0;
 }
diff --git a/libdwfl/dwfl_module.c b/libdwfl/dwfl_module.c
index e7dfdace..4aa9aa2c 100644
--- a/libdwfl/dwfl_module.c
+++ b/libdwfl/dwfl_module.c
@@ -135,8 +135,9 @@ INTDEF (dwfl_report_begin_add)
 void
 dwfl_report_begin (Dwfl *dwfl)
 {
-  /* Clear the segment lookup table.  */
+  /* Clear the segment and module lookup tables.  */
   dwfl->lookup_elts = 0;
+  dwfl->lookup_module_elts = 0;
 
   for (Dwfl_Module *m = dwfl->modulelist; m != NULL; m = m->next)
     m->gc = true;
@@ -150,13 +151,7 @@ use (Dwfl_Module *mod, Dwfl_Module **tailp, Dwfl *dwfl)
 {
   mod->next = *tailp;
   *tailp = mod;
-
-  if (unlikely (dwfl->lookup_module != NULL))
-    {
-      free (dwfl->lookup_module);
-      dwfl->lookup_module = NULL;
-    }
-
+  dwfl->lookup_module_elts = 0;
   return mod;
 }
 
diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
index d21471b1..6661cc4c 100644
--- a/libdwfl/libdwflP.h
+++ b/libdwfl/libdwflP.h
@@ -113,6 +113,13 @@ struct Dwfl_User_Core
   int fd;                       /* close if >= 0.  */
 };
 
+struct dwfl_module_segment
+{
+  GElf_Addr start;
+  GElf_Addr end;
+  Dwfl_Module *mod;
+};
+
 struct Dwfl
 {
   const Dwfl_Callbacks *callbacks;
@@ -127,14 +134,18 @@ struct Dwfl
 
   GElf_Addr segment_align;	/* Smallest granularity of segments.  */
 
-  /* Binary search table in three parallel malloc'd arrays.  */
+  /* Binary search table of segments in two parallel malloc'd arrays.  */
   size_t lookup_elts;		/* Elements in use.  */
   size_t lookup_alloc;		/* Elements allococated.  */
   GElf_Addr *lookup_addr;	/* Start address of segment.  */
-  Dwfl_Module **lookup_module;	/* Module associated with segment, or null.  */
   int *lookup_segndx;		/* User segment index, or -1.  */
   int next_segndx;
 
+  /* Binary search table of module address ranges.  */
+  struct dwfl_module_segment *lookup_module;
+  size_t lookup_module_elts;
+  size_t lookup_module_alloc;
+
   struct Dwfl_User_Core *user_core;
 };
 
@@ -216,7 +227,7 @@ struct Dwfl_Module
   Dwarf_CFI *dwarf_cfi;		/* Cached DWARF CFI for this module.  */
   Dwarf_CFI *eh_cfi;		/* Cached EH CFI for this module.  */
 
-  int segment;			/* Index of first segment table entry.  */
+  int lookup;			/* Index in module lookup table.  */
   bool gc;			/* Mark/sweep flag.  */
   bool is_executable;		/* Use Dwfl::executable_for_core?  */
 };
diff --git a/libdwfl/link_map.c b/libdwfl/link_map.c
index 29307c74..befb39e4 100644
--- a/libdwfl/link_map.c
+++ b/libdwfl/link_map.c
@@ -175,8 +175,7 @@ integrated_memory_callback (Dwfl *dwfl, int ndx,
 
   /* Now look for module text covering this address.  */
 
-  Dwfl_Module *mod;
-  (void) INTUSE(dwfl_addrsegment) (dwfl, vaddr, &mod);
+  Dwfl_Module *mod = INTUSE(dwfl_addrmodule) (dwfl, vaddr);
   if (mod == NULL)
     return false;
 
@@ -588,9 +587,7 @@ consider_executable (Dwfl_Module *mod, GElf_Addr at_phdr, GElf_Addr at_entry,
 		  mod->high_addr -= mod_bias;
 		  mod->low_addr += bias;
 		  mod->high_addr += bias;
-
-		  free (mod->dwfl->lookup_module);
-		  mod->dwfl->lookup_module = NULL;
+		  mod->dwfl->lookup_module_elts = 0;
 		}
 	    }
 	}
diff --git a/libdwfl/segment.c b/libdwfl/segment.c
index f6a3e84e..9bf8b282 100644
--- a/libdwfl/segment.c
+++ b/libdwfl/segment.c
@@ -76,19 +76,6 @@ insert (Dwfl *dwfl, size_t i, GElf_Addr start, GElf_Addr end, int segndx)
       dwfl->lookup_alloc = n;
       dwfl->lookup_addr = naddr;
       dwfl->lookup_segndx = nsegndx;
-
-      if (dwfl->lookup_module != NULL)
-	{
-	  /* Make sure this array is big enough too.  */
-	  Dwfl_Module **old = dwfl->lookup_module;
-	  dwfl->lookup_module = realloc (dwfl->lookup_module,
-					 sizeof dwfl->lookup_module[0] * n);
-	  if (unlikely (dwfl->lookup_module == NULL))
-	    {
-	      free (old);
-	      return true;
-	    }
-	}
     }
 
   if (unlikely (i < dwfl->lookup_elts))
@@ -98,17 +85,12 @@ insert (Dwfl *dwfl, size_t i, GElf_Addr start, GElf_Addr end, int segndx)
 	       move * sizeof dwfl->lookup_addr[0]);
       memmove (&dwfl->lookup_segndx[i + need], &dwfl->lookup_segndx[i],
 	       move * sizeof dwfl->lookup_segndx[0]);
-      if (dwfl->lookup_module != NULL)
-	memmove (&dwfl->lookup_module[i + need], &dwfl->lookup_module[i],
-		 move * sizeof dwfl->lookup_module[0]);
     }
 
   if (need_start)
     {
       dwfl->lookup_addr[i] = start;
       dwfl->lookup_segndx[i] = segndx;
-      if (dwfl->lookup_module != NULL)
-	dwfl->lookup_module[i] = NULL;
       ++i;
     }
   else
@@ -118,8 +100,6 @@ insert (Dwfl *dwfl, size_t i, GElf_Addr start, GElf_Addr end, int segndx)
     {
       dwfl->lookup_addr[i] = end;
       dwfl->lookup_segndx[i] = -1;
-      if (dwfl->lookup_module != NULL)
-	dwfl->lookup_module[i] = NULL;
     }
 
   dwfl->lookup_elts += need;
@@ -154,131 +134,20 @@ lookup (Dwfl *dwfl, GElf_Addr address, int hint)
   return -1;
 }
 
-static bool
-reify_segments (Dwfl *dwfl)
-{
-  int hint = -1;
-  int highest = -1;
-  bool fixup = false;
-  for (Dwfl_Module *mod = dwfl->modulelist; mod != NULL; mod = mod->next)
-    if (! mod->gc)
-      {
-	const GElf_Addr start = __libdwfl_segment_start (dwfl, mod->low_addr);
-	const GElf_Addr end = __libdwfl_segment_end (dwfl, mod->high_addr);
-	bool resized = false;
-
-	int idx = lookup (dwfl, start, hint);
-	if (unlikely (idx < 0))
-	  {
-	    /* Module starts below any segment.  Insert a low one.  */
-	    if (unlikely (insert (dwfl, 0, start, end, -1)))
-	      return true;
-	    idx = 0;
-	    resized = true;
-	  }
-	else if (dwfl->lookup_addr[idx] > start)
-	  {
-	    /* The module starts in the middle of this segment.  Split it.  */
-	    if (unlikely (insert (dwfl, idx + 1, start, end,
-				  dwfl->lookup_segndx[idx])))
-	      return true;
-	    ++idx;
-	    resized = true;
-	  }
-	else if (dwfl->lookup_addr[idx] < start)
-	  {
-	    /* The module starts past the end of this segment.
-	       Add a new one.  */
-	    if (unlikely (insert (dwfl, idx + 1, start, end, -1)))
-	      return true;
-	    ++idx;
-	    resized = true;
-	  }
-
-	if ((size_t) idx + 1 < dwfl->lookup_elts
-	    && end < dwfl->lookup_addr[idx + 1])
-	  {
-	    /* The module ends in the middle of this segment.  Split it.  */
-	    if (unlikely (insert (dwfl, idx + 1,
-				  end, dwfl->lookup_addr[idx + 1], -1)))
-	      return true;
-	    resized = true;
-	  }
-
-	if (dwfl->lookup_module == NULL)
-	  {
-	    dwfl->lookup_module = calloc (dwfl->lookup_alloc,
-					  sizeof dwfl->lookup_module[0]);
-	    if (unlikely (dwfl->lookup_module == NULL))
-	      return true;
-	  }
-
-	/* Cache a backpointer in the module.  */
-	mod->segment = idx;
-
-	/* Put MOD in the table for each segment that's inside it.  */
-	do
-	  dwfl->lookup_module[idx++] = mod;
-	while ((size_t) idx < dwfl->lookup_elts
-	       && dwfl->lookup_addr[idx] < end);
-	assert (dwfl->lookup_module[mod->segment] == mod);
-
-	if (resized && idx - 1 >= highest)
-	  /* Expanding the lookup tables invalidated backpointers
-	     we've already stored.  Reset those ones.  */
-	  fixup = true;
-
-	highest = idx - 1;
-	hint = (size_t) idx < dwfl->lookup_elts ? idx : -1;
-      }
-
-  if (fixup)
-    /* Reset backpointer indices invalidated by table insertions.  */
-    for (size_t idx = 0; idx < dwfl->lookup_elts; ++idx)
-      if (dwfl->lookup_module[idx] != NULL)
-	dwfl->lookup_module[idx]->segment = idx;
-
-  return false;
-}
-
 int
 dwfl_addrsegment (Dwfl *dwfl, Dwarf_Addr address, Dwfl_Module **mod)
 {
   if (unlikely (dwfl == NULL))
     return -1;
 
-  if (unlikely (dwfl->lookup_module == NULL)
-      && mod != NULL
-      && unlikely (reify_segments (dwfl)))
-    {
-      __libdwfl_seterrno (DWFL_E_NOMEM);
-      return -1;
-    }
-
   int idx = lookup (dwfl, address, -1);
-  if (likely (mod != NULL))
-    {
-      if (unlikely (idx < 0) || unlikely (dwfl->lookup_module == NULL))
-	*mod = NULL;
-      else
-	{
-	  *mod = dwfl->lookup_module[idx];
-
-	  /* If this segment does not have a module, but the address is
-	     the upper boundary of the previous segment's module, use that.  */
-	  if (*mod == NULL && idx > 0 && dwfl->lookup_addr[idx] == address)
-	    {
-	      *mod = dwfl->lookup_module[idx - 1];
-	      if (*mod != NULL && (*mod)->high_addr != address)
-		*mod = NULL;
-	    }
-	}
-    }
-
   if (likely (idx >= 0))
     /* Translate internal segment table index to user segment index.  */
     idx = dwfl->lookup_segndx[idx];
 
+  if (mod != NULL)
+    *mod = INTUSE(dwfl_addrmodule) (dwfl, address);
+
   return idx;
 }
 INTDEF (dwfl_addrsegment)
@@ -301,12 +170,6 @@ dwfl_report_segment (Dwfl *dwfl, int ndx, const GElf_Phdr *phdr, GElf_Addr bias,
 			    phdr->p_align < dwfl->segment_align))
     dwfl->segment_align = phdr->p_align;
 
-  if (unlikely (dwfl->lookup_module != NULL))
-    {
-      free (dwfl->lookup_module);
-      dwfl->lookup_module = NULL;
-    }
-
   GElf_Addr start = __libdwfl_segment_start (dwfl, bias + phdr->p_vaddr);
   GElf_Addr end = __libdwfl_segment_end (dwfl,
 					 bias + phdr->p_vaddr + phdr->p_memsz);
-- 
2.24.0

Re: [PATCH 0/4] libdwfl: make dwfl_addrmodule work for Linux kernel modules

[Omar Sandoval]

On Wed, Dec 11, 2019 at 05:29:42PM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Hello,
> 
> I recently encountered a bug that dwfl_addrmodule doesn't work correctly
> for Linux kernel modules. This is because each section of a kernel
> module is allocated independently, so sections from different kernel
> modules may be intermixed. For example:
> 
> # cd /sys/modules
> # cat ext4/sections/.init.text
> 0xffffffffc0f0f000
> # cat ext4/sections/.bss
> 0xffffffffc1303e80
> # cat kvm/sections/.init.text
> 0xffffffffc0f06000
> # cat kvm/sections/.bss
> 0xffffffffc10d2340
> 
> This confuses dwfl_addrmodule/dwfl_addrsegment, which builds a lookup
> table based on mod->low_addr and mod->high_addr.

I did some more testing, and I realized that my analysis was wrong :(
What's going on here is that:

1. The kernel frees the .init sections once the module is initialized,
   which means the addresses can be reused for new modules.
2. My application is reporting low_addr and high_addr based on the
   section addresses (which is different from how
   dwfl_linux_kernel_report_modules does it).

Reading the kernel code, the main sections are indeed contiguous. So
this was entirely my bug. Sorry for the noise!

On the bright side, patch 2 ("libdwfl: remove broken coalescing logic in
dwfl_report_segment") does seem like a legitimate bug.

Re: [PATCH 0/4] libdwfl: make dwfl_addrmodule work for Linux kernel modules

[Mark Wielaard]

On Thu, 2019-12-12 at 21:03 -0800, Omar Sandoval wrote:
> Reading the kernel code, the main sections are indeed contiguous. So
> this was entirely my bug. Sorry for the noise!
> 
> On the bright side, patch 2 ("libdwfl: remove broken coalescing logic in
> dwfl_report_segment") does seem like a legitimate bug.

It does indeed. How curious this code never worked as advertised.
I agree that it is probably best to just document that IDENT should
be NULL and is ignored. It comes with a testcase and removes various
extra fields from Dwfl_Module. Applied to master.

Thanks,

Mark