Maintain metadata checksums and verify on unlink #388
Conversation
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
passaro
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
mountpoint-s3/src/fs.rs
Outdated
| @@ -804,6 +804,7 @@ impl From<InodeError> for i32 { | |||
| InodeError::CannotRemoveRemoteDirectory(_) => libc::EPERM, | |||
| InodeError::DirectoryNotEmpty(_) => libc::ENOTEMPTY, | |||
| InodeError::UnlinkNotPermittedWhileWriting(_) => libc::EPERM, | |||
| InodeError::CorruptedMetadata(_, _) => libc::EPERM, | |||
There was a problem hiding this comment.
Not sure if there is a more fitting error.
There was a problem hiding this comment.
I think maybe EIO is slightly more appropriate, but it's pretty niche either way.
mountpoint-s3/src/fs.rs
Outdated
| @@ -804,6 +804,7 @@ impl From<InodeError> for i32 { | |||
| InodeError::CannotRemoveRemoteDirectory(_) => libc::EPERM, | |||
| InodeError::DirectoryNotEmpty(_) => libc::ENOTEMPTY, | |||
| InodeError::UnlinkNotPermittedWhileWriting(_) => libc::EPERM, | |||
| InodeError::CorruptedMetadata(_, _) => libc::EPERM, | |||
There was a problem hiding this comment.
I think maybe EIO is slightly more appropriate, but it's pretty niche either way.
mountpoint-s3/src/inode.rs
Outdated
| .expect("file should exist"); | ||
|
|
||
| // Inject a key mutation in the Inode. | ||
| // SAFETY: Inode and String not accessed concurrently. Mutated string is still valid utf8. |
There was a problem hiding this comment.
This is actually undefined behavior and so not safe (mutating data to which a shared reference exists is always UB). The risk here is low in practice — the way this would go wrong is if an extremely smart compiler previously cached the full_key in a register somewhere, which would be OK because it's a shared immutable field, and so didn't observe this mutation to the key the next time it read it. But in theory the compiler is free to do whatever it wants with the entire program when it sees this.
As an alternative, maybe you could manually construct a new Inode struct here as a copy of the existing one, and then overwrite it in the right places (the superblock and the parent of this inode)? It's a lot more coupled to the internals, but maybe unavoidable.
mountpoint-s3/src/inode.rs
Outdated
| @@ -360,6 +360,10 @@ impl Superblock { | |||
| return Err(InodeError::IsDirectory(inode.ino())); | |||
| } | |||
|
|
|||
| if !inode.verify_checksum() { | |||
There was a problem hiding this comment.
Maybe we should just make self.inner.get do this check every time?
There was a problem hiding this comment.
Maybe. And/or in self.inner.lookup (or it would miss unlink for example)?
There was a problem hiding this comment.
Done in latest commit.
There was a problem hiding this comment.
Or maybe instead we should verify the checksum when we get the full key?
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
passaro
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
passaro
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
mountpoint-s3/src/inode.rs
Outdated
| .read() | ||
| .unwrap() | ||
| .get(&ino) | ||
| .cloned() | ||
| .ok_or(InodeError::InodeDoesNotExist(ino)) | ||
| .ok_or(InodeError::InodeDoesNotExist(ino))?; | ||
| inode.verify_checksum()?; |
There was a problem hiding this comment.
We should also check that inode.ino() == ino here to be safe.
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
passaro
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
| } | ||
| } | ||
|
|
||
| fn verify_child(&self, expected_parent: InodeNo, expected_name: &str) -> Result<(), InodeError> { |
Description of change
Adds a checksum to each inode computed on the inode number and the full key. On unlink, verifies the checksum before deleting the object from the bucket.
Does this change impact existing behavior?
No impact on the happy path. Unlink could fail if inode metadata gets corrupted.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).