[LibOS] Fix ENOENT error in fchown on unlinked file

[dimakuv]

Description of the changes

This is conceptually similar to the commit "[LibOS] Fix ENOENT error in fchmod on unlinked file".

This PR was created while reviewing #1874.

How to test this PR?

Added a sub-test to the rename_unlink LibOS regression test.

If you want to run this sub-test on vanilla Linux, change UID and GID in fchown() to some reasonable IDs available on your system.

---

This change is 

[dimakuv]

Reviewable status: 0 of 2 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)

a discussion (no related file):
FYI: I double checked that vanilla Linux kernel correctly fchown()s the file even after it was unlinked.

---

a discussion (no related file):
This PR is modelled exactly after #1538.

---

---

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

    unlock(&hdl->inode->lock);

    ret = 0;

FYI: This is just a random improvement of the related code.

[oshogbo]

Reviewable status: 0 of 2 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)

[g2flyer]

As mentioned in discussion of #1874 i can confirm that this PR fixes the fchown issue identified in #1874 and in more minimal (and complete :-)) ways than #1874. I wonder though whether the new test-vectors from #1874 might be better add directly to this PR rather than in a separate one?

Reviewable status: 0 of 2 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)

[dimakuv]

Done, added your sub-tests too.

I force-pushed since Mariusz finished his review, others didn't start it, and I wanted to not forget to add Michael Steiner as a co-author in the commit message.

Reviewable status: 0 of 2 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)

[mkow]

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

---

libos/test/regression/rename_unlink.c line 230 at r2 (raw file):

        err(1, "create %s", path1);

    if (fchown(fd, /*owner=*/123, /*group=*/123) != 0) /* dummy owner/group just for testing */

This fails when run natively, could you change this test somehow so that it also works natively? It would make it easier to verify whether everything in Gramine works exactly as on Linux.

[g2flyer]

This fails when run natively, could you change this test somehow so that it also works natively? It would make it easier to verify whether everything in Gramine works exactly as on Linux.

In my original version from that did run natively as long as you run it as root (i actually had also a corresponding comment) and i think that is somewhat unavoidable. I've used different ids but i think it still works with 123, the ids do not necessarily have to exist in /etc/passwd et al for it to work.

[mkow]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)

---

libos/test/regression/rename_unlink.c line 230 at r2 (raw file):
@g2flyer: Please reply using Reviewable, so that your reply lands in a proper thread ;)

In my original version from that did run natively as long as you run it as root (i actually had also a corresponding comment) and i think that is somewhat unavoidable. I've used different ids but i think it still works with 123, the ids do not necessarily have to exist in /etc/passwd et al for it to work.

In the future we'd like to run these tests in CI also natively, and that requirement would prevent this. It should be possible to change it, maybe chown() to the current user? (no-op, but should still trigger all the logic).

[g2flyer]

In the future we'd like to run these tests in CI also natively, and that requirement would prevent this. It should be possible to change it, maybe chown() to the current user? (no-op, but should still trigger all the logic).

We don't run CI containerized? Otherwise running as "root" is not really a problem? Using your own id (and essentially not changing the ids) wouldn't really work, e.g., in test_rename_fchown_fchmod to detect all potential issues? In particular, i don't think you would have detected the original bug?

[mkow]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)

---

libos/test/regression/rename_unlink.c line 230 at r2 (raw file):

We don't run CI containerized? Otherwise running as "root" is not really a problem? Using your own id (and essentially not changing the ids) wouldn't really work, e.g., in test_rename_fchown_fchmod to detect all potential issues? In particular, i don't think you would have detected the original bug?

Ah, right, then it should work in CI, it will just be annoying when running the tests manually, outside of CI.
@dimakuv, @woju, @kailun-qin, @oshogbo: what do you think? Especially, is there a better way to implement this test?

@g2flyer: and please again, use Reviewable for comments :) When you comment on GitHub your comments are not linked to the discussion thread and land in a different place, without the context.

[woju]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv, @g2flyer, @kailun-qin, and @oshogbo)

---

libos/test/regression/rename_unlink.c line 230 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

We don't run CI containerized? Otherwise running as "root" is not really a problem? Using your own id (and essentially not changing the ids) wouldn't really work, e.g., in test_rename_fchown_fchmod to detect all potential issues? In particular, i don't think you would have detected the original bug?

Ah, right, then it should work in CI, it will just be annoying when running the tests manually, outside of CI.
@dimakuv, @woju, @kailun-qin, @oshogbo: what do you think? Especially, is there a better way to implement this test?

@g2flyer: and please again, use Reviewable for comments :) When you comment on GitHub your comments are not linked to the discussion thread and land in a different place, without the context.

If you're non-root (and without CAP_FOWNER), fchmod should succeed if gid matches current euid and that euid (user) is a member of the group that is the param of the operation. So you could make it this way:

1. geteuid()
2. if euid is 0 pick something, maybe 65534:65534 (nobody:nobody)
3. getgroups()
4. if there is only one group and matches the existing file, skip the test,
5. pick first group that does not match the group of the target file,
6. fchown(fd, geteuid(), <group from point 5>).

[woju]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv, @g2flyer, @kailun-qin, and @oshogbo)

---

libos/test/regression/rename_unlink.c line 230 at r2 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

If you're non-root (and without CAP_FOWNER), fchmod should succeed if gid matches current euid and that euid (user) is a member of the group that is the param of the operation. So you could make it this way:

1. geteuid()
2. if euid is 0 pick something, maybe 65534:65534 (nobody:nobody)
3. getgroups()
4. if there is only one group and matches the existing file, skip the test,
5. pick first group that does not match the group of the target file,
6. fchown(fd, geteuid(), <group from point 5>).

... if uid matches current euid