2.25 freeze status
diff mbox

Message ID f2ee2fed-3430-d9a9-3cee-64a8626e2905@redhat.com
State New
Headers show

Commit Message

Florian Weimer Jan. 27, 2017, 4:46 p.m. UTC
On 01/27/2017 05:27 PM, Szabolcs Nagy wrote:
> On 27/01/17 07:47, Florian Weimer wrote:
>> On 01/27/2017 05:06 AM, Siddhesh Poyarekar wrote:
>>> Hi,
>>>
>>> The release date of 1 Feb is upon us and there are 3 release blockers
>>> that haven't been resolved yet:
>>>
>>> - global-dynamic TLS broken on aarch64 and others
>>
>> Fix is known (revert part of a faulty commit), it just needs review.
>
> i think the hunk mentioned in
> https://sourceware.org/bugzilla/show_bug.cgi?id=20915
> should be just reverted without further review.
>
> writing to the dtv of other threads is neither
> necessary nor correct.

Let's do it then.  Is this patch okay?

Thanks,
Florian

Comments

Szabolcs Nagy Jan. 27, 2017, 5:12 p.m. UTC | #1
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.
Alexandre Oliva Feb. 1, 2017, 7:37 a.m. UTC | #2
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,
Szabolcs Nagy Feb. 1, 2017, 10:36 a.m. UTC | #3
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,
>
Alexandre Oliva Feb. 1, 2017, 2:47 p.m. UTC | #4
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.
Szabolcs Nagy Feb. 1, 2017, 2:56 p.m. UTC | #5
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.
Carlos O'Donell Feb. 1, 2017, 4:06 p.m. UTC | #6
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.

Patch
diff mbox

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);
 }