Message ID | 20210709135001.505521-1-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | [v3] elf: Fix DTV gap reuse logic (BZ #27135) | expand |
The 07/09/2021 10:50, Adhemerval Zanella wrote: > Changes from previous version: > > - Fix commit message and add a line about the bug fixes. > - Use atomic operation while setting the slotinfo. > - Use test_verbose on tst-tls20.c. > > --- > > This is updated version of the 572bd547d57a (reverted by 40ebfd016ad2) > that fixes the _dl_next_tls_modid issues. > > This issue with 572bd547d57a patch is the DTV entry will be only > update on dl_open_worker() with the update_tls_slotinfo() call after > all dependencies are being processed by _dl_map_object_deps(). However > _dl_map_object_deps() itself might call _dl_next_tls_modid(), and since > the _dl_tls_dtv_slotinfo_list::map is not yet set the entry will be > wrongly reused. > > This patch fixes by renaming the _dl_next_tls_modid() function to > _dl_assign_tls_modid() and by passing the link_map so it can set > the slotinfo value so a so subsequente _dl_next_tls_modid() call will > see the entry as allocated. this paragraph still has 'so a so subsequente' and i would add the bug number into the first sentence. > > The intermediary value is cleared up on remove_slotinfo() for the case > a library fails to load with RTLD_NOW. > > This patch fixes BZ #27135. > > Checked on x86_64-linux-gnu. the patch looks ok to me, with the commit message and the comment issue below fixed. Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com> > + > +/* The following test check DTV gaps handling with shared libraries that has > + dependencies. It defines 5 different sets: > + > + 1. Single dependency: > + mod0 -> mod1 > + 2. Double dependency: > + mod2 -> [mod3,mod4] > + 3. Double dependency with each dependency depent of another module: > + mod5 -> [mod6,mod7] -> mod8 > + 4. Long chain with one double dependency in the middle: > + mod9 -> [mod10, mod11] -> mod12 -> mod13 > + 5. Long chain with two double depedencies in the middle: > + mod15 -> mod15 -> [mod16, mod17] > + mod15 -> [mod18, mod19] mod14 -> ... > + > + This does not cover all the possible gaps and configuration, but it > + should check if different dynamic shared sets are placed correctly in > + different gaps configurations. */ > + > +static int > +nmodules (uint32_t v) > +{ > + unsigned int r = 0; > + while (v >>= 1) > + r++; > + return r + 1; > +} > + > +static inline bool > +is_mod_set (uint32_t g, uint32_t n) > +{ > + return (1U << (n - 1)) & g; > +} > + > +static void > +print_gap (uint32_t g) > +{ > + if (!test_verbose) > + return; > + printf ("gap: "); > + int nmods = nmodules (g); > + for (int n = 1; n <= nmods; n++) > + printf ("%c", ((1 << (n - 1)) & g) == 0 ? 'G' : 'M'); > + printf ("\n"); > +} > + > +static void > +do_test_dependency (void) > +{ > + /* Maps the module and its dependencies, use thread to access the TLS on > + each loaded module. */ > + static const int tlsmanydeps0[] = { 1 }; > + static const int tlsmanydeps1[] = { 3, 4 }; > + static const int tlsmanydeps2[] = { 6, 7, 8 }; > + static const int tlsmanydeps3[] = { 10, 11, 12 }; > + static const int tlsmanydeps4[] = { 15, 16, 17, 18, 19 }; > + static const struct tlsmanydeps_t > + { > + int modi; > + int ndeps; > + const int *deps; > + } tlsmanydeps[] = > + { > + { 0, array_length (tlsmanydeps0), tlsmanydeps0 }, > + { 2, array_length (tlsmanydeps1), tlsmanydeps1 }, > + { 5, array_length (tlsmanydeps2), tlsmanydeps2 }, > + { 9, array_length (tlsmanydeps3), tlsmanydeps3 }, > + { 14, array_length (tlsmanydeps4), tlsmanydeps4 }, > + };
On 7/9/21 9:50 AM, Adhemerval Zanella via Libc-alpha wrote: > Changes from previous version: > > - Fix commit message and add a line about the bug fixes. > - Use atomic operation while setting the slotinfo. > - Use test_verbose on tst-tls20.c. FYI: dj/TryBot-32bit fail Patch caused testsuite regressions PASS -> FAIL : malloc/tst-safe-linking > --- > > This is updated version of the 572bd547d57a (reverted by 40ebfd016ad2) > that fixes the _dl_next_tls_modid issues. > > This issue with 572bd547d57a patch is the DTV entry will be only > update on dl_open_worker() with the update_tls_slotinfo() call after > all dependencies are being processed by _dl_map_object_deps(). However > _dl_map_object_deps() itself might call _dl_next_tls_modid(), and since > the _dl_tls_dtv_slotinfo_list::map is not yet set the entry will be > wrongly reused. > > This patch fixes by renaming the _dl_next_tls_modid() function to > _dl_assign_tls_modid() and by passing the link_map so it can set > the slotinfo value so a so subsequente _dl_next_tls_modid() call will > see the entry as allocated. > > The intermediary value is cleared up on remove_slotinfo() for the case > a library fails to load with RTLD_NOW. > > This patch fixes BZ #27135. > > Checked on x86_64-linux-gnu. > --- > elf/Makefile | 64 ++++++++- > elf/dl-close.c | 8 +- > elf/dl-load.c | 2 +- > elf/dl-open.c | 10 -- > elf/dl-tls.c | 17 +-- > elf/rtld.c | 2 +- > elf/tst-tls20.c | 275 ++++++++++++++++++++++++++++++++++++- > sysdeps/generic/ldsodefs.h | 4 +- > 8 files changed, 349 insertions(+), 33 deletions(-) > > diff --git a/elf/Makefile b/elf/Makefile > index 4b320e8b3a..bdd5cc9e1a 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -253,6 +253,13 @@ one-hundred = $(foreach x,0 1 2 3 4 5 6 7 8 9, \ > 0$x 1$x 2$x 3$x 4$x 5$x 6$x 7$x 8$x 9$x) > tst-tls-many-dynamic-modules := \ > $(foreach n,$(one-hundred),tst-tls-manydynamic$(n)mod) > +tst-tls-many-dynamic-modules-dep-suffixes = 0 1 2 3 4 5 6 7 8 9 10 11 12 13 \ > + 14 15 16 17 18 19 > +tst-tls-many-dynamic-modules-dep = \ > + $(foreach n,$(tst-tls-many-dynamic-modules-dep-suffixes),tst-tls-manydynamic$(n)mod-dep) > +tst-tls-many-dynamic-modules-dep-bad-suffixes = 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 > +tst-tls-many-dynamic-modules-dep-bad = \ > + $(foreach n,$(tst-tls-many-dynamic-modules-dep-bad-suffixes),tst-tls-manydynamic$(n)mod-dep-bad) > extra-test-objs += $(tlsmod17a-modules:=.os) $(tlsmod18a-modules:=.os) \ > tst-tlsalign-vars.o > test-extras += tst-tlsmod17a tst-tlsmod18a tst-tlsalign-vars > @@ -325,6 +332,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \ > tst-audit11mod1 tst-audit11mod2 tst-auditmod11 \ > tst-audit12mod1 tst-audit12mod2 tst-audit12mod3 tst-auditmod12 \ > tst-latepthreadmod $(tst-tls-many-dynamic-modules) \ > + $(tst-tls-many-dynamic-modules-dep) \ > + $(tst-tls-many-dynamic-modules-dep-bad) \ > tst-nodelete-dlclose-dso tst-nodelete-dlclose-plugin \ > tst-main1mod tst-absolute-sym-lib \ > tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \ > @@ -1812,10 +1821,63 @@ $(objpfx)tst-rtld-help.out: $(objpfx)ld.so > $(evaluate-test) > > # Reuses tst-tls-many-dynamic-modules > +$(patsubst %,$(objpfx)%.os,$(tst-tls-many-dynamic-modules-dep)): \ > + $(objpfx)tst-tls-manydynamic%mod-dep.os : tst-tls-manydynamicmod.c > + $(compile-command.c) \ > + -DNAME=tls_global_$* -DSETTER=set_value_$* -DGETTER=get_value_$* > +$(patsubst %,$(objpfx)%.os,$(tst-tls-many-dynamic-modules-dep-bad)): \ > + $(objpfx)tst-tls-manydynamic%mod-dep-bad.os : tst-tls-manydynamicmod.c > + $(compile-command.c) \ > + -DNAME=tls_global_$* -DSETTER=set_value_$* -DGETTER=get_value_$* > tst-tls20mod-bad.so-no-z-defs = yes > +# Single dependency. > +$(objpfx)tst-tls-manydynamic0mod-dep.so: $(objpfx)tst-tls-manydynamic1mod-dep.so > +# Double dependencies. > +$(objpfx)tst-tls-manydynamic2mod-dep.so: $(objpfx)tst-tls-manydynamic3mod-dep.so \ > + $(objpfx)tst-tls-manydynamic4mod-dep.so > +# Double dependencies with each dependency depent of another module. > +$(objpfx)tst-tls-manydynamic5mod-dep.so: $(objpfx)tst-tls-manydynamic6mod-dep.so \ > + $(objpfx)tst-tls-manydynamic7mod-dep.so > +$(objpfx)tst-tls-manydynamic6mod-dep.so: $(objpfx)tst-tls-manydynamic8mod-dep.so > +$(objpfx)tst-tls-manydynamic7mod-dep.so: $(objpfx)tst-tls-manydynamic8mod-dep.so > +# Long chain with one double dependency in the middle > +$(objpfx)tst-tls-manydynamic9mod-dep.so: $(objpfx)tst-tls-manydynamic10mod-dep.so \ > + $(objpfx)tst-tls-manydynamic11mod-dep.so > +$(objpfx)tst-tls-manydynamic10mod-dep.so: $(objpfx)tst-tls-manydynamic12mod-dep.so > +$(objpfx)tst-tls-manydynamic12mod-dep.so: $(objpfx)tst-tls-manydynamic13mod-dep.so > +# Long chain with two double depedencies in the middle > +$(objpfx)tst-tls-manydynamic14mod-dep.so: $(objpfx)tst-tls-manydynamic15mod-dep.so > +$(objpfx)tst-tls-manydynamic15mod-dep.so: $(objpfx)tst-tls-manydynamic16mod-dep.so \ > + $(objpfx)tst-tls-manydynamic17mod-dep.so > +$(objpfx)tst-tls-manydynamic16mod-dep.so: $(objpfx)tst-tls-manydynamic18mod-dep.so \ > + $(objpfx)tst-tls-manydynamic19mod-dep.so > +# Same but with an invalid module. > +# Single dependency. > +$(objpfx)tst-tls-manydynamic0mod-dep-bad.so: $(objpfx)tst-tls20mod-bad.so > +# Double dependencies. > +$(objpfx)tst-tls-manydynamic1mod-dep-bad.so: $(objpfx)tst-tls-manydynamic2mod-dep-bad.so \ > + $(objpfx)tst-tls20mod-bad.so > +# Double dependencies with each dependency depent of another module. > +$(objpfx)tst-tls-manydynamic3mod-dep-bad.so: $(objpfx)tst-tls-manydynamic4mod-dep-bad.so \ > + $(objpfx)tst-tls-manydynamic5mod-dep-bad.so > +$(objpfx)tst-tls-manydynamic4mod-dep-bad.so: $(objpfx)tst-tls20mod-bad.so > +$(objpfx)tst-tls-manydynamic5mod-dep-bad.so: $(objpfx)tst-tls20mod-bad.so > +# Long chain with one double dependency in the middle > +$(objpfx)tst-tls-manydynamic6mod-dep-bad.so: $(objpfx)tst-tls-manydynamic7mod-dep-bad.so \ > + $(objpfx)tst-tls-manydynamic8mod-dep-bad.so > +$(objpfx)tst-tls-manydynamic7mod-dep-bad.so: $(objpfx)tst-tls-manydynamic9mod-dep-bad.so > +$(objpfx)tst-tls-manydynamic9mod-dep-bad.so: $(objpfx)tst-tls20mod-bad.so > +# Long chain with two double depedencies in the middle > +$(objpfx)tst-tls-manydynamic10mod-dep-bad.so: $(objpfx)tst-tls-manydynamic11mod-dep-bad.so > +$(objpfx)tst-tls-manydynamic11mod-dep-bad.so: $(objpfx)tst-tls-manydynamic12mod-dep-bad.so \ > + $(objpfx)tst-tls-manydynamic13mod-dep-bad.so > +$(objpfx)tst-tls-manydynamic12mod-dep-bad.so: $(objpfx)tst-tls-manydynamic14mod-dep-bad.so \ > + $(objpfx)tst-tls20mod-bad.so > $(objpfx)tst-tls20: $(shared-thread-library) > $(objpfx)tst-tls20.out: $(objpfx)tst-tls20mod-bad.so \ > - $(tst-tls-many-dynamic-modules:%=$(objpfx)%.so) > + $(tst-tls-many-dynamic-modules:%=$(objpfx)%.so) \ > + $(tst-tls-many-dynamic-modules-dep:%=$(objpfx)%.so) \ > + $(tst-tls-many-dynamic-modules-dep-bad:%=$(objpfx)%.so) \ > > # Reuses tst-tls-many-dynamic-modules > $(objpfx)tst-tls21: $(shared-thread-library) > diff --git a/elf/dl-close.c b/elf/dl-close.c > index 3720e47dd1..f39001cab9 100644 > --- a/elf/dl-close.c > +++ b/elf/dl-close.c > @@ -77,8 +77,6 @@ remove_slotinfo (size_t idx, struct dtv_slotinfo_list *listp, size_t disp, > object that wasn't fully set up. */ > if (__glibc_likely (old_map != NULL)) > { > - assert (old_map->l_tls_modid == idx); > - > /* Mark the entry as unused. These can be read concurrently. */ > atomic_store_relaxed (&listp->slotinfo[idx - disp].gen, > GL(dl_tls_generation) + 1); > @@ -88,7 +86,11 @@ remove_slotinfo (size_t idx, struct dtv_slotinfo_list *listp, size_t disp, > /* If this is not the last currently used entry no need to look > further. */ > if (idx != GL(dl_tls_max_dtv_idx)) > - return true; > + { > + /* There is an unused dtv entry in the middle. */ > + GL(dl_tls_dtv_gaps) = true; > + return true; > + } > } > > while (idx - disp > (disp == 0 ? 1 + GL(dl_tls_static_nelem) : 0)) > diff --git a/elf/dl-load.c b/elf/dl-load.c > index a08df001af..650e4edc35 100644 > --- a/elf/dl-load.c > +++ b/elf/dl-load.c > @@ -1498,7 +1498,7 @@ cannot enable executable stack as shared object requires"); > not set up TLS data structures, so don't use them now. */ > || __glibc_likely (GL(dl_tls_dtv_slotinfo_list) != NULL))) > /* Assign the next available module ID. */ > - l->l_tls_modid = _dl_next_tls_modid (); > + _dl_assign_tls_modid (l); > > #ifdef DL_AFTER_LOAD > DL_AFTER_LOAD (l); > diff --git a/elf/dl-open.c b/elf/dl-open.c > index a066f39bd0..d2240d8747 100644 > --- a/elf/dl-open.c > +++ b/elf/dl-open.c > @@ -899,16 +899,6 @@ no more namespaces available for dlmopen()")); > state if relocation failed, for example. */ > if (args.map) > { > - /* Maybe some of the modules which were loaded use TLS. > - Since it will be removed in the following _dl_close call > - we have to mark the dtv array as having gaps to fill the > - holes. This is a pessimistic assumption which won't hurt > - if not true. There is no need to do this when we are > - loading the auditing DSOs since TLS has not yet been set > - up. */ > - if ((mode & __RTLD_AUDIT) == 0) > - GL(dl_tls_dtv_gaps) = true; > - > _dl_close_worker (args.map, true); > > /* All l_nodelete_pending objects should have been deleted > diff --git a/elf/dl-tls.c b/elf/dl-tls.c > index 2b5161d10a..423e380f7c 100644 > --- a/elf/dl-tls.c > +++ b/elf/dl-tls.c > @@ -126,8 +126,8 @@ oom (void) > } > > > -size_t > -_dl_next_tls_modid (void) > +void > +_dl_assign_tls_modid (struct link_map *l) > { > size_t result; > > @@ -157,7 +157,11 @@ _dl_next_tls_modid (void) > } > > if (result - disp < runp->len) > - break; > + { > + /* Mark the entry as used, so any dependency see it. */ > + atomic_store_relaxed (&runp->slotinfo[result - disp].map, l); > + break; > + } > > disp += runp->len; > } > @@ -184,17 +188,14 @@ _dl_next_tls_modid (void) > atomic_store_relaxed (&GL(dl_tls_max_dtv_idx), result); > } > > - return result; > + l->l_tls_modid = result; > } > > > size_t > _dl_count_modids (void) > { > - /* It is rare that we have gaps; see elf/dl-open.c (_dl_open) where > - we fail to load a module and unload it leaving a gap. If we don't > - have gaps then the number of modids is the current maximum so > - return that. */ > + /* The count is the max unless dlclose or failed dlopen created gaps. */ > if (__glibc_likely (!GL(dl_tls_dtv_gaps))) > return GL(dl_tls_max_dtv_idx); > > diff --git a/elf/rtld.c b/elf/rtld.c > index fbbd60b446..160faaf5ab 100644 > --- a/elf/rtld.c > +++ b/elf/rtld.c > @@ -1722,7 +1722,7 @@ dl_main (const ElfW(Phdr) *phdr, > /* Add the dynamic linker to the TLS list if it also uses TLS. */ > if (GL(dl_rtld_map).l_tls_blocksize != 0) > /* Assign a module ID. Do this before loading any audit modules. */ > - GL(dl_rtld_map).l_tls_modid = _dl_next_tls_modid (); > + _dl_assign_tls_modid (&GL(dl_rtld_map)); > > audit_list_add_dynamic_tag (&state.audit_list, main_map, DT_AUDIT); > audit_list_add_dynamic_tag (&state.audit_list, main_map, DT_DEPAUDIT); > diff --git a/elf/tst-tls20.c b/elf/tst-tls20.c > index 9977ec8032..f0db2d1a27 100644 > --- a/elf/tst-tls20.c > +++ b/elf/tst-tls20.c > @@ -16,12 +16,14 @@ > License along with the GNU C Library; if not, see > <http://www.gnu.org/licenses/>. */ > > +#include <array_length.h> > #include <dlfcn.h> > #include <pthread.h> > #include <stdio.h> > #include <stdlib.h> > #include <support/check.h> > #include <support/support.h> > +#include <support/test-driver.h> > #include <support/xdlfcn.h> > #include <support/xthread.h> > > @@ -59,28 +61,75 @@ access (int i) > char *buf = xasprintf ("tls_global_%02d", i); > dlerror (); > int *p = dlsym (mod[i], buf); > - printf ("mod[%d]: &tls = %p\n", i, p); > + if (test_verbose) > + printf ("mod[%d]: &tls = %p\n", i, p); > if (p == NULL) > FAIL_EXIT1 ("dlsym failed: %s\n", dlerror ()); > + TEST_COMPARE (*p, 0); > ++*p; > free (buf); > } > > +static void > +access_mod (const char *modname, void *mod, int i) > +{ > + char *modsym = xasprintf ("tls_global_%d", i); > + dlerror (); > + int *p = dlsym (mod, modsym); > + if (test_verbose) > + printf ("%s: &tls = %p\n", modname, p); > + if (p == NULL) > + FAIL_EXIT1 ("dlsym failed: %s\n", dlerror ()); > + TEST_COMPARE (*p, 0); > + ++*p; > + free (modsym); > +} > + > +static void > +access_dep (int i) > +{ > + char *modname = xasprintf ("tst-tls-manydynamic%dmod-dep.so", i); > + void *moddep = xdlopen (modname, RTLD_LAZY); > + access_mod (modname, moddep, i); > + free (modname); > + xdlclose (moddep); > +} > + > +struct start_args > +{ > + const char *modname; > + void *mod; > + int modi; > + int ndeps; > + const int *deps; > +}; > + > static void * > start (void *a) > { > + struct start_args *args = a; > + > for (int i = 0; i < NMOD; i++) > if (mod[i] != NULL) > access (i); > + > + if (args != NULL) > + { > + access_mod (args->modname, args->mod, args->modi); > + for (int n = 0; n < args->ndeps; n++) > + access_dep (args->deps[n]); > + } > + > return 0; > } > > -static int > -do_test (void) > +/* This test gaps with shared libraries with dynamic TLS that has no > + dependencies. The DTV gap is set with by trying to load an invalid > + module, the entry should be used on the dlopen. */ > +static void > +do_test_no_depedency (void) > { > - int i; > - > - for (i = 0; i < NMOD; i++) > + for (int i = 0; i < NMOD; i++) > { > load_mod (i); > /* Bump the generation of mod[0] without using new dtv slot. */ > @@ -91,8 +140,220 @@ do_test (void) > pthread_t t = xpthread_create (0, start, 0); > xpthread_join (t); > } > - for (i = 0; i < NMOD; i++) > + for (int i = 0; i < NMOD; i++) > unload_mod (i); > +} > + > +/* The following test check DTV gaps handling with shared libraries that has > + dependencies. It defines 5 different sets: > + > + 1. Single dependency: > + mod0 -> mod1 > + 2. Double dependency: > + mod2 -> [mod3,mod4] > + 3. Double dependency with each dependency depent of another module: > + mod5 -> [mod6,mod7] -> mod8 > + 4. Long chain with one double dependency in the middle: > + mod9 -> [mod10, mod11] -> mod12 -> mod13 > + 5. Long chain with two double depedencies in the middle: > + mod15 -> mod15 -> [mod16, mod17] > + mod15 -> [mod18, mod19] > + > + This does not cover all the possible gaps and configuration, but it > + should check if different dynamic shared sets are placed correctly in > + different gaps configurations. */ > + > +static int > +nmodules (uint32_t v) > +{ > + unsigned int r = 0; > + while (v >>= 1) > + r++; > + return r + 1; > +} > + > +static inline bool > +is_mod_set (uint32_t g, uint32_t n) > +{ > + return (1U << (n - 1)) & g; > +} > + > +static void > +print_gap (uint32_t g) > +{ > + if (!test_verbose) > + return; > + printf ("gap: "); > + int nmods = nmodules (g); > + for (int n = 1; n <= nmods; n++) > + printf ("%c", ((1 << (n - 1)) & g) == 0 ? 'G' : 'M'); > + printf ("\n"); > +} > + > +static void > +do_test_dependency (void) > +{ > + /* Maps the module and its dependencies, use thread to access the TLS on > + each loaded module. */ > + static const int tlsmanydeps0[] = { 1 }; > + static const int tlsmanydeps1[] = { 3, 4 }; > + static const int tlsmanydeps2[] = { 6, 7, 8 }; > + static const int tlsmanydeps3[] = { 10, 11, 12 }; > + static const int tlsmanydeps4[] = { 15, 16, 17, 18, 19 }; > + static const struct tlsmanydeps_t > + { > + int modi; > + int ndeps; > + const int *deps; > + } tlsmanydeps[] = > + { > + { 0, array_length (tlsmanydeps0), tlsmanydeps0 }, > + { 2, array_length (tlsmanydeps1), tlsmanydeps1 }, > + { 5, array_length (tlsmanydeps2), tlsmanydeps2 }, > + { 9, array_length (tlsmanydeps3), tlsmanydeps3 }, > + { 14, array_length (tlsmanydeps4), tlsmanydeps4 }, > + }; > + > + /* The gap configuration is defined as a bitmap: the bit set represents a > + loaded module prior the tests execution, while a bit unsed is a module > + unloaded. Not all permtation will show gaps, but it is simpler than > + define each one independently. */ > + for (uint32_t g = 0; g < 64; g++) > + { > + print_gap (g); > + int nmods = nmodules (g); > + > + int mods[nmods]; > + /* We use '0' as indication for a gap, to avoid the dlclose on iteration > + cleanup. */ > + for (int n = 1; n <= nmods; n++) > + { > + load_mod (n); > + mods[n] = n; > + } > + for (int n = 1; n <= nmods; n++) > + { > + if (!is_mod_set (g, n)) > + { > + unload_mod (n); > + mods[n] = 0; > + } > + } > + > + for (int t = 0; t < array_length (tlsmanydeps); t++) > + { > + char *moddepname = xasprintf ("tst-tls-manydynamic%dmod-dep.so", > + tlsmanydeps[t].modi); > + void *moddep = xdlopen (moddepname, RTLD_LAZY); > + > + /* Access TLS in all loaded modules. */ > + struct start_args args = > + { > + moddepname, > + moddep, > + tlsmanydeps[t].modi, > + tlsmanydeps[t].ndeps, > + tlsmanydeps[t].deps > + }; > + pthread_t t = xpthread_create (0, start, &args); > + xpthread_join (t); > + > + free (moddepname); > + xdlclose (moddep); > + } > + > + for (int n = 1; n <= nmods; n++) > + if (mods[n] != 0) > + unload_mod (n); > + } > +} > + > +/* The following test check DTV gaps handling with shared libraries that has > + invalid dependencies. It defines 5 different sets: > + > + 1. Single dependency: > + mod0 -> invalid > + 2. Double dependency: > + mod1 -> [mod2,invalid] > + 3. Double dependency with each dependency depent of another module: > + mod3 -> [mod4,mod5] -> invalid > + 4. Long chain with one double dependency in the middle: > + mod6 -> [mod7, mod8] -> mod12 -> invalid > + 5. Long chain with two double depedencies in the middle: > + mod10 -> mod11 -> [mod12, mod13] > + mod12 -> [mod14, invalid] > + > + This does not cover all the possible gaps and configuration, but it > + should check if different dynamic shared sets are placed correctly in > + different gaps configurations. */ > + > +static void > +do_test_invalid_dependency (bool bind_now) > +{ > + static const int tlsmanydeps[] = { 0, 1, 3, 6, 10 }; > + > + /* The gap configuration is defined as a bitmap: the bit set represents a > + loaded module prior the tests execution, while a bit unsed is a module > + unloaded. Not all permtation will show gaps, but it is simpler than > + define each one independently. */ > + for (uint32_t g = 0; g < 64; g++) > + { > + print_gap (g); > + int nmods = nmodules (g); > + > + int mods[nmods]; > + /* We use '0' as indication for a gap, to avoid the dlclose on iteration > + cleanup. */ > + for (int n = 1; n <= nmods; n++) > + { > + load_mod (n); > + mods[n] = n; > + } > + for (int n = 1; n <= nmods; n++) > + { > + if (!is_mod_set (g, n)) > + { > + unload_mod (n); > + mods[n] = 0; > + } > + } > + > + for (int t = 0; t < array_length (tlsmanydeps); t++) > + { > + char *moddepname = xasprintf ("tst-tls-manydynamic%dmod-dep-bad.so", > + tlsmanydeps[t]); > + void *moddep; > + if (bind_now) > + { > + moddep = dlopen (moddepname, RTLD_NOW); > + TEST_VERIFY (moddep == 0); > + } > + else > + moddep = dlopen (moddepname, RTLD_LAZY); > + > + /* Access TLS in all loaded modules. */ > + pthread_t t = xpthread_create (0, start, NULL); > + xpthread_join (t); > + > + free (moddepname); > + if (!bind_now) > + xdlclose (moddep); > + } > + > + for (int n = 1; n <= nmods; n++) > + if (mods[n] != 0) > + unload_mod (n); > + } > +} > + > +static int > +do_test (void) > +{ > + do_test_no_depedency (); > + do_test_dependency (); > + do_test_invalid_dependency (true); > + do_test_invalid_dependency (false); > + > return 0; > } > > diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h > index 176394de4d..9c15259236 100644 > --- a/sysdeps/generic/ldsodefs.h > +++ b/sysdeps/generic/ldsodefs.h > @@ -1171,8 +1171,8 @@ extern ElfW(Addr) _dl_sysdep_start (void **start_argptr, > extern void _dl_sysdep_start_cleanup (void) attribute_hidden; > > > -/* Determine next available module ID. */ > -extern size_t _dl_next_tls_modid (void) attribute_hidden; > +/* Determine next available module ID and set the L l_tls_modid. */ > +extern void _dl_assign_tls_modid (struct link_map *l) attribute_hidden; > > /* Count the modules with TLS segments. */ > extern size_t _dl_count_modids (void) attribute_hidden; >
On 09/07/2021 12:05, Szabolcs Nagy wrote: > The 07/09/2021 10:50, Adhemerval Zanella wrote: >> Changes from previous version: >> >> - Fix commit message and add a line about the bug fixes. >> - Use atomic operation while setting the slotinfo. >> - Use test_verbose on tst-tls20.c. >> >> --- >> >> This is updated version of the 572bd547d57a (reverted by 40ebfd016ad2) >> that fixes the _dl_next_tls_modid issues. >> >> This issue with 572bd547d57a patch is the DTV entry will be only >> update on dl_open_worker() with the update_tls_slotinfo() call after >> all dependencies are being processed by _dl_map_object_deps(). However >> _dl_map_object_deps() itself might call _dl_next_tls_modid(), and since >> the _dl_tls_dtv_slotinfo_list::map is not yet set the entry will be >> wrongly reused. >> >> This patch fixes by renaming the _dl_next_tls_modid() function to >> _dl_assign_tls_modid() and by passing the link_map so it can set >> the slotinfo value so a so subsequente _dl_next_tls_modid() call will >> see the entry as allocated. > > this paragraph still has 'so a so subsequente' > and i would add the bug number into the first sentence. Fixed. > >> >> The intermediary value is cleared up on remove_slotinfo() for the case >> a library fails to load with RTLD_NOW. >> >> This patch fixes BZ #27135. >> >> Checked on x86_64-linux-gnu. > > the patch looks ok to me, with the commit message > and the comment issue below fixed. > > Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com> Carlos, is it for push? > >> + >> +/* The following test check DTV gaps handling with shared libraries that has >> + dependencies. It defines 5 different sets: >> + >> + 1. Single dependency: >> + mod0 -> mod1 >> + 2. Double dependency: >> + mod2 -> [mod3,mod4] >> + 3. Double dependency with each dependency depent of another module: >> + mod5 -> [mod6,mod7] -> mod8 >> + 4. Long chain with one double dependency in the middle: >> + mod9 -> [mod10, mod11] -> mod12 -> mod13 >> + 5. Long chain with two double depedencies in the middle: >> + mod15 -> mod15 -> [mod16, mod17] >> + mod15 -> [mod18, mod19] > > mod14 -> ... Fixed. > >> + >> + This does not cover all the possible gaps and configuration, but it >> + should check if different dynamic shared sets are placed correctly in >> + different gaps configurations. */ >> + >> +static int >> +nmodules (uint32_t v) >> +{ >> + unsigned int r = 0; >> + while (v >>= 1) >> + r++; >> + return r + 1; >> +} >> + >> +static inline bool >> +is_mod_set (uint32_t g, uint32_t n) >> +{ >> + return (1U << (n - 1)) & g; >> +} >> + >> +static void >> +print_gap (uint32_t g) >> +{ >> + if (!test_verbose) >> + return; >> + printf ("gap: "); >> + int nmods = nmodules (g); >> + for (int n = 1; n <= nmods; n++) >> + printf ("%c", ((1 << (n - 1)) & g) == 0 ? 'G' : 'M'); >> + printf ("\n"); >> +} >> + >> +static void >> +do_test_dependency (void) >> +{ >> + /* Maps the module and its dependencies, use thread to access the TLS on >> + each loaded module. */ >> + static const int tlsmanydeps0[] = { 1 }; >> + static const int tlsmanydeps1[] = { 3, 4 }; >> + static const int tlsmanydeps2[] = { 6, 7, 8 }; >> + static const int tlsmanydeps3[] = { 10, 11, 12 }; >> + static const int tlsmanydeps4[] = { 15, 16, 17, 18, 19 }; >> + static const struct tlsmanydeps_t >> + { >> + int modi; >> + int ndeps; >> + const int *deps; >> + } tlsmanydeps[] = >> + { >> + { 0, array_length (tlsmanydeps0), tlsmanydeps0 }, >> + { 2, array_length (tlsmanydeps1), tlsmanydeps1 }, >> + { 5, array_length (tlsmanydeps2), tlsmanydeps2 }, >> + { 9, array_length (tlsmanydeps3), tlsmanydeps3 }, >> + { 14, array_length (tlsmanydeps4), tlsmanydeps4 }, >> + };
On 7/14/21 9:52 AM, Adhemerval Zanella wrote: > > > On 09/07/2021 12:05, Szabolcs Nagy wrote: >> The 07/09/2021 10:50, Adhemerval Zanella wrote: >>> Changes from previous version: >>> >>> - Fix commit message and add a line about the bug fixes. >>> - Use atomic operation while setting the slotinfo. >>> - Use test_verbose on tst-tls20.c. >>> >>> --- >>> >>> This is updated version of the 572bd547d57a (reverted by 40ebfd016ad2) >>> that fixes the _dl_next_tls_modid issues. >>> >>> This issue with 572bd547d57a patch is the DTV entry will be only >>> update on dl_open_worker() with the update_tls_slotinfo() call after >>> all dependencies are being processed by _dl_map_object_deps(). However >>> _dl_map_object_deps() itself might call _dl_next_tls_modid(), and since >>> the _dl_tls_dtv_slotinfo_list::map is not yet set the entry will be >>> wrongly reused. >>> >>> This patch fixes by renaming the _dl_next_tls_modid() function to >>> _dl_assign_tls_modid() and by passing the link_map so it can set >>> the slotinfo value so a so subsequente _dl_next_tls_modid() call will >>> see the entry as allocated. >> >> this paragraph still has 'so a so subsequente' >> and i would add the bug number into the first sentence. > > Fixed. > >> >>> >>> The intermediary value is cleared up on remove_slotinfo() for the case >>> a library fails to load with RTLD_NOW. >>> >>> This patch fixes BZ #27135. >>> >>> Checked on x86_64-linux-gnu. >> >> the patch looks ok to me, with the commit message >> and the comment issue below fixed. >> >> Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com> > > Carlos, is it for push? It's a non-ABI bug fix, so we can push it. Thanks for asking.
On 14/07/2021 13:57, Carlos O'Donell wrote: > On 7/14/21 9:52 AM, Adhemerval Zanella wrote: >> >> >> On 09/07/2021 12:05, Szabolcs Nagy wrote: >>> The 07/09/2021 10:50, Adhemerval Zanella wrote: >>>> Changes from previous version: >>>> >>>> - Fix commit message and add a line about the bug fixes. >>>> - Use atomic operation while setting the slotinfo. >>>> - Use test_verbose on tst-tls20.c. >>>> >>>> --- >>>> >>>> This is updated version of the 572bd547d57a (reverted by 40ebfd016ad2) >>>> that fixes the _dl_next_tls_modid issues. >>>> >>>> This issue with 572bd547d57a patch is the DTV entry will be only >>>> update on dl_open_worker() with the update_tls_slotinfo() call after >>>> all dependencies are being processed by _dl_map_object_deps(). However >>>> _dl_map_object_deps() itself might call _dl_next_tls_modid(), and since >>>> the _dl_tls_dtv_slotinfo_list::map is not yet set the entry will be >>>> wrongly reused. >>>> >>>> This patch fixes by renaming the _dl_next_tls_modid() function to >>>> _dl_assign_tls_modid() and by passing the link_map so it can set >>>> the slotinfo value so a so subsequente _dl_next_tls_modid() call will >>>> see the entry as allocated. >>> >>> this paragraph still has 'so a so subsequente' >>> and i would add the bug number into the first sentence. >> >> Fixed. >> >>> >>>> >>>> The intermediary value is cleared up on remove_slotinfo() for the case >>>> a library fails to load with RTLD_NOW. >>>> >>>> This patch fixes BZ #27135. >>>> >>>> Checked on x86_64-linux-gnu. >>> >>> the patch looks ok to me, with the commit message >>> and the comment issue below fixed. >>> >>> Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com> >> >> Carlos, is it for push? > > It's a non-ABI bug fix, so we can push it. Thanks for asking. > And it is in, let's hope it does not brake anything again ;)
On 14/07/2021 20:11, Adhemerval Zanella via Libc-alpha wrote: > > > On 14/07/2021 13:57, Carlos O'Donell wrote: >> On 7/14/21 9:52 AM, Adhemerval Zanella wrote: >>> >>> >>> On 09/07/2021 12:05, Szabolcs Nagy wrote: >>>> The 07/09/2021 10:50, Adhemerval Zanella wrote: >>>>> Changes from previous version: >>>>> >>>>> - Fix commit message and add a line about the bug fixes. >>>>> - Use atomic operation while setting the slotinfo. >>>>> - Use test_verbose on tst-tls20.c. >>>>> >>>>> --- >>>>> >>>>> This is updated version of the 572bd547d57a (reverted by 40ebfd016ad2) >>>>> that fixes the _dl_next_tls_modid issues. >>>>> >>>>> This issue with 572bd547d57a patch is the DTV entry will be only >>>>> update on dl_open_worker() with the update_tls_slotinfo() call after >>>>> all dependencies are being processed by _dl_map_object_deps(). However >>>>> _dl_map_object_deps() itself might call _dl_next_tls_modid(), and since >>>>> the _dl_tls_dtv_slotinfo_list::map is not yet set the entry will be >>>>> wrongly reused. >>>>> >>>>> This patch fixes by renaming the _dl_next_tls_modid() function to >>>>> _dl_assign_tls_modid() and by passing the link_map so it can set >>>>> the slotinfo value so a so subsequente _dl_next_tls_modid() call will >>>>> see the entry as allocated. >>>> >>>> this paragraph still has 'so a so subsequente' >>>> and i would add the bug number into the first sentence. >>> >>> Fixed. >>> >>>> >>>>> >>>>> The intermediary value is cleared up on remove_slotinfo() for the case >>>>> a library fails to load with RTLD_NOW. >>>>> >>>>> This patch fixes BZ #27135. >>>>> >>>>> Checked on x86_64-linux-gnu. >>>> >>>> the patch looks ok to me, with the commit message >>>> and the comment issue below fixed. >>>> >>>> Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com> >>> >>> Carlos, is it for push? >> >> It's a non-ABI bug fix, so we can push it. Thanks for asking. >> > > And it is in, let's hope it does not brake anything again ;) > Hi Adhemerval, I'm getting a segfault on s390x in elf/tst-tls20. It is at the end of do_test() when the stack-protector-canary is compared. I'm also getting such an error on x86_64, $ <glibc>/configure --prefix=/usr --enable-stack-protector=strong $ make $ make subdirs=elf check $ make t=elf/tst-tls20 test ... *** stack smashing detected ***: terminated make[2]: Leaving directory 'glibc/elf' FAIL: elf/tst-tls20 original exit status 1 Didn't expect signal from child: got `Aborted' If configuring without --enable-stack-protector=strong, then elf/tst-tls20 succeeds. Can you please have a look? Bye, Stefan
On 15/07/2021 10:36, Stefan Liebler via Libc-alpha wrote: > On 14/07/2021 20:11, Adhemerval Zanella via Libc-alpha wrote: >> >> >> On 14/07/2021 13:57, Carlos O'Donell wrote: >>> On 7/14/21 9:52 AM, Adhemerval Zanella wrote: >>>> >>>> >>>> On 09/07/2021 12:05, Szabolcs Nagy wrote: >>>>> The 07/09/2021 10:50, Adhemerval Zanella wrote: >>>>>> Changes from previous version: >>>>>> >>>>>> - Fix commit message and add a line about the bug fixes. >>>>>> - Use atomic operation while setting the slotinfo. >>>>>> - Use test_verbose on tst-tls20.c. >>>>>> >>>>>> --- >>>>>> >>>>>> This is updated version of the 572bd547d57a (reverted by 40ebfd016ad2) >>>>>> that fixes the _dl_next_tls_modid issues. >>>>>> >>>>>> This issue with 572bd547d57a patch is the DTV entry will be only >>>>>> update on dl_open_worker() with the update_tls_slotinfo() call after >>>>>> all dependencies are being processed by _dl_map_object_deps(). However >>>>>> _dl_map_object_deps() itself might call _dl_next_tls_modid(), and since >>>>>> the _dl_tls_dtv_slotinfo_list::map is not yet set the entry will be >>>>>> wrongly reused. >>>>>> >>>>>> This patch fixes by renaming the _dl_next_tls_modid() function to >>>>>> _dl_assign_tls_modid() and by passing the link_map so it can set >>>>>> the slotinfo value so a so subsequente _dl_next_tls_modid() call will >>>>>> see the entry as allocated. >>>>> >>>>> this paragraph still has 'so a so subsequente' >>>>> and i would add the bug number into the first sentence. >>>> >>>> Fixed. >>>> >>>>> >>>>>> >>>>>> The intermediary value is cleared up on remove_slotinfo() for the case >>>>>> a library fails to load with RTLD_NOW. >>>>>> >>>>>> This patch fixes BZ #27135. >>>>>> >>>>>> Checked on x86_64-linux-gnu. >>>>> >>>>> the patch looks ok to me, with the commit message >>>>> and the comment issue below fixed. >>>>> >>>>> Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com> >>>> >>>> Carlos, is it for push? >>> >>> It's a non-ABI bug fix, so we can push it. Thanks for asking. >>> >> >> And it is in, let's hope it does not brake anything again ;) >> > > Hi Adhemerval, > > I'm getting a segfault on s390x in elf/tst-tls20. It is at the end of > do_test() when the stack-protector-canary is compared. > > I'm also getting such an error on x86_64, > $ <glibc>/configure --prefix=/usr --enable-stack-protector=strong > $ make > $ make subdirs=elf check > $ make t=elf/tst-tls20 test > ... > *** stack smashing detected ***: terminated > make[2]: Leaving directory 'glibc/elf' > FAIL: elf/tst-tls20 > original exit status 1 > Didn't expect signal from child: got `Aborted' > > > If configuring without --enable-stack-protector=strong, then > elf/tst-tls20 succeeds. > > Can you please have a look? Let me check.
On 15/07/2021 10:36, Stefan Liebler via Libc-alpha wrote: > On 14/07/2021 20:11, Adhemerval Zanella via Libc-alpha wrote: >> >> >> On 14/07/2021 13:57, Carlos O'Donell wrote: >>> On 7/14/21 9:52 AM, Adhemerval Zanella wrote: >>>> >>>> >>>> On 09/07/2021 12:05, Szabolcs Nagy wrote: >>>>> The 07/09/2021 10:50, Adhemerval Zanella wrote: >>>>>> Changes from previous version: >>>>>> >>>>>> - Fix commit message and add a line about the bug fixes. >>>>>> - Use atomic operation while setting the slotinfo. >>>>>> - Use test_verbose on tst-tls20.c. >>>>>> >>>>>> --- >>>>>> >>>>>> This is updated version of the 572bd547d57a (reverted by 40ebfd016ad2) >>>>>> that fixes the _dl_next_tls_modid issues. >>>>>> >>>>>> This issue with 572bd547d57a patch is the DTV entry will be only >>>>>> update on dl_open_worker() with the update_tls_slotinfo() call after >>>>>> all dependencies are being processed by _dl_map_object_deps(). However >>>>>> _dl_map_object_deps() itself might call _dl_next_tls_modid(), and since >>>>>> the _dl_tls_dtv_slotinfo_list::map is not yet set the entry will be >>>>>> wrongly reused. >>>>>> >>>>>> This patch fixes by renaming the _dl_next_tls_modid() function to >>>>>> _dl_assign_tls_modid() and by passing the link_map so it can set >>>>>> the slotinfo value so a so subsequente _dl_next_tls_modid() call will >>>>>> see the entry as allocated. >>>>> >>>>> this paragraph still has 'so a so subsequente' >>>>> and i would add the bug number into the first sentence. >>>> >>>> Fixed. >>>> >>>>> >>>>>> >>>>>> The intermediary value is cleared up on remove_slotinfo() for the case >>>>>> a library fails to load with RTLD_NOW. >>>>>> >>>>>> This patch fixes BZ #27135. >>>>>> >>>>>> Checked on x86_64-linux-gnu. >>>>> >>>>> the patch looks ok to me, with the commit message >>>>> and the comment issue below fixed. >>>>> >>>>> Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com> >>>> >>>> Carlos, is it for push? >>> >>> It's a non-ABI bug fix, so we can push it. Thanks for asking. >>> >> >> And it is in, let's hope it does not brake anything again ;) >> > > Hi Adhemerval, > > I'm getting a segfault on s390x in elf/tst-tls20. It is at the end of > do_test() when the stack-protector-canary is compared. > > I'm also getting such an error on x86_64, > $ <glibc>/configure --prefix=/usr --enable-stack-protector=strong > $ make > $ make subdirs=elf check > $ make t=elf/tst-tls20 test > ... > *** stack smashing detected ***: terminated > make[2]: Leaving directory 'glibc/elf' > FAIL: elf/tst-tls20 > original exit status 1 > Didn't expect signal from child: got `Aborted' > > > If configuring without --enable-stack-protector=strong, then > elf/tst-tls20 succeeds. > > Can you please have a look? Sigh, it is overlook in array access. I reproduced it on x86_64 as well, this should fix it: diff --git a/elf/tst-tls20.c b/elf/tst-tls20.c index d8d04fe574..831c3336c9 100644 --- a/elf/tst-tls20.c +++ b/elf/tst-tls20.c @@ -226,12 +226,12 @@ do_test_dependency (void) int mods[nmods]; /* We use '0' as indication for a gap, to avoid the dlclose on iteration cleanup. */ - for (int n = 1; n <= nmods; n++) + for (int n = 1; n < nmods; n++) { load_mod (n); mods[n] = n; } - for (int n = 1; n <= nmods; n++) + for (int n = 1; n < nmods; n++) { if (!is_mod_set (g, n)) { @@ -304,12 +304,12 @@ do_test_invalid_dependency (bool bind_now) int mods[nmods]; /* We use '0' as indication for a gap, to avoid the dlclose on iteration cleanup. */ - for (int n = 1; n <= nmods; n++) + for (int n = 1; n < nmods; n++) { load_mod (n); mods[n] = n; } - for (int n = 1; n <= nmods; n++) + for (int n = 1; n < nmods; n++) { if (!is_mod_set (g, n)) {
On 15/07/2021 15:51, Adhemerval Zanella via Libc-alpha wrote: > > > On 15/07/2021 10:36, Stefan Liebler via Libc-alpha wrote: >> On 14/07/2021 20:11, Adhemerval Zanella via Libc-alpha wrote: >>> >>> >>> On 14/07/2021 13:57, Carlos O'Donell wrote: >>>> On 7/14/21 9:52 AM, Adhemerval Zanella wrote: >>>>> >>>>> >>>>> On 09/07/2021 12:05, Szabolcs Nagy wrote: >>>>>> The 07/09/2021 10:50, Adhemerval Zanella wrote: >>>>>>> Changes from previous version: >>>>>>> >>>>>>> - Fix commit message and add a line about the bug fixes. >>>>>>> - Use atomic operation while setting the slotinfo. >>>>>>> - Use test_verbose on tst-tls20.c. >>>>>>> >>>>>>> --- >>>>>>> >>>>>>> This is updated version of the 572bd547d57a (reverted by 40ebfd016ad2) >>>>>>> that fixes the _dl_next_tls_modid issues. >>>>>>> >>>>>>> This issue with 572bd547d57a patch is the DTV entry will be only >>>>>>> update on dl_open_worker() with the update_tls_slotinfo() call after >>>>>>> all dependencies are being processed by _dl_map_object_deps(). However >>>>>>> _dl_map_object_deps() itself might call _dl_next_tls_modid(), and since >>>>>>> the _dl_tls_dtv_slotinfo_list::map is not yet set the entry will be >>>>>>> wrongly reused. >>>>>>> >>>>>>> This patch fixes by renaming the _dl_next_tls_modid() function to >>>>>>> _dl_assign_tls_modid() and by passing the link_map so it can set >>>>>>> the slotinfo value so a so subsequente _dl_next_tls_modid() call will >>>>>>> see the entry as allocated. >>>>>> >>>>>> this paragraph still has 'so a so subsequente' >>>>>> and i would add the bug number into the first sentence. >>>>> >>>>> Fixed. >>>>> >>>>>> >>>>>>> >>>>>>> The intermediary value is cleared up on remove_slotinfo() for the case >>>>>>> a library fails to load with RTLD_NOW. >>>>>>> >>>>>>> This patch fixes BZ #27135. >>>>>>> >>>>>>> Checked on x86_64-linux-gnu. >>>>>> >>>>>> the patch looks ok to me, with the commit message >>>>>> and the comment issue below fixed. >>>>>> >>>>>> Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com> >>>>> >>>>> Carlos, is it for push? >>>> >>>> It's a non-ABI bug fix, so we can push it. Thanks for asking. >>>> >>> >>> And it is in, let's hope it does not brake anything again ;) >>> >> >> Hi Adhemerval, >> >> I'm getting a segfault on s390x in elf/tst-tls20. It is at the end of >> do_test() when the stack-protector-canary is compared. >> >> I'm also getting such an error on x86_64, >> $ <glibc>/configure --prefix=/usr --enable-stack-protector=strong >> $ make >> $ make subdirs=elf check >> $ make t=elf/tst-tls20 test >> ... >> *** stack smashing detected ***: terminated >> make[2]: Leaving directory 'glibc/elf' >> FAIL: elf/tst-tls20 >> original exit status 1 >> Didn't expect signal from child: got `Aborted' >> >> >> If configuring without --enable-stack-protector=strong, then >> elf/tst-tls20 succeeds. >> >> Can you please have a look? > > Sigh, it is overlook in array access. I reproduced it on x86_64 as well, > this should fix it: > > diff --git a/elf/tst-tls20.c b/elf/tst-tls20.c > index d8d04fe574..831c3336c9 100644 > --- a/elf/tst-tls20.c > +++ b/elf/tst-tls20.c > @@ -226,12 +226,12 @@ do_test_dependency (void) > int mods[nmods]; > /* We use '0' as indication for a gap, to avoid the dlclose on iteration > cleanup. */ > - for (int n = 1; n <= nmods; n++) > + for (int n = 1; n < nmods; n++) > { > load_mod (n); > mods[n] = n; > } > - for (int n = 1; n <= nmods; n++) > + for (int n = 1; n < nmods; n++) > { > if (!is_mod_set (g, n)) > { > @@ -304,12 +304,12 @@ do_test_invalid_dependency (bool bind_now) > int mods[nmods]; > /* We use '0' as indication for a gap, to avoid the dlclose on iteration > cleanup. */ > - for (int n = 1; n <= nmods; n++) > + for (int n = 1; n < nmods; n++) > { > load_mod (n); > mods[n] = n; > } > - for (int n = 1; n <= nmods; n++) > + for (int n = 1; n < nmods; n++) > { > if (!is_mod_set (g, n)) > { > Tested on s390x/s390 with and without --enable-stack-protector=strong. The test elf/tst-tls20 is now passing. Thanks, Stefan
diff --git a/elf/Makefile b/elf/Makefile index 4b320e8b3a..bdd5cc9e1a 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -253,6 +253,13 @@ one-hundred = $(foreach x,0 1 2 3 4 5 6 7 8 9, \ 0$x 1$x 2$x 3$x 4$x 5$x 6$x 7$x 8$x 9$x) tst-tls-many-dynamic-modules := \ $(foreach n,$(one-hundred),tst-tls-manydynamic$(n)mod) +tst-tls-many-dynamic-modules-dep-suffixes = 0 1 2 3 4 5 6 7 8 9 10 11 12 13 \ + 14 15 16 17 18 19 +tst-tls-many-dynamic-modules-dep = \ + $(foreach n,$(tst-tls-many-dynamic-modules-dep-suffixes),tst-tls-manydynamic$(n)mod-dep) +tst-tls-many-dynamic-modules-dep-bad-suffixes = 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 +tst-tls-many-dynamic-modules-dep-bad = \ + $(foreach n,$(tst-tls-many-dynamic-modules-dep-bad-suffixes),tst-tls-manydynamic$(n)mod-dep-bad) extra-test-objs += $(tlsmod17a-modules:=.os) $(tlsmod18a-modules:=.os) \ tst-tlsalign-vars.o test-extras += tst-tlsmod17a tst-tlsmod18a tst-tlsalign-vars @@ -325,6 +332,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \ tst-audit11mod1 tst-audit11mod2 tst-auditmod11 \ tst-audit12mod1 tst-audit12mod2 tst-audit12mod3 tst-auditmod12 \ tst-latepthreadmod $(tst-tls-many-dynamic-modules) \ + $(tst-tls-many-dynamic-modules-dep) \ + $(tst-tls-many-dynamic-modules-dep-bad) \ tst-nodelete-dlclose-dso tst-nodelete-dlclose-plugin \ tst-main1mod tst-absolute-sym-lib \ tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \ @@ -1812,10 +1821,63 @@ $(objpfx)tst-rtld-help.out: $(objpfx)ld.so $(evaluate-test) # Reuses tst-tls-many-dynamic-modules +$(patsubst %,$(objpfx)%.os,$(tst-tls-many-dynamic-modules-dep)): \ + $(objpfx)tst-tls-manydynamic%mod-dep.os : tst-tls-manydynamicmod.c + $(compile-command.c) \ + -DNAME=tls_global_$* -DSETTER=set_value_$* -DGETTER=get_value_$* +$(patsubst %,$(objpfx)%.os,$(tst-tls-many-dynamic-modules-dep-bad)): \ + $(objpfx)tst-tls-manydynamic%mod-dep-bad.os : tst-tls-manydynamicmod.c + $(compile-command.c) \ + -DNAME=tls_global_$* -DSETTER=set_value_$* -DGETTER=get_value_$* tst-tls20mod-bad.so-no-z-defs = yes +# Single dependency. +$(objpfx)tst-tls-manydynamic0mod-dep.so: $(objpfx)tst-tls-manydynamic1mod-dep.so +# Double dependencies. +$(objpfx)tst-tls-manydynamic2mod-dep.so: $(objpfx)tst-tls-manydynamic3mod-dep.so \ + $(objpfx)tst-tls-manydynamic4mod-dep.so +# Double dependencies with each dependency depent of another module. +$(objpfx)tst-tls-manydynamic5mod-dep.so: $(objpfx)tst-tls-manydynamic6mod-dep.so \ + $(objpfx)tst-tls-manydynamic7mod-dep.so +$(objpfx)tst-tls-manydynamic6mod-dep.so: $(objpfx)tst-tls-manydynamic8mod-dep.so +$(objpfx)tst-tls-manydynamic7mod-dep.so: $(objpfx)tst-tls-manydynamic8mod-dep.so +# Long chain with one double dependency in the middle +$(objpfx)tst-tls-manydynamic9mod-dep.so: $(objpfx)tst-tls-manydynamic10mod-dep.so \ + $(objpfx)tst-tls-manydynamic11mod-dep.so +$(objpfx)tst-tls-manydynamic10mod-dep.so: $(objpfx)tst-tls-manydynamic12mod-dep.so +$(objpfx)tst-tls-manydynamic12mod-dep.so: $(objpfx)tst-tls-manydynamic13mod-dep.so +# Long chain with two double depedencies in the middle +$(objpfx)tst-tls-manydynamic14mod-dep.so: $(objpfx)tst-tls-manydynamic15mod-dep.so +$(objpfx)tst-tls-manydynamic15mod-dep.so: $(objpfx)tst-tls-manydynamic16mod-dep.so \ + $(objpfx)tst-tls-manydynamic17mod-dep.so +$(objpfx)tst-tls-manydynamic16mod-dep.so: $(objpfx)tst-tls-manydynamic18mod-dep.so \ + $(objpfx)tst-tls-manydynamic19mod-dep.so +# Same but with an invalid module. +# Single dependency. +$(objpfx)tst-tls-manydynamic0mod-dep-bad.so: $(objpfx)tst-tls20mod-bad.so +# Double dependencies. +$(objpfx)tst-tls-manydynamic1mod-dep-bad.so: $(objpfx)tst-tls-manydynamic2mod-dep-bad.so \ + $(objpfx)tst-tls20mod-bad.so +# Double dependencies with each dependency depent of another module. +$(objpfx)tst-tls-manydynamic3mod-dep-bad.so: $(objpfx)tst-tls-manydynamic4mod-dep-bad.so \ + $(objpfx)tst-tls-manydynamic5mod-dep-bad.so +$(objpfx)tst-tls-manydynamic4mod-dep-bad.so: $(objpfx)tst-tls20mod-bad.so +$(objpfx)tst-tls-manydynamic5mod-dep-bad.so: $(objpfx)tst-tls20mod-bad.so +# Long chain with one double dependency in the middle +$(objpfx)tst-tls-manydynamic6mod-dep-bad.so: $(objpfx)tst-tls-manydynamic7mod-dep-bad.so \ + $(objpfx)tst-tls-manydynamic8mod-dep-bad.so +$(objpfx)tst-tls-manydynamic7mod-dep-bad.so: $(objpfx)tst-tls-manydynamic9mod-dep-bad.so +$(objpfx)tst-tls-manydynamic9mod-dep-bad.so: $(objpfx)tst-tls20mod-bad.so +# Long chain with two double depedencies in the middle +$(objpfx)tst-tls-manydynamic10mod-dep-bad.so: $(objpfx)tst-tls-manydynamic11mod-dep-bad.so +$(objpfx)tst-tls-manydynamic11mod-dep-bad.so: $(objpfx)tst-tls-manydynamic12mod-dep-bad.so \ + $(objpfx)tst-tls-manydynamic13mod-dep-bad.so +$(objpfx)tst-tls-manydynamic12mod-dep-bad.so: $(objpfx)tst-tls-manydynamic14mod-dep-bad.so \ + $(objpfx)tst-tls20mod-bad.so $(objpfx)tst-tls20: $(shared-thread-library) $(objpfx)tst-tls20.out: $(objpfx)tst-tls20mod-bad.so \ - $(tst-tls-many-dynamic-modules:%=$(objpfx)%.so) + $(tst-tls-many-dynamic-modules:%=$(objpfx)%.so) \ + $(tst-tls-many-dynamic-modules-dep:%=$(objpfx)%.so) \ + $(tst-tls-many-dynamic-modules-dep-bad:%=$(objpfx)%.so) \ # Reuses tst-tls-many-dynamic-modules $(objpfx)tst-tls21: $(shared-thread-library) diff --git a/elf/dl-close.c b/elf/dl-close.c index 3720e47dd1..f39001cab9 100644 --- a/elf/dl-close.c +++ b/elf/dl-close.c @@ -77,8 +77,6 @@ remove_slotinfo (size_t idx, struct dtv_slotinfo_list *listp, size_t disp, object that wasn't fully set up. */ if (__glibc_likely (old_map != NULL)) { - assert (old_map->l_tls_modid == idx); - /* Mark the entry as unused. These can be read concurrently. */ atomic_store_relaxed (&listp->slotinfo[idx - disp].gen, GL(dl_tls_generation) + 1); @@ -88,7 +86,11 @@ remove_slotinfo (size_t idx, struct dtv_slotinfo_list *listp, size_t disp, /* If this is not the last currently used entry no need to look further. */ if (idx != GL(dl_tls_max_dtv_idx)) - return true; + { + /* There is an unused dtv entry in the middle. */ + GL(dl_tls_dtv_gaps) = true; + return true; + } } while (idx - disp > (disp == 0 ? 1 + GL(dl_tls_static_nelem) : 0)) diff --git a/elf/dl-load.c b/elf/dl-load.c index a08df001af..650e4edc35 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -1498,7 +1498,7 @@ cannot enable executable stack as shared object requires"); not set up TLS data structures, so don't use them now. */ || __glibc_likely (GL(dl_tls_dtv_slotinfo_list) != NULL))) /* Assign the next available module ID. */ - l->l_tls_modid = _dl_next_tls_modid (); + _dl_assign_tls_modid (l); #ifdef DL_AFTER_LOAD DL_AFTER_LOAD (l); diff --git a/elf/dl-open.c b/elf/dl-open.c index a066f39bd0..d2240d8747 100644 --- a/elf/dl-open.c +++ b/elf/dl-open.c @@ -899,16 +899,6 @@ no more namespaces available for dlmopen()")); state if relocation failed, for example. */ if (args.map) { - /* Maybe some of the modules which were loaded use TLS. - Since it will be removed in the following _dl_close call - we have to mark the dtv array as having gaps to fill the - holes. This is a pessimistic assumption which won't hurt - if not true. There is no need to do this when we are - loading the auditing DSOs since TLS has not yet been set - up. */ - if ((mode & __RTLD_AUDIT) == 0) - GL(dl_tls_dtv_gaps) = true; - _dl_close_worker (args.map, true); /* All l_nodelete_pending objects should have been deleted diff --git a/elf/dl-tls.c b/elf/dl-tls.c index 2b5161d10a..423e380f7c 100644 --- a/elf/dl-tls.c +++ b/elf/dl-tls.c @@ -126,8 +126,8 @@ oom (void) } -size_t -_dl_next_tls_modid (void) +void +_dl_assign_tls_modid (struct link_map *l) { size_t result; @@ -157,7 +157,11 @@ _dl_next_tls_modid (void) } if (result - disp < runp->len) - break; + { + /* Mark the entry as used, so any dependency see it. */ + atomic_store_relaxed (&runp->slotinfo[result - disp].map, l); + break; + } disp += runp->len; } @@ -184,17 +188,14 @@ _dl_next_tls_modid (void) atomic_store_relaxed (&GL(dl_tls_max_dtv_idx), result); } - return result; + l->l_tls_modid = result; } size_t _dl_count_modids (void) { - /* It is rare that we have gaps; see elf/dl-open.c (_dl_open) where - we fail to load a module and unload it leaving a gap. If we don't - have gaps then the number of modids is the current maximum so - return that. */ + /* The count is the max unless dlclose or failed dlopen created gaps. */ if (__glibc_likely (!GL(dl_tls_dtv_gaps))) return GL(dl_tls_max_dtv_idx); diff --git a/elf/rtld.c b/elf/rtld.c index fbbd60b446..160faaf5ab 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -1722,7 +1722,7 @@ dl_main (const ElfW(Phdr) *phdr, /* Add the dynamic linker to the TLS list if it also uses TLS. */ if (GL(dl_rtld_map).l_tls_blocksize != 0) /* Assign a module ID. Do this before loading any audit modules. */ - GL(dl_rtld_map).l_tls_modid = _dl_next_tls_modid (); + _dl_assign_tls_modid (&GL(dl_rtld_map)); audit_list_add_dynamic_tag (&state.audit_list, main_map, DT_AUDIT); audit_list_add_dynamic_tag (&state.audit_list, main_map, DT_DEPAUDIT); diff --git a/elf/tst-tls20.c b/elf/tst-tls20.c index 9977ec8032..f0db2d1a27 100644 --- a/elf/tst-tls20.c +++ b/elf/tst-tls20.c @@ -16,12 +16,14 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#include <array_length.h> #include <dlfcn.h> #include <pthread.h> #include <stdio.h> #include <stdlib.h> #include <support/check.h> #include <support/support.h> +#include <support/test-driver.h> #include <support/xdlfcn.h> #include <support/xthread.h> @@ -59,28 +61,75 @@ access (int i) char *buf = xasprintf ("tls_global_%02d", i); dlerror (); int *p = dlsym (mod[i], buf); - printf ("mod[%d]: &tls = %p\n", i, p); + if (test_verbose) + printf ("mod[%d]: &tls = %p\n", i, p); if (p == NULL) FAIL_EXIT1 ("dlsym failed: %s\n", dlerror ()); + TEST_COMPARE (*p, 0); ++*p; free (buf); } +static void +access_mod (const char *modname, void *mod, int i) +{ + char *modsym = xasprintf ("tls_global_%d", i); + dlerror (); + int *p = dlsym (mod, modsym); + if (test_verbose) + printf ("%s: &tls = %p\n", modname, p); + if (p == NULL) + FAIL_EXIT1 ("dlsym failed: %s\n", dlerror ()); + TEST_COMPARE (*p, 0); + ++*p; + free (modsym); +} + +static void +access_dep (int i) +{ + char *modname = xasprintf ("tst-tls-manydynamic%dmod-dep.so", i); + void *moddep = xdlopen (modname, RTLD_LAZY); + access_mod (modname, moddep, i); + free (modname); + xdlclose (moddep); +} + +struct start_args +{ + const char *modname; + void *mod; + int modi; + int ndeps; + const int *deps; +}; + static void * start (void *a) { + struct start_args *args = a; + for (int i = 0; i < NMOD; i++) if (mod[i] != NULL) access (i); + + if (args != NULL) + { + access_mod (args->modname, args->mod, args->modi); + for (int n = 0; n < args->ndeps; n++) + access_dep (args->deps[n]); + } + return 0; } -static int -do_test (void) +/* This test gaps with shared libraries with dynamic TLS that has no + dependencies. The DTV gap is set with by trying to load an invalid + module, the entry should be used on the dlopen. */ +static void +do_test_no_depedency (void) { - int i; - - for (i = 0; i < NMOD; i++) + for (int i = 0; i < NMOD; i++) { load_mod (i); /* Bump the generation of mod[0] without using new dtv slot. */ @@ -91,8 +140,220 @@ do_test (void) pthread_t t = xpthread_create (0, start, 0); xpthread_join (t); } - for (i = 0; i < NMOD; i++) + for (int i = 0; i < NMOD; i++) unload_mod (i); +} + +/* The following test check DTV gaps handling with shared libraries that has + dependencies. It defines 5 different sets: + + 1. Single dependency: + mod0 -> mod1 + 2. Double dependency: + mod2 -> [mod3,mod4] + 3. Double dependency with each dependency depent of another module: + mod5 -> [mod6,mod7] -> mod8 + 4. Long chain with one double dependency in the middle: + mod9 -> [mod10, mod11] -> mod12 -> mod13 + 5. Long chain with two double depedencies in the middle: + mod15 -> mod15 -> [mod16, mod17] + mod15 -> [mod18, mod19] + + This does not cover all the possible gaps and configuration, but it + should check if different dynamic shared sets are placed correctly in + different gaps configurations. */ + +static int +nmodules (uint32_t v) +{ + unsigned int r = 0; + while (v >>= 1) + r++; + return r + 1; +} + +static inline bool +is_mod_set (uint32_t g, uint32_t n) +{ + return (1U << (n - 1)) & g; +} + +static void +print_gap (uint32_t g) +{ + if (!test_verbose) + return; + printf ("gap: "); + int nmods = nmodules (g); + for (int n = 1; n <= nmods; n++) + printf ("%c", ((1 << (n - 1)) & g) == 0 ? 'G' : 'M'); + printf ("\n"); +} + +static void +do_test_dependency (void) +{ + /* Maps the module and its dependencies, use thread to access the TLS on + each loaded module. */ + static const int tlsmanydeps0[] = { 1 }; + static const int tlsmanydeps1[] = { 3, 4 }; + static const int tlsmanydeps2[] = { 6, 7, 8 }; + static const int tlsmanydeps3[] = { 10, 11, 12 }; + static const int tlsmanydeps4[] = { 15, 16, 17, 18, 19 }; + static const struct tlsmanydeps_t + { + int modi; + int ndeps; + const int *deps; + } tlsmanydeps[] = + { + { 0, array_length (tlsmanydeps0), tlsmanydeps0 }, + { 2, array_length (tlsmanydeps1), tlsmanydeps1 }, + { 5, array_length (tlsmanydeps2), tlsmanydeps2 }, + { 9, array_length (tlsmanydeps3), tlsmanydeps3 }, + { 14, array_length (tlsmanydeps4), tlsmanydeps4 }, + }; + + /* The gap configuration is defined as a bitmap: the bit set represents a + loaded module prior the tests execution, while a bit unsed is a module + unloaded. Not all permtation will show gaps, but it is simpler than + define each one independently. */ + for (uint32_t g = 0; g < 64; g++) + { + print_gap (g); + int nmods = nmodules (g); + + int mods[nmods]; + /* We use '0' as indication for a gap, to avoid the dlclose on iteration + cleanup. */ + for (int n = 1; n <= nmods; n++) + { + load_mod (n); + mods[n] = n; + } + for (int n = 1; n <= nmods; n++) + { + if (!is_mod_set (g, n)) + { + unload_mod (n); + mods[n] = 0; + } + } + + for (int t = 0; t < array_length (tlsmanydeps); t++) + { + char *moddepname = xasprintf ("tst-tls-manydynamic%dmod-dep.so", + tlsmanydeps[t].modi); + void *moddep = xdlopen (moddepname, RTLD_LAZY); + + /* Access TLS in all loaded modules. */ + struct start_args args = + { + moddepname, + moddep, + tlsmanydeps[t].modi, + tlsmanydeps[t].ndeps, + tlsmanydeps[t].deps + }; + pthread_t t = xpthread_create (0, start, &args); + xpthread_join (t); + + free (moddepname); + xdlclose (moddep); + } + + for (int n = 1; n <= nmods; n++) + if (mods[n] != 0) + unload_mod (n); + } +} + +/* The following test check DTV gaps handling with shared libraries that has + invalid dependencies. It defines 5 different sets: + + 1. Single dependency: + mod0 -> invalid + 2. Double dependency: + mod1 -> [mod2,invalid] + 3. Double dependency with each dependency depent of another module: + mod3 -> [mod4,mod5] -> invalid + 4. Long chain with one double dependency in the middle: + mod6 -> [mod7, mod8] -> mod12 -> invalid + 5. Long chain with two double depedencies in the middle: + mod10 -> mod11 -> [mod12, mod13] + mod12 -> [mod14, invalid] + + This does not cover all the possible gaps and configuration, but it + should check if different dynamic shared sets are placed correctly in + different gaps configurations. */ + +static void +do_test_invalid_dependency (bool bind_now) +{ + static const int tlsmanydeps[] = { 0, 1, 3, 6, 10 }; + + /* The gap configuration is defined as a bitmap: the bit set represents a + loaded module prior the tests execution, while a bit unsed is a module + unloaded. Not all permtation will show gaps, but it is simpler than + define each one independently. */ + for (uint32_t g = 0; g < 64; g++) + { + print_gap (g); + int nmods = nmodules (g); + + int mods[nmods]; + /* We use '0' as indication for a gap, to avoid the dlclose on iteration + cleanup. */ + for (int n = 1; n <= nmods; n++) + { + load_mod (n); + mods[n] = n; + } + for (int n = 1; n <= nmods; n++) + { + if (!is_mod_set (g, n)) + { + unload_mod (n); + mods[n] = 0; + } + } + + for (int t = 0; t < array_length (tlsmanydeps); t++) + { + char *moddepname = xasprintf ("tst-tls-manydynamic%dmod-dep-bad.so", + tlsmanydeps[t]); + void *moddep; + if (bind_now) + { + moddep = dlopen (moddepname, RTLD_NOW); + TEST_VERIFY (moddep == 0); + } + else + moddep = dlopen (moddepname, RTLD_LAZY); + + /* Access TLS in all loaded modules. */ + pthread_t t = xpthread_create (0, start, NULL); + xpthread_join (t); + + free (moddepname); + if (!bind_now) + xdlclose (moddep); + } + + for (int n = 1; n <= nmods; n++) + if (mods[n] != 0) + unload_mod (n); + } +} + +static int +do_test (void) +{ + do_test_no_depedency (); + do_test_dependency (); + do_test_invalid_dependency (true); + do_test_invalid_dependency (false); + return 0; } diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h index 176394de4d..9c15259236 100644 --- a/sysdeps/generic/ldsodefs.h +++ b/sysdeps/generic/ldsodefs.h @@ -1171,8 +1171,8 @@ extern ElfW(Addr) _dl_sysdep_start (void **start_argptr, extern void _dl_sysdep_start_cleanup (void) attribute_hidden; -/* Determine next available module ID. */ -extern size_t _dl_next_tls_modid (void) attribute_hidden; +/* Determine next available module ID and set the L l_tls_modid. */ +extern void _dl_assign_tls_modid (struct link_map *l) attribute_hidden; /* Count the modules with TLS segments. */ extern size_t _dl_count_modids (void) attribute_hidden;