Page MenuHomeFreeBSD

rtld/arm: fix initial-exec (IE) thread-local storage relocation
ClosedPublic

Authored by rcm on Oct 31 2023, 6:48 PM.
Tags
Referenced Files
Unknown Object (File)
Sun, May 5, 9:01 PM
Unknown Object (File)
Sun, May 5, 12:51 PM
Unknown Object (File)
Tue, Apr 30, 11:39 PM
Unknown Object (File)
Sun, Apr 28, 12:31 AM
Unknown Object (File)
Jan 25 2024, 5:44 AM
Unknown Object (File)
Jan 25 2024, 5:44 AM
Unknown Object (File)
Jan 25 2024, 5:44 AM
Unknown Object (File)
Jan 25 2024, 5:44 AM

Details

Summary

net/frr[89] revealed an interesting edge-case on arm when dynamically linking
a shared library that declares more than one static TLS variable with at least one
using the "initial-exec" TLS model. In the case of frr[89], this library was libfrr.so
which essentially does the following:

#include <stdio.h>

#include "lib.h"

static __thread int *a
	__attribute__((tls_model("initial-exec")));

void lib_test()
{
	static __thread int b = -1;

	printf("&a = %p\n", &a);
	printf(" a = %p\n", a);

	printf("\n");

	printf("&b = %p\n", &b);
	printf(" b = %d\n", b);
}

Allocates a file scoped static __thread pointer with tls_model("initial-exec") and
later a block scoped TLS int. Notice in the above minimal reproducer, b == -1.
The relocation process does the wrong thing and ends up pointing both a and b
at the same place in memory.

The output of the above in the broken state is:

&a = 0x4009c018
 a = 0xffffffff

&b = 0x4009c018
 b = -1

With the patch applied, the output becomes:

&a = 0x4009c01c
 a = 0x0

&b = 0x4009c018
 b = -1

Sponsored by: Rubicon Communications, LLC ("Netgate")

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

rcm requested review of this revision.Oct 31 2023, 6:48 PM
kp added a subscriber: kp.
libexec/rtld-elf/arm/reloc.c
247

I am not sure this is right. Spec says R_ARM_TLS_DTPOFF32 Resolves to the index of the specified TLS symbol within its TLS block. There is no mention of application of the addend, and it is not logical.

264

This change definitely does not have sense. DTV index has nothing to do with the location value.

286–287

Again, spec states

R_ARM_TLS_TPOFF32
(S ≠ 0) Resolves to the offset of the specified TLS symbol, S, from the Thread Pointer, TP.
(S = 0) Resolves to the offset of the current module’s TLS block from the Thread Pointer, TP (the addend contains the offset of the local symbol within the TLS block).

So it might be that the fix is only needed for specific value of the symbol. Can you run the test program with LD_DEBUG=1 and see which specific relocation you encounter?

libexec/rtld-elf/arm/reloc.c
247

Thank you for the link. I'm very much novice when it comes to low-level work like this. This is a good resource. I was mostly hacking on this trying to get my test program to run.

libexec/rtld-elf/arm/reloc.c
286–287

I'm following you, thanks for the clue. The problematic relocation is indeed R_ARM_TLS_TPOFF32 I am working on a follow-up diff.

I studied what NetBSD is doing here. Here is an updated test program:

#include <stdio.h>
#include <assert.h>

#include "lib.h"

__attribute__((tls_model("initial-exec")))
static __thread int *a = (void *)0xafafafaf;

__attribute__((tls_model("initial-exec")))
static __thread int *b = (void *)0xfafafafa;

void lib_test(void)
{
	static __thread int c = -1;
	static __thread int d = 42;

	printf("&a = %p\n", &a);
	printf(" a = %p\n", a);
	printf("&b = %p\n", &b);
	printf(" b = %p\n", b);

	printf("&c = %p\n", &c);
	printf(" c = %d\n", c);
	printf("&d = %p\n", &d);
	printf(" d = %d\n", d);
}

output:

&a = 0x4009b028
 a = 0xafafafaf
&b = 0x4009b02c
 b = 0xfafafafa
&c = 0x4009b018
 c = -1
&d = 0x4009b01c
 d = 42
rcm marked 3 inline comments as done.Nov 1 2023, 7:10 PM
libexec/rtld-elf/arm/reloc.c
283–284

Elsewhere these are generally just folded into one if/else with a tiny bit of code duplication for the calculation

287

What's with the parentheses?

289

Gratuitous whitespace changes here and below

If IE is fixed then lib/libc/Makefile probably should enable it on arm as a follow-up, which I *think* is the only architecture not covered by that if statement, unless I'm missing something

libexec/rtld-elf/arm/reloc.c
283–284

Thank you, I will follow suit

287

monkey see, monkey do

(new patch is forthcoming)

289

bleh. yes, I will clean up.

rcm marked an inline comment as done.Nov 1 2023, 7:34 PM

I have now reduced the diff down to the minimum. Thanks all for your continued comments.

libexec/rtld-elf/arm/reloc.c
285

This isn't quite right though, look at what other relocations do, e.g. R_ARM_ABS32; you need to update tmp otherwise the dbg will print the wrong value

libexec/rtld-elf/arm/reloc.c
285

ah shoot, thanks.

rcm marked an inline comment as done.Nov 1 2023, 7:45 PM

Ensure we update tmp for dbg output

If IE is fixed then lib/libc/Makefile probably should enable it on arm as a follow-up, which I *think* is the only architecture not covered by that if statement, unless I'm missing something

That appears to be correct.

rcm retitled this revision from rtld/arm: fix thread-local storage relocation to rtld/arm: fix initial-exec (IE) thread-local storage relocation.Nov 1 2023, 7:59 PM
libexec/rtld-elf/arm/reloc.c
284
287

collapse tmp = *where + tmp -> tmp += *where

rcm marked an inline comment as done.
rcm marked 4 inline comments as done.

Thanks for suggestions.

This revision is now accepted and ready to land.Nov 3 2023, 5:21 AM

I see you have a reduced test case, would it be possible to turn it into a regression test for in the ld-elf tests?

I see you have a reduced test case, would it be possible to turn it into a regression test for in the ld-elf tests?

Yep. Great suggestion. I will take care of that in a forthcoming patch. This was committed already by kp@ and it wasn't auto closed.

https://cgit.freebsd.org/src/commit/?id=98fd69f0090da73d9d0451bd769d7752468284c6