[RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[Vivek Das Mohapatra]

As discussed in https://sourceware.org/bugzilla/show_bug.cgi?id=22745
dlmopen requires a mechanism for [optionally] sharing some objects
between more than one namespace.

The following patchset attempts an implementation for this: If an object
is loaded with the new RTLD_SHARED flag we instead ensure that a "master"
copy exists (and is flagged as no-delete) in the main namespace and a
thin wrapper or clone is placed in the target namespace.

I have attached the test program(s) I am using to the bug above.

It is not intended as a final implementation but I wanted to check
that the basic approach is acceptable/workable.

If it is, then I plan to extend the patchset as follows:

 - dlmopen will implicitly apply RTLD_SHARED to the libc/libpthread group
 - The user will be able to request that this sharing _not_ occur
   by passing a different flag to dlmopen (name TBD)
 - LD_AUDIT paths will not apply this implict sharing rule, so audit libraries
   will continue to be completely isolated.

If it isn't, then I guess it's back to the drawing board (but reasons why
it isn't acceptable/workable would be appreciated so I can figure out how
to do it right).

Vivek Das Mohapatra (5):
  bits/dlfcn.h: Declare and describe the dlmopen RTLD_SHARED flag
  include/link.h: Update the link_map struct to allow clones
  elf/dl-object.c: Implement a helper function to clone link_map entries
  elf/dl-load.c, elf-dl-open.c: Implement RTLD_SHARED dlmopen cloning
  elf/dl-fini.c: Handle cloned link_map entries in the shutdown path

 bits/dlfcn.h               |  7 +++++
 elf/dl-fini.c              | 50 ++++++++++++++++++++++++++++++
 elf/dl-load.c              | 34 ++++++++++++++++++++
 elf/dl-object.c            | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 elf/dl-open.c              | 31 +++++++++++++++++--
 include/link.h             |  6 ++--
 sysdeps/generic/ldsodefs.h |  6 ++++
 7 files changed, 207 insertions(+), 4 deletions(-)

-- 
2.11.0

[RFC][PATCH v1 5/5] elf/dl-fini.c: Handle cloned link_map entries in the shutdown path

[Vivek Das Mohapatra]

From: Vivek Das Mohapatra <vivek@collabora.co.uk>

When cleaning up before exit we should not call destructors or
otherwise free [most of] the contents of a cloned link_map entry
since they share [most of] their contents with the LM_ID_BASE
object from which they were cloned.

Instead we do the minimal cleanup necessary and remove them from
the namespace linked list(s) before the main cleanup code path
is triggered: This prevemts double-frees and double-invocation
of any destructors (which might not be idempotent).
---
 elf/dl-fini.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/elf/dl-fini.c b/elf/dl-fini.c
index 3cfc262400..643a68504e 100644
--- a/elf/dl-fini.c
+++ b/elf/dl-fini.c
@@ -24,6 +24,50 @@
 /* Type of the constructor functions.  */
 typedef void (*fini_t) (void);
 
+/* Remove (and free) cloned entries from the namespace specifid by `ns'.  */
+void
+_dl_forget_clones (Lmid_t ns)
+{
+#ifdef SHARED /* Obviously none of this applies if */
+  struct link_map *clone;
+  struct link_map *next;
+
+  if (ns < 0 || ns >= DL_NNS)
+    return;
+
+  for (clone = GL(dl_ns)[ns]._ns_loaded; clone != NULL; clone = next)
+    {
+      next = clone->l_next;
+
+      /* Not actually clone, don't care.  */
+      if (!clone->l_clone)
+        continue;
+
+      if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_IMPCALLS))
+        _dl_debug_printf ("\nclone removed %s [%lu]\n", clone->l_name, ns);
+
+      /* If item is a clone, slice it out of the linked list.  */
+      if (clone == GL(dl_ns)[ns]._ns_loaded)
+        GL(dl_ns)[ns]._ns_loaded = clone->l_next;
+      else
+        clone->l_prev->l_next = clone->l_next;
+
+      /* Remember to fix up the links in both directions.  */
+      if (clone->l_next)
+        clone->l_next->l_prev = clone->l_prev;
+
+      /* Don't need to do most destructor handling for clones.  */
+      if (clone->l_free_initfini)
+        free (clone->l_initfini);
+
+      /* Do need to fix the global load count which is updated in
+         _dl_add_to_namespace_list.  */
+      GL(dl_ns)[ns]._ns_nloaded--;
+
+      free (clone);
+    }
+#endif
+}
 
 void
 _dl_fini (void)
@@ -52,6 +96,12 @@ _dl_fini (void)
       /* Protect against concurrent loads and unloads.  */
       __rtld_lock_lock_recursive (GL(dl_load_lock));
 
+      /* We need to remove any pointers to cloned entries (link_map
+         structs that are copied from one namespace to another to
+         implement RTLD_SHARED semantics) before the regular cleanup
+         code gets to them.  */
+      _dl_forget_clones (ns);
+
       unsigned int nloaded = GL(dl_ns)[ns]._ns_nloaded;
       /* No need to do anything for empty namespaces or those used for
 	 auditing DSOs.  */
-- 
2.11.0

Re: [RFC][PATCH v1 5/5] elf/dl-fini.c: Handle cloned link_map entries in the shutdown path

[Carlos O'Donell]

On 05/16/2018 01:11 PM, Vivek Das Mohapatra wrote:
> From: Vivek Das Mohapatra <vivek@collabora.co.uk>
> 
> When cleaning up before exit we should not call destructors or
> otherwise free [most of] the contents of a cloned link_map entry
> since they share [most of] their contents with the LM_ID_BASE
> object from which they were cloned.

s/clones/proxies/g

> Instead we do the minimal cleanup necessary and remove them from
> the namespace linked list(s) before the main cleanup code path
> is triggered: This prevemts double-frees and double-invocation
> of any destructors (which might not be idempotent).
> ---
>  elf/dl-fini.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/elf/dl-fini.c b/elf/dl-fini.c
> index 3cfc262400..643a68504e 100644
> --- a/elf/dl-fini.c
> +++ b/elf/dl-fini.c
> @@ -24,6 +24,50 @@
>  /* Type of the constructor functions.  */
>  typedef void (*fini_t) (void);
>  
> +/* Remove (and free) cloned entries from the namespace specifid by `ns'.  */
> +void
> +_dl_forget_clones (Lmid_t ns)
> +{
> +#ifdef SHARED /* Obviously none of this applies if */

Move the #ifdef up to the caller please.

> +  struct link_map *clone;
> +  struct link_map *next;
> +
> +  if (ns < 0 || ns >= DL_NNS)
> +    return;

Make this an assert. It is an internal error to have an invalid ns at 
this point. We should not return and ignore this.

> +
> +  for (clone = GL(dl_ns)[ns]._ns_loaded; clone != NULL; clone = next)
> +    {
> +      next = clone->l_next;
> +
> +      /* Not actually clone, don't care.  */
> +      if (!clone->l_clone)
> +        continue;
> +
> +      if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_IMPCALLS))
> +        _dl_debug_printf ("\nclone removed %s [%lu]\n", clone->l_name, ns);
> +
> +      /* If item is a clone, slice it out of the linked list.  */
> +      if (clone == GL(dl_ns)[ns]._ns_loaded)
> +        GL(dl_ns)[ns]._ns_loaded = clone->l_next;
> +      else
> +        clone->l_prev->l_next = clone->l_next;
> +
> +      /* Remember to fix up the links in both directions.  */
> +      if (clone->l_next)
> +        clone->l_next->l_prev = clone->l_prev;
> +
> +      /* Don't need to do most destructor handling for clones.  */
> +      if (clone->l_free_initfini)
> +        free (clone->l_initfini);
> +
> +      /* Do need to fix the global load count which is updated in
> +         _dl_add_to_namespace_list.  */
> +      GL(dl_ns)[ns]._ns_nloaded--;
> +
> +      free (clone);
> +    }
> +#endif
> +}

