Re: [PATCH 11/15] x86_64: Avoid lazy relocation of tlsdesc [BZ #27137]

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

The 04/08/2021 17:14, Ben Woodard wrote:
> Don’t you also need to modify elf_machine_runtime_setup It also has a reference to _dl_tlsdesc_resolve_rela that becomes undefined when you try to compile with your patchset including patch 13 where you remove the code.
> 
> To make a test build I just commented it out but I think that this patch should remove that if statement as well.

thanks,
indeed this was wrong, i tested the wrong branch on x86_64.

i will fix this and post a v2 set with the other feedback.

> 
> diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
> index 9a876a371e..2b1b36a739 100644
> --- a/sysdeps/x86_64/dl-machine.h
> +++ b/sysdeps/x86_64/dl-machine.h
> @@ -127,9 +127,11 @@ elf_machine_runtime_setup (struct link_map *l, int lazy, int profile)
>         }
>      }
> 
> -  if (l->l_info[ADDRIDX (DT_TLSDESC_GOT)] && lazy)
> -    *(ElfW(Addr)*)(D_PTR (l, l_info[ADDRIDX (DT_TLSDESC_GOT)]) + l->l_addr)
> -      = (ElfW(Addr)) &_dl_tlsdesc_resolve_rela;
> +  /* Lazy binding of TLSDESC relocations is no longer done so this logic
> +     won't apply */
> +  /* if (l->l_info[ADDRIDX (DT_TLSDESC_GOT)] && lazy) */
> +  /*   *(ElfW(Addr)*)(D_PTR (l, l_info[ADDRIDX (DT_TLSDESC_GOT)]) + l->l_addr) */
> +  /*     = (ElfW(Addr)) &_dl_tlsdesc_resolve_rela; */
> 
>    return lazy;
>  }
> 
> 
> > On Feb 15, 2021, at 4:02 AM, Szabolcs Nagy via Libc-alpha <libc-alpha@sourceware.org> wrote:
> > 
> > Lazy tlsdesc relocation is racy because the static tls optimization and
> > tlsdesc management operations are done without holding the dlopen lock.
> > 
> > This similar to the commit b7cf203b5c17dd6d9878537d41e0c7cc3d270a67
> > for aarch64, but it fixes a different race: bug 27137.
> > ---
> > sysdeps/x86_64/dl-machine.h | 19 ++++++++++++++-----
> > 1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
> > index 103eee6c3f..9a876a371e 100644
> > --- a/sysdeps/x86_64/dl-machine.h
> > +++ b/sysdeps/x86_64/dl-machine.h
> > @@ -570,12 +570,21 @@ elf_machine_lazy_rel (struct link_map *map,
> >     }
> >   else if (__glibc_likely (r_type == R_X86_64_TLSDESC))
> >     {
> > -      struct tlsdesc volatile * __attribute__((__unused__)) td =
> > -	(struct tlsdesc volatile *)reloc_addr;
> > +      const Elf_Symndx symndx = ELFW (R_SYM) (reloc->r_info);
> > +      const ElfW (Sym) *symtab = (const void *)D_PTR (map, l_info[DT_SYMTAB]);
> > +      const ElfW (Sym) *sym = &symtab[symndx];
> > +      const struct r_found_version *version = NULL;
> > 
> > -      td->arg = (void*)reloc;
> > -      td->entry = (void*)(D_PTR (map, l_info[ADDRIDX (DT_TLSDESC_PLT)])
> > -			  + map->l_addr);
> > +      if (map->l_info[VERSYMIDX (DT_VERSYM)] != NULL)
> > +	{
> > +	  const ElfW (Half) *vernum =
> > +	    (const void *)D_PTR (map, l_info[VERSYMIDX (DT_VERSYM)]);
> > +	  version = &map->l_versions[vernum[symndx] & 0x7fff];
> > +	}
> > +
> > +      /* Always initialize TLS descriptors completely at load time, in
> > +	 case static TLS is allocated for it that requires locking.  */
> > +      elf_machine_rela (map, reloc, sym, version, reloc_addr, skip_ifunc);
> >     }
> >   else if (__glibc_unlikely (r_type == R_X86_64_IRELATIVE))
> >     {
> > -- 
> > 2.17.1
> > 
> 

--