Add RTLD_RELOAD to dlopen

  Hello,

In our parallel programming projects, we would like to load some DSO
several times within the same process, because we want to share the
addresse space for passing data pointers between parallel executions,
and the DSO has global variables and such which we want to see
duplicated.

Unfortunately, dlopen() does not re-load the DSO when it is already
loaded. One workaround is to cp the file under another name, but that's
ugly and does not share the memory pages.

The patch proposed here simply adds an RTLD_RELOAD flag which disables
checking for the DSO being already loaded, thus always loading the DSO
again. There is no actual code modification, only the addition of two
if()s and reindent.

Samuel

2017-07-19  Samuel Thibault  <samuel.thibault@ens-lyon.org>

	* bits/dlfcn.h (RTLD_RELOAD): New macro
	* sysdeps/mips/bits/dlfcn.h (RTLD_RELOAD): New macro
	* dlfcn/dlopen.c (dlopen_doit): Let args->mode contain RTLD_RELOAD.
	* elf/dl-load.c (_dl_map_object_from_fd): Do not match the file id
	when mode contains RTLD_RELOAD.
	(_dl_map_object): Do not match the file name when mode contains
	RTLD_RELOAD.
	* elf/tst-reload.c: New file.
  

On 07/20/2017 03:15 PM, Samuel Thibault wrote:
> Hello,
> 
> In our parallel programming projects, we would like to load some DSO
> several times within the same process, because we want to share the
> addresse space for passing data pointers between parallel executions,
> and the DSO has global variables and such which we want to see
> duplicated.
> 
> Unfortunately, dlopen() does not re-load the DSO when it is already
> loaded. One workaround is to cp the file under another name, but that's
> ugly and does not share the memory pages.
> 
> The patch proposed here simply adds an RTLD_RELOAD flag which disables
> checking for the DSO being already loaded, thus always loading the DSO
> again. There is no actual code modification, only the addition of two
> if()s and reindent.
 
This is what Solaris designed dlmopen() for, is there any reason you
can't use dlmopen()?

I have a branch with fixes for dlmopen() which finish implementing the
missing pieces that we need e.g. carlos/dlmopen.

I strongly suggest dlmopen() as the way to solve this problem, particularly
since dlmopen() offers much stronger isolation, and ensures that the globals
get the correct binding and don't escape the namespace and accidentally
resolve to the first definition by the first opened library.
  

Carlos O'Donell, on jeu. 20 juil. 2017 15:31:38 -0400, wrote:
> On 07/20/2017 03:15 PM, Samuel Thibault wrote:
> > In our parallel programming projects, we would like to load some DSO
> > several times within the same process, because we want to share the
> > addresse space for passing data pointers between parallel executions,
> > and the DSO has global variables and such which we want to see
> > duplicated.
> > 
> > Unfortunately, dlopen() does not re-load the DSO when it is already
> > loaded. One workaround is to cp the file under another name, but that's
> > ugly and does not share the memory pages.
> > 
> > The patch proposed here simply adds an RTLD_RELOAD flag which disables
> > checking for the DSO being already loaded, thus always loading the DSO
> > again. There is no actual code modification, only the addition of two
> > if()s and reindent.
>  
> This is what Solaris designed dlmopen() for, is there any reason you
> can't use dlmopen()?

Because only that DSO should be reloaded. All the dependencies are fine
to use as such, to avoid memory usage duplication.

Samuel
  

On 07/20/2017 03:34 PM, Samuel Thibault wrote:
> Carlos O'Donell, on jeu. 20 juil. 2017 15:31:38 -0400, wrote:
>> On 07/20/2017 03:15 PM, Samuel Thibault wrote:
>>> In our parallel programming projects, we would like to load some DSO
>>> several times within the same process, because we want to share the
>>> addresse space for passing data pointers between parallel executions,
>>> and the DSO has global variables and such which we want to see
>>> duplicated.
>>>
>>> Unfortunately, dlopen() does not re-load the DSO when it is already
>>> loaded. One workaround is to cp the file under another name, but that's
>>> ugly and does not share the memory pages.
>>>
>>> The patch proposed here simply adds an RTLD_RELOAD flag which disables
>>> checking for the DSO being already loaded, thus always loading the DSO
>>> again. There is no actual code modification, only the addition of two
>>> if()s and reindent.
>>  
>> This is what Solaris designed dlmopen() for, is there any reason you
>> can't use dlmopen()?
> 
> Because only that DSO should be reloaded. All the dependencies are fine
> to use as such, to avoid memory usage duplication.

