[Pal] Support opening TTY devices with O_TRUNC flag

[jkr0103]

Description of the changes

Write to /proc/self/fd/2 (which is symlink to /dev/tty) fails with O_TRUNC flag which translates to DkStreamSetLength as setlength function is not implemented for dev:tty devices.

This PR adds dummy function setlength to support O_TRUNC flag.

Fixes #303

How to test this PR?

Should be able to execute below commands successfully:

gramine-direct busybox sh -c 'echo foo>/proc/self/fd/2'
gramine-sgx busybox sh -c 'echo foo>/proc/self/fd/2'

as described in link #303 (comment)

---

This change is 

[dimakuv]

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

---

-- commits, line 2 at r1:
I think the better commit message will be:

[Pal] Add `setlength` callback to PAL device handles

This is required to support opening TTY devices with O_TRUNC flag.

---

Pal/src/host/Skeleton/db_devices.c, line 45 at r1 (raw file):

/* this dummy function is implemented to support opening TTY devices with O_TRUNC flag */
static int64_t dev_setlength(PAL_HANDLE handle, uint64_t length) {
    return 0;

Because we are in the Skeleton PAL (which doesn't implement even dummy stuff, but instead always returns -PAL_ERROR_NOTIMPLEMENTED).

So please replace with return -PAL_ERROR_NOTIMPLEMENTED.

[jkr0103]

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @pwmarcz)

---

-- commits, line 2 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I think the better commit message will be:

[Pal] Add `setlength` callback to PAL device handles

This is required to support opening TTY devices with O_TRUNC flag.

Done.

---

Pal/src/host/Skeleton/db_devices.c, line 45 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Because we are in the Skeleton PAL (which doesn't implement even dummy stuff, but instead always returns -PAL_ERROR_NOTIMPLEMENTED).

So please replace with return -PAL_ERROR_NOTIMPLEMENTED.

Done.

[dimakuv]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @pwmarcz)

[dimakuv]

Jenkins, test this please

[dimakuv]

Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @pwmarcz)

a discussion (no related file):
FYI: I manually verified that this bug fix works on the Busybox example (provided in the "How to test this PR" section). Looks good.

---

[boryspoplawski]

Reviewed 1 of 3 files at r1.
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, @jkr0103, and @pwmarcz)

a discussion (no related file):
From O_TRUNC description: If the file is a FIFO or terminal device file, the O_TRUNC flag is ignored. so the solution should be to ignore this flag for such handle types in LibOS.

---

[boryspoplawski]

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 @pwmarcz)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

From O_TRUNC description: If the file is a FIFO or terminal device file, the O_TRUNC flag is ignored. so the solution should be to ignore this flag for such handle types in LibOS.

Oh wait, the problem is that this is host TTY, not LibOS one

---

[boryspoplawski]

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, 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 @jkr0103 and @pwmarcz)

---

Pal/src/host/Linux/db_devices.c, line 203 at r2 (raw file):

    __UNUSED(handle);
    __UNUSED(length);
    return 0;

Would be good to check if a handle is really a tty and return -PAL_ERROR_NOTSUPPORT otherwise.
Also we need to check length and return -PAL_ERROR_INVAL if it's != 0.

---

Pal/src/host/Linux-SGX/db_devices.c, line 213 at r2 (raw file):

    __UNUSED(handle);
    __UNUSED(length);
    return 0;

ditto (tty check)

[dimakuv]

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, @jkr0103, and @pwmarcz)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

Oh wait, the problem is that this is host TTY, not LibOS one

Yes, it's about behavior of the LibOS created fd[0], fd[1], fd[2].

---

---

Pal/src/host/Linux/db_devices.c, line 203 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Would be good to check if a handle is really a tty and return -PAL_ERROR_NOTSUPPORT otherwise.
Also we need to check length and return -PAL_ERROR_INVAL if it's != 0.

• Looking at the other code in this file, we have "checks if a handle is really a tty" like this:

    if (HANDLE_HDR(handle)->type != PAL_TYPE_DEV)
        return -PAL_ERROR_INVAL;

(In other words, they return -PAL_ERROR_INVAL.)

I suggest to use the same check as in other places in this file.

• I agree with Borys's comment on checking length.

[boryspoplawski]

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 @dimakuv, @jkr0103, and @pwmarcz)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yes, it's about behavior of the LibOS created fd[0], fd[1], fd[2].

Yeah, I've got completely confused because there was no tty inside dev emulation file in LibOS. Turns out it's added later via some mounts.

---

---

Pal/src/host/Linux/db_devices.c, line 203 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

• Looking at the other code in this file, we have "checks if a handle is really a tty" like this:

    if (HANDLE_HDR(handle)->type != PAL_TYPE_DEV)
        return -PAL_ERROR_INVAL;

(In other words, they return -PAL_ERROR_INVAL.)

I suggest to use the same check as in other places in this file.

• I agree with Borys's comment on checking length.

Ideally we would check not only if type is PAL_TYPE_DEV but also if it's really a tty. Probably need to get the uri from handle and compare? Not sure, you need to find the good solution :)

[dimakuv]

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, @jkr0103, and @pwmarcz)

---

Pal/src/host/Linux/db_devices.c, line 32 at r2 (raw file):

    assert(WITHIN_MASK(options, PAL_OPTION_MASK));

    PAL_HANDLE hdl = calloc(1, HANDLE_SIZE(dev));

FYI: @boryspoplawski If you look at this function, you can see that we don't memorize uri anywhere. Which is probably a bad thing, but definitely not for this PR.

---

Pal/src/host/Linux/db_devices.c, line 203 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Ideally we would check not only if type is PAL_TYPE_DEV but also if it's really a tty. Probably need to get the uri from handle and compare? Not sure, you need to find the good solution :)

Unfortunately, we don't have a URI or any other "name" field for device handles: https://github.com/gramineproject/gramine/blob/master/Pal/src/host/Linux/pal_host.h#L64-L67

In my other comment, you can see that we don't even try to memorize the name/uri anywhere upon device-handle creation.

[jkr0103]

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, @jkr0103, and @pwmarcz)

---

Pal/src/host/Linux/db_devices.c, line 203 at r2 (raw file):

-PAL_ERROR_INVAL
can we have below check for tty device, I see similar kind of check in same file?
if (handle->dev.fd < 0 || handle->dev.fd > 2) return -PAL_ERROR_NOTSUPPORT;

[dimakuv]

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, @jkr0103, and @pwmarcz)

---

Pal/src/host/Linux/db_devices.c, line 203 at r2 (raw file):

Previously, jkr0103 (Jitender Kumar) wrote…

-PAL_ERROR_INVAL
can we have below check for tty device, I see similar kind of check in same file?
if (handle->dev.fd < 0 || handle->dev.fd > 2) return -PAL_ERROR_NOTSUPPORT;

Yes, I think we can also add such a check. So I would recommend the following change:

    if (HANDLE_HDR(handle)->type != PAL_TYPE_DEV)
        return -PAL_ERROR_INVAL;

    if (!(handle->dev.fd == 0 || handle->dev.fd == 1))
        return -PAL_ERROR_NOTSUPPORT;

[jkr0103]

Reviewable status: 1 of 3 files reviewed, 2 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 @boryspoplawski, @dimakuv, and @pwmarcz)

---

Pal/src/host/Linux/db_devices.c, line 203 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yes, I think we can also add such a check. So I would recommend the following change:

    if (HANDLE_HDR(handle)->type != PAL_TYPE_DEV)
        return -PAL_ERROR_INVAL;

    if (!(handle->dev.fd == 0 || handle->dev.fd == 1))
        return -PAL_ERROR_NOTSUPPORT;

Done.

---

Pal/src/host/Linux-SGX/db_devices.c, line 213 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ditto (tty check)

Done.

[boryspoplawski]

Reviewed 2 of 2 files at r3, 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 (waiting on @pwmarcz)

[boryspoplawski]

Jenkins, test this please

[dimakuv]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

[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) (waiting on @pwmarcz)

[dimakuv]

Jenkins, test this please

[boryspoplawski]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @pwmarcz)