Page MenuHomeFreeBSD

Reduce size of rtld by 22% by pulling in less code from libc
ClosedPublic

Authored by arichardson on Jun 16 2019, 3:45 PM.

Details

Summary

Currently RTLD is linked against libc_nossp_pic which means that any libc
symbol used in rtld can pull in a lot of depedencies. This was causing
symbol such as __libc_interposing and all the pthread stubs to be included
in RTLD even though they are not required. It turns out most of these
dependencies can easily be avoided by providing overrides inside of rtld.

This change is motivated by CHERI, where we have an experimental ABI that
requires additional relocation processing to allow the use of function
pointers inside of rtld. Instead of adding this self-relocation code to
RTLD I attempted to remove most function pointers from RTLD and discovered
that most of them came from the libc dependencies instead of being actually
used inside rtld.

A nice side-effect of this change is that rtld is now 18% smaller on amd64.

text	   data	    bss	    dec	    hex	filename

0x21eb6 0xce0 0xe60 145910 239f6 /home/alr48/ld-elf-x86.before.so.1
0x1a6ed 0x728 0xdd8 113645 1bbed /home/alr48/ld-elf-x86.after.so.1

The number of R_X86_64_RELATIVE relocations that need to be processed on
startup has also gone down from 368 to 187 (almost 50% less).

Test Plan

Starting some binaries with /home/alr48/ld-elf-x86.after.so.1 works.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

arichardson created this revision.Jun 16 2019, 3:45 PM
kib added a comment.Jun 23 2019, 10:01 AM

I always wanted to remove -lc (or any currently used variant of libc) from the rtld linking. Ideally, we would just compile required libc sources second time for rtld, inside libexec/rtld-elf. I am sure that this would be a lot of work, but I am not sure that it would be much more work than continuing the approach of patching libc.a to cut off unneeded pieces.

