[LibOS] Close open fds on thread exit

[kailun-qin]

Fixes #824.

This change is 

[mkow]

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @kailun-qin)

---

libos/src/bookkeep/libos_handle.c line 306 at r1 (raw file):

    if (!handle_map_locked) {
        lock(&handle_map->lock);

waait, you can't do this, it's inherently racy... locked() is correct almost only in asserts

[kailun-qin]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)

---

libos/src/bookkeep/libos_handle.c line 306 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

waait, you can't do this, it's inherently racy... locked() is correct almost only in asserts

emm... I didn't check the impl. of locked().

Is adding an additional arg to detach_fd_handle() to explicitly sepecify whether a lock is needed makes sense to you? Or any better idea?

[mkow]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @kailun-qin)

---

libos/src/bookkeep/libos_handle.c line 306 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

emm... I didn't check the impl. of locked().

Is adding an additional arg to detach_fd_handle() to explicitly sepecify whether a lock is needed makes sense to you? Or any better idea?

In Gramine codebase we usually split functions into asdf() and __asdf(), with the former being mostly asdf() { lock(); __asdf(); unlock(); }. And the callsite calls the correct one, depending whether it has the lock taken or not.
Here the already implemented construction is a bit weirder though (the lock-taking function has some additional logic), so you may need to try some refactoring and see whether it's possible to fix it while preserving our conventions.

[kailun-qin]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)

---

libos/src/bookkeep/libos_handle.c line 306 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

In Gramine codebase we usually split functions into asdf() and __asdf(), with the former being mostly asdf() { lock(); __asdf(); unlock(); }. And the callsite calls the correct one, depending whether it has the lock taken or not.
Here the already implemented construction is a bit weirder though (the lock-taking function has some additional logic), so you may need to try some refactoring and see whether it's possible to fix it while preserving our conventions.

Good to know, thanks! Let me take a closer look.

[kailun-qin]

@mkow Got a bit confused here:

gramine/libos/src/bookkeep/libos_handle.c

Line 378 in 90f8b9d

lock(&handle_map->lock);

. Looks like not all __asdf() are following the convention?

[mkow]

I think that part of the codebase is super old and we just never refactored it. It seems that there __ prefix means just "internal helper".

btw. Please never link to master branch, this will be a dangling link in half a year and will confuse people. You have a shortcut on GitHub to covert the current address to the bound one: [Y].

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)

[boryspoplawski]

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @kailun-qin and @mkow)

---

-- commits line 2 at r1:
Should be on process exit

---

libos/src/bookkeep/libos_handle.c line 306 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Good to know, thanks! Let me take a closer look.

Why do you need this change at all? Please just create another function in this file, like detach_all_fds or something and call it in exit.
Btw, we use 1 underscore not 2 (because 2 is reserved by C standard)

---

libos/src/sys/libos_exit.c line 57 at r1 (raw file):

    terminate_ipc_worker();

    struct libos_handle_map* handle_map = get_thread_handle_map(NULL);

This shouldn't be here but in related file. This function should not be aware of handle details, it should just call something from different "module"

[kailun-qin]

Got it and linked to a specific commit, thanks!

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

---

-- commits line 2 at r1:

Previously, boryspoplawski (Borys Popławski) wrote…

Should be on process exit

Done.

---

libos/src/bookkeep/libos_handle.c line 306 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why do you need this change at all? Please just create another function in this file, like detach_all_fds or something and call it in exit.
Btw, we use 1 underscore not 2 (because 2 is reserved by C standard)

Done.

---

libos/src/sys/libos_exit.c line 57 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This shouldn't be here but in related file. This function should not be aware of handle details, it should just call something from different "module"

Agree, updated.

[boryspoplawski]

Reviewed 2 of 3 files at r2.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)

---

libos/src/bookkeep/libos_handle.c line 346 at r3 (raw file):

                }

                clear_posix_locks(handle);

Now you run this function under handle_map->lock, which previously wasn't the case. Are you sure this is correct?

Update: actually, all posix locks are cleared on exit, this is not needed.

---

libos/src/sys/libos_exit.c line 57 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Agree, updated.

This should be called in thread_exit below, after int ret = posix_lock_clear_pid(g_process.pid); and before child exit notification.

