diff mbox series

[v3,18/32] elf: Move link map fields used by dependency sorting to writable part

Message ID 3d5c8f150d9112ce00b183638d93ae52694b3b58.1701944612.git.fweimer@redhat.com
State New
Headers show
Series RELRO linkmaps | expand

Commit Message

Florian Weimer Dec. 7, 2023, 10:32 a.m. UTC 
Currently, ld.so re-runs dependency sorting during process shutdown
in _dl_fini, instead of simply using the reverse initialization order.
This means that the l_idx and l_visited fields are written to.  There
is no way to report errors during shutdown.  If these fields are
always writable, this avoids the need to make link maps writable
during _dl_fini, avoiding the error reporting issue.

This commit can be reverted once we stop re-sorting dependencies
in _dl_fini.
---
 elf/dl-close.c     | 33 +++++++++++++++++----------------
 elf/dl-fini.c      |  2 +-
 elf/dl-sort-maps.c | 14 +++++++-------
 include/link.h     | 12 +++++++-----
 4 files changed, 32 insertions(+), 29 deletions(-)

Comments

Joseph Myers Feb. 27, 2024, 5:51 p.m. UTC | #1 
On Thu, 7 Dec 2023, Florian Weimer wrote:

> Currently, ld.so re-runs dependency sorting during process shutdown
> in _dl_fini, instead of simply using the reverse initialization order.
> This means that the l_idx and l_visited fields are written to.  There
> is no way to report errors during shutdown.  If these fields are
> always writable, this avoids the need to make link maps writable
> during _dl_fini, avoiding the error reporting issue.
> 
> This commit can be reverted once we stop re-sorting dependencies
> in _dl_fini.

OK.
diff mbox series

Patch

diff --git a/elf/dl-close.c b/elf/dl-close.c
index f242dcee9e..8f9d57df39 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -147,7 +147,7 @@  _dl_close_worker (struct link_map_private *map, bool force)
     {
       l->l_map_used = 0;
       l->l_map_done = 0;
-      l->l_idx = idx;
+      l->l_rw->l_idx = idx;
       maps[idx] = l;
       ++idx;
     }
@@ -157,10 +157,10 @@  _dl_close_worker (struct link_map_private *map, bool force)
      The map variable is NULL after a retry.  */
   if (map != NULL)
     {
-      maps[map->l_idx] = maps[0];
-      maps[map->l_idx]->l_idx = map->l_idx;
+      maps[map->l_rw->l_idx] = maps[0];
+      maps[map->l_rw->l_idx]->l_rw->l_idx = map->l_rw->l_idx;
       maps[0] = map;
-      maps[0]->l_idx = 0;
+      maps[0]->l_rw->l_idx = 0;
     }
 
   /* Keep track of the lowest index link map we have covered already.  */
@@ -187,7 +187,7 @@  _dl_close_worker (struct link_map_private *map, bool force)
       l->l_map_used = 1;
       l->l_map_done = 1;
       /* Signal the object is still needed.  */
-      l->l_idx = IDX_STILL_USED;
+      l->l_rw->l_idx = IDX_STILL_USED;
 
       /* Mark all dependencies as used.  */
       if (l->l_initfini != NULL)
