dlopen: Rework handling of pending NODELETE status

Commit a2e8aa0d9ea648068d8be52dd7b15f1b6a008e23 ("Block signals during
the initial part of dlopen") was deemed necessary because of
read-modify-write operations like the one in  add_dependency in
elf/dl-lookup.c.  In the old code, we check for any kind of NODELETE
status and bail out:

      /* Redo the NODELETE check, as when dl_load_lock wasn't held
	 yet this could have changed.  */
      if (map->l_nodelete != link_map_nodelete_inactive)
	goto out;

And then set pending status (during relocation):

	  if (flags & DL_LOOKUP_FOR_RELOCATE)
	    map->l_nodelete = link_map_nodelete_pending;
	  else
	    map->l_nodelete = link_map_nodelete_active;

If a signal arrives during relocation and the signal handler, through
lazy binding, adds a global scope dependency on the same map, it will
set map->l_nodelete to link_map_nodelete_active.  This will be
overwritten with link_map_nodelete_pending by the dlopen relocation
code.

To avoid such problems in relation to the l_nodelete member, this
commit introduces two flags for active NODELETE status (irrevocable)
and pending NODELETE status (revocable until activate_nodelete is
invoked).  As a result, NODELETE processing in dlopen does not
introduce further reasons why lazy binding from signal handlers
is unsafe during dlopen, and a subsequent commit can remove signal
blocking from dlopen.

This does not address pre-existing issues (unrelated to the NODELETE
changes) which make lazy binding in a signal handler during dlopen
unsafe, such as the use of malloc in both cases.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Reviewed-by: Carlos O'Donell <carlos@redhat.com>

1 files changed, 13 insertions, 9 deletions

diff --git a/elf/dl-open.c b/elf/dl-open.c
index 56f2133..c23341b 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -440,13 +440,17 @@ activate_nodelete (struct link_map *new)
      NODELETE status for objects outside the local scope.  */
   for (struct link_map *l = GL (dl_ns)[new->l_ns]._ns_loaded; l != NULL;
        l = l->l_next)
-    if (l->l_nodelete == link_map_nodelete_pending)
+    if (l->l_nodelete_pending)
       {
 	if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES))
 	  _dl_debug_printf ("activating NODELETE for %s [%lu]\n",
 			    l->l_name, l->l_ns);
 
-	l->l_nodelete = link_map_nodelete_active;
+	l->l_nodelete_active = true;
+
+	/* This is just a debugging aid, to indicate that
+	   activate_nodelete has run for this map.  */
+	l->l_nodelete_pending = false;
       }
 }
 
@@ -549,10 +553,10 @@ dl_open_worker (void *a)
       if (__glibc_unlikely (mode & RTLD_NODELETE))
 	{
 	  if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES)
-	      && new->l_nodelete == link_map_nodelete_inactive)
+	      && !new->l_nodelete_active)
 	    _dl_debug_printf ("marking %s [%lu] as NODELETE\n",
 			      new->l_name, new->l_ns);
-	  new->l_nodelete = link_map_nodelete_active;
+	  new->l_nodelete_active = true;
 	}
 
       /* Finalize the addition to the global scope.  */
@@ -568,7 +572,7 @@ dl_open_worker (void *a)
   /* Schedule NODELETE marking for the directly loaded object if
      requested.  */
   if (__glibc_unlikely (mode & RTLD_NODELETE))
-    new->l_nodelete = link_map_nodelete_pending;
+    new->l_nodelete_pending = true;
 
   /* Load that object's dependencies.  */
   _dl_map_object_deps (new, NULL, 0, 0,
@@ -683,7 +687,7 @@ dl_open_worker (void *a)
 	      _dl_start_profile ();
 
 	      /* Prevent unloading the object.  */
-	      GL(dl_profile_map)->l_nodelete = link_map_nodelete_active;
+	      GL(dl_profile_map)->l_nodelete_active = true;
 	    }
 	}
       else
@@ -882,9 +886,9 @@ no more namespaces available for dlmopen()"));
 	     happens inside dl_open_worker.  */
 	  __libc_signal_restore_set (&args.original_signal_mask);
 
-	  /* All link_map_nodelete_pending objects should have been
-	     deleted at this point, which is why it is not necessary
-	     to reset the flag here.  */
+	  /* All l_nodelete_pending objects should have been deleted
+	     at this point, which is why it is not necessary to reset
+	     the flag here.  */
 	}
       else
 	__libc_signal_restore_set (&args.original_signal_mask);