[LibOS] Simplify RTLD code

[pwmarcz]

Description of the changes

• Remove searchable ELF map list (we only need to access two maps: executable and interpreter)
• Refactor interpreter loading code
• Convert some globals to g_* naming scheme

This is a byproduct of upcoming FS refactoring.

How to test this PR?

Existing tests should be enough.

---

This change is 

[mkow]

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL)

[dimakuv]

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @pwmarcz)

---

LibOS/shim/include/shim_utils.h, line 149 at r1 (raw file):

int load_elf_interp(struct link_map* exec_map);
noreturn void execute_elf_object(struct link_map* exec_map, void* argp, elf_auxv_t* auxp);
void remove_loaded_libraries(void);

I very much dislike the old naming. Shouldn't this be remove_loaded_elf_objects? (See my other comments on libraries in the names.)

---

LibOS/shim/src/shim_init.c, line 448 at r1 (raw file):

    RUN_INIT(init_stack, argv, envp, &new_argp, &new_auxv);

    RUN_INIT(init_loader);

I would prefer to rename it to something more meaningful now. init_exec_and_its_interp?

---

LibOS/shim/src/shim_init.c, line 502 at r1 (raw file):

    /* At this point, the exec map has been either copied from checkpoint, or initialized in
     * `init_loader`. */
    execute_elf_object(/*exec_map=*/NULL, new_argp, new_auxv);

Can you add a new line with /* UNREACHABLE */?

---

LibOS/shim/src/shim_rtld.c, line 42 at r1 (raw file):

/*
 * Structure describing a loaded shared object. The `l_next' and `l_prev' members form a chain of
 * all the shared objects loaded at startup.

Please remove the comment about l_next/l_prev

---

LibOS/shim/src/shim_rtld.c, line 106 at r1 (raw file):

};

struct link_map* g_exec_map = NULL;

Why not static?

---

LibOS/shim/src/shim_rtld.c, line 636 at r1 (raw file):

            filename_len = interp_name_len - i - 1;
        }
    }

Actually, why do we extract the filename from the full interpreter path? This is non-standard behavior, right? Is this purely a Graphene feature (to always use our /lib/ld-linux-x86-64.so.2)?

---

LibOS/shim/src/shim_rtld.c, line 661 at r1 (raw file):

}

static int __load_interp_object(struct link_map* exec_map) {

Any reason to keep __?

---

LibOS/shim/src/shim_rtld.c, line 697 at r1 (raw file):

}

void remove_loaded_libraries(void) {

I very much dislike the old naming. Shouldn't this be remove_loaded_elf_objects? (See my other comments on libraries in the names.)

---

LibOS/shim/src/shim_rtld.c, line 768 at r1 (raw file):

        ret = load_elf_object(exec, &g_exec_map);
        if (ret < 0) {
            // TODO: Actually verify that the non-PIE-ness was the real cause of loading failure.

This would be cool to finally implement (as a separate PR). In general, we give the below confusing message even when the executable file doesn't exist or is a Bash/Perl script. This mightily confuses end users, who try sgx.nonpie_binary and still fail.

It would be better to assign proper return error codes inside load_elf_object() and print different messages based on these returned error codes. Not blocking, just something I remembered.

---

LibOS/shim/src/shim_rtld.c, line 799 at r1 (raw file):

}

int register_library(const char* name, unsigned long load_address) {

Why don't we have a counterpart function to this one? To stop tracking loaded libraries for debugging? Is it because they are removed during checkpoint-and-restore? Or this is simply not needed during a debug session?

---

LibOS/shim/src/shim_rtld.c, line 821 at r1 (raw file):

    if (exec_map) {
        /* If a new map is provided, it means we have cleared the existing one by calling
         * `remove_loaded_libraries`. */

Maybe add a comment that this can happen on execve emulation?

---

LibOS/shim/src/shim_rtld.c, line 895 at r1 (raw file):

}