@@ -197,9 +197,10 @@  _dl_close_worker (struct link_map_private *map, bool force)
 	  struct link_map_private **lp = &l->l_initfini[1];
 	  while (*lp != NULL)
 	    {
-	      if ((*lp)->l_idx != IDX_STILL_USED)
+	      if ((*lp)->l_rw->l_idx != IDX_STILL_USED)
 		{
-		  assert ((*lp)->l_idx >= 0 && (*lp)->l_idx < nloaded);
+		  assert ((*lp)->l_rw->l_idx >= 0
+			  && (*lp)->l_rw->l_idx < nloaded);
 
 		  if (!(*lp)->l_map_used)
 		    {
@@ -208,8 +209,8 @@  _dl_close_worker (struct link_map_private *map, bool force)
 			 already processed it, then we need to go back
 			 and process again from that point forward to
 			 ensure we keep all of its dependencies also.  */
-		      if ((*lp)->l_idx - 1 < done_index)
-			done_index = (*lp)->l_idx - 1;
+		      if ((*lp)->l_rw->l_idx - 1 < done_index)
+			done_index = (*lp)->l_rw->l_idx - 1;
 		    }
 		}
 
@@ -222,15 +223,15 @@  _dl_close_worker (struct link_map_private *map, bool force)
 	  {
 	    struct link_map_private *jmap = l->l_rw->l_reldeps->list[j];
 
-	    if (jmap->l_idx != IDX_STILL_USED)
+	    if (jmap->l_rw->l_idx != IDX_STILL_USED)
 	      {
-		assert (jmap->l_idx >= 0 && jmap->l_idx < nloaded);
+		assert (jmap->l_rw->l_idx >= 0 && jmap->l_rw->l_idx < nloaded);
 
 		if (!jmap->l_map_used)
 		  {
 		    jmap->l_map_used = 1;
-		    if (jmap->l_idx - 1 < done_index)
-		      done_index = jmap->l_idx - 1;
+		    if (jmap->l_rw->l_idx - 1 < done_index)
+		      done_index = jmap->l_rw->l_idx - 1;
 		  }
 	      }
 	  }
@@ -321,7 +322,7 @@  _dl_close_worker (struct link_map_private *map, bool force)
 		  ((char *) imap->l_scope[cnt]
 		   - offsetof (struct link_map_private, l_searchlist));
 		assert (tmap->l_ns == nsid);
-		if (tmap->l_idx == IDX_STILL_USED)
+		if (tmap->l_rw->l_idx == IDX_STILL_USED)
 		  ++remain;
 		else
 		  removed_any = true;
@@ -368,7 +369,7 @@  _dl_close_worker (struct link_map_private *map, bool force)
 			   ((char *) imap->l_scope[cnt]
 			    - offsetof (struct link_map_private,
 					l_searchlist)));
-		      if (tmap->l_idx != IDX_STILL_USED)
+		      if (tmap->l_rw->l_idx != IDX_STILL_USED)
 			{
 			  /* Remove the scope.  Or replace with own map's
 			     scope.  */
@@ -413,7 +414,7 @@  _dl_close_worker (struct link_map_private *map, bool force)
 	  /* The loader is gone, so mark the object as not having one.
 	     Note: l_idx != IDX_STILL_USED -> object will be removed.  */
 	  if (imap->l_loader != NULL
-	      && imap->l_loader->l_idx != IDX_STILL_USED)
+	      && imap->l_loader->l_rw->l_idx != IDX_STILL_USED)
 	    imap->l_loader = NULL;
 
 	  /* Remember where the first dynamically loaded object is.  */
diff --git a/elf/dl-fini.c b/elf/dl-fini.c
index 5c78159fee..2abd63cb08 100644
--- a/elf/dl-fini.c
+++ b/elf/dl-fini.c
@@ -77,7 +77,7 @@  _dl_fini (void)
 		assert (i < nloaded);
 
 		maps[i] = l;
-		l->l_idx = i;
+		l->l_rw->l_idx = i;
 		++i;
 
 		/* Bump l_direct_opencount of all objects so that they
diff --git a/elf/dl-sort-maps.c b/elf/dl-sort-maps.c
index e3a547e4da..ae8e7bb528 100644
--- a/elf/dl-sort-maps.c
+++ b/elf/dl-sort-maps.c
@@ -51,7 +51,7 @@  _dl_sort_maps_original (struct link_map_private **maps, unsigned int nmaps,
 	{
 	  /* Do not handle ld.so in secondary namespaces and objects which
 	     are not removed.  */
-	  if (thisp != thisp->l_real || thisp->l_idx == -1)
+	  if (thisp != thisp->l_real || thisp->l_rw->l_idx == -1)
 	    goto skip;
 	}
 
