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

  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(-)
  

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.


>  }
>  
>  
>
  

> 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.