[PATCH v2 1/1] rtld: add glibc.rtld.optional_static_tls_alignment

[Florian Weimer]

* DJ Delorie:

>  ifeq (no,$(build-hardcoded-path-in-tests))
> @@ -1084,6 +1085,7 @@ modules-names += \
>    tst-tls22-mod2-gnu2 \
>    tst-tls23-mod \
>    tst-tlsalign-lib \
> +  tst-tlsalign-tunable-a \

We usually call those auditmods, so tst-tlsalign-tunable-auditmod.

>    tst-tlsgap-mod0 \
>    tst-tlsgap-mod1 \
>    tst-tlsgap-mod2 \
> @@ -1824,6 +1826,12 @@ $(objpfx)tst-nodelete-opened.out: $(objpfx)tst-nodelete-opened-lib.so
>  $(objpfx)tst-tlsalign-extern: $(objpfx)tst-tlsalign-vars.o
>  $(objpfx)tst-tlsalign-extern-static: $(objpfx)tst-tlsalign-vars.o
>  
> +tst-tlsalign-tunable-TUNABLES = glibc.rtld.optional_static_tls_alignment=0x1000
> +tst-tlsalign-tunable-TUNABLES += glibc.rtld.optional_static_tls=0x3000
> +tst-tlsalign-tunable-ENV = LD_AUDIT=$(objpfx)tst-tlsalign-tunable-a.so
> +#tst-tlsalign-tunable-ENV = LD_DEBUG=all

Leftover comment?

> +$(objpfx)tst-tlsalign-tunable.out: $(objpfx)tst-tlsalign-tunable-a.so
> +
>  # The resolver translation unit must always be compiled with
>  # -fstack-protector-all so the canary code is emitted regardless of the
>  # default.
> diff --git a/elf/dl-tls.c b/elf/dl-tls.c
> index 1380bd7083..e4c93bace0 100644
> --- a/elf/dl-tls.c
> +++ b/elf/dl-tls.c
> @@ -126,6 +126,8 @@ tls_static_surplus (int nns, int opt_tls)
>     backwards compatibility.  */
>  #define LEGACY_TLS (1664 - tls_static_surplus (DEFAULT_NNS, OPTIONAL_TLS))
>  
> +static size_t optional_static_tls_alignment = 0;

Maybe add a comment where this value comes from?

>  /* Calculate the size of the static TLS surplus, when the given
>     number of audit modules are loaded.  Must be called after the
>     number of audit modules is known and before static TLS allocation.  */
> @@ -136,6 +138,7 @@ _dl_tls_static_surplus_init (size_t naudit)
>  
>    nns = TUNABLE_GET (nns, size_t, NULL);
>    opt_tls = TUNABLE_GET (optional_static_tls, size_t, NULL);
> +  optional_static_tls_alignment = TUNABLE_GET (optional_static_tls_alignment, size_t, NULL);

I think this should ignore the value if it's not a power of two.

> diff --git a/elf/tst-tlsalign-tunable-a.c b/elf/tst-tlsalign-tunable-a.c
> new file mode 100644
> index 0000000000..d1a2063e39
> --- /dev/null
> +++ b/elf/tst-tlsalign-tunable-a.c
> @@ -0,0 +1,32 @@
> +#include <unistd.h>
> +
> +__thread int x;
> +__thread int b[4] __attribute__((tls_model("initial-exec"))) __attribute__((aligned(0x1000)));

This line is too long, and there are missing spaces before '('.  There
are more such missing spaces below.

Please also add the usual file header.

> +/* These exist just to make the above variables "used".  */
> +int *
> +func_x(void)
> +{
> +  return &x;
> +}

I guess this is needed because static TLS allocation can in theory be
reference-based.

> diff --git a/elf/tst-tlsalign-tunable.c b/elf/tst-tlsalign-tunable.c
> new file mode 100644
> index 0000000000..3ef2ae2e14
> --- /dev/null
> +++ b/elf/tst-tlsalign-tunable.c

> +static int
> +do_test (void)
> +{
> +  printf("tst-tlsalign-tunable!\n");
> +  /* If the test passes, the auditor itself will call exit with a
> +     value of 0.  If we get here, we've failed.  "Use" tdata1 here
> +     too.  */
> +  return tdata1 ? EXIT_FAILURE : 43;
> +}

I think you need to make tdata1 volatile, otherwise this is still
optimized out (because tdata1 is static and does not have its address
taken).

> diff --git a/manual/tunables.texi b/manual/tunables.texi
> index 713f669c4c..772e55167a 100644
> --- a/manual/tunables.texi
> +++ b/manual/tunables.texi
> @@ -422,6 +422,14 @@ changed once allocated at process startup.  The default allocation of
>  optional static TLS is 512 bytes and is allocated in every thread.
>  @end deftp
>  
> +@deftp Tunable glibc.rtld.optional_static_tls_alignment
> +Sets a minimum alignment for the static TLS.  The loader normally uses
> +the alignment requirements of shared objects, but this might be needed
> +for audit modules that have a higher-than-default alignment
> +requirement, as an alternative to requesting excessive
> +optional_static_tls (above).
> +@end deftp

It's not an alternative, both can be needed.  It's also not just about
the auditor, but anything loaded in audit mode counts (whether by the
auditor or anything else).

Thanks,
Florian