@@ -138,17 +138,17 @@  dfs_traversal (struct link_map_private ***rpo, struct link_map_private *map,
 {
   /* _dl_map_object_deps ignores l_faked objects when calculating the
      number of maps before calling _dl_sort_maps, ignore them as well.  */
-  if (map->l_visited || map->l_faked)
+  if (map->l_rw->l_visited || map->l_faked)
     return;
 
-  map->l_visited = 1;
+  map->l_rw->l_visited = 1;
 
   if (map->l_initfini)
     {
       for (int i = 0; map->l_initfini[i] != NULL; i++)
 	{
 	  struct link_map_private *dep = map->l_initfini[i];
-	  if (dep->l_visited == 0
+	  if (dep->l_rw->l_visited == 0
 	      && dep->l_main_map == 0)
 	    dfs_traversal (rpo, dep, do_reldeps);
 	}
@@ -163,7 +163,7 @@  dfs_traversal (struct link_map_private ***rpo, struct link_map_private *map,
       for (int m = map->l_rw->l_reldeps->act - 1; m >= 0; m--)
 	{
 	  struct link_map_private *dep = map->l_rw->l_reldeps->list[m];
-	  if (dep->l_visited == 0
+	  if (dep->l_rw->l_visited == 0
 	      && dep->l_main_map == 0)
 	    dfs_traversal (rpo, dep, do_reldeps);
 	}
@@ -182,7 +182,7 @@  _dl_sort_maps_dfs (struct link_map_private **maps, unsigned int nmaps,
 {
   struct link_map_private *first_map = maps[0];
   for (int i = nmaps - 1; i >= 0; i--)
-    maps[i]->l_visited = 0;
+    maps[i]->l_rw->l_visited = 0;
 
   /* We apply DFS traversal for each of maps[i] until the whole total order
      is found and we're at the start of the Reverse-Postorder (RPO) sequence,
@@ -245,7 +245,7 @@  _dl_sort_maps_dfs (struct link_map_private **maps, unsigned int nmaps,
   if (do_reldeps)
     {
       for (int i = nmaps - 1; i >= 0; i--)
-	rpo[i]->l_visited = 0;
+	rpo[i]->l_rw->l_visited = 0;
 
       struct link_map_private **maps_head = &maps[nmaps];
       for (int i = nmaps - 1; i >= 0; i--)
diff --git a/include/link.h b/include/link.h
index c752bc2cb7..2632337e29 100644
--- a/include/link.h
+++ b/include/link.h
@@ -132,6 +132,13 @@  struct link_map_rw
      ignored.  */
   bool l_nodelete_active;
   bool l_nodelete_pending;
+
+  /* Used for dependency sorting in dlclose/_dl_fini.  These need to
+     be writable all the time because there is no way to report an
+     error in _dl_fini.  These flags can be moved into struct
+     link_map_private once _dl_fini no longer re-sorts link maps.  */
+  bool l_visited;
+  int l_idx;
 };
 
 /* Structure describing a loaded shared object.  The `l_next' and `l_prev'
@@ -231,8 +238,6 @@  struct link_map_private
     unsigned int l_global:1;	/* Nonzero if object in _dl_global_scope.  */
     unsigned int l_reserved:2;	/* Reserved for internal use.  */
     unsigned int l_main_map:1;  /* Nonzero for the map of the main program.  */
-    unsigned int l_visited:1;   /* Used internally for map dependency
-				   graph traversal.  */
     unsigned int l_map_used:1;  /* These two bits are used during traversal */
     unsigned int l_map_done:1;  /* of maps in _dl_close_worker. */
     unsigned int l_phdr_allocated:1; /* Nonzero if the data structure pointed
@@ -319,9 +324,6 @@  struct link_map_private
     ElfW(Word) l_flags_1;
     ElfW(Word) l_flags;
 
-    /* Temporarily used in `dl_close'.  */
-    int l_idx;
-
     struct link_map_machine l_mach;
 
     struct