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

[Carlos O'Donell <carlos at redhat dot com>]

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.