Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LibOS] Fix ENOENT error in fchmod on unlinked file #1538

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

sahason
Copy link
Contributor

@sahason sahason commented Sep 6, 2023

Description of the changes

Fix ENOENT error when fchmod is called on unlinked file

How to test this PR?

rename_unlink regression test is extended to validate this implementation.


This change is Reviewable

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 files at r1, 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: ) (waiting on @sahason)

a discussion (no related file):
Generally a good PR! What was the workload that failed on it, and why? (It's rather uncommon for application to remove the file, and then continue modifying its parameters.)



libos/include/libos_fs.h line 386 at r1 (raw file):

     * On success, the caller should update `hdl->inode->perm`.
     */
    int (*fchmod)(struct libos_handle* hdl, mode_t perm);

This callback should be declared in libos_fs_ops, not in libos_d_ops. Please move it there.


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

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

You don't need these three lines, because you already have it in the fchmod() implementations.

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 6 files at r1, 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: ) (waiting on @sahason)

Copy link
Contributor Author

@sahason sahason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 6 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! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Generally a good PR! What was the workload that failed on it, and why? (It's rather uncommon for application to remove the file, and then continue modifying its parameters.)

Gunicorn workload failed on it. I assume this is the file (https://github.com/benoitc/gunicorn/blob/master/gunicorn/workers/workertmp.py).



libos/include/libos_fs.h line 386 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This callback should be declared in libos_fs_ops, not in libos_d_ops. Please move it there.

Done.


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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You don't need these three lines, because you already have it in the fchmod() implementations.

These lines are not part of fchmod implementation for tmpfs if I remove this chmod_stat in libos/fs/test fails for tmpfs file. https://github.com/gramineproject/gramine/blob/94123cb244cf1b4dbfc0bfcb0805806ca6fcb1a5/libos/test/fs/chmod_stat.c#L35C1-L35C1. So, I have kept this part as this is common for all filesystems and removed fromfchmod implementation of chroot and encrypted.

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 5 files at r2, all commit messages.
Reviewable status: 3 of 6 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: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @sahason)


libos/include/libos_fs.h line 192 at r2 (raw file):

     * \brief Change file permissions.
     *
     * \param hdl  File handle.

Please fix the alignment.


libos/include/libos_fs.h line 195 at r2 (raw file):

     * \param perm  New permissions for the file.
     *
     * Changes the permissions of a file associated with a given file descriptor.

Suggestion:

associated with a given file handle

libos/include/libos_fs.h line 197 at r2 (raw file):

     * Changes the permissions of a file associated with a given file descriptor.
     *
     * On success, the caller should update `hdl->inode->perm`.

Hmm, why the caller, not this function? Seems error prone.

Code quote:

On success, the caller should update `hdl->inode->perm`.

libos/src/fs/chroot/encrypted.c line 236 at r2 (raw file):

    hdl->type = TYPE_CHROOT_ENCRYPTED;
    hdl->pos = 0;
    hdl->pal_handle = enc->pal_handle;

Can you explain this change? What happens with the old hdl->pal_handle? Is it leaked? Or maybe it is guaranteed to always be NULL here?


libos/src/fs/chroot/encrypted.c line 386 at r2 (raw file):

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

Where's the matching lock()?

Copy link
Contributor Author

@sahason sahason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 6 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: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)


libos/include/libos_fs.h line 192 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please fix the alignment.

Done.


libos/include/libos_fs.h line 195 at r2 (raw file):

     * \param perm  New permissions for the file.
     *
     * Changes the permissions of a file associated with a given file descriptor.

Done.


libos/include/libos_fs.h line 197 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Hmm, why the caller, not this function? Seems error prone.

Done.


libos/src/fs/chroot/encrypted.c line 236 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Can you explain this change? What happens with the old hdl->pal_handle? Is it leaked? Or maybe it is guaranteed to always be NULL here?

This associates the PAL handle with a LibOS handle. It is not leaked and it is guaranteed to be always NULL here. After this is set to 0 at the time of LibOS handle creation there is no other place where it is assigned to any value.


libos/src/fs/chroot/encrypted.c line 386 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Where's the matching lock()?

Missed removing it in last commit. It is not required.

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 5 files at r2, 3 of 3 files at r3, 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! " found in commit messages' one-liners (waiting on @mkow and @sahason)


libos/include/libos_fs.h line 197 at r2 (raw file):

Previously, sahason wrote…

Done.

Yes, this function should update it. Not the caller. (Which was so in your first commit, but then you changed it in your fixup commits.)


libos/src/fs/chroot/encrypted.c line 236 at r2 (raw file):