Is that really a problem? Have you measured this?

The kernel will share every page except data/bss.

Using dlmopen will protect you from all kinds of issues with dependent
libraries only supporting a single global state since you'll get unique
state for each of the loaded libraries.

I'm deeply worried about users trying to use this generically for any
DSO, and finding that initializing a DSO twice results in, for example,
overwriting global state in a dependent child library, which means the
dependent child should also have been loaded in parallel.

I see RTLD_RELOAD as an shortcut to simply having multiple copies of the
shared library on disk. I believe you could use hardlinks and glibc should
load each as a unique library. So all you need is a parallel_dlopen()
function that does the hardlink and then issues the dlopen.

We would be well served by documenting the rules under which glibc will
reload or not reload similar files on disk e.g. SONAME? hardlink? symlink?
  

Carlos O'Donell, on jeu. 20 juil. 2017 16:03:46 -0400, wrote:
> On 07/20/2017 03:34 PM, Samuel Thibault wrote:
> > Carlos O'Donell, on jeu. 20 juil. 2017 15:31:38 -0400, wrote:
> >> On 07/20/2017 03:15 PM, Samuel Thibault wrote:
> >>> In our parallel programming projects, we would like to load some DSO
> >>> several times within the same process, because we want to share the
> >>> addresse space for passing data pointers between parallel executions,
> >>> and the DSO has global variables and such which we want to see
> >>> duplicated.
> >>>
> >>> Unfortunately, dlopen() does not re-load the DSO when it is already
> >>> loaded. One workaround is to cp the file under another name, but that's
> >>> ugly and does not share the memory pages.
> >>>
> >>> The patch proposed here simply adds an RTLD_RELOAD flag which disables
> >>> checking for the DSO being already loaded, thus always loading the DSO
> >>> again. There is no actual code modification, only the addition of two
> >>> if()s and reindent.
> >>  
> >> This is what Solaris designed dlmopen() for, is there any reason you
> >> can't use dlmopen()?
> > 
> > Because only that DSO should be reloaded. All the dependencies are fine
> > to use as such, to avoid memory usage duplication.
> 
> Is that really a problem? Have you measured this?

Yes and yes. We are targetting loadling like thousands of replicates of
the DSO, to emulate a very large parallel execution.

> The kernel will share every page except data/bss.

But that also eats addressing space and VMAs.

> Using dlmopen will protect you from all kinds of issues with dependent
> libraries only supporting a single global state since you'll get unique
> state for each of the loaded libraries.

Sure, but it's really too costly for us, and it loads glibc several
times in the process, which can pose other problems.

> I see RTLD_RELOAD as an shortcut to simply having multiple copies of the
> shared library on disk. I believe you could use hardlinks and glibc should
> load each as a unique library.

Hardlinks would have the same st_ino, so libc would detect it is already
loaded.

Samuel
  

Samuel Thibault, on jeu. 20 juil. 2017 15:19:34 -0500, wrote:
> Carlos O'Donell, on jeu. 20 juil. 2017 16:03:46 -0400, wrote:
> > > Because only that DSO should be reloaded. All the dependencies are fine
> > > to use as such, to avoid memory usage duplication.
> > 
> > Is that really a problem? Have you measured this?
> 
> Yes and yes. We are targetting loadling like thousands of replicates of
> the DSO, to emulate a very large parallel execution.

And actually we do want to share some DSOs for coherent semantic.

Samuel
  

Carlos O'Donell, on jeu. 20 juil. 2017 16:03:46 -0400, wrote:
> I'm deeply worried about users trying to use this generically for any
> DSO, and finding that initializing a DSO twice results in, for example,
> overwriting global state in a dependent child library, which means the
> dependent child should also have been loaded in parallel.

Agreed, that potential issue can be highlighted along the flag in the
manpage.

Samuel
  

* Samuel Thibault:

