Re: [PATCH v4 2/2] x86: Update _dl_tlsdesc_dynamic to preserve caller-saved registers

["H.J. Lu" <hjl.tools@gmail.com>]

On Fri, Feb 16, 2024 at 5:24 AM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 16/02/24 09:58, H.J. Lu wrote:
> > On Fri, Feb 16, 2024 at 4:47 AM Adhemerval Zanella Netto
> > <adhemerval.zanella@linaro.org> wrote:
> >>
> >>
> >>
> >> On 16/02/24 09:37, H.J. Lu wrote:
> >>> On Fri, Feb 16, 2024 at 4:20 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>>>
> >>>> On Fri, Feb 16, 2024 at 4:18 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>>>>
> >>>>> * H. J. Lu:
> >>>>>
> >>>>>> On Thu, Feb 15, 2024 at 10:23 PM Florian Weimer <fweimer@redhat.com> wrote:
> >>>>>>>
> >>>>>>> * Adhemerval Zanella Netto:
> >>>>>>>
> >>>>>>>>> diff --git a/elf/malloc-for-test.map b/elf/malloc-for-test.map
> >>>>>>>>> new file mode 100644
> >>>>>>>>> index 0000000000..8437cf4346
> >>>>>>>>> --- /dev/null
> >>>>>>>>> +++ b/elf/malloc-for-test.map
> >>>>>>>>> @@ -0,0 +1,6 @@
> >>>>>>>>> +GLIBC_2.0 {
> >>>>>>>>
> >>>>>>>> You need to use the correct version to override the malloc:
> >>>>>>>>
> >>>>>>>> $ gdb --args tst-gnu2-tls2 --direct
> >>>>>>>> [...]
> >>>>>>>> (gdb) b apply_tls
> >>>>>>>> (gdb) r
> >>>>>>>> Thread 2 "tst-gnu2-tls2" hit Breakpoint 1, apply_tls (p=0x7ffff7bfee80) at tst-gnu2-tls2mod1.c:25
> >>>>>>>> 25 {
> >>>>>>>> (gdb) b malloc
> >>>>>>>> Breakpoint 2 at 0x7ffff7ca8ad0: malloc. (3 locations)
> >>>>>>>> (gdb) c
> >>>>>>>> Continuing.
> >>>>>>>
> >>>>>>> Why do we need to set a symbol version here?  I think this can be
> >>>>>>> removed.
> >>>>>>
> >>>>>> malloc-for-test.so provides a fake malloc to clobber caller-save
> >>>>>> registers.  Since malloc in ld.so has a symbol version,
> >>>>>>
> >>>>>>   struct r_found_version version;
> >>>>>>   version.name = symbol_version_string (libc, GLIBC_2_0);
> >>>>>>   version.hidden = 0;
> >>>>>>   version.hash = _dl_elf_hash (version.name);
> >>>>>>   version.filename = NULL;
> >>>>>>
> >>>>>>   void *new_calloc = lookup_malloc_symbol (main_map, "calloc", &version);
> >>>>>>   void *new_free = lookup_malloc_symbol (main_map, "free", &version);
> >>>>>>   void *new_malloc = lookup_malloc_symbol (main_map, "malloc", &version);
> >>>>>>   void *new_realloc = lookup_malloc_symbol (main_map, "realloc", &version);
> >>>>>>
> >>>>>> malloc in malloc-for-test.so must have the same version.  Otherwise,
> >>>>>> it won't be used in the test.
> >>>>>
> >>>>> I thought that unversioned symbols interpose all versioned symbols.  Has
> >>>>> this changed in the dynamic linker?
> >>>>
> >>>> Only for this case.
> >>>>
> >>>
> >>> I was wrong.  Version isn't needed.  Will fix it.
> >>>
> >>
> >> Indeed, but I think we also improve the register check testing since just clobbering
> >> on malloc does not really provide much coverage.
> >
> > Why?  The slow path will always call malloc in malloc-for-test.so.
> > We can clobber any caller-save registers we need.  Without
> > malloc-for-test.so, the test doesn't fail for i386 since malloc in libc.so
> > doesn't use any vector registers.
> >
>
> The idea is to check whether _dl_tlsdesc_dynamic call does preserve any
> possible caller-saved register and since this test is generic and afaik
> all affected ABIs follow the same idea (compiler won't save/restore such
> register), clobbering the register will only trigger a possible issue
> iff the thread that actually issue the TLS usage does actually use any
> possible register.
>
> That's why I think a better coverage would to also clobber the register
> before the TLS access, and check if their values does not change over
> the TLS access.

How about this

struct tls *
apply_tls (struct tls *p)
{
  tls_var1[1] = *p;
  BEFORE_TLSDESC_CALL ();
  struct tls *ret = &tls_var1[1];
  AFTER_TLSDESC_CALL ();
  return ret;
}

An architecture can define BEFORE_TLSDESC_CALL and
tAFTER_TLSDESC_CALL o verify that clobber caller-saved
registers aren't changed by the implicit TLSDESC call.


H.J.