[kailun-qin]

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

---

libos/src/bookkeep/libos_handle.c line 346 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Now you run this function under handle_map->lock, which previously wasn't the case. Are you sure this is correct?

Update: actually, all posix locks are cleared on exit, this is not needed.

Good catch, thanks for checking, done.

---

libos/src/sys/libos_exit.c line 57 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This should be called in thread_exit below, after int ret = posix_lock_clear_pid(g_process.pid); and before child exit notification.

Done.

[boryspoplawski]

Reviewed 1 of 2 files at r5, all commit messages.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)

---

libos/src/bookkeep/libos_handle.c line 294 at r5 (raw file):

}

static void clear_posix_locks(struct libos_handle* handle) {

This change doesn't seem needed now, can be restored.

---

libos/src/bookkeep/libos_handle.c line 333 at r5 (raw file):

    struct libos_handle_map* handle_map = get_thread_handle_map(NULL);
    assert(handle_map);
    lock(&handle_map->lock);

You can use walk_handle_map, no need for duplicating this code.

[kailun-qin]

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

---

libos/src/bookkeep/libos_handle.c line 294 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This change doesn't seem needed now, can be restored.

Done.

---

libos/src/bookkeep/libos_handle.c line 333 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

You can use walk_handle_map, no need for duplicating this code.

Done.

---

libos/src/bookkeep/libos_handle.c line 669 at r6 (raw file):

    lock(&map->lock);

    for (uint32_t i = 0; i <= map->fd_top && map->fd_top != FD_NULL; i++) {

callback may change map->fd_top to FD_NULL in some cases, so updated the logic here.

[boryspoplawski]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)

---

libos/src/bookkeep/libos_handle.c line 328 at r6 (raw file):

static int detach_fd(struct libos_fd_handle* fd_hdl, struct libos_handle_map* map) {
    struct libos_handle* hdl = __detach_fd_handle(fd_hdl, NULL, map);
    if (hdl) {

How can this be NULL?

---

libos/src/bookkeep/libos_handle.c line 669 at r6 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

callback may change map->fd_top to FD_NULL in some cases, so updated the logic here.

Please swap the condition (so it's more like: is_fd_top_valid && use_fd_top)

---

libos/src/sys/libos_exec.c line 21 at r6 (raw file):

    if (fd_hdl->flags & FD_CLOEXEC) {
        struct libos_handle* hdl = __detach_fd_handle(fd_hdl, NULL, map);
        if (hdl) {

Why this change? How can this be NULL?

[kailun-qin]

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

---

libos/src/bookkeep/libos_handle.c line 328 at r6 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

How can this be NULL?

It's inherited from a previous logic, but indeed, after switching to walk_handle_map(), it's not needed. Thanks.

---

libos/src/bookkeep/libos_handle.c line 669 at r6 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Please swap the condition (so it's more like: is_fd_top_valid && use_fd_top)

Done.

---

libos/src/sys/libos_exec.c line 21 at r6 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why this change? How can this be NULL?

Ah right, after switching to walk_handle_map(), it's not needed.

[boryspoplawski]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, 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 @mkow)

---

libos/src/sys/libos_exec.c line 21 at r6 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Ah right, after switching to walk_handle_map(), it's not needed.

But this was always walk_handle_map() :)

[mkow]

Reviewed 1 of 3 files at r2, 1 of 2 files at r5, 2 of 2 files at r7, 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), "fixup! " found in commit messages' one-liners

[mkow]

Jenkins, test this please

[dimakuv]

Reviewed 1 of 3 files at r2, 1 of 2 files at r5, 1 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

[dimakuv]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)

a discussion (no related file):
Does this PR fix #824 ? It should. Please verify and add Fixes #824 to the PR description.

---

[kailun-qin]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Does this PR fix #824 ? It should. Please verify and add Fixes #824 to the PR description.

Yes, I've verified manually. Updated the PR description.

---

[dimakuv]

Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

[dimakuv]

Reviewed 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: Intel)

---

-- commits line 2 at r1:

Previously, kailun-qin (Kailun Qin) wrote…

Done.

Done

[dimakuv]

Jenkins, test this please

[mkow]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[boryspoplawski]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved