ON HOLD [LibOS] Add optional app input and output redirection

[boryspoplawski]

PR ON HOLD, due to a unrelated bug (inability to write to one file from two processes concurrently)

Description of the changes

New manifest keys are added: "libos.redirect_fd.stdin", "libos.redirect_fd.stdout" and "libos.redirect_fd.stderr", which enable redirecting stdin, stdout and stderr (respectively). Values associated with those keys should be in-Graphene fs paths.

How to test this PR?

fork_and_exec LibOS regression test was modified.

---

This change is 

[mkow]

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

a discussion (no related file):

New manifest keys are added: "libos.redirect.stdin", "libos.redirect.stdout" and "libos.redirect.stderr"

I'd rather go with "libos.redirect_stdin" etc., because the subkey "libos.redirect" looks like asking for a collision in the future - e.g. if we add some other kind of redirects (network? something else?).

CC: @dimakuv, @pwmarcz, @woju.

---

a discussion (no related file):
Please also update docs about manifest syntax.

---

[boryspoplawski]

Reviewable status: 0 of 4 files reviewed, 2 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, @dimakuv, @pwmarcz, and @woju)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

New manifest keys are added: "libos.redirect.stdin", "libos.redirect.stdout" and "libos.redirect.stderr"

I'd rather go with "libos.redirect_stdin" etc., because the subkey "libos.redirect" looks like asking for a collision in the future - e.g. if we add some other kind of redirects (network? something else?).

CC: @dimakuv, @pwmarcz, @woju.

There is no way we can do anything meaningful with network on LibOS layer (as host can do anything it wants anyway). On other layers the prefix libos is not used. I like how currently all these 3 options are grouped under one subkey (well logically only, but sill).
I understand your concerns, but I do not like libos.redirect_stdin

---

[pwmarcz]

Reviewed 3 of 4 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 (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, @dimakuv, and @woju)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

There is no way we can do anything meaningful with network on LibOS layer (as host can do anything it wants anyway). On other layers the prefix libos is not used. I like how currently all these 3 options are grouped under one subkey (well logically only, but sill).
I understand your concerns, but I do not like libos.redirect_stdin

libos.redirect_fd.*?

---

a discussion (no related file):
@boryspoplawski @mkow IIRC, the original request was about redirecting guest stderr to host stderr (possibly even by default). This change does not make that redirection possible (host stderr is not visible inside Graphene). Is that OK?

---

---

LibOS/shim/src/bookkeep/shim_handle.c, line 244 at r2 (raw file):

    /* initialize stdin */
    if (!HANDLE_ALLOCATED(handle_map->map[0])) {

Is this correct? What if I close stdin/stdout/stderr and then fork?

---

LibOS/shim/test/regression/fork_and_exec.manifest.template, line 22 at r2 (raw file):

fs.mount.bin.uri = "file:/bin"

fs.mount.stdin.type = "chroot"

Is this mount (and of stdin specifically, but not stdout and stderr) necessary, or is it to cover another case? Either way, I think it deserves a comment.

---

LibOS/shim/test/regression/test_libos.py, line 146 at r2 (raw file):

        self.assertIn(b'child exited with status: 0', file_stdout)
        self.assertIn(b'test completed successfully', file_stdout)

Can you also add an assertion for output from child/exec_victim? And maybe also check that Graphene's stdout (from run_binary) is empty.

[mkow]

Reviewable status: all files reviewed, 6 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, @dimakuv, and @woju)

a discussion (no related file):

Previously, pwmarcz (Paweł Marczewski) wrote…

libos.redirect_fd.*?

+1 to Paweł's suggestion

---

a discussion (no related file):

Previously, pwmarcz (Paweł Marczewski) wrote…

@boryspoplawski @mkow IIRC, the original request was about redirecting guest stderr to host stderr (possibly even by default). This change does not make that redirection possible (host stderr is not visible inside Graphene). Is that OK?

But the original proposal is not really consistent, it doesn't say what to do with Graphene logs. Having stderr mixed with them doesn't seem useful.
Also, I don't think any production setup relies on enclave emitting output to raw stdout/stderr, I'm assuming that an ability to write them to a file solves any possible problems the reporter might have had.

---

[boryspoplawski]

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

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

+1 to Paweł's suggestion

Done.

---

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

But the original proposal is not really consistent, it doesn't say what to do with Graphene logs. Having stderr mixed with them doesn't seem useful.
Also, I don't think any production setup relies on enclave emitting output to raw stdout/stderr, I'm assuming that an ability to write them to a file solves any possible problems the reporter might have had.

That feature would only be useful in testing and debugging. On production we should no allow using host stdout/stderr.

---

a discussion (no related file):
Unfortunately writing to the same file from multiple processes does not work currently (due to lack of offset synchronization). This means that this PR needs to be on hold until that's implemented (hopefully it will happen some day).

---

---

LibOS/shim/src/bookkeep/shim_handle.c, line 244 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Is this correct? What if I close stdin/stdout/stderr and then fork?

Discussed offline and done.

---

LibOS/shim/test/regression/fork_and_exec.manifest.template, line 22 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Is this mount (and of stdin specifically, but not stdout and stderr) necessary, or is it to cover another case? Either way, I think it deserves a comment.

Done.

---

LibOS/shim/test/regression/test_libos.py, line 146 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Can you also add an assertion for output from child/exec_victim? And maybe also check that Graphene's stdout (from run_binary) is empty.

Done.

[dimakuv]

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

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

Done.

libos.redirect_fd.* sounds good to me.

---