BEGIN_CP_FUNC(library) {

I dislike this library name, let's rename to elf_object. Or at least link_map. We never deal with libraries here, right? It is only ever the executable and its interpreter (if applicable).

---

LibOS/shim/src/shim_rtld.c, line 941 at r1 (raw file):

END_RS_FUNC(library)

BEGIN_CP_FUNC(loaded_libraries) {

I dislike this loaded_libraries name, let's rename to exec_and_its_interp?

[pwmarcz]

Reviewable status: 1 of 7 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)

---

LibOS/shim/include/shim_utils.h, line 149 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I very much dislike the old naming. Shouldn't this be remove_loaded_elf_objects? (See my other comments on libraries in the names.)

Renamed the way you suggested.

---

LibOS/shim/src/shim_init.c, line 448 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I would prefer to rename it to something more meaningful now. init_exec_and_its_interp?

Changed to init_elf_objects, to match loaded_elf_objects etc.

(It's true that, by design, we track only the executable and interpreter ELFs, but I think there's no reason to expose that outside of the module).

---

LibOS/shim/src/shim_init.c, line 502 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can you add a new line with /* UNREACHABLE */?

Done.

---

LibOS/shim/src/shim_rtld.c, line 42 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please remove the comment about l_next/l_prev

Done.

---

LibOS/shim/src/shim_rtld.c, line 106 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why not static?

Right, it should be.

(Initially I kept it exposed so that you can run execute_elf_object(g_exec_map), but I decided execute_elf_object(NULL) is slightly less ugly).

---

LibOS/shim/src/shim_rtld.c, line 636 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Actually, why do we extract the filename from the full interpreter path? This is non-standard behavior, right? Is this purely a Graphene feature (to always use our /lib/ld-linux-x86-64.so.2)?

Yeah... I think 64-bit executables compiled with glibc always use /lib64/ld-linux-x86-64.so.2 (the same path with lib64). We seem to also support LD_LIBRARY_PATH as override, which is again non-standard (it shouldn't change the interpreter).

I'm tempted to just change this code to use the interpreter path directly (and remove LD_LIBRARY_PATH parsing). @boryspoplawski @mkow any objections?

(I don't want to do it in this PR, though: because we currently mount Graphene libs under /lib, this is a breaking change, requires changing all manifests, etc. I can ticket it.)

---

LibOS/shim/src/shim_rtld.c, line 661 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Any reason to keep __?

Actually I see we just rid of all func / __func pairs, so there is no reason to keep this naming scheme (looks like originally it was also used for static functions but I'm pretty sure we don't want that).

I removed __ from remaining names in this file.

---

LibOS/shim/src/shim_rtld.c, line 697 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I very much dislike the old naming. Shouldn't this be remove_loaded_elf_objects? (See my other comments on libraries in the names.)

Renamed the way you suggested.

---

LibOS/shim/src/shim_rtld.c, line 768 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This would be cool to finally implement (as a separate PR). In general, we give the below confusing message even when the executable file doesn't exist or is a Bash/Perl script. This mightily confuses end users, who try sgx.nonpie_binary and still fail.

It would be better to assign proper return error codes inside load_elf_object() and print different messages based on these returned error codes. Not blocking, just something I remembered.

Acknowledged, although I don't want to go deeper right now.

---

LibOS/shim/src/shim_rtld.c, line 799 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why don't we have a counterpart function to this one? To stop tracking loaded libraries for debugging? Is it because they are removed during checkpoint-and-restore? Or this is simply not needed during a debug session?

During execve, we unregister all libraries. But also we try to unregister a library during munmap, probably to cover the case of loading and unloading a library with dlopen.

(And I just realized it's probably not 100% correct because we look at l_addr... I added a TODO).

By the way, I know that perf tracks loaded libraries by tracking mmap and munmap operations (if a region is backed by a file, and is executable). Maybe we could do the same, and we wouldn't need register_library at all.

---

LibOS/shim/src/shim_rtld.c, line 821 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Maybe add a comment that this can happen on execve emulation?

Done.

---

LibOS/shim/src/shim_rtld.c, line 895 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I dislike this library name, let's rename to elf_object. Or at least link_map. We never deal with libraries here, right? It is only ever the executable and its interpreter (if applicable).

Renamed to elf_object.

---

LibOS/shim/src/shim_rtld.c, line 941 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I dislike this loaded_libraries name, let's rename to exec_and_its_interp?

Renamed to loaded_elf_objects, for the same reason as the other comment (this is a public function called in shim_clone).

[dimakuv]

Reviewed 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners

---

LibOS/shim/src/shim_rtld.c, line 636 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Yeah... I think 64-bit executables compiled with glibc always use /lib64/ld-linux-x86-64.so.2 (the same path with lib64). We seem to also support LD_LIBRARY_PATH as override, which is again non-standard (it shouldn't change the interpreter).

I'm tempted to just change this code to use the interpreter path directly (and remove LD_LIBRARY_PATH parsing). @boryspoplawski @mkow any objections?

(I don't want to do it in this PR, though: because we currently mount Graphene libs under /lib, this is a breaking change, requires changing all manifests, etc. I can ticket it.)

OK, thanks for explanations. This is also my understanding that it's just some Graphene hack (probably to stash all dependent binaries under <graphene>/Runtime/ -> /lib/). Definitely for another PR.

[dimakuv]

Reviewed all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)

[mkow]

Reviewed 7 of 7 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

LibOS/shim/src/shim_rtld.c, line 636 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

OK, thanks for explanations. This is also my understanding that it's just some Graphene hack (probably to stash all dependent binaries under <graphene>/Runtime/ -> /lib/). Definitely for another PR.

+1 to using the interp path directly. But can be done in a separate PR.