OK.
  
>  void
>  _dl_fini (void)
> @@ -52,6 +96,12 @@ _dl_fini (void)
>        /* Protect against concurrent loads and unloads.  */
>        __rtld_lock_lock_recursive (GL(dl_load_lock));
>  
> +      /* We need to remove any pointers to cloned entries (link_map
> +         structs that are copied from one namespace to another to
> +         implement RTLD_SHARED semantics) before the regular cleanup
> +         code gets to them.  */
> +      _dl_forget_clones (ns);

While this is easy to implement like this, I think we are going to run
into use cases where this doesn't work.

Consider this test case:

- Only a fini in a library.
- Lazy binding.
- fini calls a libc.so.6 function for the first time.
- When fini is called we jump into ld.so to find the symbol to call,
  but the libc.so.6 proxy has been removed, and so we can no longer
  resolve calls to it.

You will, IMO, have to remove the proxies at the point the library
would have been unloaded normally from the namespace, but avoid doing
all the other work.

> +
>        unsigned int nloaded = GL(dl_ns)[ns]._ns_nloaded;
>        /* No need to do anything for empty namespaces or those used for
>  	 auditing DSOs.  */
> 

In summary I think we need a v2 of this patch which implements a more complex
free-at-unload support.

-- 
Cheers,
Carlos.

Re: [RFC][PATCH v1 5/5] elf/dl-fini.c: Handle cloned link_map entries in the shutdown path

[Vivek Das Mohapatra]

