[LibOS] Add missing locks around dentry->inode dereference

[g2flyer]

Description of the changes

This fixes two cases where a dentry->inode derefernce was not lock-protected. It also adds a missing call to put_handle during error handling and (in a separate commit) removes an unused function.

How to test this PR?

Usual regression tests ...

---

This change is 

[dimakuv]

Reviewed 4 of 4 files at r1, 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: ) (waiting on @g2flyer)

---

-- commits line 9 at r1:
Would be better to also specify in which function exactly: in libos_syscall_fchdir() func.

---

libos/include/libos_process.h line 65 at r1 (raw file):

    struct libos_lock children_lock;
    struct libos_lock fs_lock; /* Note: has lower priority than g_dcache_lock. */

This is confusing wording, I would prefer a longer description:

    struct libos_lock children_lock;

    /* If g_dcache_lock is also required, acquire g_dcache_lock first and then fs_lock */
    struct libos_lock fs_lock;

---

libos/src/sys/libos_getcwd.c line 85 at r1 (raw file):

        return -EBADF;

    int ret = 0;

It's better to do like this:

    int ret;
    lock(&g_dcache_lock);
    ...
    ...
    ret = 0;
out:
    ...

This way the GCC compiler has more opportunities to detect uninitialized ret on code paths.

---

libos/src/sys/libos_open.c line 377 at r1 (raw file):

    lock(&g_dcache_lock);
    if (!hdl->dentry->inode) {

Is it possible that hdl->dentry == NULL here? Don't we want to error out on this condition too?

[g2flyer]

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

---

-- commits line 9 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Would be better to also specify in which function exactly: in libos_syscall_fchdir() func.

Done.

---

libos/include/libos_process.h line 65 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is confusing wording, I would prefer a longer description:

    struct libos_lock children_lock;

    /* If g_dcache_lock is also required, acquire g_dcache_lock first and then fs_lock */
    struct libos_lock fs_lock;

Done.

---

libos/src/sys/libos_getcwd.c line 85 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

It's better to do like this:

    int ret;
    lock(&g_dcache_lock);
    ...
    ...
    ret = 0;
out:
    ...

This way the GCC compiler has more opportunities to detect uninitialized ret on code paths.

There shouldn't be unitialized with above ;-) But i see what you mean, makes sense as it can be easily overlooked to redefine it in error case.

---

libos/src/sys/libos_open.c line 377 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Is it possible that hdl->dentry == NULL here? Don't we want to error out on this condition too?

In this version, hdl->dentry should still be immutable (and defined), this becomes only an issue in PR #1979 but even in that case it should always be defined

[dimakuv]

Reviewed 2 of 2 files at r2, 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 (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @g2flyer)

---

-- commits line 15 at r2:
Misses a verb, should be [LibOS] Add missing lock ...

---

libos/src/sys/libos_getcwd.c line 85 at r1 (raw file):

makes sense as it can be easily overlooked to redefine it in error case.

Yes, exactly. And without initialization, GCC will spit out an error. But with initialization (as it was previously), GCC would just use ret = 0 in the error path and not spit out any warnings.

---

libos/src/sys/libos_open.c line 377 at r1 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

In this version, hdl->dentry should still be immutable (and defined), this becomes only an issue in PR #1979 but even in that case it should always be defined

Ok, thanks for explanation.

[mkow]

Reviewed 2 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, 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 (waiting on @g2flyer)

a discussion (no related file):
TODO for myself: check commit split after the final rebase.

---

---

-- commits line 3 at r1:

Suggestion:

Remove an unused function

---

-- commits line 9 at r1:
nitpick: that's how we usually write such notes

Suggestion:

  Also fixes one missing call to put_handle in error handling

---

-- commits line 7 at r2:
But: the places you wrapped in the lock here are not dereferencing the inode pointer.

Suggestion:

dereferences

[dimakuv]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @g2flyer and @mkow)

---

-- commits line 3 at r1:
Done

---

-- commits line 9 at r1:

Previously, mkow (Michał Kowalczyk) wrote…

nitpick: that's how we usually write such notes

Done

---

-- commits line 7 at r2:

Previously, mkow (Michał Kowalczyk) wrote…

But: the places you wrapped in the lock here are not dereferencing the inode pointer.

Done. Changed to accesses.

---

-- commits line 15 at r2:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Misses a verb, should be [LibOS] Add missing lock ...

Done

[dimakuv]

Jenkins, test this please

[dimakuv]

Jenkins, retest Jenkins-SGX-EDMM please (PCCS cache is reactivated)

[mkow]

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