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.

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

Jump to bottom

Correctly restore containers with nested bind mounts #2798

Merged
merged 2 commits into from Mar 17, 2021
Merged

Correctly restore containers with nested bind mounts #2798

merged 2 commits into from Mar 17, 2021

Conversation

adrianreber
Copy link
Contributor

This tries to fix #2748

This change is motivated by the work in kubernetes/kubernetes#97194

During restore CRIU mounts all mounts as they were during checkpointing. Using nested bind mounts this can lead to situation where a mountpoint in the parent bind mount is missing.

During initial container creation 'runc' mounts a bind mount and then creates the mountpoints for nested mountpoints.

This PR changes runc to also mount all bind mounts before telling CRIU to restore the container to re-create missing mountpoints.

@cyphar cyphar changed the title Correcty restore containers with nested bind mounts Correctly restore containers with nested bind mounts Feb 8, 2021
@adrianreber adrianreber force-pushed the 2021-02-08-nested-bind.mounts branch 2 times, most recently from 11c7d94 to 0283608 Compare February 9, 2021 06:50
kolyshkin
kolyshkin requested changes Feb 9, 2021
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

Just a single nit about making temp dirs under CWD, LGTM otherwise, thanks @adrianreber!

I have also checked that the test case indeed fails before the fix.

@adrianreber
Re-create mountpoints during restore
705b6cc 
runc was already re-creating mountpoints before calling CRIU to restore
a container. But mountpoints inside a bind mount were not re-created.

During initial container creation runc will mount bind mounts and then
create the necessary mountpoints for further mounts inside those bind
mounts.

If, for example, one of the bind mounts is a tmpfs and empty before
restore, CRIU will fail re-mounting all mounts because the mountpoints
in the bind mounted tmpfs no longer exist.

CRIU expects all mount points to exist as during checkpointing.

This changes runc to mount bind mounts after mountpoint creation to
ensure nested bind mounts have their mountpoints created before CRIU
does the restore.

Signed-off-by: Adrian Reber <areber@redhat.com>
@adrianreber
tests: test nested bind mount restore
051646a 
This adds a checkpoint/restore test to ensure that nested bind mount
mountpoints are correctly re-created.

Signed-off-by: Adrian Reber <areber@redhat.com>
@adrianreber
Copy link
Contributor Author

Fixed mktemp to use -p . and removed old comment.

kolyshkin
kolyshkin approved these changes Feb 11, 2021
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM, and thanks!

@adrianreber adrianreber mentioned this pull request Mar 9, 2021
Closed
@kolyshkin kolyshkin added this to the 1.0.0-rc94 milestone Mar 10, 2021
@kolyshkin
Copy link
Contributor

@cyphar @AkihiroSuda @mrunalp PTAL

@AkihiroSuda
Copy link
Member

test (1.16.x) Expected — Waiting for status to be reported

CI seems to have got stuck, attempting to restart by closing & reopening this PR