[-- Attachment #1: Type: text/plain, Size: 1895 bytes --]

>> --- a/elf/dl-fini.c
>> +++ b/elf/dl-fini.c
>> @@ -24,6 +24,50 @@
>>  /* Type of the constructor functions.  */
>>  typedef void (*fini_t) (void);
>>
>> +/* Remove (and free) cloned entries from the namespace specifid by `ns'.  */
>> +void
>> +_dl_forget_clones (Lmid_t ns)
>> +{
>> +#ifdef SHARED /* Obviously none of this applies if */
>
> Move the #ifdef up to the caller please.

I think it was there originally but when I was building the
full suite (including the static variant) gcc got upset at me
about the bounds checking of GL(dl_ns)[…]. I'll revisit it
and see if I can figure out what the problem was.

>> +      /* We need to remove any pointers to cloned entries (link_map
>> +         structs that are copied from one namespace to another to
>> +         implement RTLD_SHARED semantics) before the regular cleanup
>> +         code gets to them.  */
>> +      _dl_forget_clones (ns);
>
> While this is easy to implement like this, I think we are going to run
> into use cases where this doesn't work.
>
> Consider this test case:
>
> - Only a fini in a library.
> - Lazy binding.
> - fini calls a libc.so.6 function for the first time.
> - When fini is called we jump into ld.so to find the symbol to call,
>  but the libc.so.6 proxy has been removed, and so we can no longer
>  resolve calls to it.

> You will, IMO, have to remove the proxies at the point the library
> would have been unloaded normally from the namespace, but avoid doing
> all the other work.

Hm. Ok, I'll get on that.

>> +
>>        unsigned int nloaded = GL(dl_ns)[ns]._ns_nloaded;
>>        /* No need to do anything for empty namespaces or those used for
>>  	 auditing DSOs.  */
>>
>
> In summary I think we need a v2 of this patch which implements a more complex
> free-at-unload support.

Right. I will work through this and the other changes, hopefully a new patchset
should land next week.

[RFC][PATCH v1 3/5] elf/dl-object.c: Implement a helper function to clone link_map entries

[Vivek Das Mohapatra]

From: Vivek Das Mohapatra <vivek@collabora.co.uk>

Provides the minimal functionality needed to take an existing
link_map entry and create a clone of it in the specified namespace.
---
 elf/dl-object.c            | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 sysdeps/generic/ldsodefs.h |  6 ++++
 2 files changed, 83 insertions(+)

diff --git a/elf/dl-object.c b/elf/dl-object.c
index b37bcc1295..144ba6c993 100644
--- a/elf/dl-object.c
+++ b/elf/dl-object.c
@@ -21,6 +21,7 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <ldsodefs.h>
+#include <libintl.h>
 
 #include <assert.h>
 
@@ -50,6 +51,82 @@ _dl_add_to_namespace_list (struct link_map *new, Lmid_t nsid)
   __rtld_lock_unlock_recursive (GL(dl_load_write_lock));
 }
 
+/* Clone an existing link map entry into a new link map.  */
+/* This is based on _dl_new_object, skipping the steps we know we won't need
+   because this is mostly just a shell for the l_real pointer holding the real
+   link map entry (normally l == l->l_real, but not for ld.so in non-main
+   link maps or RTLD_SHARED clones).
+   It also flags the clone by setting l_clone, and sets the the no-delete
+   flag in the original.  */
+struct link_map *
+_dl_clone_object (struct link_map *old, int mode, Lmid_t nsid)
+{
+  const char *name;
+  struct link_map *new;
+  struct libname_list *newname;
+#ifdef SHARED
+  /* See _dl_new_object for how this number is arrived at:  */
+  unsigned int na = GLRO(dl_naudit) ?: ((mode & __RTLD_OPENEXEC) ? DL_NNS : 0);
+  size_t audit_space = na * sizeof (new->l_audit[0]);
+#else
+# define audit_space 0
+#endif
+
+  name = old->l_name;
+
+  /* Don't clone a clone: Go to the progenitor.  */
+  while (old && old->l_clone)
+    old = old->l_real;
+
+  if (old == NULL)
+    _dl_signal_error (EINVAL, name, NULL, N_("cannot clone NULL link_map"));
+
+  /* Object already exists in the target namespace. This should get handled
+     by dl_open_worker but just in case we get this far, handle it:  */
+  if (__glibc_unlikely (old->l_ns == nsid))
+    {
+      /* Not actually possible, given the sanity checks above.  */
+      if (old->l_clone)
+        return old;
+
+      _dl_signal_error (EEXIST, name, NULL,
+                        N_("object cannot be demoted to a clone"));
+    }
+
+  /* Now duplicate as little of _dl_new_object as possible to get a
+     working cloned object in the target link map.  */
+  new = (struct link_map *) calloc (sizeof (*new) + audit_space
+                                    + sizeof (struct link_map *)
+                                    + sizeof (*newname) + PATH_MAX, 1);
+
+  /* Specific to the clone.  */
+  new->l_real = old;
+  new->l_clone = 1;
+  new->l_ns = nsid;
+
+  /* Copied from the origin.  */
+  new->l_libname = old->l_libname;
+  new->l_name = old->l_name;
+  new->l_type = old->l_type;
+
+  if (__glibc_unlikely (mode & RTLD_NODELETE))
+    new->l_flags_1 |= DF_1_NODELETE;
+
+  /* Specific to the origin. Ideally we'd do some accounting here but
+     for now it's easier to pin the original so the clone remains valid.  */
+  old->l_flags_1 |= DF_1_NODELETE;
+
+  /* Fix up the searchlist so that relocations work.  */
+  /* TODO: figure out if we should call _dl_map_object_deps
+     or copy the contents of l_scope, l_searchlist et al.  */
+  _dl_map_object_deps (new, NULL, 0, 0,
+		       mode & (__RTLD_DLOPEN | RTLD_DEEPBIND | __RTLD_AUDIT));
+
+  /* And finally put the clone into the target namespace.  */
+  _dl_add_to_namespace_list (new, nsid);
+
+  return new;
+}
 
 /* Allocate a `struct link_map' for a new object being loaded,
    and enter it into the _dl_loaded list.  */
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index 5e1b24ecb5..573d24f611 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -913,6 +913,12 @@ extern lookup_t _dl_lookup_symbol_x (const char *undef,
 extern void _dl_add_to_namespace_list (struct link_map *new, Lmid_t nsid)
      attribute_hidden;
 
+/* Clone an existing link map entry into a new link map */
+extern struct link_map *_dl_clone_object (struct link_map *old,
+                                          int mode,
+                                          Lmid_t nsid)
+     attribute_hidden;
+
 /* Allocate a `struct link_map' for a new object being loaded.  */
 extern struct link_map *_dl_new_object (char *realname, const char *libname,
 					int type, struct link_map *loader,
-- 
2.11.0

Re: [RFC][PATCH v1 3/5] elf/dl-object.c: Implement a helper function to clone link_map entries

[Carlos O'Donell]

On 05/16/2018 01:11 PM, Vivek Das Mohapatra wrote:
> From: Vivek Das Mohapatra <vivek@collabora.co.uk>
> 
> Provides the minimal functionality needed to take an existing
> link_map entry and create a clone of it in the specified namespace.
> ---
>  elf/dl-object.c            | 77 ++++++++++++++++++++++++++++++++++++++++++++++
>  sysdeps/generic/ldsodefs.h |  6 ++++
>  2 files changed, 83 insertions(+)
> 
> diff --git a/elf/dl-object.c b/elf/dl-object.c
> index b37bcc1295..144ba6c993 100644
> --- a/elf/dl-object.c
> +++ b/elf/dl-object.c
> @@ -21,6 +21,7 @@
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <ldsodefs.h>
> +#include <libintl.h>
>  
>  #include <assert.h>
>  
> @@ -50,6 +51,82 @@ _dl_add_to_namespace_list (struct link_map *new, Lmid_t nsid)
>    __rtld_lock_unlock_recursive (GL(dl_load_write_lock));
>  }
>  
> +/* Clone an existing link map entry into a new link map.  */

Existing comment about the use of "clone" apply here too.

> +/* This is based on _dl_new_object, skipping the steps we know we won't need

Both of these comments should be merged into one.

> +   because this is mostly just a shell for the l_real pointer holding the real
> +   link map entry (normally l == l->l_real, but not for ld.so in non-main
> +   link maps or RTLD_SHARED clones).
> +   It also flags the clone by setting l_clone, and sets the the no-delete
> +   flag in the original.  */
> +struct link_map *
> +_dl_clone_object (struct link_map *old, int mode, Lmid_t nsid)
> +{
> +  const char *name;
> +  struct link_map *new;
> +  struct libname_list *newname;
> +#ifdef SHARED
> +  /* See _dl_new_object for how this number is arrived at:  */
> +  unsigned int na = GLRO(dl_naudit) ?: ((mode & __RTLD_OPENEXEC) ? DL_NNS : 0);

This is too complex. It should be expanded out for readability, and likely so anywhere
else you see it, but let's start by making this copy easy to read.

> +  size_t audit_space = na * sizeof (new->l_audit[0]);
> +#else
> +# define audit_space 0
> +#endif
> +
> +  name = old->l_name;
> +
> +  /* Don't clone a clone: Go to the progenitor.  */

I think we'd probably say "Find the original link map from the proxy."

> +  while (old && old->l_clone)
> +    old = old->l_real;
> +
> +  if (old == NULL)
> +    _dl_signal_error (EINVAL, name, NULL, N_("cannot clone NULL link_map"));
> +
> +  /* Object already exists in the target namespace. This should get handled
> +     by dl_open_worker but just in case we get this far, handle it:  */
> +  if (__glibc_unlikely (old->l_ns == nsid))
> +    {
> +      /* Not actually possible, given the sanity checks above.  */
> +      if (old->l_clone)
> +        return old;
> +
> +      _dl_signal_error (EEXIST, name, NULL,
> +                        N_("object cannot be demoted to a clone"));
> +    }
> +
> +  /* Now duplicate as little of _dl_new_object as possible to get a
> +     working cloned object in the target link map.  */
> +  new = (struct link_map *) calloc (sizeof (*new) + audit_space
> +                                    + sizeof (struct link_map *)
> +                                    + sizeof (*newname) + PATH_MAX, 1);

calloc could fail, you need to _dl_signal_error ENOMEM.

> +
> +  /* Specific to the clone.  */
> +  new->l_real = old;
> +  new->l_clone = 1;
> +  new->l_ns = nsid;
> +
> +  /* Copied from the origin.  */
> +  new->l_libname = old->l_libname;
> +  new->l_name = old->l_name;
> +  new->l_type = old->l_type;
> +
> +  if (__glibc_unlikely (mode & RTLD_NODELETE))
> +    new->l_flags_1 |= DF_1_NODELETE;
> +
> +  /* Specific to the origin. Ideally we'd do some accounting here but
> +     for now it's easier to pin the original so the clone remains valid.  */
> +  old->l_flags_1 |= DF_1_NODELETE;
> +
> +  /* Fix up the searchlist so that relocations work.  */
> +  /* TODO: figure out if we should call _dl_map_object_deps
> +     or copy the contents of l_scope, l_searchlist et al.  */
> +  _dl_map_object_deps (new, NULL, 0, 0,
> +		       mode & (__RTLD_DLOPEN | RTLD_DEEPBIND | __RTLD_AUDIT));

Calling _dl_map_object_deps seems like a conservative thing to do here.

> +
> +  /* And finally put the clone into the target namespace.  */
> +  _dl_add_to_namespace_list (new, nsid);
> +
> +  return new;
> +}
>  
>  /* Allocate a `struct link_map' for a new object being loaded,
>     and enter it into the _dl_loaded list.  */
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index 5e1b24ecb5..573d24f611 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -913,6 +913,12 @@ extern lookup_t _dl_lookup_symbol_x (const char *undef,
>  extern void _dl_add_to_namespace_list (struct link_map *new, Lmid_t nsid)
>       attribute_hidden;
>  
> +/* Clone an existing link map entry into a new link map */
> +extern struct link_map *_dl_clone_object (struct link_map *old,
> +                                          int mode,
> +                                          Lmid_t nsid)
> +     attribute_hidden;

OK.

But I think we'd call this _dl_proxy_object (...);

> +
>  /* Allocate a `struct link_map' for a new object being loaded.  */
>  extern struct link_map *_dl_new_object (char *realname, const char *libname,
>  					int type, struct link_map *loader,
> 


-- 
Cheers,
Carlos.

[RFC][PATCH v1 1/5] bits/dlfcn.h: Declare and describe the dlmopen RTLD_SHARED flag

[Vivek Das Mohapatra]

From: Vivek Das Mohapatra <vivek@collabora.co.uk>

This flag will instruct dlmopen to create a shared object present
in both the main namespace and the selected namespace when set.
---
 bits/dlfcn.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/bits/dlfcn.h b/bits/dlfcn.h
index b0b129b66b..a96270d499 100644
--- a/bits/dlfcn.h
+++ b/bits/dlfcn.h
@@ -32,6 +32,13 @@
    visible as if the object were linked directly into the program.  */
 #define RTLD_GLOBAL	0x00100
 
+/* If the following bit is set in the MODE argument to dlmopen
+   then the target object is loaded into the main namespace (if
+   it is not already there) and a shallow copy (clone) is placed
+   in the target namespace: This allows multiple namespaces to
+   share a single instance of a DSO.  */
+#define RTLD_SHARED 0x00080
+
 /* Unix98 demands the following flag which is the inverse to RTLD_GLOBAL.
    The implementation does this by default and so we can define the
    value to zero.  */
-- 
2.11.0

Re: [RFC][PATCH v1 1/5] bits/dlfcn.h: Declare and describe the dlmopen RTLD_SHARED flag

[Carlos O'Donell]

On 05/16/2018 01:11 PM, Vivek Das Mohapatra wrote:
> From: Vivek Das Mohapatra <vivek@collabora.co.uk>
> 
> This flag will instruct dlmopen to create a shared object present
> in both the main namespace and the selected namespace when set.
> ---
>  bits/dlfcn.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/bits/dlfcn.h b/bits/dlfcn.h
> index b0b129b66b..a96270d499 100644
> --- a/bits/dlfcn.h
> +++ b/bits/dlfcn.h
> @@ -32,6 +32,13 @@
>     visible as if the object were linked directly into the program.  */
>  #define RTLD_GLOBAL	0x00100
>  
> +/* If the following bit is set in the MODE argument to dlmopen
> +   then the target object is loaded into the main namespace (if
> +   it is not already there) and a shallow copy (clone) is placed
> +   in the target namespace: This allows multiple namespaces to
> +   share a single instance of a DSO.  */
> +#define RTLD_SHARED 0x00080

This is an RFC, but also note there is a MIPS-specific dlfcn.h which needs
patching also to define RTLD_SHARED. We should probably try to look at how
the constants overlap, and find a common value to start with for everyone.

I agree that we need a constant with which to choose between:

* Old dlmopen behaviour.

* New dlmopen behaviour.

> +
>  /* Unix98 demands the following flag which is the inverse to RTLD_GLOBAL.
>     The implementation does this by default and so we can define the
>     value to zero.  */
> 


-- 
Cheers,
Carlos.

Re: [RFC][PATCH v1 1/5] bits/dlfcn.h: Declare and describe the dlmopen RTLD_SHARED flag

[Vivek Das Mohapatra]

>> +#define RTLD_SHARED 0x00080
>
> This is an RFC, but also note there is a MIPS-specific dlfcn.h which needs
> patching also to define RTLD_SHARED. We should probably try to look at how
> the constants overlap, and find a common value to start with for everyone.

Noted.

> I agree that we need a constant with which to choose between:
>
> * Old dlmopen behaviour.
> * New dlmopen behaviour.

My plan is to put that in the "policy" patchset once this
"mechanism" patchset is acceptable:

Something like:

   RTLD_SHARED   - this library and its dependencies will be proxied
   RTLD_ISOLATED - nothing (except ld.so) will be proxied (we hope you
                   know what you're doing) aka the old behaviour
   -nothing-     - libc/libpthread/ld.so will be proxied from the main ns.
                   everything else will be isolated.

Details to be thrashed out later.

[RFC][PATCH v1 2/5] include/link.h: Update the link_map struct to allow clones

[Vivek Das Mohapatra]

From: Vivek Das Mohapatra <vivek@collabora.co.uk>

We already have an l_real pointer, used for a similar purpose by
the linker for copies of ld.so in secondary namespaces. Update its
documentation and add a bitfield to indicate when link_map entry
is a clone.
---
 include/link.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/link.h b/include/link.h
index 5924594548..e051b5cd36 100644
--- a/include/link.h
+++ b/include/link.h
@@ -104,8 +104,9 @@ struct link_map
        They may change without notice.  */
 
     /* This is an element which is only ever different from a pointer to
-       the very same copy of this type for ld.so when it is used in more
-       than one namespace.  */
+       the very same copy of this type when:
+       - A shallow copy of ld.so is placed in namespaces other than LM_ID_BASE.
+       - An object is cloned into a namespace by dlmopen with RTLD_SHARED.  */
     struct link_map *l_real;
 
     /* Number of the namespace this link map belongs to.  */
@@ -177,6 +178,7 @@ struct link_map
     unsigned int l_relocated:1;	/* Nonzero if object's relocations done.  */
     unsigned int l_init_called:1; /* Nonzero if DT_INIT function called.  */
     unsigned int l_global:1;	/* Nonzero if object in _dl_global_scope.  */
+    unsigned int l_clone:1;    /* Nonzero if object is a clone.  */
     unsigned int l_reserved:2;	/* Reserved for internal use.  */
     unsigned int l_phdr_allocated:1; /* Nonzero if the data structure pointed
 					to by `l_phdr' is allocated.  */
-- 
2.11.0

Re: [RFC][PATCH v1 2/5] include/link.h: Update the link_map struct to allow clones

[Carlos O'Donell]

On 05/16/2018 01:11 PM, Vivek Das Mohapatra wrote:
> From: Vivek Das Mohapatra <vivek@collabora.co.uk>
> 
> We already have an l_real pointer, used for a similar purpose by
> the linker for copies of ld.so in secondary namespaces. Update its
> documentation and add a bitfield to indicate when link_map entry
> is a clone.

I would avoid calling this a "clone" because it implies a direct
copy that is mutable.

This is just bikeshedding but I think 'l_proxy' might be more accurate?

That way we can talk about inserting a proxy object into a namespace
and know that the proxy is just that, a thin proxy which points to
complete link map.

> ---
>  include/link.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/link.h b/include/link.h
> index 5924594548..e051b5cd36 100644
> --- a/include/link.h
> +++ b/include/link.h
> @@ -104,8 +104,9 @@ struct link_map
>         They may change without notice.  */
>  
>      /* This is an element which is only ever different from a pointer to
> -       the very same copy of this type for ld.so when it is used in more
> -       than one namespace.  */
> +       the very same copy of this type when:
> +       - A shallow copy of ld.so is placed in namespaces other than LM_ID_BASE.
> +       - An object is cloned into a namespace by dlmopen with RTLD_SHARED.  */

OK.

>      struct link_map *l_real;
>  
>      /* Number of the namespace this link map belongs to.  */
> @@ -177,6 +178,7 @@ struct link_map
>      unsigned int l_relocated:1;	/* Nonzero if object's relocations done.  */
>      unsigned int l_init_called:1; /* Nonzero if DT_INIT function called.  */
>      unsigned int l_global:1;	/* Nonzero if object in _dl_global_scope.  */
> +    unsigned int l_clone:1;    /* Nonzero if object is a clone.  */

Suggest l_proxy:1;  /* Nonzero if object is a proxy.  */

>      unsigned int l_reserved:2;	/* Reserved for internal use.  */
>      unsigned int l_phdr_allocated:1; /* Nonzero if the data structure pointed
>  					to by `l_phdr' is allocated.  */
> 


-- 
Cheers,
Carlos.

Re: [RFC][PATCH v1 2/5] include/link.h: Update the link_map struct to allow clones

[Vivek Das Mohapatra]

> This is just bikeshedding but I think 'l_proxy' might be more accurate?

Sounds good. I initially named it clone before I'd fully dug into
the implementation details.

> That way we can talk about inserting a proxy object into a namespace
> and know that the proxy is just that, a thin proxy which points to
> complete link map.

Sounds good to me.

[RFC][PATCH v1 4/5] elf/dl-load.c, elf-dl-open.c: Implement RTLD_SHARED dlmopen cloning

[Vivek Das Mohapatra]

From: Vivek Das Mohapatra <vivek@collabora.co.uk>

This uses the new infrastructure to implement RTLD_SHARED object
cloning via dlmopen: Instead of opening the specified object in
the requested namespace we open it in the main namespace (if it
is not already present there) and clone it into the requated
namespace.

The following rules apply:

If a clone of the object is already present in the requested namespace,
we simply return it (with an incremented direct-open count).

If the object is already present in the requested namespace, a dl
error is signalled, since we cannot satisfy the user's request.

Clones are never created in the main namespace: RTLD_SHARED has no
effect when the requested namespace is LM_ID_BASE.
---
 elf/dl-load.c | 34 ++++++++++++++++++++++++++++++++++
 elf/dl-open.c | 31 +++++++++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/elf/dl-load.c b/elf/dl-load.c
index a5e3a25462..a3bc85fb0a 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1847,6 +1847,40 @@ _dl_map_object (struct link_map *loader, const char *name,
   assert (nsid >= 0);
   assert (nsid < GL(dl_nns));
 
+#ifdef SHARED
+  /* Only need to do cloning work if `nsid' is not LM_ID_BASE.  */
+  if (__glibc_unlikely ((mode & RTLD_SHARED) && (nsid != LM_ID_BASE)))
+    {
+      /* Search the target namespace, in case the object is already there.
+         Note that unlike the search in the next section we do not attempt to
+         extract the object's name if it does not yet have one.  */
+      for (l = GL(dl_ns)[nsid]._ns_loaded; l; l = l->l_next)
+        {
+          if (__glibc_unlikely ((l->l_faked | l->l_removed) != 0))
+            continue;
+
+          if (!_dl_name_match_p (name, l))
+            continue;
+
+          /* We have a match - stop searching.  */
+          break;
+        }
+
+      if (l)
+        {
+          if (l->l_clone)
+            return l;
+
+          _dl_signal_error (EEXIST, name, NULL,
+                            N_("object cannot be demoted to a clone"));
+        }
+
+      /* Further searches should be in the base ns: We will clone the
+         resulting object in dl_open_worker *after* it is initialised.  */
+      nsid = LM_ID_BASE;
+    }
+#endif
+
   /* Look for this name among those already loaded.  */
   for (l = GL(dl_ns)[nsid]._ns_loaded; l; l = l->l_next)
     {
diff --git a/elf/dl-open.c b/elf/dl-open.c
index 9dde4acfbc..0c5c75c137 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -237,6 +237,10 @@ dl_open_worker (void *a)
   /* This object is directly loaded.  */
   ++new->l_direct_opencount;
 
+  /* Clone already existed in the target ns, nothing left to do.  */
+  if (__glibc_unlikely (new->l_clone))
+    return;
+
   /* It was already open.  */
   if (__glibc_unlikely (new->l_searchlist.r_list != NULL))
     {
@@ -252,6 +256,16 @@ dl_open_worker (void *a)
 
       assert (_dl_debug_initialize (0, args->nsid)->r_state == RT_CONSISTENT);
 
+      if (__glibc_unlikely (mode & RTLD_SHARED))
+        {
+          args->map = new = _dl_clone_object (new, mode, args->nsid);
+          ++new->l_direct_opencount;
+
+          if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES))
+            _dl_debug_printf ("cloning file=%s [%lu]; direct_opencount=%u\n\n",
+                              new->l_name, new->l_ns, new->l_direct_opencount);
+        }
+
       return;
     }
 
@@ -509,6 +523,11 @@ TLS generation counter wrapped!  Please report this."));
       /* It failed.  */
       return;
 
+  if (__glibc_unlikely (mode & RTLD_SHARED))
+    {
+      args->map = _dl_clone_object (new, mode, args->nsid);
+      ++args->map->l_direct_opencount;
+    }
 #ifndef SHARED
   /* We must be the static _dl_open in libc.a.  A static program that
      has loaded a dynamic object now has competition.  */
@@ -517,8 +536,16 @@ TLS generation counter wrapped!  Please report this."));
 
   /* Let the user know about the opencount.  */
   if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES))
-    _dl_debug_printf ("opening file=%s [%lu]; direct_opencount=%u\n\n",
-		      new->l_name, new->l_ns, new->l_direct_opencount);
+    {
+      _dl_debug_printf ("opening file=%s [%lu]; direct_opencount=%u\n\n",
+                        new->l_name, new->l_ns, new->l_direct_opencount);
+
+      if (mode & RTLD_SHARED)
+        _dl_debug_printf ("cloning file=%s [%lu]; direct_opencount=%u\n\n",
+                          args->map->l_name,
+                          args->map->l_ns,
+                          args->map->l_direct_opencount);
+    }
 }
 
 
-- 
2.11.0

Re: [RFC][PATCH v1 4/5] elf/dl-load.c, elf-dl-open.c: Implement RTLD_SHARED dlmopen cloning

[Carlos O'Donell]

On 05/16/2018 01:11 PM, Vivek Das Mohapatra wrote:
> From: Vivek Das Mohapatra <vivek@collabora.co.uk>
> 
> This uses the new infrastructure to implement RTLD_SHARED object
> cloning via dlmopen: Instead of opening the specified object in
> the requested namespace we open it in the main namespace (if it
> is not already present there) and clone it into the requated
> namespace.

My comment about "proxy' applies here too, it is a better name than
clone IMO, but feel free to discuss.

> The following rules apply:
> 
> If a clone of the object is already present in the requested namespace,
> we simply return it (with an incremented direct-open count).
> 
> If the object is already present in the requested namespace, a dl
> error is signalled, since we cannot satisfy the user's request.
> 
> Clones are never created in the main namespace: RTLD_SHARED has no
> effect when the requested namespace is LM_ID_BASE.

Sounds good to me.

Where are you ensuring that libc and libpthread are proxied into all
the new namespaces? I assume that's going to be in some other patch
which has to handle the basic glibc group of libraries?

> ---
>  elf/dl-load.c | 34 ++++++++++++++++++++++++++++++++++
>  elf/dl-open.c | 31 +++++++++++++++++++++++++++++--
>  2 files changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index a5e3a25462..a3bc85fb0a 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1847,6 +1847,40 @@ _dl_map_object (struct link_map *loader, const char *name,
>    assert (nsid >= 0);
>    assert (nsid < GL(dl_nns));
>  
> +#ifdef SHARED
> +  /* Only need to do cloning work if `nsid' is not LM_ID_BASE.  */
> +  if (__glibc_unlikely ((mode & RTLD_SHARED) && (nsid != LM_ID_BASE)))
> +    {
> +      /* Search the target namespace, in case the object is already there.
> +         Note that unlike the search in the next section we do not attempt to
> +         extract the object's name if it does not yet have one.  */
> +      for (l = GL(dl_ns)[nsid]._ns_loaded; l; l = l->l_next)
> +        {
> +          if (__glibc_unlikely ((l->l_faked | l->l_removed) != 0))
> +            continue;
> +
> +          if (!_dl_name_match_p (name, l))
> +            continue;
> +
> +          /* We have a match - stop searching.  */
> +          break;
> +        }
> +
> +      if (l)
> +        {
> +          if (l->l_clone)
> +            return l;
> +
> +          _dl_signal_error (EEXIST, name, NULL,
> +                            N_("object cannot be demoted to a clone"));
> +        }
> +
> +      /* Further searches should be in the base ns: We will clone the
> +         resulting object in dl_open_worker *after* it is initialised.  */
> +      nsid = LM_ID_BASE;
> +    }
This should be split into a helper function.

I would like _dl_ma_object() to be easier to read.

> +#endif
> +
>    /* Look for this name among those already loaded.  */
>    for (l = GL(dl_ns)[nsid]._ns_loaded; l; l = l->l_next)
>      {
> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index 9dde4acfbc..0c5c75c137 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -237,6 +237,10 @@ dl_open_worker (void *a)
>    /* This object is directly loaded.  */
>    ++new->l_direct_opencount;
>  
> +  /* Clone already existed in the target ns, nothing left to do.  */
> +  if (__glibc_unlikely (new->l_clone))
> +    return;

OK.

> +
>    /* It was already open.  */
>    if (__glibc_unlikely (new->l_searchlist.r_list != NULL))
>      {
> @@ -252,6 +256,16 @@ dl_open_worker (void *a)
>  
>        assert (_dl_debug_initialize (0, args->nsid)->r_state == RT_CONSISTENT);
>  
> +      if (__glibc_unlikely (mode & RTLD_SHARED))
> +        {
> +          args->map = new = _dl_clone_object (new, mode, args->nsid);
> +          ++new->l_direct_opencount;
> +
> +          if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES))
> +            _dl_debug_printf ("cloning file=%s [%lu]; direct_opencount=%u\n\n",
> +                              new->l_name, new->l_ns, new->l_direct_opencount);
> +        }


OK.

> +
>        return;
>      }
>  
> @@ -509,6 +523,11 @@ TLS generation counter wrapped!  Please report this."));
>        /* It failed.  */
>        return;
>  
> +  if (__glibc_unlikely (mode & RTLD_SHARED))
> +    {
> +      args->map = _dl_clone_object (new, mode, args->nsid);
> +      ++args->map->l_direct_opencount;
> +    }

OK.

>  #ifndef SHARED
>    /* We must be the static _dl_open in libc.a.  A static program that
>       has loaded a dynamic object now has competition.  */
> @@ -517,8 +536,16 @@ TLS generation counter wrapped!  Please report this."));
>  
>    /* Let the user know about the opencount.  */
>    if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES))
> -    _dl_debug_printf ("opening file=%s [%lu]; direct_opencount=%u\n\n",
> -		      new->l_name, new->l_ns, new->l_direct_opencount);
> +    {
> +      _dl_debug_printf ("opening file=%s [%lu]; direct_opencount=%u\n\n",
> +                        new->l_name, new->l_ns, new->l_direct_opencount);
> +
> +      if (mode & RTLD_SHARED)
> +        _dl_debug_printf ("cloning file=%s [%lu]; direct_opencount=%u\n\n",
> +                          args->map->l_name,
> +                          args->map->l_ns,
> +                          args->map->l_direct_opencount)
> +    }

OK.


>  }
>  
>  
> 


-- 
Cheers,
Carlos.

Re: [RFC][PATCH v1 4/5] elf/dl-load.c, elf-dl-open.c: Implement RTLD_SHARED dlmopen cloning

[Vivek Das Mohapatra]

> My comment about "proxy' applies here too, it is a better name than
> clone IMO, but feel free to discuss.

No, proxy sounds better to me too. Naming things is hard.

> Sounds good to me.
>
> Where are you ensuring that libc and libpthread are proxied into all
> the new namespaces? I assume that's going to be in some other patch
> which has to handle the basic glibc group of libraries?

Yes, that's going to land once the basic isolation/sharing mechanism
is in an acceptable state.

> This should be split into a helper function.
>
> I would like _dl_ma_object() to be easier to read.

Ok, will do.

Re: [RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[Joseph Myers]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]

On Wed, 16 May 2018, Vivek Das Mohapatra wrote:

> It is not intended as a final implementation but I wanted to check
> that the basic approach is acceptable/workable.

I strongly advise completing the copyright assignment process before 
posting detailed patches, as many people may be reluctant to look in 
detail at anything large that doesn't have a copyright assignment on file.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[Vivek Das Mohapatra]

> I strongly advise completing the copyright assignment process before

I did that decades ago. Do they lapse?

Re: [RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[Joseph Myers]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

On Wed, 16 May 2018, Vivek Das Mohapatra wrote:

> > I strongly advise completing the copyright assignment process before
> 
> I did that decades ago. Do they lapse?

They're specific to particular GNU projects, unless they say they are for 
all GNU projects.  The only assignment I see on file for you is for Emacs 
(copyright.list entry dated 2006-05-15).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[Vivek Das Mohapatra]

> They're specific to particular GNU projects, unless they say they are for
> all GNU projects.  The only assignment I see on file for you is for Emacs
> (copyright.list entry dated 2006-05-15).

I'll get on that then.

Re: [RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[Vivek Das Mohapatra]

[-- Attachment #1: Type: text/plain, Size: 48 bytes --]

> I'll get on that then.

Sent to assign@…

Re: [RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[Carlos O'Donell]

On 05/16/2018 03:39 PM, Vivek Das Mohapatra wrote:
>> I strongly advise completing the copyright assignment process before
> 
> I did that decades ago. Do they lapse?

You have to file them per-project.

Do you have an assignment on file for glibc?

-- 
Cheers,
Carlos.

Re: [RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[Vivek Das Mohapatra]

On Wed, 16 May 2018, Carlos O'Donell wrote:

> Do you have an assignment on file for glibc?

I do not.

Re: [RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[Carlos O'Donell]

On 05/16/2018 03:45 PM, Vivek Das Mohapatra wrote:
> On Wed, 16 May 2018, Carlos O'Donell wrote:
> 
>> Do you have an assignment on file for glibc?
> 
> I do not.

I suggest a Future's assignment for glibc.

"9. FSF copyright assignment"
https://sourceware.org/glibc/wiki/Contribution%20checklist

"Future's Assignment"
http://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/request-assign.future

The FSF can do the assignments digitally and so they should be
quick and painless, and a future's assignment allows us to take
any future patches or changes you want to make to this code.

Once you have an assignment in place it makes it much much easier
to review complex changes like this.

Thank you for your patience.

-- 
Cheers,
Carlos.

Re: [RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[Vivek Das Mohapatra]

> Once you have an assignment in place it makes it much much easier
> to review complex changes like this.

It's done.

Re: [RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[Carlos O'Donell]

On 05/18/2018 07:46 AM, Vivek Das Mohapatra wrote:
>> Once you have an assignment in place it makes it much much easier
>> to review complex changes like this.
> 
> It's done.
> 

I confirm you have a futures assignment for glibc.

Thank you for that!

-- 
Cheers,
Carlos.

Re: [RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[Carlos O'Donell]

On 05/16/2018 01:11 PM, Vivek Das Mohapatra wrote:
> As discussed in https://sourceware.org/bugzilla/show_bug.cgi?id=22745
> dlmopen requires a mechanism for [optionally] sharing some objects
> between more than one namespace.
> 
> The following patchset attempts an implementation for this: If an object
> is loaded with the new RTLD_SHARED flag we instead ensure that a "master"
> copy exists (and is flagged as no-delete) in the main namespace and a
> thin wrapper or clone is placed in the target namespace.
> 
> I have attached the test program(s) I am using to the bug above.
> 
> It is not intended as a final implementation but I wanted to check
> that the basic approach is acceptable/workable.
> 
> If it is, then I plan to extend the patchset as follows:
> 
>  - dlmopen will implicitly apply RTLD_SHARED to the libc/libpthread group
>  - The user will be able to request that this sharing _not_ occur
>    by passing a different flag to dlmopen (name TBD)
>  - LD_AUDIT paths will not apply this implict sharing rule, so audit libraries
>    will continue to be completely isolated.

Thank you for working through this!

> If it isn't, then I guess it's back to the drawing board (but reasons why
> it isn't acceptable/workable would be appreciated so I can figure out how
> to do it right).

I think the basic design question is "Where does the namespace split exist?"

If you say the namespace split is below ld.so, then the inner namespace must
load it's own loader, and handle everything on it's own. This is possible,
but means that outer threads *must never* call into the inner namespace,
and that limits the usefullness to a kind of client<->server model where
shared memory must be used across the namespaces. Your proposal in no way
limits the choice of eventually having this kind of model.

Astute readers might argue that if the inner and outer ld.so's agreed on
an API/ABI then the thread *could* cross the boundary and retain a coherent
TLS storage. Florian Weimer has been discussing something like this, which
might let users replace ld.so with their own instrumented version to support
broader use case models for debugging or more complex auditing than LD_AUDIT
supports.

It's clear to me that dlmopen is *most* useful when threads can cross the
boundary of the dlmopen call, and in order to do that the thread and all
the associated thread-local data must be coherent between the outer
namespace and the inner namespace. Here we are proposing that dlmopen is
for all shared libraries *above* glibc. This model also does not limit
making the split lower at a later date and using a different flag. Having
the namespace split exist just beyond glibc makes the most sense for the
average user.

Note that LD_AUDIT actually implements a third split position, just above
ld.so, and so it uses the same dynamic loader, but new versions of all
the other libraries.

So your proposal is really to put the split at just above glibc, or to
be more precision, above ld.so/libc.so.6/libpthread.so.0, which form a
coherent group that implements threads.

If I have that wrong, please correct me.

> Vivek Das Mohapatra (5):
>   bits/dlfcn.h: Declare and describe the dlmopen RTLD_SHARED flag
>   include/link.h: Update the link_map struct to allow clones
>   elf/dl-object.c: Implement a helper function to clone link_map entries
>   elf/dl-load.c, elf-dl-open.c: Implement RTLD_SHARED dlmopen cloning
>   elf/dl-fini.c: Handle cloned link_map entries in the shutdown path
> 
>  bits/dlfcn.h               |  7 +++++
>  elf/dl-fini.c              | 50 ++++++++++++++++++++++++++++++
>  elf/dl-load.c              | 34 ++++++++++++++++++++
>  elf/dl-object.c            | 77 ++++++++++++++++++++++++++++++++++++++++++++++
>  elf/dl-open.c              | 31 +++++++++++++++++--
>  include/link.h             |  6 ++--
>  sysdeps/generic/ldsodefs.h |  6 ++++
>  7 files changed, 207 insertions(+), 4 deletions(-)
> 


-- 
Cheers,
Carlos.

Re: [RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[Vivek Das Mohapatra]

> I think the basic design question is "Where does the namespace split exist?"

> If you say the namespace split is below ld.so, then the inner namespace must
> load it's own loader, and handle everything on it's own. This is possible,
> but means that outer threads *must never* call into the inner namespace,
> and that limits the usefullness to a kind of client<->server model where
> shared memory must be used across the namespaces. Your proposal in no way
> limits the choice of eventually having this kind of model.

Right, I've not touched anything down at that level at all.

> Astute readers might argue that if the inner and outer ld.so's agreed on
> an API/ABI then the thread *could* cross the boundary and retain a coherent
> TLS storage. Florian Weimer has been discussing something like this, which
> might let users replace ld.so with their own instrumented version to support
> broader use case models for debugging or more complex auditing than LD_AUDIT
> supports.

Indeed - although I suspect a _lot_ of careful thought about what does
and doesn't have to be agreed on would need to be done - even with the
same libc* mapped into each namespace separately you can get deadlocks
(like the bug that triggered all this).

*where libc = the core group of glibc DSOs

> It's clear to me that dlmopen is *most* useful when threads can cross the
> boundary of the dlmopen call, and in order to do that the thread and all
> the associated thread-local data must be coherent between the outer
> namespace and the inner namespace. Here we are proposing that dlmopen is
> for all shared libraries *above* glibc. This model also does not limit
> making the split lower at a later date and using a different flag. Having
> the namespace split exist just beyond glibc makes the most sense for the
> average user.

Agreed.

> Note that LD_AUDIT actually implements a third split position, just above
> ld.so, and so it uses the same dynamic loader, but new versions of all
> the other libraries.

I had noticed this, and my implementation is pretty much that mechanism
on [a small dose of] steroids.

> So your proposal is really to put the split at just above glibc, or to
> be more precision, above ld.so/libc.so.6/libpthread.so.0, which form a
> coherent group that implements threads.
>
> If I have that wrong, please correct me.

That all sounds right.

Further things I should mention:

  - I was wondering if anything other than the core cluster mentioned
    above needed to be shared.

  - On a semi related note: The runtime-isolation work that triggered
    the original project needs to capture the whole of the libc cluster:
    would you be averse to there being some sort of mechanism that would
    allow a program to determine the list of .so files that constituted
    the GLIBC installation? Could be API returning a list of sonames, or
    a canonical manifest file would probably also do.

Re: [RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[Carlos O'Donell]

On 05/18/2018 03:05 PM, Vivek Das Mohapatra wrote:
> Further things I should mention:
> 
>  - I was wondering if anything other than the core cluster mentioned
>    above needed to be shared.

I was going to talk about this, but forgot.

You must group all libraries that use GLIBC_PRIVATE interfaces into
a cluster, otherwise you risk mix-matched versions of glibc attempting
to communicate with each other via non-existant or incorrect versions
of the unversioned GLIBC_PRIVATE APIs.

In truth this turns out to be possible in *all* of the shared objects
installed by the implementation. So I think this is really going to have
to be "namespace split *above* glibc" and *everything* in glibc is shared
among all the namespaces.

>  - On a semi related note: The runtime-isolation work that triggered
>    the original project needs to capture the whole of the libc cluster:
>    would you be averse to there being some sort of mechanism that would
>    allow a program to determine the list of .so files that constituted
>    the GLIBC installation? Could be API returning a list of sonames, or
>    a canonical manifest file would probably also do.

I think we want a markup approach to this.

Define a new DT_LMNS and set it to a string value "$PROXY" if the object
should be part of the global set of loaded objects which are loaded into
the base namespace and proxied into all other namespaces.

Likewise we could say DT_LMNS can be equal to a string that represents
a unique namespace, and any other objects with the same string will be
loaded into the same namespace. With all strings starting with "$" being
reserved.

We need to fix binutils to add DT_LMNS, and set it via a new command line
flag, and then build all glibc SOs with DT_LMNS set to "$PROXY" so they
appear in all link namespaces.

Now we have a few good win/win scenarios:

* You can now force objects into a dlmopen namespace even if you link
  directly with them by setting DT_LMNS to a value other than $.*
  You would have to look these objects up to use them via a namespaced
  dl_iterate_phtr?

* You can add new objects to $PROXY if you want them to be exposed
  through all of the namespaces too.

Thoughts?

-- 
Cheers,
Carlos.

Re: [RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[Vivek Das Mohapatra]

> Now we have a few good win/win scenarios:
>
> * You can now force objects into a dlmopen namespace even if you link
>  directly with them by setting DT_LMNS to a value other than $.*
>  You would have to look these objects up to use them via a namespaced
>  dl_iterate_phtr?
>
> * You can add new objects to $PROXY if you want them to be exposed
>  through all of the namespaces too.
>
> Thoughts?

  - namespaced dl_iterate_phdr would make my life easier, so thumbs up.

  - need to refresh my memory regarding dl_map_object - I think we'd need
    to harvest this info in _dl_map_object_from_fd and set a flag in
    the struct for easy checking later (or maybe keep a list of
    must-proxy objects, sort of analogous to how RTLD_GLOBAL objects
    are tracked, I guess).

Which reminds me - the code currently has a comment in it that says
RTLD_GLOBAL is nonsensical for namespaces but this isn't exactly true:
I think it makes sense for RTLD_GLOBAL to mean "use this for everything
in the target namesapace" (Mesa libGL for example RTLD_GLOBAL dlopens
_itself_ to export symbols to modules it is about to open, which I have
to trap when isolating libGL).

In effect, the $PROXY DT_LMNS DSOs become super-global (Solar?)
and RTLD_GLOBAL is extended to mean "for this namespace".

Re: [RFC][PATCH v1 0/5] Proof-of-Concept implementation of RTLD_SHARED for dlmopen

[Carlos O'Donell]

On 05/18/2018 03:53 PM, Vivek Das Mohapatra wrote:
>> Now we have a few good win/win scenarios:
>>
>> * You can now force objects into a dlmopen namespace even if you link
>>  directly with them by setting DT_LMNS to a value other than $.*
>>  You would have to look these objects up to use them via a namespaced
>>  dl_iterate_phtr?
>>
>> * You can add new objects to $PROXY if you want them to be exposed
>>  through all of the namespaces too.
>>
>> Thoughts?
> 
>  - namespaced dl_iterate_phdr would make my life easier, so thumbs up.
> 
>  - need to refresh my memory regarding dl_map_object - I think we'd need
>    to harvest this info in _dl_map_object_from_fd and set a flag in
>    the struct for easy checking later (or maybe keep a list of
>    must-proxy objects, sort of analogous to how RTLD_GLOBAL objects
>    are tracked, I guess).
> 
> Which reminds me - the code currently has a comment in it that says
> RTLD_GLOBAL is nonsensical for namespaces but this isn't exactly true:
> I think it makes sense for RTLD_GLOBAL to mean "use this for everything
> in the target namesapace" (Mesa libGL for example RTLD_GLOBAL dlopens
> _itself_ to export symbols to modules it is about to open, which I have
> to trap when isolating libGL).

I agree 100%.

The use of RTLD_GLOBAL is *absolutely* critical and it must be interpreted
to mean "GLOBAL within the namespace" as you imply.

There may be objects which use RTLD_GLOBAL which you cannot change but must
be able to load safely in a namespace.

> In effect, the $PROXY DT_LMNS DSOs become super-global (Solar?)
> and RTLD_GLOBAL is extended to mean "for this namespace".

Right. I think it's a bad design to allow objects to break out of the namespace
in a dynamic way, so I do not think we need RTLD_SUPER_GLOBAL which means to
add symbols to the base namespace.

However, I think that a compile-time, verifiable, DT_LMNS tag in .dynamic, can
be audited and verified from a security perspective to know that it will be
proxied to all namespaces.

Lastly we need test cases for things like using RTLD_GLOBAL within a namespace,
and using RTLD_SHARED, and nested dlopen within dlmopen, etc. etc. So you have
your work cutout, but I can probably help write some more test cases :-)

-- 
Cheers,
Carlos.