| Message ID | f2ee2fed-3430-d9a9-3cee-64a8626e2905@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 27/01/17 16:46, Florian Weimer wrote: > On 01/27/2017 05:27 PM, Szabolcs Nagy wrote: >> i think the hunk mentioned in >> https://sourceware.org/bugzilla/show_bug.cgi?id=20915 >> should be just reverted without further review. > > Let's do it then. Is this patch okay? ... > nptl: Do not overwrite the DTV of other threads [BZ #20915] > > This reverts part of commit 17af5da98cd2c9ec958421ae2108f877e0945451. > > 2017-01-27 Florian Weimer <fweimer@redhat.com> > > [BZ #20915] > * nptl/allocatestack.c (init_one_static_tls): Do not write to the > DTV of other threads. looks ok to me.
On Jan 27, 2017, Florian Weimer <fweimer@redhat.com> wrote: >> writing to the dtv of other threads is neither >> necessary nor correct. > Let's do it then. Is this patch okay? I see you're taken out the reversal of * elf/dl-reloc.c (_dl_nothread_init_static_tls) from the patch I proposed back on Sept 24 for BZ #19826. I don't think it is right to drop that part. That's the nptl-less TLS initializer analogous to * nptl/allocatestack.c (init_one_static_tls) in programs that link with libpthread. Before the fix for BZs #17090, #17620, #17621 and #17628 (f8aeae3473), we'd just abort if the static TLS modid grew past the initial DTV size. I'm not sure there's code in place to resize the DTV within dlopen before calling GL(dl_init_static_tls), that resolves to either _dl_nothread_init_static_tls, that deals with the single thread, or __pthread_init_static_tls, that calls init_one_static_tls for each thread, so dlopening multiple static-TLS libs in a program not linked with libpthread might abort at the assert that checks that the modid is lower than the DTV length. Unless you've verified that this is safe, I'd strongly advise against dropping the analogous change for single-threaded programs from the patch I had proposed. I'd also suggest adding comments to the effect that 'we leave it for tls_get_addr to update each thread's DTV, since it might need resizing'. Thanks for taking care of this,
On 01/02/17 07:37, Alexandre Oliva wrote: > On Jan 27, 2017, Florian Weimer <fweimer@redhat.com> wrote: > >>> writing to the dtv of other threads is neither >>> necessary nor correct. > >> Let's do it then. Is this patch okay? > > I see you're taken out the reversal of > > * elf/dl-reloc.c (_dl_nothread_init_static_tls) > > from the patch I proposed back on Sept 24 for BZ #19826. > > I don't think it is right to drop that part. > > That's the nptl-less TLS initializer analogous to > > * nptl/allocatestack.c (init_one_static_tls) > > in programs that link with libpthread. but it does not touch the dtv of other threads. so it does not seem harmful only inconsistent. > Before the fix for BZs #17090, #17620, #17621 and #17628 (f8aeae3473), > we'd just abort if the static TLS modid grew past the initial DTV size. > I'm not sure there's code in place to resize the DTV within dlopen > before calling GL(dl_init_static_tls), that resolves to either > _dl_nothread_init_static_tls, that deals with the single thread, or > __pthread_init_static_tls, that calls init_one_static_tls for each > thread, so dlopening multiple static-TLS libs in a program not linked > with libpthread might abort at the assert that checks that the modid is > lower than the DTV length. _dl_update_slotinfo_list is called there which does _dl_resize_dtv so it's fine. https://sourceware.org/git/?p=glibc.git;a=blob;f=elf/dl-open.c;h=cec54db413cc47e5840ebf94d2d9881bf1f7bf34#l547 (_dl_resize_dtv aborts in case of allocation failure, so dlopen should do this differently, but oom crash is a separate long standing issue) > Unless you've verified that this is safe, I'd strongly advise against > dropping the analogous change for single-threaded programs from the > patch I had proposed. reverting both hunks is also fine with me if you consider that safer. i thought it would be easier to get a quick consensus if we only involve the problematic hunk. > I'd also suggest adding comments to the effect that 'we leave it for > tls_get_addr to update each thread's DTV, since it might need resizing'. > > Thanks for taking care of this, >
On Feb 1, 2017, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote: > On 01/02/17 07:37, Alexandre Oliva wrote: >> On Jan 27, 2017, Florian Weimer <fweimer@redhat.com> wrote: >> >>>> writing to the dtv of other threads is neither >>>> necessary nor correct. >> >>> Let's do it then. Is this patch okay? >> >> I see you're taken out the reversal of >> >> * elf/dl-reloc.c (_dl_nothread_init_static_tls) >> >> from the patch I proposed back on Sept 24 for BZ #19826. >> >> I don't think it is right to drop that part. >> >> That's the nptl-less TLS initializer analogous to >> >> * nptl/allocatestack.c (init_one_static_tls) >> >> in programs that link with libpthread. > but it does not touch the dtv of other threads. > so it does not seem harmful only inconsistent. No, it could be harmful: if the DTV needed resizing but it didn't perform resizing, the assert in _dl_nothread_init_static_tls would fail, causing the program to abort when it didn't have to. Reverting the change that brought the assert back would avoid that. > _dl_update_slotinfo_list is called guarded by #ifdef SHARED. Do you see it called in some other path within the to-be-statically-linked dl_open_worker? If not, this assert could fail not just when users called dlopen in statically-linked programs, but even when they called functions that rely on dlopening internally (nss comes to mind). IMHO, reverting both hunks is the conservative approach. We (well, I) know they were introduced unnecessarily, just for (false) symmetry, and we are sure at least one of them is harmful, and we have no hard evidence that the other isn't. Considering that the same code path takes us to each of them, my conservative assessment is to revert both, unless deeper analysis points strongly in a different direction.
On 01/02/17 14:47, Alexandre Oliva wrote: > On Feb 1, 2017, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote: >> _dl_update_slotinfo_list is called > > guarded by #ifdef SHARED. Do you see it called in some other path > within the to-be-statically-linked dl_open_worker? If not, this assert > could fail not just when users called dlopen in statically-linked > programs, but even when they called functions that rely on dlopening > internally (nss comes to mind). i assumed dlopen in a static linked program is unsupported (as it is broken when it is deployed to a system with different libc which is the main use of static linking) if that's supposed to work then i agree that both dtv setting hunks should be reverted.
On 02/01/2017 09:56 AM, Szabolcs Nagy wrote: > On 01/02/17 14:47, Alexandre Oliva wrote: >> On Feb 1, 2017, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote: >>> _dl_update_slotinfo_list is called >> >> guarded by #ifdef SHARED. Do you see it called in some other path >> within the to-be-statically-linked dl_open_worker? If not, this assert >> could fail not just when users called dlopen in statically-linked >> programs, but even when they called functions that rely on dlopening >> internally (nss comes to mind). > > i assumed dlopen in a static linked program is > unsupported (as it is broken when it is deployed > to a system with different libc which is the > main use of static linking) > > if that's supposed to work then i agree that > both dtv setting hunks should be reverted. We want dlopen from a static executable to work. There are some things that don't work, but I consider those bugs we need to solve anyway to get dlmopen working (the static->dynamic namespace issues are the same issues present in the ns1->ns2 namespace issues). Both dlopen from a static executable and dlmopen are important features.
nptl: Do not overwrite the DTV of other threads [BZ #20915] This reverts part of commit 17af5da98cd2c9ec958421ae2108f877e0945451. 2017-01-27 Florian Weimer <fweimer@redhat.com> [BZ #20915] * nptl/allocatestack.c (init_one_static_tls): Do not write to the DTV of other threads. diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c index 6402ea4..18d2001 100644 --- a/nptl/allocatestack.c +++ b/nptl/allocatestack.c @@ -1191,12 +1191,9 @@ init_one_static_tls (struct pthread *curp, struct link_map *map) # error "Either TLS_TCB_AT_TP or TLS_DTV_AT_TP must be defined" # endif - /* Fill in the DTV slot so that a later LD/GD access will find it. */ - dtv_t *dtv = GET_DTV (TLS_TPADJ (curp)); - dtv[map->l_tls_modid].pointer.to_free = NULL; - dtv[map->l_tls_modid].pointer.val = dest; - - /* Initialize the memory. */ + /* We cannot delay the initialization of the Static TLS area, since + it can be accessed with LE or IE, but since the DTV is only used + by GD and LD, we can delay its update to avoid a race. */ memset (__mempcpy (dest, map->l_tls_initimage, map->l_tls_initimage_size), '\0', map->l_tls_blocksize - map->l_tls_initimage_size); }