@AkihiroSuda AkihiroSuda reopened this Mar 17, 2021
@AkihiroSuda AkihiroSuda merged commit 0ae1475 into opencontainers:master Mar 17, 2021
pothos added a commit to flatcar-archive/coreos-overlay that referenced this pull request May 19, 2021
@pothos
app-emulation/docker-runc: Backport embargoed CVE-2021-30465 for rc92
0c3f58b 
The upstream patch (itself backported for rc93) was backported to rc92.
Original cover letter:
	From 14faf1c20948688a48edb9b41367ab07ac11ca91 Mon Sep 17 00:00:00 2001
	From: Aleksa Sarai <cyphar@cyphar.com>
	Date: Wed, 28 Apr 2021 15:44:36 +1000
	Subject: [PATCH 0/5] rootfs: add mount destination validation

	This is a backport of the fix for CVE-2021-30465 to the v1.0.0-rc93
	release. However, because the patch does not apply cleanly it was
	necessary to backport the following commits (from [1]):

	 * deb8a8dd7767 ("libct/newInitConfig: nit")
	 * 1e476578b6cd ("libct/rootfs: introduce and use mountConfig")
	 * 3826db196d59 ("libct/rootfs/mountCgroupV2: minor refactor")
	 * ff692f289b60 ("Fix cgroup2 mount for rootless case")

	And the patch itself was modified to remove hardenings for code which
	didn't exist in v1.0.0-rc93 (in particular, the mount changes in [2]).

	[1]: opencontainers/runc#2818
	[2]: opencontainers/runc#2798

	Aleksa Sarai (1):
	  rootfs: add mount destination validation

	Kir Kolyshkin (4):
	  libct/newInitConfig: nit
	  libct/rootfs: introduce and use mountConfig
	  libct/rootfs/mountCgroupV2: minor refactor
	  Fix cgroup2 mount for rootless case

	 libcontainer/container_linux.go  |  11 +-
	 libcontainer/init_linux.go       |   1 +
	 libcontainer/rootfs_linux.go     | 291 +++++++++++++++++--------------
	 libcontainer/specconv/example.go |  18 +-
	 libcontainer/utils/utils.go      |  54 ++++++
	 libcontainer/utils/utils_test.go |  35 ++++
	 6 files changed, 263 insertions(+), 147 deletions(-)

	--
	2.31.1
pothos added a commit to flatcar-archive/coreos-overlay that referenced this pull request May 19, 2021
@pothos
app-emulation/docker-runc: Backport embargoed CVE-2021-30465 for rc92
6e0acb8 
The upstream patch (itself backported for rc93) was backported to rc92.
Original cover letter:
	From 14faf1c20948688a48edb9b41367ab07ac11ca91 Mon Sep 17 00:00:00 2001
	From: Aleksa Sarai <cyphar@cyphar.com>
	Date: Wed, 28 Apr 2021 15:44:36 +1000
	Subject: [PATCH 0/5] rootfs: add mount destination validation

	This is a backport of the fix for CVE-2021-30465 to the v1.0.0-rc93
	release. However, because the patch does not apply cleanly it was
	necessary to backport the following commits (from [1]):

	 * deb8a8dd7767 ("libct/newInitConfig: nit")
	 * 1e476578b6cd ("libct/rootfs: introduce and use mountConfig")
	 * 3826db196d59 ("libct/rootfs/mountCgroupV2: minor refactor")
	 * ff692f289b60 ("Fix cgroup2 mount for rootless case")

	And the patch itself was modified to remove hardenings for code which
	didn't exist in v1.0.0-rc93 (in particular, the mount changes in [2]).

	[1]: opencontainers/runc#2818
	[2]: opencontainers/runc#2798

	Aleksa Sarai (1):
	  rootfs: add mount destination validation

	Kir Kolyshkin (4):
	  libct/newInitConfig: nit
	  libct/rootfs: introduce and use mountConfig
	  libct/rootfs/mountCgroupV2: minor refactor
	  Fix cgroup2 mount for rootless case

	 libcontainer/container_linux.go  |  11 +-
	 libcontainer/init_linux.go       |   1 +
	 libcontainer/rootfs_linux.go     | 291 +++++++++++++++++--------------
	 libcontainer/specconv/example.go |  18 +-
	 libcontainer/utils/utils.go      |  54 ++++++
	 libcontainer/utils/utils_test.go |  35 ++++
	 6 files changed, 263 insertions(+), 147 deletions(-)

	--
	2.31.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kolyshkin kolyshkin kolyshkin approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@cyphar cyphar Awaiting requested review from cyphar

Assignees
No one assigned
Labels
area/checkpoint-restore
Projects
None yet
Milestone
1.0.0-rc94
Development

Successfully merging this pull request may close these issues.

container restore layering conundrum
4 participants
@adrianreber @kolyshkin @AkihiroSuda @cyphar