lib/libc/gen/gen-private.h
56 ↗(On Diff #58707)

Can you keep the member for rtld, perhaps changing it to void * ? I want the structure to have the same layout inside rtld.

lib/libc/gen/opendir.c
103 ↗(On Diff #58707)

Commit this chunk on its own.

lib/libc/gen/telldir.c
67 ↗(On Diff #58707)

Again, commit this separately.

libexec/rtld-elf/debug.h
42 ↗(On Diff #58707)

Spurious blank line.

libexec/rtld-elf/rtld-libc/Makefile.inc
15 ↗(On Diff #58707)

Is this needed ?

23 ↗(On Diff #58707)

Is this a stray comment ?

84 ↗(On Diff #58707)

FIXME ?

libexec/rtld-elf/rtld-libc/rtld_avoid_libc_deps.h
33 ↗(On Diff #58707)

May be rename this header to rtld_libc.h ? The name is too long and I do not think that telling the story is the name is useful.

libexec/rtld-elf/rtld-libc/rtld_libc_deps.c
64 ↗(On Diff #58707)

should be return (val);. Please fix all the places below.

arichardson marked 7 inline comments as done.Jun 23 2019, 10:50 AM
In D20663#448138, @kib wrote:

I always wanted to remove -lc (or any currently used variant of libc) from the rtld linking. Ideally, we would just compile required libc sources second time for rtld, inside libexec/rtld-elf. I am sure that this would be a lot of work, but I am not sure that it would be much more work than continuing the approach of patching libc.a to cut off unneeded pieces.

There are not actually that many files from libc that are used in rtld. Here is the -Wl,--trace output:

/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(__xuname.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(errlst.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(getcwd.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(raise.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(sigsetops.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(sysctl.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(sysctlnametomib.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(sigsetjmp.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(lstat.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(stat.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(fstat.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(fstatat.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(fstatfs.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(getdirentries.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(syscall.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(cerror.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(geteuid.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(getegid.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(munmap.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(mprotect.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(sysarch.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(__sysctl.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(issetugid.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(__getcwd.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(utrace.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(thr_self.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(thr_kill.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(pread.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(mmap.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(lseek.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(_exit.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(_fstat.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(_fstatat.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(_fstatfs.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(_getdirentries.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(_close.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(_fcntl.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(_open.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(_openat.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(_read.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(_sigprocmask.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(_write.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(memcpy.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(strcat.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(strcmp.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(getenv.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(getprogname.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(memmove.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(merge.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(reallocarray.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(reallocf.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(realpath.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(readlink.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(bcopy.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(stpncpy.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(strchr.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(strcpy.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(stpcpy.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(strcspn.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(strdup.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(strlcat.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(strlcpy.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(strlen.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(strncmp.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(strncpy.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(strrchr.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(strsep.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(strspn.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(strstr.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(memchr.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(memcmp.nossppico)
/home/alr48/obj/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/lib/libc/libc_nossp_pic.a(strtok.nossppico)

I think if we linked against a libsyscalls (D14609) we only really need to build the string-related files (plus a few more which seem to mostly come from getenv()) again.

lib/libc/gen/gen-private.h
56 ↗(On Diff #58707)

Sure, sounds good. I only added the ifdef to make sure that the lock is not accidentally being used. I chose not to use void* since that will implicitly convert to struct pthread_mutex *

libexec/rtld-elf/rtld-libc/Makefile.inc
15 ↗(On Diff #58707)

This is needed for CHERI where we convert the variadic system call to always pass the optional argument (since the variadic calling convention is different and passes all arguments on the stack.

I forgot to remove that part of the diff and will only add it to CheriBSD.

23 ↗(On Diff #58707)

I could keep this in under .ifdef DEBUG_RTLD_LIBC_DEPENDENCIES so that we can check that other architectures don't accidentally pull in more files (I only tested MIPS and x86).

arichardson marked an inline comment as done.
  • Address feedback
  • Only enable link error logic with RTLD_VERIFY_LIBC_DEPENDENCIES=1 since it otherwise builds many unnecessary .o files (this can go away when we stop linking against libc).
arichardson added inline comments.Jun 23 2019, 11:44 AM
libexec/rtld-elf/rtld.c
72 ↗(On Diff #58907)

I added all these at the end of the include list, should I insert them alphabetically instead?

kib added a comment.Jun 23 2019, 3:44 PM
In D20663#448138, @kib wrote:

I always wanted to remove -lc (or any currently used variant of libc) from the rtld linking. Ideally, we would just compile required libc sources second time for rtld, inside libexec/rtld-elf. I am sure that this would be a lot of work, but I am not sure that it would be much more work than continuing the approach of patching libc.a to cut off unneeded pieces.

There are not actually that many files from libc that are used in rtld. Here is the -Wl,--trace output:
...
I think if we linked against a libsyscalls (D14609) we only really need to build the string-related files (plus a few more which seem to mostly come from getenv()) again.

If there are not that many files, I would prefer to directly add them to the build (or just add .o files from libc nossp pic) instead of adding hackery to cut libc internal mechanisms. Mostly because that adding interposable symbol to libc becomes even more tricky, with no clear indication to the author that he forget or not aware about rtld.

I know about libsyscall, but the existing review seems to be not reviewable, and a way to get that in is to re-do it from scratch.

arichardson added a comment.EditedJun 23 2019, 3:49 PM
In D20663#448175, @kib wrote:
In D20663#448138, @kib wrote:

I always wanted to remove -lc (or any currently used variant of libc) from the rtld linking. Ideally, we would just compile required libc sources second time for rtld, inside libexec/rtld-elf. I am sure that this would be a lot of work, but I am not sure that it would be much more work than continuing the approach of patching libc.a to cut off unneeded pieces.

There are not actually that many files from libc that are used in rtld. Here is the -Wl,--trace output:
...
I think if we linked against a libsyscalls (D14609) we only really need to build the string-related files (plus a few more which seem to mostly come from getenv()) again.

If there are not that many files, I would prefer to directly add them to the build (or just add .o files from libc nossp pic) instead of adding hackery to cut libc internal mechanisms. Mostly because that adding interposable symbol to libc becomes even more tricky, with no clear indication to the author that he forget or not aware about rtld.
I know about libsyscall, but the existing review seems to be not reviewable, and a way to get that in is to re-do it from scratch.

Okay, I'll try extracting the required files from libc_nossp_pic.a using ar and link against those files. Is that what you were suggesting? I would prefer building all the .o files, but I think there might be some per-architecture assembly files that need to be included and adding that logic to the makefile is slightly annoying.

kib added a comment.Jun 23 2019, 4:07 PM

Okay, I'll try extracting the required files from libc_nossp_pic.a using ar and link against those files. Is that what you were suggesting? I would prefer building all the .o files, but I think there might be some per-architecture assembly files that need to be included and adding that logic to the makefile is slightly annoying.

Ok for me, but I wonder how would it work. Basically, to avoid the interposing code, you need to use _xxx or __sys_xxx symbols directly, right ? Then if you do, wouldn't simply linking with libc_nossp_pic.a enough ?

In D20663#448177, @kib wrote:

Okay, I'll try extracting the required files from libc_nossp_pic.a using ar and link against those files. Is that what you were suggesting? I would prefer building all the .o files, but I think there might be some per-architecture assembly files that need to be included and adding that logic to the makefile is slightly annoying.

Ok for me, but I wonder how would it work. Basically, to avoid the interposing code, you need to use _xxx or __sys_xxx symbols directly, right ? Then if you do, wouldn't simply linking with libc_nossp_pic.a enough ?

I just started on the ar variant and it is kinda awkward. So yes, I think building all the machine-independent .c files in rtld and then linking against libc_nossp_pic.a for the system calls seems like a good idea. Then we can change that to link against libsyscalls once that review is ready.

Extract the required .o files from libc_nossp_pic
Size reduction is now 22% instead of 18%

arichardson retitled this revision from Reduce size of rtld by 18% by pulling in less code from libc to Reduce size of rtld by 22% by pulling in less code from libc.Jun 23 2019, 5:58 PM
arichardson edited the summary of this revision. (Show Details)
arichardson added inline comments.
lib/libc/stdlib/realpath.c
94 ↗(On Diff #58916)

I will commmit these three signed/unsigned comparison warnings separately. Didn't mean to include them in this diff.

Fix build for non-x86 and non-MIPS64 architectures

kib added a comment.Jun 26 2019, 12:57 PM

Overall, I like where you get at. The realpath.c and rtld-elf/debug.h changes can be already committed separately.

libexec/rtld-elf/rtld-libc/Makefile.inc
16 ↗(On Diff #59047)

Is that happens due to -DIN_RTLD or because of rtld-hacked namespace.h ?

libexec/rtld-elf/rtld-libc/errlst.h
33 ↗(On Diff #59047)

Why can't we ? I think that using SRCTOP in Makefile would be much more clean, and perhaps even allows to remove rtld-libc/errlst.h at all.

Remove the unncessary errlst.h header

arichardson marked an inline comment as done.Jun 26 2019, 2:25 PM
arichardson added inline comments.
libexec/rtld-elf/rtld-libc/Makefile.inc
16 ↗(On Diff #59047)

I'm afraid I don't understand the question. Are you asking why we need those files for CHERI? For all non-CHERI
architectures it is fine to call the system call with a variadic calling convention, but for CHERI we need the wrapper that fetches the optional argument from the stack and places it in the register that is expected by the syscall.

libexec/rtld-elf/rtld-libc/errlst.h
33 ↗(On Diff #59047)

Seems like I had the CFLAGS messed up. It does work without adding this files.

kib added inline comments.Jun 26 2019, 3:59 PM
libexec/rtld-elf/rtld-libc/Makefile.inc
16 ↗(On Diff #59047)

No, I am asking, what magic gets rid of the interposing calls. Your comment claims that this is because of -DIN_RTLD defined while *dir.c files are compiled, but IMO it happens because you provide different namespace.h which directs interposed calls to non-interposed functions.

libexec/rtld-elf/rtld-libc/rtld_libc.c
104 ↗(On Diff #59053)

mib and value can be defined on the same line.

106 ↗(On Diff #59053)

No need for this blank line.

arichardson marked 6 inline comments as done.

fix a style issue and expand comment on how we avoid the use of __libc_interposing

arichardson added inline comments.Jun 26 2019, 4:06 PM
libexec/rtld-elf/rtld-libc/Makefile.inc
16 ↗(On Diff #59047)

Yes, it is a combination of the two. For opendir.o it's both the ifdefs and the macros that redirect to __sys_foo

kib added a comment.Jun 26 2019, 4:24 PM

What testing was done ? Did you boot amd64 multiuser with the patched rtld ?

libthr consumes some bits from rtld, mostly rtld_malloc.c.

libexec/rtld-elf/rtld-libc/Makefile.inc
16 ↗(On Diff #59047)

Which specific #ifdef do you mean there ?

In D20663#449299, @kib wrote:

What testing was done ? Did you boot amd64 multiuser with the patched rtld ?
libthr consumes some bits from rtld, mostly rtld_malloc.c.

libthr still builds fine (I did a full universe build).

For testing I booted MIPS64 multiuser with these changes and ran a few programs with rtld direct exec on amd64. I've also been running CheriBSD with these changes for a few weeks.

libexec/rtld-elf/rtld-libc/Makefile.inc
16 ↗(On Diff #59047)

-DIN_RTLD

kib added a comment.Jun 26 2019, 5:11 PM
In D20663#449299, @kib wrote:

What testing was done ? Did you boot amd64 multiuser with the patched rtld ?
libthr consumes some bits from rtld, mostly rtld_malloc.c.

libthr still builds fine (I did a full universe build).

Did you do any runtime testing of threaded apps ?

For testing I booted MIPS64 multiuser with these changes and ran a few programs with rtld direct exec on amd64. I've also been running CheriBSD with these changes for a few weeks.

I believe that for such changes, a multiuser boot on amd64 (or i386) is required.

libexec/rtld-elf/rtld-libc/Makefile.inc
16 ↗(On Diff #59047)

I should be extremely explicit. Please give me the line number in opendir.c which contains that #ifdef.

In D20663#449349, @kib wrote:
In D20663#449299, @kib wrote:

What testing was done ? Did you boot amd64 multiuser with the patched rtld ?
libthr consumes some bits from rtld, mostly rtld_malloc.c.

libthr still builds fine (I did a full universe build).

Did you do any runtime testing of threaded apps ?

I did not try any threaded applications yet. Do you have any suggestions what would be a good test case?

For testing I booted MIPS64 multiuser with these changes and ran a few programs with rtld direct exec on amd64. I've also been running CheriBSD with these changes for a few weeks.

I believe that for such changes, a multiuser boot on amd64 (or i386) is required.

Okay, I'll report back once I've booted on amd64.

libexec/rtld-elf/rtld-libc/Makefile.inc
16 ↗(On Diff #59047)

Sorry I got confused here. -DIN_RTLD only affects the CheriBSD-specific compiles of fctnl.c and open.c. In this patch it doesn't actually change anything in opendir. The changes in opendir are due to the defines at the start of rtld_libc.h to avoid calling pthreads functions.

kib added a comment.Jun 26 2019, 6:23 PM
In D20663#449349, @kib wrote:
In D20663#449299, @kib wrote:

What testing was done ? Did you boot amd64 multiuser with the patched rtld ?
libthr consumes some bits from rtld, mostly rtld_malloc.c.

libthr still builds fine (I did a full universe build).

Did you do any runtime testing of threaded apps ?

I did not try any threaded applications yet. Do you have any suggestions what would be a good test case?

Perhaps run libc and libthr tests from /usr/tests ...

For testing I booted MIPS64 multiuser with these changes and ran a few programs with rtld direct exec on amd64. I've also been running CheriBSD with these changes for a few weeks.

I believe that for such changes, a multiuser boot on amd64 (or i386) is required.

Okay, I'll report back once I've booted on amd64.

... on amd64.

libexec/rtld-elf/rtld-libc/Makefile.inc
16 ↗(On Diff #59047)

So the comment is misleading, which is why I am so attached to this point. It is not -DIN_RTLD which avoids interposing for FreeBSD.

Update comment in makefile.inc

In D20663#449410, @kib wrote:
In D20663#449349, @kib wrote:
In D20663#449299, @kib wrote:

What testing was done ? Did you boot amd64 multiuser with the patched rtld ?
libthr consumes some bits from rtld, mostly rtld_malloc.c.

libthr still builds fine (I did a full universe build).

Did you do any runtime testing of threaded apps ?

I did not try any threaded applications yet. Do you have any suggestions what would be a good test case?

Perhaps run libc and libthr tests from /usr/tests ...

For testing I booted MIPS64 multiuser with these changes and ran a few programs with rtld direct exec on amd64. I've also been running CheriBSD with these changes for a few weeks.

I believe that for such changes, a multiuser boot on amd64 (or i386) is required.

Okay, I'll report back once I've booted on amd64.

... on amd64.

I've managed to boot amd64 with these changes. The libc and libthr tests also complete successfully.
The only thing I noticed was the following during boot:

lock order reversal:
 1st 0xfffffe0000847200 bufwait (bufwait) @ /exports/users/alr48/sources/freebsd-universe/sys/kern/vfs_bio.c:3904
 2nd 0xfffff800033e0200 dirhash (dirhash) @ /exports/users/alr48/sources/freebsd-universe/sys/ufs/ufs/ufs_dirhash.c:289
stack backtrace:
#0 0xffffffff80c30bf3 at witness_debugger+0x73
#1 0xffffffff80c3093d at witness_checkorder+0xa7d
#2 0xffffffff80bd0548 at _sx_xlock+0x68
#3 0xffffffff80ed741d at ufsdirhash_add+0x3d
#4 0xffffffff80eda206 at ufs_direnter+0x446
#5 0xffffffff80ee14a2 at ufs_rename+0xe82
#6 0xffffffff81217fe0 at VOP_RENAME_APV+0x70
#7 0xffffffff80ca812b at kern_renameat+0x36b
#8 0xffffffff81092af6 at amd64_syscall+0x276
#9 0xffffffff8106ad6d at fast_syscall_common+0x101

However, it seems unlikely that this is caused by my rtld changes and more likely that I booted with a broken kernel version.

kib accepted this revision.Jun 28 2019, 3:02 PM

I've managed to boot amd64 with these changes. The libc and libthr tests also complete successfully.
The only thing I noticed was the following during boot:

lock order reversal:
 1st 0xfffffe0000847200 bufwait (bufwait) @ /exports/users/alr48/sources/freebsd-universe/sys/kern/vfs_bio.c:3904
 2nd 0xfffff800033e0200 dirhash (dirhash) @ /exports/users/alr48/sources/freebsd-universe/sys/ufs/ufs/ufs_dirhash.c:289
stack backtrace:
#0 0xffffffff80c30bf3 at witness_debugger+0x73
#1 0xffffffff80c3093d at witness_checkorder+0xa7d
#2 0xffffffff80bd0548 at _sx_xlock+0x68
#3 0xffffffff80ed741d at ufsdirhash_add+0x3d
#4 0xffffffff80eda206 at ufs_direnter+0x446
#5 0xffffffff80ee14a2 at ufs_rename+0xe82
#6 0xffffffff81217fe0 at VOP_RENAME_APV+0x70
#7 0xffffffff80ca812b at kern_renameat+0x36b
#8 0xffffffff81092af6 at amd64_syscall+0x276
#9 0xffffffff8106ad6d at fast_syscall_common+0x101

However, it seems unlikely that this is caused by my rtld changes and more likely that I booted with a broken kernel version.

Of course, rtld changes cannot cause LOR in kernel. This LOR is known and is innocent, it is reported due to limitations WITNESS.

This revision is now accepted and ready to land.Jun 28 2019, 3:02 PM