Previously, sahason wrote…

This associates the PAL handle with a LibOS handle. It is not leaked and it is guaranteed to be always NULL here. After this is set to 0 at the time of LibOS handle creation there is no other place where it is assigned to any value.

@mkow There is no "old" pal_handle. It was previously intentionally set to NULL. You can check the top-level comments in this file.

Pawel did it so to encapsulate operations with the "real host FD" (which is basically what pal_handle gives access to). The "real host FD" is supposed to be accessed only via the struct libos_encrypted_file object, to signify that all operations are supposed to go through the "encryption/decryption" layer of logic.

To get to the pal_handle, it typically looks like this:

struct libos_encrypted_file* enc = hdl->inode->data;
do_something_with(enc->pal_handle);

In here, @sahason added a shortcut to be able to directly access hdl->pal_handle. I first regarded it as fine, but now I think it's a slippery slope. I think we need to do the above, and not touch hdl->pal_handle.


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

Previously, sahason wrote…

These lines are not part of fchmod implementation for tmpfs if I remove this chmod_stat in libos/fs/test fails for tmpfs file. https://github.com/gramineproject/gramine/blob/94123cb244cf1b4dbfc0bfcb0805806ca6fcb1a5/libos/test/fs/chmod_stat.c#L35C1-L35C1. So, I have kept this part as this is common for all filesystems and removed fromfchmod implementation of chroot and encrypted.

Well, it's a bug in tmpfs... I see that the same problem is with chmod() implementation, so this bug was already there.

Please revert your last change, and keep hdl->inode->perm = perm; line in each of the fchmod() callback implementations (and the surrounding locks and asserts, as it was before your fixup commit).

We can keep these three lines here for now, and we'll decide what to do with them in the next iteration of this PR.

Copy link
Contributor Author

@sahason sahason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 6 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! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)


libos/include/libos_fs.h line 197 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yes, this function should update it. Not the caller. (Which was so in your first commit, but then you changed it in your fixup commits.)

No Dmitrii it was there in first commit and for existing 'chmod' description.


libos/src/fs/chroot/encrypted.c line 236 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@mkow There is no "old" pal_handle. It was previously intentionally set to NULL. You can check the top-level comments in this file.

Pawel did it so to encapsulate operations with the "real host FD" (which is basically what pal_handle gives access to). The "real host FD" is supposed to be accessed only via the struct libos_encrypted_file object, to signify that all operations are supposed to go through the "encryption/decryption" layer of logic.

To get to the pal_handle, it typically looks like this:

struct libos_encrypted_file* enc = hdl->inode->data;
do_something_with(enc->pal_handle);

In here, @sahason added a shortcut to be able to directly access hdl->pal_handle. I first regarded it as fine, but now I think it's a slippery slope. I think we need to do the above, and not touch hdl->pal_handle.

Thanks for the explanation. Updated it.


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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Well, it's a bug in tmpfs... I see that the same problem is with chmod() implementation, so this bug was already there.

Please revert your last change, and keep hdl->inode->perm = perm; line in each of the fchmod() callback implementations (and the surrounding locks and asserts, as it was before your fixup commit).

We can keep these three lines here for now, and we'll decide what to do with them in the next iteration of this PR.

Reverted.

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


libos/include/libos_fs.h line 197 at r2 (raw file):

No Dmitrii it was there in first commit

But that's exactly what he said? It was in the first version of the PR, but then you removed it when changing code?

this function should update

Now you're describing the function, not the caller, so it should be "this function updates".


libos/src/fs/chroot/encrypted.c line 386 at r2 (raw file):

Previously, sahason wrote…

Missed removing it in last commit. It is not required.

It is definitely required, you can't have unmatched locks/unlocks.


libos/src/fs/chroot/encrypted.c line 375 at r4 (raw file):

