Re: [PATCH] Reserve static TLS for dynamically loaded initial-exec TLS only [BZ #25051]

[Carlos O'Donell <codonell@redhat.com>]

On 2/13/20 11:38 AM, Szabolcs Nagy wrote:
> On 07/01/2020 12:50, Szabolcs Nagy wrote:
>> On 07/01/2020 12:09, Florian Weimer wrote:
>>> * Szabolcs Nagy:
>>>
>>>> This patch reserves 128 bytes of the surplus TLS that is not used
>>>> opportunistically. TLS_STATIC_SURPLUS is currently 1664, so this still
>>>> allows 1536 bytes for opportunistic use. A new test is added to verify
>>>> this ABI contract: dynamic loading of libraries with initial-exec TLS
>>>> is supported up to 128 bytes in total on all targets. This should be
>>>> enough for system libraries such as libgomp.
>>>
>>> I'm not sure if it's enough for loading another libc.so.6 via dlmopen.
>>> Have you tested this, by chance?
>>
>> i haven't tested, but that wont work reliably with
>> this patch.
>>
>> libc.so on aarch64 has 144 byte TLS (8byte alignmed),
>> so the reserved 128byte surplus TLS is not enough
>> (can be increased to 144 though).
>>
>> however if a lib with ie TLS is loaded before the static
>> TLS runs out then that works: the 128byte reserve will be
>> kept available until the 'opportunistic' part of TLS runs
>> out and then the reserve can only be used for ie TLS.
>> (so early dlmopen of libc.so.6 and later dlopen of libgomp
>> works)
> 
> i plan to work on this, but
> 
> elf/dl-tls.c has
> 
> /* Amount of excess space to allocate in the static TLS area
>    to allow dynamic loading of modules defining IE-model TLS data.  */
> #define TLS_STATIC_SURPLUS	64 + DL_NNS * 100
> 
> with DL_NNS == 16 this gives 1664 byte surplus tls (+4 byte
> extra in some cases because of alignment holes), but ideally
> it should be
> 
> 15 * 144 +  // 2160 bytes for dlmopen of libc.so.6
> 16 * 128 +  // 2048 bytes for dlopen/dlmopen of libgomp
> 16 * 8      // 128 bytes for dlopen/dlmopen of libGL* ?
> 
> = 4336 bytes.

Yes.

Each loaded library in dlmopen will get a unique map->l_tls_offset.

Each library will take a block from the static tls.

All of this is handled by _dl_try_allocate_static_tls in elf/dl-reloc.c.

> (i think if rseq is committed libc tls might increase to
> 192 bytes, because of the 32 byte alignment of __rseq_abi,
> with that it's 5056 bytes.)

Yes.

> my understanding is that namespaces always need at least
> surplus tls for libc (except for the first namespace) so
> supporting DL_NNS == 16 would need 2160 bytes at least
> (the current surplus only allows 12 namespaces at most).

Yes.

> i think either the supported namespaces should be reduced
> (is that an option?) or the surplus tls size increased.
> we also need a policy when non-ie tls libs or ie tls libs
> other than libc/libgomp/libGL may use the surplus tls.

Yes, we can reduce DL_NNS IMO, you could reduce it to 4 if
you wanted, but I'll want a way to increase it too. See my
suggestions.

Let me take a stab at this:

(1) File a bug to indicate all libraries must stop using
    TLS IE to fix dlmopen issues with TLS IE.

(2) Set DL_NNS to 4, and add a tunable that lets you change
    the limit to reduce surplus memory allocated.

    (2.a) send linux man-pages a patch to make dlmopen(3)
    ambiguous about how many namespaces are supported and
    to point at the tunable to increase the memory used.

(3) I think that the surplus should be split into two
    blocks:
    (3.a) Some reserved for TLS IE libs.
    (3.b) Some reserved for tlsdesc optimization.

    My opinion is that any library that is loaded and
    needs TLS IE should be able to use (3.a). We should
    reserve enough for (3.a) to be able to load glibc
    libraries, libgomp, and libgl (as you note).

> example solution:
> 
> #define DL_NNS 16
> #define DL_NNS_SUPPORTED 4
> #define LIBC_TLS 200
> #define IE_LIB_TLS 144
> #define NON_IE_LIB_TLS 1000

In the final patch these need way more comments to explain
how you arrived at the magic numbers and the intent of your
computation.

> 
> #define TLS_STATIC_SURPLUS \
>  (DL_NNS_SUPPORTED - 1) * LIBC_TLS /* 600 bytes.  */ \
>  + DL_NNS_SUPPORTED * IE_LIB_TLS /* 576 bytes.    */ \
>  + NON_IE_LIB_TLS
> /* = 2176 bytes surplus tls.  */
> 
> opportunistic tls use for non-ie tls libs is only allowed
> up to 1000 bytes and 4 namespaces can be supported at all
> times, beyond that dlmopen may not work (opportunistic tls
> use is target dependent so the dlmopen limit will be so too).
> 
> does this make sense?

Yes, but I'm suggesting you just change DL_NNS to 4 and allow
a tunable override.

... and as always MOAR COMMENTS.

-- 
Cheers,
Carlos.