[LibOS] Fix poll() crashing on pseudo-files

[pwmarcz]

Found when investigating stress-ng (#2419).

Description of the changes

The code assumed that all handles (other than TYPE_FILE and TYPE_DEV) had an associated PAL handle, which caused a crash when polling files from /proc. This change adds support for polling TYPE_STR, and also adds a check for PAL handle in all cases.

(I think poll deserves a bigger rewrite, this is just a quick fix).

How to test this PR?

• The procfs stress test (stress-ng --procfs 8 --timeout 40s) stopped crashing for me (see Stress-ng test suite causing memory fault and heap corruption failure in Graphene #2419).
• I rewrote poll regression test to be more thorough, it checks several types of files now.

---

This change is 

[dimakuv]

Reviewed 2 of 6 files at r1.
Reviewable status: 2 of 6 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: ITL)

[dimakuv]

Reviewed 4 of 6 files at r1.
Reviewable status: all 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 @pwmarcz)

---

LibOS/shim/src/fs/shim_fs_pseudo.c, line 446 at r1 (raw file):

/* TODO: add support for polling TYPE_STR handles; currently `shim_do_poll` doesn't call this for
 * anything else than TYPE_STR and TYPE_DEV */

Btw, this comment was wrong? I see that shim_do_poll() calls hdl->fs->fs_ops->poll() on TYPE_FILE and TYPE_DEV. There was no mention of TYPE_STR.

---

LibOS/shim/src/fs/str/fs.c, line 280 at r1 (raw file):

    }
    if (poll_type & FS_POLL_WR)
        ret |= FS_POLL_WR;

Can we really always write in a pseudo-str-file? Some files are read-only (e.g. /dev/attestation/report), don't we want to check whether the permissions allow writing?

---

LibOS/shim/src/sys/shim_poll.c, line 110 at r1 (raw file):

             * callback.
             *
             * TODO: we probably should use the poll() callback in all cases. */

What do you mean by this comment?

[pwmarcz]

Reviewable status: all 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 @dimakuv)

---

LibOS/shim/src/fs/shim_fs_pseudo.c, line 446 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Btw, this comment was wrong? I see that shim_do_poll() calls hdl->fs->fs_ops->poll() on TYPE_FILE and TYPE_DEV. There was no mention of TYPE_STR.

Yes, that was supposed to be "anything else than TYPE_FILE and TYPE_DEV".

---

LibOS/shim/src/fs/str/fs.c, line 280 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can we really always write in a pseudo-str-file? Some files are read-only (e.g. /dev/attestation/report), don't we want to check whether the permissions allow writing?

We check file permissions when setting handle mode, and the caller of poll callback (such as shim_do_poll) checks the handle mode. I don't think the poll callback itself should do that - the check is the same regardless of handle.

(This does allow a loophole where a user chmods the file and then writes to it. I just checked and Linux allows you to chmod a file like /proc/meminfo to be writable, and even open it, but fails with EIO on the actual write call... I don't want to bother with replicating that now, but I'm planning to rewrite the str_* filesystem so maybe that will be the time.)

---

LibOS/shim/src/sys/shim_poll.c, line 110 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What do you mean by this comment?

This function collects PAL handles and uses DkStreamsWaitEvents on them, disregarding the fact that most of filesystems actually have a poll callback defined. It calls the poll callback only for some specific handle types. That seems weird to me: what are the other poll callbacks even for?

I don't want to touch that right now, though, it looks like bigger change and I would need to understand our implementation of poll/epoll/select better. Right now I want to fix this one thing with STR handles.

[dimakuv]

Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)

---

LibOS/shim/src/fs/str/fs.c, line 280 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

We check file permissions when setting handle mode, and the caller of poll callback (such as shim_do_poll) checks the handle mode. I don't think the poll callback itself should do that - the check is the same regardless of handle.

(This does allow a loophole where a user chmods the file and then writes to it. I just checked and Linux allows you to chmod a file like /proc/meminfo to be writable, and even open it, but fails with EIO on the actual write call... I don't want to bother with replicating that now, but I'm planning to rewrite the str_* filesystem so maybe that will be the time.)

OK, fair enough.

---

LibOS/shim/src/sys/shim_poll.c, line 110 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

This function collects PAL handles and uses DkStreamsWaitEvents on them, disregarding the fact that most of filesystems actually have a poll callback defined. It calls the poll callback only for some specific handle types. That seems weird to me: what are the other poll callbacks even for?

I don't want to touch that right now, though, it looks like bigger change and I would need to understand our implementation of poll/epoll/select better. Right now I want to fix this one thing with STR handles.

OK. The idea of a poll callback was to have a fall-back for those handle types that do not have a corresonding PAL handle (= emulated completely inside LibOS, without the help of the host OS). But let's not touch this now.

[mkow]

Reviewed 6 of 6 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved

[mkow]

Reviewed 2 of 2 files at r2.
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)

[dimakuv]

Reviewable status: complete! all files reviewed, all discussions resolved