Fix syscall error codes and apply minor documentation fixes

[efu39]

Description of the changes

Fix syscall fchmodat(), mmap(), readv() error codes and apply minor documentation fixes. These are found while testing with a newer LTP version.

How to test this PR?

LTP Test.

---

This change is 

[kailun-qin]

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

---

libos/src/sys/libos_file.c line 191 at r1 (raw file):

    if (strnlen(filename, PATH_MAX + 1) == PATH_MAX + 1)
        return -ENAMETOOLONG;

Why this change? Shouldn't it end up here:

gramine/libos/src/fs/libos_namei.c

Lines 234 to 235 in 4629f06

	if (name_len > NAME_MAX)
	return -ENAMETOOLONG;

?

Code quote:

    if (strnlen(filename, PATH_MAX + 1) == PATH_MAX + 1)
        return -ENAMETOOLONG;

---

libos/src/sys/libos_file.c line 194 at r1 (raw file):

    if (strlen(filename) == 0)
        return -ENOENT;

Why this change? Shouldn't it end up here:

gramine/libos/src/fs/libos_namei.c

Lines 272 to 274 in 4629f06

	/* Empty path is invalid in POSIX */
	if (*path == '\0')
	return -ENOENT;

?

Code quote:

    if (strlen(filename) == 0)
        return -ENOENT;

---

libos/src/sys/libos_wrappers.c line 21 at r1 (raw file):

    size_t arr_size;
    if ((int)fd < 0)
        return -EBADF;

Why this change? Shouldn't it end up here:

gramine/libos/src/sys/libos_wrappers.c

Lines 34 to 36 in 4629f06

	struct libos_handle* hdl = get_fd_handle(fd, NULL, NULL);
	if (!hdl)
	return -EBADF;

?

Code quote:

    if ((int)fd < 0)
        return -EBADF;

[efu39]

Reviewable status: 4 of 5 files reviewed, 3 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 @kailun-qin)

---

libos/src/sys/libos_file.c line 191 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Why this change? Shouldn't it end up here:

gramine/libos/src/fs/libos_namei.c

Lines 234 to 235 in 4629f06

	if (name_len > NAME_MAX)
	return -ENAMETOOLONG;

?

This is for case https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/fchmodat/fchmodat02.c#L46
It returned at the code below first before reaching the code you mentioned.

gramine/libos/src/sys/libos_file.c

Lines 190 to 191 in 4629f06

	if (*filename != '/' && (ret = get_dirfd_dentry(dfd, &dir)) < 0)
	return ret;

Therefore got "TFAIL: fchmodat() with pathname too long expected ENAMETOOLONG: ENOTDIR (20)"

---

libos/src/sys/libos_file.c line 194 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Why this change? Shouldn't it end up here:

gramine/libos/src/fs/libos_namei.c

Lines 272 to 274 in 4629f06

	/* Empty path is invalid in POSIX */
	if (*path == '\0')
	return -ENOENT;

?

This is for case https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/fchmodat/fchmodat02.c#L47
It returned at the code below first before reaching the code you mentioned.

gramine/libos/src/sys/libos_file.c

Lines 190 to 191 in 4629f06

	if (*filename != '/' && (ret = get_dirfd_dentry(dfd, &dir)) < 0)
	return ret;

Therefore got "TFAIL: fchmodat() with path is empty expected ENOENT: ENOTDIR (20)"

---

libos/src/sys/libos_wrappers.c line 21 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Why this change? Shouldn't it end up here:

gramine/libos/src/sys/libos_wrappers.c

Lines 34 to 36 in 4629f06

	struct libos_handle* hdl = get_fd_handle(fd, NULL, NULL);
	if (!hdl)
	return -EBADF;

?

It actually ended up at here:

gramine/libos/src/sys/libos_wrappers.c

Lines 29 to 30 in 4629f06

	if (!is_user_memory_writable(vec[i].iov_base, vec[i].iov_len))
	return -EFAULT;

for the readv02 test item 5 https://github.com/linux-test-project/ltp/blame/master/testcases/kernel/syscalls/readv/readv02.c#L65

{&badfd, valid_iovec, 3, EBADF},

Thus got "TFAIL: readv(-1, 0x389e9131f020, 3) expected EBADF: EFAULT (14)"
But I guess I should just re-arrange the code. I commited a fixup.

[kailun-qin]