static int chroot_encrypted_fchmod(struct libos_handle* hdl, mode_t perm) {
    assert(hdl->inode);

Why the assert here?


libos/src/fs/chroot/fs.c line 480 at r4 (raw file):

static int chroot_fchmod(struct libos_handle* hdl, mode_t perm) {
    assert(hdl->inode);

What's the point of this assert?

Copy link
Contributor Author

@sahason sahason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


libos/include/libos_fs.h line 197 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

No Dmitrii it was there in first commit

But that's exactly what he said? It was in the first version of the PR, but then you removed it when changing code?

this function should update

Now you're describing the function, not the caller, so it should be "this function updates".

Ok. It is addressed.


libos/src/fs/chroot/encrypted.c line 386 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

It is definitely required, you can't have unmatched locks/unlocks.

Wanted to remove both lock() and unlcok() as the hdl->node->perm was updated in the caller as well but missed removingunlock(). But reverted the code as per the review comments.


libos/src/fs/chroot/encrypted.c line 375 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why the assert here?

Before accessing lock checking if hdl->inode is non-null.


libos/src/fs/chroot/fs.c line 480 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

What's the point of this assert?

Before accessing lock checking if hdl->inode is non-null.

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


libos/include/libos_fs.h line 197 at r2 (raw file):

Previously, sahason wrote…

Ok. It is addressed.

Hmm, it's not?


libos/src/fs/chroot/encrypted.c line 375 at r4 (raw file):

Previously, sahason wrote…

Before accessing lock checking if hdl->inode is non-null.

I mean, I know what this line does, the question is why you added it. No other function here asserts for this, and also there's no point in asserting all possible caller errors, only the likely ones (e.g. you don't assert here that hdl != NULL, but assert hdl->inode , why?).

Copy link
Contributor Author

@sahason sahason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


libos/include/libos_fs.h line 197 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Hmm, it's not?

Ok. I thought it was about caller vs function. Updated it.


libos/src/fs/chroot/encrypted.c line 375 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I mean, I know what this line does, the question is why you added it. No other function here asserts for this, and also there's no point in asserting all possible caller errors, only the likely ones (e.g. you don't assert here that hdl != NULL, but assert hdl->inode , why?).

The fchmod is written similarly as chmod

assert(dent->inode);
. In chmod there is an assert for dent->inode. Hence added in fchmod as well. If it is unnecessary, I will remove it.

mkow
mkow previously approved these changes Sep 8, 2023
Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r5, all commit messages.
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), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)


libos/src/fs/chroot/encrypted.c line 375 at r4 (raw file):

Previously, sahason wrote…

The fchmod is written similarly as chmod

assert(dent->inode);
. In chmod there is an assert for dent->inode. Hence added in fchmod as well. If it is unnecessary, I will remove it.

Ok, I see that chmod also does this, let's leave it then for consistency.

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, 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 @sahason)


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

Previously, sahason wrote…

Reverted.

Ok, sorry to go back and forth, but I did a deep dive, and the perm assignment should actually happen in the caller, not in this fchmod() callback.

Similarly to the case of chmod(), which I fixed in my PR: #1542

So actually we'll need to do the following:

  1. Wait until that PR is merged (it becomes a prerequisite)
  2. Rebase this PR.
  3. Revert to the no-perm-assignment version (which will also have no lock/unlock).

I can take this PR over, and drive it to conclusion.

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 6 files at r1, 2 of 5 files at r2, 2 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, 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 @sahason)

Copy link
Contributor Author

@sahason sahason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all 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: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)


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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Ok, sorry to go back and forth, but I did a deep dive, and the perm assignment should actually happen in the caller, not in this fchmod() callback.

Similarly to the case of chmod(), which I fixed in my PR: #1542

So actually we'll need to do the following:

  1. Wait until that PR is merged (it becomes a prerequisite)
  2. Rebase this PR.
  3. Revert to the no-perm-assignment version (which will also have no lock/unlock).

I can take this PR over, and drive it to conclusion.

I have gone through PR #1542. I would like to finish this PR as we have one fixup commit like this. What do you say?

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all 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: ITL), "fixup! " found in commit messages' one-liners (waiting on @sahason)


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

Previously, sahason wrote…

I have gone through PR #1542. I would like to finish this PR as we have one fixup commit like this. What do you say?

@sahason Sure, you can continue with this PR then. You'll need to wait until #1542 is merged, then rebase your PR on top of it, and apply that fixup commit that does the same logic.

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all 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: ITL), "fixup! " found in commit messages' one-liners (waiting on @sahason)


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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@sahason Sure, you can continue with this PR then. You'll need to wait until #1542 is merged, then rebase your PR on top of it, and apply that fixup commit that does the same logic.

#1542 was just merged. @sahason Can you rebase this PR now?

Copy link
Contributor Author

@sahason sahason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion, 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 @dimakuv, @kailun-qin, and @mkow)


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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

#1542 was just merged. @sahason Can you rebase this PR now?

Done.

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r6, 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), "fixup! " found in commit messages' one-liners

@dimakuv
Copy link
Contributor

dimakuv commented Sep 20, 2023

Jenkins, test this please

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

Signed-off-by: Sonali Saha <sonali.saha@intel.com>
Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@mkow
Copy link
Member

mkow commented Sep 20, 2023

Jenkins, retest this please

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@mkow mkow merged commit ac8fc77 into gramineproject:master Sep 20, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants