Skip to content

Fix data loss in rootfs overlayfs when unmount of tmp dirs fail with idmap mounts #10721

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.

Sign up for GitHub

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

Merged
fuweid merged 3 commits into from
Oct 15, 2024
Merged

Fix data loss in rootfs overlayfs when unmount of tmp dirs fail with idmap mounts #10721

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
004f395
core/mount: Use MNT_DETACH for umount of tmp layers
rata Sep 23, 2024
f8d84ec
core/mount: Prevent accidental removal of rootfs files
rata Sep 23, 2024
30f2893
core/mount: Only remove dirs if unmount succeeded
rata Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions core/mount/mount_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,12 +251,22 @@ func doPrepareIDMappedOverlay(lowerDirs []string, usernsFd int) (tmpLowerDirs []
}
cleanUp := func() {
for _, lowerDir := range tmpLowerDirs {
if err := unix.Unmount(lowerDir, 0); err != nil {
// Do a detached unmount so even if the resource is busy, the mount will be
// gone (eventually) and we can safely delete the directory too.
if err := unix.Unmount(lowerDir, unix.MNT_DETACH); err != nil {
Copy link
Copy Markdown
Member

@fuweid fuweid Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

MNT_DETACH isn't a best option :p
The cleanup is defer callback. It's hard to notify that caller to retry.
Based on this fact, the change is acceptable to me.

However, I think we can handle this issue in other way.
Based on mount_setattr manpage, the MOUNT_ATTR_RDONLY can force the temporary mount in read-only. And no one can delete the content.

NOTE: Even if there are leaky mountpoint, containerd will clean it up during booting.

containerd/cmd/containerd/command/main.go

Line 181 in 08037e7

warnings, err := mount.CleanupTempMounts(0)

It's kind of enhancement and it needs to change IdMapMount interface.
It should be handled in the follow-up.

Copy link
Copy Markdown
Contributor Author

@rata rata Oct 17, 2024

Choose a reason for hiding this comment

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

@mbaynton are you interested in tackling this?

Copy link
Copy Markdown
Contributor

@mbaynton mbaynton Oct 23, 2024

Choose a reason for hiding this comment

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

Hmm, potentially. I'm most interested in investigating a new code path for Linux 6.13+ using the new file descriptor based layer setup and the new mount API. I think this will keep the idmap temp mounts off the host filesystem tree at all times, so the one and only accessor of them will be the desired overlayfs, so (I hypothesize) there will be no possibility of EBUSY failures / no need to use MNT_DETACH / no possibility of leaky mountpoints.

But an alternative implementation that removes MNT_DETACH, instead makes these read-only as another safeguard, and accepts the possibility that the unmount may fail without MNT_DETACH seems pretty easy to put together too.

Copy link
Copy Markdown
Contributor

@mbaynton mbaynton Oct 26, 2024

Choose a reason for hiding this comment

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

@rata it sounds like you already started a follow-up?

In the follow-up PR this thing is removed and handled differently (IMHO, it is clearer).

I don't see one pushed to rata/containerd but are there improvements you've got?

Copy link
Copy Markdown
Contributor

@mbaynton mbaynton Oct 30, 2024

Choose a reason for hiding this comment

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

I should have something for this to address @fuweid's request in a week.

Copy link
Copy Markdown
Contributor

@mbaynton mbaynton Nov 6, 2024

Choose a reason for hiding this comment

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

#10955

log.L.WithError(err).Warnf("failed to unmount temp lowerdir %s", lowerDir)
continue
}
// Using os.Remove() so if it's not empty, we don't delete files in the
// rootfs.
if err := os.Remove(lowerDir); err != nil {
log.L.WithError(err).Warnf("failed to remove temporary overlay lowerdir's")
}
}
if terr := os.RemoveAll(filepath.Clean(filepath.Join(tmpLowerDirs[0], ".."))); terr != nil {
log.L.WithError(terr).Warnf("failed to remove temporary overlay lowerdir's")

// This dir should be empty now. Otherwise, we don't do anything.
if err := os.Remove(filepath.Join(tmpLowerDirs[0], "..")); err != nil {
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda Sep 25, 2024

Choose a reason for hiding this comment

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

Can't we just use filepath.Dir? (And validate the path)

Copy link
Copy Markdown
Contributor Author

@rata rata Sep 25, 2024

Choose a reason for hiding this comment

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

Yes, I'm preparing a follow-up PR with more improvements. This is how it was written before, I'd improve it (that and other things, hopefully including tests :))

This PR is just about fixing the data loss, if that is fine, to land ASAP.

Copy link
Copy Markdown
Contributor Author

@rata rata Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the follow-up PR this thing is removed and handled differently (IMHO, it is clearer). So I'd avoid improving lines that we will remove anyways :)

log.L.WithError(err).Infof("failed to remove temporary overlay dir")
}
}
for i, lowerDir := range lowerDirs {
Expand Down