Reviewed 1 of 1 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

---

libos/src/sys/libos_file.c line 194 at r1 (raw file):

Previously, efu39 (Erica Fu) wrote…

This is for case https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/fchmodat/fchmodat02.c#L47
It returned at the code below first before reaching the code you mentioned.

gramine/libos/src/sys/libos_file.c

Lines 190 to 191 in 4629f06

	if (*filename != '/' && (ret = get_dirfd_dentry(dfd, &dir)) < 0)
	return ret;

Therefore got "TFAIL: fchmodat() with path is empty expected ENOENT: ENOTDIR (20)"

I see, thanks!

---

libos/src/sys/libos_file.c line 190 at r2 (raw file):

    int ret = 0;

    if (strnlen(filename, PATH_MAX + 1) == PATH_MAX + 1)

Perhaps you've reviewed all other syscalls w/ const char* filename (not necessarily covered in LTP). Do they exhibit similar issues?

[kailun-qin]

Jenkins, test this please

[mkow]

Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all 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 @kailun-qin)

---

libos/src/sys/libos_file.c line 190 at r2 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Perhaps you've reviewed all other syscalls w/ const char* filename (not necessarily covered in LTP). Do they exhibit similar issues?

Where is this check located in Linux? I can't find it.

---

-- commits line 12 at r2:
We don't aim to align the error codes with the manpages, they are often wrong or at least unspecified in terms of which error is returned when exactly. We try to mirror what Linux does.

Suggestion:

[LibOS] Fix syscall fchmodat(), mmap(), readv() error codes

---

libos/src/sys/libos_mmap.c line 66 at r2 (raw file):

void* libos_syscall_mmap(void* addr, size_t length, int prot, int flags, int fd,
                         unsigned long offset) {

This signature differs from Linux, should be: unsigned long addr, unsigned long len, unsigned long prot, unsigned long flags, unsigned long fd, unsigned long off.

---

libos/src/sys/libos_mmap.c line 119 at r2 (raw file):

                /* fall through */
            case MAP_PRIVATE:
                if (fd < 0) {

This whole if will be no-op if you fix the signature (see above).

[efu39]

Reviewable status: 4 of 6 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: ) (waiting on @kailun-qin and @mkow)

---

-- commits line 12 at r2:

Previously, mkow (Michał Kowalczyk) wrote…

We don't aim to align the error codes with the manpages, they are often wrong or at least unspecified in terms of which error is returned when exactly. We try to mirror what Linux does.

Commit message is updated.

---

libos/src/sys/libos_file.c line 190 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Where is this check located in Linux? I can't find it.

I think the linux check is here https://github.com/torvalds/linux/blob/c62f4b82d57155f35befb5c8bbae176614b87623/fs/namei.c#L210

In Gramine, several syscalls (openat, newfstatat, readlinkat, faccess, fchownat..) would have similar behavior to return ENOTDIR before EBADF or EINVAL.  

Our code checks for EBADF or EINVAL occur in lookup_advance(), which is called after get_dirfd_dentry(). Do you think we should make the change to pass LTP, or is it preferable to skip the test case when needed?

---

libos/src/sys/libos_mmap.c line 66 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This signature differs from Linux, should be: unsigned long addr, unsigned long len, unsigned long prot, unsigned long flags, unsigned long fd, unsigned long off.

Let me know if you'd like the additional commit to fix the signature or I can revert it and separate it with this PR. (Turned out the change of signature is not needed to fix error code, see next comment response)

---

libos/src/sys/libos_mmap.c line 119 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This whole if will be no-op if you fix the signature (see above).

You are right. Removed the code and the error will be handled in following lines anyway even without fixing the signature:

                hdl = get_fd_handle(fd, NULL, NULL);
                if (!hdl) {
                    return (void*)-EBADF;
                }

Thanks!

[mkow]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all 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: ) (waiting on @kailun-qin)

---

libos/src/sys/libos_file.c line 190 at r2 (raw file):

Previously, efu39 (Erica Fu) wrote…

I think the linux check is here https://github.com/torvalds/linux/blob/c62f4b82d57155f35befb5c8bbae176614b87623/fs/namei.c#L210

In Gramine, several syscalls (openat, newfstatat, readlinkat, faccess, fchownat..) would have similar behavior to return ENOTDIR before EBADF or EINVAL.  

Our code checks for EBADF or EINVAL occur in lookup_advance(), which is called after get_dirfd_dentry(). Do you think we should make the change to pass LTP, or is it preferable to skip the test case when needed?

Better to change the implementation in Gramine. But it's not super-important, most software doesn't really rely on such behavior (except accidentally), because it's not documented well.

---

libos/src/sys/libos_mmap.c line 66 at r2 (raw file):

Previously, efu39 (Erica Fu) wrote…

Let me know if you'd like the additional commit to fix the signature or I can revert it and separate it with this PR. (Turned out the change of signature is not needed to fix error code, see next comment response)

It's ok this way.

---

a discussion (no related file):
TODO for myself: check commit split before merging

---

libos/src/sys/libos_mmap.c line 263 at r4 (raw file):

            int update_valid_length_ret = bkeep_vma_update_valid_length((void*)addr, valid_length);
            if (update_valid_length_ret < 0) {
                log_error("[mmap] Failed to update valid length to %lu of bookkeeped memory %lu-%lu!",

%p is not equal to %lu. Just to be sure, please run this case and paste here (in a comment) a sample output to ensure it's formatted correctly.

Suggestion:

%#lx-%#lx

---

libos/src/sys/libos_mmap.c line 273 at r4 (raw file):

        void* tmp_vma = NULL;
        if (bkeep_munmap((void*)addr, length, /*is_internal=*/false, &tmp_vma) < 0) {
            log_error("[mmap] Failed to remove bookkeeped memory that was not allocated at %lu-%lu!",

ditto

[efu39]

Reviewable status: 5 of 6 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 @kailun-qin and @mkow)

---

libos/src/sys/libos_file.c line 190 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Better to change the implementation in Gramine. But it's not super-important, most software doesn't really rely on such behavior (except accidentally), because it's not documented well.

I'll keep the change here for fchmodat only then. After checking LTP code, I don't see other similar test scenarios for the syscalls supported by Gramine. It only happens when both dirfd and filename are invalid.

---

libos/src/sys/libos_mmap.c line 263 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

%p is not equal to %lu. Just to be sure, please run this case and paste here (in a comment) a sample output to ensure it's formatted correctly.

Fixed. The output %#lx is good like following:

(libos_mmap.c:268:libos_syscall_mmap) [P1:T1:mmap01] debug: [mmap] Updated valid length to 4096 of bookkeeped memory 0x4eaad2848000-0x4eaad2849000
(libos_mmap.c:268:libos_syscall_mmap) [P1:T1:mmap01] debug: [mmap] Updated valid length to 4096 of bookkeeped memory 0x4eaad2849000-0x4eaad284a000
(libos_mmap.c:268:libos_syscall_mmap) [P1:T1:mmap01] debug: [mmap] Updated valid length to 8192 of bookkeeped memory 0x4eaad284a000-0x4eaad284c000

---

libos/src/sys/libos_mmap.c line 273 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Updated as well.

[mkow]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 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 @efu39 and @kailun-qin)

---

libos/src/sys/libos_mmap.c line 263 at r5 (raw file):

            int update_valid_length_ret = bkeep_vma_update_valid_length((void*)addr, valid_length);
            if (update_valid_length_ret < 0) {
                log_error("[mmap] Failed to update valid length to %lu of bookkeeped memory %#lx-%#lx!",

Please wrap (by splitting the string literal).

[efu39]

Reviewable status: 5 of 6 files reviewed, 3 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 @kailun-qin and @mkow)

---

libos/src/sys/libos_mmap.c line 263 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please wrap (by splitting the string literal).

Wrapped. Thanks.

[mkow]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 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 @efu39 and @kailun-qin)

[kailun-qin]

Reviewed 1 of 2 files at r3, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @efu39)

[kailun-qin]

Jenkins, test this please

[mkow]

FYI: I'll rebase this PR and fix a bug I found when double-checking the code.

Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @efu39)

[mkow]

Reviewed 1 of 1 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)

---

libos/src/sys/libos_wrappers.c line 18 at r7 (raw file):

 * notably affects pipes. */

long libos_syscall_readv(unsigned long fd, struct iovec* vec, unsigned long vlen) {

FYI: I noticed a handle leak here introduced by the original version of this PR, please review my fix.
CC: @kailun-qin, @efu39

[mkow]

Jenkins, retest this please

[kailun-qin]

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