[RFC,v8,14/20] When loading DSOs into alternate namespaces check for DT_GNU_UNIQUE

  If a DSO has not already been loaded and the target is not the main
namespace then we must check to see if it's been DT_GNU_UNIQUE tagged
and load it into the main namespace instead.

dl_open_worker has alread been modified to notice the discrepancy
between the request and the result in such cases, and will set up
a proxy in the target namespace.
---
 elf/dl-load.c | 96 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 82 insertions(+), 14 deletions(-)
  

On 09/02/2021 14:18, Vivek Das Mohapatra via Libc-alpha wrote:
> If a DSO has not already been loaded and the target is not the main
> namespace then we must check to see if it's been DT_GNU_UNIQUE tagged
> and load it into the main namespace instead.
> 
> dl_open_worker has alread been modified to notice the discrepancy
> between the request and the result in such cases, and will set up
> a proxy in the target namespace.
> ---
>  elf/dl-load.c | 96 +++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 82 insertions(+), 14 deletions(-)
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 13ac2053b8..13879af82c 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -837,6 +837,62 @@ _dl_init_paths (const char *llp, const char *source,
>      __rtld_env_path_list.dirs = (void *) -1;
>  }
>  
> +static ElfW(Word)
> +_has_gnu_unique (int fd, const ElfW(Ehdr) *header, const ElfW(Phdr) *phdr)

I think it make sense to make it return a 'bool' and there is no need to
prepend the function name '_' since it is static.

> +{
> +  int unique = 0;
> +  const ElfW(Phdr) *ph;
> +  ElfW(Dyn) entry = {};

It seems the only field really required to be initialized is d_tag.

> +  off_t reset;
> +  off_t pos;
> +  off_t end;
> +
> +  reset = __lseek (fd, 0, SEEK_CUR);

I think newer code should not use non-LFS interfaces, it means replace
off_t with off64_t and __lseek with __lseek64.

> +
> +  for (ph = phdr; ph < &phdr[header->e_phnum]; ++ph)
> +    {
> +      switch (ph->p_type)
> +	{
> +        case PT_DYNAMIC:
> +          pos = __lseek (fd, ph->p_offset, SEEK_SET);

Same comment as before regarding non-LFS interfaces.

> +          end = pos + ph->p_filesz;
> +
> +          while (pos < end)
> +            {
> +              ssize_t rb = 0;
> +              do
> +                {
> +                  ssize_t rretl = __read_nocancel (fd, &entry + rb,
> +                                                   sizeof (ElfW(Dyn)) - rb);
> +                  if (rretl <= 0)
> +                    goto cleanup;
> +
> +                  rb += rretl;
> +                }
> +              while (__glibc_unlikely (rb < sizeof (ElfW(Dyn))));

Other occurrences on loader code does not handle partial read, so I think
it would be better to follow current practice.  Also use __pread64_nocancel
instead, it allows to remove the two __lseek calls.  


> +
> +              switch (entry.d_tag)
> +                {
> +                case DT_GNU_FLAGS_1:
> +                  unique = entry.d_un.d_val & DF_GNU_1_UNIQUE;

Add a /* fallthrough */ comment here.  I will send a patch to add the
FALLTHROUGH macro to at least on internal headers (and my on cdefs.h)
so we might use instead of the multiple different comment variants used
by  -Wimplicit-fallthrough. 

> +                case DT_NULL:
> +                  goto cleanup;
> +                  break;
> +                default:
> +                  break;
> +                }
> +              pos += rb;
> +            }
> +          break;
> +        }
> +    }
> +
> + cleanup:
> +  /* Put the file descriptor offset back where it was when we were called.  */
> +  __lseek (fd, reset, SEEK_SET);
> +
> +  return unique;
> +}
>  
>  /* Process PT_GNU_PROPERTY program header PH in module L after
>     PT_LOAD segments are mapped.  Only one NT_GNU_PROPERTY_TYPE_0
> @@ -1098,6 +1154,32 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>    else
>      assert (r->r_state == RT_ADD);
>  
> +  /* Load the ELF header using preallocated struct space if it's big enough.  */
> +  maplength = header->e_phnum * sizeof (ElfW(Phdr));
> +  if (header->e_phoff + maplength <= (size_t) fbp->len)
> +    phdr = (void *) (fbp->buf + header->e_phoff);
> +  else
> +    {
> +      phdr = alloca (maplength);
> +      if ((size_t) __pread64_nocancel (fd, (void *) phdr, maplength,
> +				       header->e_phoff) != maplength)
> +	{
> +	  errstring = N_("cannot read file data");
> +	  goto lose_errno;
> +	}
> +    }
> +
> +  /* We need to check for DT_GNU_FLAGS_1/DF_GNU_1_UNIQUE before we start
> +     initialising any namespace dependent metatada.  */
> +  if (nsid != LM_ID_BASE)
> +    {
> +      /* Target DSO is flagged as unique: Make sure it gets loaded into
> +         the base namespace. It is up to our caller to generate a proxy in

Double space after period.

> +         the target nsid.  */
> +      if (_has_gnu_unique (fd, header, phdr))
> +        nsid = LM_ID_BASE;
> +    }
> +
>    /* Enter the new object in the list of loaded objects.  */
>    l = _dl_new_object (realname, name, l_type, loader, mode, nsid);
>    if (__glibc_unlikely (l == NULL))
> @@ -1115,20 +1197,6 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>    type = header->e_type;
>    l->l_phnum = header->e_phnum;
>  
> -  maplength = header->e_phnum * sizeof (ElfW(Phdr));
> -  if (header->e_phoff + maplength <= (size_t) fbp->len)
> -    phdr = (void *) (fbp->buf + header->e_phoff);
> -  else
> -    {
> -      phdr = alloca (maplength);
> -      if ((size_t) __pread64_nocancel (fd, (void *) phdr, maplength,
> -				       header->e_phoff) != maplength)
> -	{
> -	  errstring = N_("cannot read file data");
> -	  goto lose_errno;
> -	}
> -    }
> -
>     /* On most platforms presume that PT_GNU_STACK is absent and the stack is
>      * executable.  Other platforms default to a nonexecutable stack and don't
>      * need PT_GNU_STACK to do so.  */
>