[LibOS] Add fadvise64 syscall implementation

[kailun-qin]

The original requirement is from #511.

In Gramine's threat model, fadvise64() is not required to do anything meaningful. Thus, only do sanity checks and simply implement all the others as no-op.

---

This change is 

[boryspoplawski]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 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)

---

-- commits line 5 at r1:
What do you mean by "threat model"? We are not required to do anything because of that syscall semantics (which we emulate).

---

-- commits line 11 at r1:
I would recommend reading point 7 from https://gramine.readthedocs.io/en/stable/devel/onboarding.html#fixing-a-bug (or the whole document). This commit is should be just squashed into the previous one it serves no purpose.

---

libos/include/shim_table.h line 162 at r1 (raw file):

                                     unsigned long* user_mask_ptr);
long libos_syscall_set_tid_address(int* tidptr);
long libos_syscall_fadvise64(int fd, loff_t offset, loff_t len, int advice);

Wrong signature (type of len): https://elixir.bootlin.com/linux/latest/source/mm/fadvise.c#L212

---

libos/src/sys/shim_open.c line 644 at r1 (raw file):

    int ret;

    if (offset < 0) {

Why this check? This is not required by the kernel from what I can see.

---

libos/src/sys/shim_open.c line 647 at r1 (raw file):

        return -EINVAL;
    }
    if (len < 0) {

If you fix the signature this check won't be needed.

---

libos/src/sys/shim_open.c line 650 at r1 (raw file):

        return -EINVAL;
    }
    if (!fadvise_advice_valid(advice)) {

What's the point of having this as a separate function? Both functions (caller and callee) are small and this doesn't improve readability - just inline it

---

libos/src/sys/shim_open.c line 658 at r1 (raw file):

        return -EBADF;
    }
    if (handle->type == TYPE_PIPE) {

We need to check if it's really a file, not that it's not a pipe.

---

libos/src/sys/shim_open.c line 662 at r1 (raw file):

        goto out;
    }
    // In Gramine's threat model, it is not required to do anything meaningful with fadvise,

ditto (this comment makes no sense to me, as in commit message)

[kailun-qin]

Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @boryspoplawski)

---

-- commits line 5 at r1:

Previously, boryspoplawski (Borys Popławski) wrote…

What do you mean by "threat model"? We are not required to do anything because of that syscall semantics (which we emulate).

The wording is a copy-paste from the initial issue. I can guess the intention but I think it's fine to just remove it to avoid any confusions.

---

-- commits line 11 at r1:

Previously, boryspoplawski (Borys Popławski) wrote…

I would recommend reading point 7 from https://gramine.readthedocs.io/en/stable/devel/onboarding.html#fixing-a-bug (or the whole document). This commit is should be just squashed into the previous one it serves no purpose.

Got it, thanks.

---

libos/include/shim_table.h line 162 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Wrong signature (type of len): https://elixir.bootlin.com/linux/latest/source/mm/fadvise.c#L212

I just followed https://elixir.bootlin.com/linux/latest/source/mm/fadvise.c#L205, also the posix_fadvise signature in manpage. WDYT?

---

libos/src/sys/shim_open.c line 644 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why this check? This is not required by the kernel from what I can see.

Yep indeed. Fixed.

---

libos/src/sys/shim_open.c line 647 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

If you fix the signature this check won't be needed.

I'll leave it for now and let you confirm the signature.

---

libos/src/sys/shim_open.c line 650 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

What's the point of having this as a separate function? Both functions (caller and callee) are small and this doesn't improve readability - just inline it

Done.

---

libos/src/sys/shim_open.c line 658 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

We need to check if it's really a file, not that it's not a pipe.

