Skip to content

Make argument validation of mount.UnmountRecursive compatible to mount.UnmountAll#8035

Merged
estesp merged 1 commit intocontainerd:mainfrom
ktock:fixunmount
Feb 2, 2023
Merged

Make argument validation of mount.UnmountRecursive compatible to mount.UnmountAll#8035
estesp merged 1 commit intocontainerd:mainfrom
ktock:fixunmount

Conversation

@ktock
Copy link
Copy Markdown
Member

@ktock ktock commented Jan 31, 2023

This commit fixes the cleanup failure of containers with custom location of rootfs.

Recently mount.UnmountAll invocations have been relaced by mount.UnmountRecursive. When ""(empty path) is passed to mount.UnmountAll, it simply ignores that path and returns immediately without error. However, mount.UnmountRecursive accepts such argument and try to perform unmounting on the root directory. Containers with custom rootfs location cause invocations of mount.UnmountRecursive with "".

if err2 := mount.UnmountRecursive(p.Rootfs, 0); err2 != nil {

It results in cleanup failure of containers and unmounting of mountpoints under "/" (if shim process has privilege). This commit tries to fix this issue by fixing mount.UnmountRecursive to return immediately when "" (empty path) is passed. IIUC this is the same behaviour with mount.UnmountAll.

Reproducer:
NOTE: the following can result in unmounting of mountpoints on the host so should run in the sandboxed environemnt.

ctr i pull ghcr.io/containerd/busybox:1.36
mkdir -p /tmp/rootfs
ctr i mount --rw ghcr.io/containerd/busybox:1.36 /tmp/rootfs
ctr run --rootfs /tmp/rootfs foo /bin/echo hello

Then it hangs after "hello" is printed and mountpoints under "/" will be unmounted.

# ctr version
Client:
  Version:  v1.7.0-beta.3-29-ge307f8797
  Revision: e307f87971391efb47eef4487e93be4cfd36fe85
  Go version: go1.19.2

Server:
  Version:  v1.7.0-beta.3-29-ge307f8797
  Revision: e307f87971391efb47eef4487e93be4cfd36fe85
  UUID: e324e475-8abf-495e-bfb4-0197add33abc

Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM on green

Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 60363db into containerd:main Feb 2, 2023
@ktock ktock deleted the fixunmount branch February 2, 2023 23:49
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.

3 participants