> In our parallel programming projects, we would like to load some DSO
> several times within the same process, because we want to share the
> addresse space for passing data pointers between parallel executions,
> and the DSO has global variables and such which we want to see
> duplicated.

I think this needs a discussion of the impact on symbol binding
behavior.  The behavior when dlopen is later called for the same
soname without RTLD_RELOAD needs clarification, too.  And the intended
behavior needs to be demonstrated with tests.
  

Hello,

So, is it OK to add this? Considering that dlmopen() brings us far from
enough factorization for our needs, and dlopen() currently never reloads
unless a real fat copy of the file is done (hardlinks/symlinks don't
work, as it is based on st_ino).

Samuel

Samuel Thibault, on jeu. 20 juil. 2017 14:15:17 -0500, wrote:
> In our parallel programming projects, we would like to load some DSO
> several times within the same process, because we want to share the
> addresse space for passing data pointers between parallel executions,
> and the DSO has global variables and such which we want to see
> duplicated.
> 
> Unfortunately, dlopen() does not re-load the DSO when it is already
> loaded. One workaround is to cp the file under another name, but that's
> ugly and does not share the memory pages.
> 
> The patch proposed here simply adds an RTLD_RELOAD flag which disables
> checking for the DSO being already loaded, thus always loading the DSO
> again. There is no actual code modification, only the addition of two
> if()s and reindent.
> 
> Samuel
> 
> 2017-07-19  Samuel Thibault  <samuel.thibault@ens-lyon.org>
> 
> 	* bits/dlfcn.h (RTLD_RELOAD): New macro
> 	* sysdeps/mips/bits/dlfcn.h (RTLD_RELOAD): New macro
> 	* dlfcn/dlopen.c (dlopen_doit): Let args->mode contain RTLD_RELOAD.
> 	* elf/dl-load.c (_dl_map_object_from_fd): Do not match the file id
> 	when mode contains RTLD_RELOAD.
> 	(_dl_map_object): Do not match the file name when mode contains
> 	RTLD_RELOAD.
> 	* elf/tst-reload.c: New file.
> 
> diff --git a/bits/dlfcn.h b/bits/dlfcn.h
> index 7786d8f939..371e8a0e3c 100644
> --- a/bits/dlfcn.h
> +++ b/bits/dlfcn.h
> @@ -26,6 +26,7 @@
>  #define	RTLD_BINDING_MASK   0x3	/* Mask of binding time value.  */
>  #define RTLD_NOLOAD	0x00004	/* Do not load the object.  */
>  #define RTLD_DEEPBIND	0x00008	/* Use deep binding.  */
> +#define RTLD_RELOAD	0x00010	/* Reload the object.  */
>  
>  /* If the following bit is set in the MODE argument to `dlopen',
>     the symbols of the loaded object and its dependencies are made
> diff --git a/dlfcn/dlopen.c b/dlfcn/dlopen.c
> index 22120655d2..d317565b7f 100644
> --- a/dlfcn/dlopen.c
> +++ b/dlfcn/dlopen.c
> @@ -60,7 +60,7 @@ dlopen_doit (void *a)
>  
>    if (args->mode & ~(RTLD_BINDING_MASK | RTLD_NOLOAD | RTLD_DEEPBIND
>  		     | RTLD_GLOBAL | RTLD_LOCAL | RTLD_NODELETE
> -		     | __RTLD_SPROF))
> +		     | RTLD_RELOAD | __RTLD_SPROF))
>      _dl_signal_error (0, NULL, NULL, _("invalid mode parameter"));
>  
>    args->new = GLRO(dl_open) (args->file ?: "", args->mode | __RTLD_DLOPEN,
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index c1b6d4ba0f..6cd28dc15e 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -894,20 +894,23 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>      }
>  
>    /* Look again to see if the real name matched another already loaded.  */
> -  for (l = GL(dl_ns)[nsid]._ns_loaded; l != NULL; l = l->l_next)
> -    if (!l->l_removed && _dl_file_id_match_p (&l->l_file_id, &id))
> -      {
> -	/* The object is already loaded.
> -	   Just bump its reference count and return it.  */
> -	__close (fd);
> +  if ((mode & RTLD_RELOAD) == 0)
> +    {
> +      for (l = GL(dl_ns)[nsid]._ns_loaded; l != NULL; l = l->l_next)
> +	if (!l->l_removed && _dl_file_id_match_p (&l->l_file_id, &id))
> +	  {
> +	    /* The object is already loaded.
> +	       Just bump its reference count and return it.  */
> +	    __close (fd);
>  
> -	/* If the name is not in the list of names for this object add
> -	   it.  */
> -	free (realname);
> -	add_name_to_object (l, name);
> +	    /* If the name is not in the list of names for this object add
> +	       it.  */
> +	    free (realname);
> +	    add_name_to_object (l, name);
>  
> -	return l;
> -      }
> +	    return l;
> +	  }
> +    }
>  
>  #ifdef SHARED
>    /* When loading into a namespace other than the base one we must
> @@ -1902,33 +1905,36 @@ _dl_map_object (struct link_map *loader, const char *name,
>    assert (nsid < GL(dl_nns));
>  
>    /* Look for this name among those already loaded.  */
> -  for (l = GL(dl_ns)[nsid]._ns_loaded; l; l = l->l_next)
> +  if ((mode & RTLD_RELOAD) == 0)
>      {
> -      /* If the requested name matches the soname of a loaded object,
> -	 use that object.  Elide this check for names that have not
> -	 yet been opened.  */
> -      if (__glibc_unlikely ((l->l_faked | l->l_removed) != 0))
> -	continue;
> -      if (!_dl_name_match_p (name, l))
> +      for (l = GL(dl_ns)[nsid]._ns_loaded; l; l = l->l_next)
>  	{
> -	  const char *soname;
> -
> -	  if (__glibc_likely (l->l_soname_added)
> -	      || l->l_info[DT_SONAME] == NULL)
> +	  /* If the requested name matches the soname of a loaded object,
> +	     use that object.  Elide this check for names that have not
> +	     yet been opened.  */
> +	  if (__glibc_unlikely ((l->l_faked | l->l_removed) != 0))
>  	    continue;
> +	  if (!_dl_name_match_p (name, l))
> +	    {
> +	      const char *soname;
>  
> -	  soname = ((const char *) D_PTR (l, l_info[DT_STRTAB])
> -		    + l->l_info[DT_SONAME]->d_un.d_val);
> -	  if (strcmp (name, soname) != 0)
> -	    continue;
> +	      if (__glibc_likely (l->l_soname_added)
> +		  || l->l_info[DT_SONAME] == NULL)
> +		continue;
>  
> -	  /* We have a match on a new name -- cache it.  */
> -	  add_name_to_object (l, soname);
> -	  l->l_soname_added = 1;
> -	}
> +	      soname = ((const char *) D_PTR (l, l_info[DT_STRTAB])
> +			+ l->l_info[DT_SONAME]->d_un.d_val);
> +	      if (strcmp (name, soname) != 0)
> +		continue;
>  
> -      /* We have a match.  */
> -      return l;
> +	      /* We have a match on a new name -- cache it.  */
> +	      add_name_to_object (l, soname);
> +	      l->l_soname_added = 1;
> +	    }
> +
> +	  /* We have a match.  */
> +	  return l;
> +	}
>      }
>  
>    /* Display information if we are debugging.  */
> diff --git a/elf/tst-reload.c b/elf/tst-reload.c
> new file mode 100644
> index 0000000000..1fb25e7c97
> --- /dev/null
> +++ b/elf/tst-reload.c
> @@ -0,0 +1,83 @@
> +/* Verify that RTLD_NOLOAD works as expected.
> +
> +   Copyright (C) 2016-2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library 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
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <dlfcn.h>
> +#include <stdio.h>
> +#include <gnu/lib-names.h>
> +
> +static int
> +do_test (void)
> +{
> +  /* Test that no object is loaded with RTLD_NOLOAD.  */
> +  void *h1 = dlopen (LIBM_SO, RTLD_LAZY | RTLD_NOLOAD);
> +  if (h1 != NULL)
> +    {
> +      printf ("h1: DSO has been loaded while it should have not\n");
> +      return 1;
> +    }
> +
> +  /* Test that loading an already loaded object returns the same handle.  */
> +  void *h2 = dlopen (LIBM_SO, RTLD_LAZY);
> +  if (h2 == NULL)
> +    {
> +      printf ("h2: failed to open DSO: %s\n", dlerror ());
> +      return 1;
> +    }
> +  void *h3 = dlopen (LIBM_SO, RTLD_LAZY);
> +  if (h3 == NULL)
> +    {
> +      printf ("h3: failed to open DSO: %s\n", dlerror ());
> +      return 1;
> +    }
> +  if (h3 != h2)
> +    {
> +      printf ("h3: should return the same object\n");
> +      return 1;
> +    }
> +
> +  /* Test that reloading an already loaded object returns a different handle.  */
> +  void *h4 = dlopen (LIBM_SO, RTLD_LAZY | RTLD_RELOAD);
> +  if (h4 == NULL)
> +    {
> +      printf ("h4: failed to open DSO: %s\n", dlerror ());
> +      return 1;
> +    }
> +  if (h4 == h2)
> +    {
> +      printf ("h4: should not return the same object\n");
> +      return 1;
> +    }
> +
> +  /* Cleanup */
> +  if (dlclose (h4) != 0)
> +    {
> +      printf ("h4: dlclose failed: %s\n", dlerror ());
> +      return 1;
> +    }
> +  if (dlclose (h2) != 0)
> +    {
> +      printf ("h2: dlclose failed: %s\n", dlerror ());
> +      return 1;
> +    }
> +
> +  return 0;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> diff --git a/sysdeps/mips/bits/dlfcn.h b/sysdeps/mips/bits/dlfcn.h
> index 95b2fa0973..c4bf5e149b 100644
> --- a/sysdeps/mips/bits/dlfcn.h
> +++ b/sysdeps/mips/bits/dlfcn.h
> @@ -26,6 +26,7 @@
>  #define RTLD_BINDING_MASK  0x3	/* Mask of binding time value.  */
>  #define RTLD_NOLOAD	0x00008	/* Do not load the object.  */
>  #define RTLD_DEEPBIND	0x00010	/* Use deep binding.  */
> +#define RTLD_RELOAD	0x00020	/* Reload the object.  */
>  
>  /* If the following bit is set in the MODE argument to `dlopen',
>     the symbols of the loaded object and its dependencies are made
  

On 08/03/2017 04:37 PM, Samuel Thibault wrote:
> Hello,
> 
> So, is it OK to add this? Considering that dlmopen() brings us far from
> enough factorization for our needs, and dlopen() currently never reloads
> unless a real fat copy of the file is done (hardlinks/symlinks don't
> work, as it is based on st_ino).

I would still see a discussion of the intended symbol binding behavior.

What happens if a subsequently loaded object references a duplicated
library?  Will it reference the most recent duplicate, or the original
library?

What happens if libA depends on libB, and you need to duplicate both?

What happens with RTLD_NEXT in a duplicated object?  Will it look at
earlier duplicates, too?

What happens if you reload libc.so?  Your test case assumes that
reloading works for libm.so, but I think even that is a bit of a stretch.

Thanks,
Florian
  

On 08/03/2017 01:26 PM, Florian Weimer wrote:
> On 08/03/2017 04:37 PM, Samuel Thibault wrote:
>> Hello,
>>
>> So, is it OK to add this? Considering that dlmopen() brings us far from
>> enough factorization for our needs, and dlopen() currently never reloads
>> unless a real fat copy of the file is done (hardlinks/symlinks don't
>> work, as it is based on st_ino).
> 
> I would still see a discussion of the intended symbol binding behavior.
> 
> What happens if a subsequently loaded object references a duplicated
> library?  Will it reference the most recent duplicate, or the original
> library?
> 
> What happens if libA depends on libB, and you need to duplicate both?
> 
> What happens with RTLD_NEXT in a duplicated object?  Will it look at
> earlier duplicates, too?
> 
> What happens if you reload libc.so?  Your test case assumes that
> reloading works for libm.so, but I think even that is a bit of a stretch.

Agreed. If we add RTLD_RELOAD the semantics must be clearly documented.
We have enough problems with the existing interfaces that I would like
to see this documented better if you're adding a new constant.

On top of this you need tests for the various semantically different
claims we make to show they still work.

And it is *not* OK to go in as-is.