Well, the intention was to check whether its a pipe, if so then -ESPIPE. (similar check for fallocate:

gramine/libos/src/sys/shim_open.c

Line 578 in 0a7112a

if (handle->type == TYPE_PIPE) {

, also see https://elixir.bootlin.com/linux/latest/source/mm/fadvise.c#L43)

---

libos/src/sys/shim_open.c line 662 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ditto (this comment makes no sense to me, as in commit message)

Done.

[boryspoplawski]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all 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! " and "squash! " found in commit messages' one-liners (waiting on @boryspoplawski and @kailun-qin)

---

-- commits line 24 at r2:
For the next time please also put what you want to do with the new message (append, replaces, etc).

---

libos/include/shim_table.h line 162 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

I just followed https://elixir.bootlin.com/linux/latest/source/mm/fadvise.c#L205, also the posix_fadvise signature in manpage. WDYT?

fadvise64_64 is a different syscall (mainly used in 32bit arch, afaik). posix_fadvise is a lib C function signature, not syscall's.

---

libos/src/sys/shim_open.c line 658 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Well, the intention was to check whether its a pipe, if so then -ESPIPE. (similar check for fallocate:

gramine/libos/src/sys/shim_open.c

Line 578 in 0a7112a

if (handle->type == TYPE_PIPE) {

, also see https://elixir.bootlin.com/linux/latest/source/mm/fadvise.c#L43)

This is weird, but indeed Linux kernel does return 0, e.g. for sockets...

[dimakuv]

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

---

-- commits line 5 at r1:

Previously, kailun-qin (Kailun Qin) wrote…

The wording is a copy-paste from the initial issue. I can guess the intention but I think it's fine to just remove it to avoid any confusions.

Yes, the "threat model" was only about Linux-SGX PAL. There we have Intel SGX which distrusts the kernel, and since the kernel manages files access, the threat model does not consider "kernel does not perform any access-to-files optimizations".

Yes, we can just remove the part In Gramine's thread model.

---

libos/include/shim_table.h line 162 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

fadvise64_64 is a different syscall (mainly used in 32bit arch, afaik). posix_fadvise is a lib C function signature, not syscall's.

@kailun-qin But your link refers to another system call called fadvise64_64. So I agree with Borys.

---

libos/src/sys/shim_open.c line 647 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

I'll leave it for now and let you confirm the signature.

+1 to Borys, not needed

---

libos/src/sys/shim_open.c line 628 at r2 (raw file):

}

long libos_syscall_fadvise64(int fd, loff_t offset, loff_t len, int advice) {

loff_t len -> size_t len

---

libos/src/sys/shim_open.c line 650 at r2 (raw file):

    if (!handle) {
        return -EBADF;
    }

Please add empty lines between separate code snippets (like different checks), otherwise it is hard to read.

---

libos/src/sys/shim_open.c line 655 at r2 (raw file):

        goto out;
    }
    // It is not required to do anything meaningful with fadvise, so just make it a no-op here.

First, we prefer to use /* */. Second, the sentence looks weird, I suggest a simple:

/* currently just a no-op */

[kailun-qin]

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

---

libos/include/shim_table.h line 162 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@kailun-qin But your link refers to another system call called fadvise64_64. So I agree with Borys.

Done.

---

libos/src/sys/shim_open.c line 647 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

+1 to Borys, not needed

Done.

---

libos/src/sys/shim_open.c line 628 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

loff_t len -> size_t len

Done.

---

libos/src/sys/shim_open.c line 650 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please add empty lines between separate code snippets (like different checks), otherwise it is hard to read.

Done.

---

libos/src/sys/shim_open.c line 655 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

First, we prefer to use /* */. Second, the sentence looks weird, I suggest a simple:

/* currently just a no-op */

Done.

[boryspoplawski]

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 (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)

[boryspoplawski]

Jenkins, test this please

[dimakuv]

Reviewed 1 of 3 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)

[mkow]

Reviewed 1 of 3 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @kailun-qin)

---

-- commits line 24 at r3:

Thus, only do sanity checks and simply implement all the others as no-op.

All the others what? I don't understand this sentence.

[kailun-qin]

Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @mkow)

---

-- commits line 24 at r3:

Previously, mkow (Michał Kowalczyk) wrote…

Thus, only do sanity checks and simply implement all the others as no-op.

All the others what? I don't understand this sentence.

Currently, we only implement sanity checks infadvise64and the core logic is just a no-op (returning 0 directly). Please feel free to rephrase the commit message you think is more appropriate. Thanks!

[dimakuv]

Reviewed 6 of 6 files at r4, 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: Intel) (waiting on @boryspoplawski and @mkow)

---

-- commits line 11 at r1:

Previously, kailun-qin (Kailun Qin) wrote…

Got it, thanks.

Done.

---

-- commits line 24 at r3:

Previously, kailun-qin (Kailun Qin) wrote…

Currently, we only implement sanity checks infadvise64and the core logic is just a no-op (returning 0 directly). Please feel free to rephrase the commit message you think is more appropriate. Thanks!

Done. I rephrased during the final rebase.

[dimakuv]

Jenkins, test this please

[mkow]

Reviewed 3 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @boryspoplawski)

[boryspoplawski]

Reviewed 3 of 6 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved