runtime: Don' call bindUnmountContainerRootfs for devicemapper device

buildContainerRootfs don't call bindMountContainerRootfs if container's rootfs use devicemapper device in https://github.com/kata-containers/runtime/blob/master/virtcontainers/kata_agent.go#L1300, so bindUnmountContainerRootfs should not be called if container's rootfs use devicemapper device in https://github.com/kata-containers/runtime/blob/master/virtcontainers/container.go#L1123 Fixes: #2914 Signed-off-by: Shukui Yang <keloyangsk@gmail.com>
kata-containers · Sep 9, 2020 · 3f9f4b8 · 3f9f4b8
1 parent a1d993f
commit 3f9f4b8
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 10 deletions.
diff --git a/virtcontainers/container.go b/virtcontainers/container.go
@@ -850,7 +850,7 @@ func (c *Container) rollbackFailingContainerCreation() {
 	if err := c.unmountHostMounts(); err != nil {
 		c.Logger().WithError(err).Error("rollback failed unmountHostMounts()")
 	}
-	if err := bindUnmountContainerRootfs(c.ctx, getMountPath(c.sandbox.id), c.id); err != nil {
+	if err := bindUnmountContainerRootfs(c.ctx, getMountPath(c.sandbox.id), c); err != nil {
 		c.Logger().WithError(err).Error("rollback failed bindUnmountContainerRootfs()")
 	}
 }
@@ -1120,7 +1120,7 @@ func (c *Container) stop(force bool) error {
 		return err
 	}
 
-	if err := bindUnmountContainerRootfs(c.ctx, getMountPath(c.sandbox.id), c.id); err != nil && !force {
+	if err := bindUnmountContainerRootfs(c.ctx, getMountPath(c.sandbox.id), c); err != nil && !force {
 		return err
 	}
 

diff --git a/virtcontainers/kata_agent.go b/virtcontainers/kata_agent.go
@@ -1290,7 +1290,7 @@ func (k *kataAgent) rollbackFailingContainerCreation(c *Container) {
 			k.Logger().WithError(err2).Error("rollback failed unmountHostMounts()")
 		}
 
-		if err2 := bindUnmountContainerRootfs(k.ctx, getMountPath(c.sandbox.id), c.id); err2 != nil {
+		if err2 := bindUnmountContainerRootfs(k.ctx, getMountPath(c.sandbox.id), c); err2 != nil {
 			k.Logger().WithError(err2).Error("rollback failed bindUnmountContainerRootfs()")
 		}
 	}

diff --git a/virtcontainers/mount.go b/virtcontainers/mount.go
@@ -315,13 +315,17 @@ func isSymlink(path string) bool {
 	return stat.Mode()&os.ModeSymlink != 0
 }
 
-func bindUnmountContainerRootfs(ctx context.Context, sharedDir, cID string) error {
+func bindUnmountContainerRootfs(ctx context.Context, sharedDir string, con *Container) error {
 	span, _ := trace(ctx, "bindUnmountContainerRootfs")
 	defer span.Finish()
 
-	rootfsDest := filepath.Join(sharedDir, cID, rootfsDir)
-	if isSymlink(filepath.Join(sharedDir, cID)) || isSymlink(rootfsDest) {
-		logrus.Warnf("container dir %s is a symlink, malicious guest?", cID)
+	if con.state.Fstype != "" && con.state.BlockDeviceID != "" {
+		return nil
+	}
+
+	rootfsDest := filepath.Join(sharedDir, con.id, rootfsDir)
+	if isSymlink(filepath.Join(sharedDir, con.id)) || isSymlink(rootfsDest) {
+		logrus.Warnf("container dir %s is a symlink, malicious guest?", con.id)
 		return nil
 	}
 
@@ -351,7 +355,7 @@ func bindUnmountAllRootfs(ctx context.Context, sharedDir string, sandbox *Sandbo
 		if c.state.Fstype == "" {
 			// even if error found, don't break out of loop until all mounts attempted
 			// to be unmounted, and collect all errors
-			errors = merr.Append(errors, bindUnmountContainerRootfs(c.ctx, sharedDir, c.id))
+			errors = merr.Append(errors, bindUnmountContainerRootfs(c.ctx, sharedDir, c))
 		}
 	}
 	return errors.ErrorOrNil()

diff --git a/virtcontainers/mount_test.go b/virtcontainers/mount_test.go
@@ -19,6 +19,7 @@ import (
 	"testing"
 
 	ktu "github.com/kata-containers/runtime/pkg/katatestutils"
+	"github.com/kata-containers/runtime/virtcontainers/types"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -378,6 +379,10 @@ func TestBindUnmountContainerRootfsENOENTNotError(t *testing.T) {
 	testMnt := "/tmp/test_mount"
 	sID := "sandIDTest"
 	cID := "contIDTest"
+	container := &Container{
+		id: cID,
+	}
+
 	assert := assert.New(t)
 
 	// check to make sure the file doesn't exist
@@ -386,7 +391,7 @@ func TestBindUnmountContainerRootfsENOENTNotError(t *testing.T) {
 		assert.NoError(os.Remove(testPath))
 	}
 
-	err := bindUnmountContainerRootfs(context.Background(), filepath.Join(testMnt, sID), cID)
+	err := bindUnmountContainerRootfs(context.Background(), filepath.Join(testMnt, sID), container)
 	assert.NoError(err)
 }
 
@@ -398,6 +403,9 @@ func TestBindUnmountContainerRootfsRemoveRootfsDest(t *testing.T) {
 
 	sID := "sandIDTestRemoveRootfsDest"
 	cID := "contIDTestRemoveRootfsDest"
+	container := &Container{
+		id: cID,
+	}
 
 	testPath := filepath.Join(testDir, sID, cID, rootfsDir)
 	syscall.Unmount(testPath, 0)
@@ -407,11 +415,55 @@ func TestBindUnmountContainerRootfsRemoveRootfsDest(t *testing.T) {
 	assert.NoError(err)
 	defer os.RemoveAll(filepath.Join(testDir, sID))
 
-	bindUnmountContainerRootfs(context.Background(), filepath.Join(testDir, sID), cID)
+	bindUnmountContainerRootfs(context.Background(), filepath.Join(testDir, sID), container)
 
 	if _, err := os.Stat(testPath); err == nil {
 		t.Fatal("empty rootfs dest should be removed")
 	} else if !os.IsNotExist(err) {
 		t.Fatal(err)
 	}
 }
+
+func TestBindUnmountContainerRootfsDevicemapper(t *testing.T) {
+	assert := assert.New(t)
+	if tc.NotValid(ktu.NeedRoot()) {
+		t.Skip(ktu.TestDisabledNeedRoot)
+	}
+
+	sID := "sandIDDevicemapper"
+
+	cID1 := "contIDDevicemapper1"
+	container := &Container{
+		id: cID1,
+		state: types.ContainerState{
+			Fstype:        "xfs",
+			BlockDeviceID: "4b073a87f3c9242a",
+		},
+	}
+
+	testPath1 := filepath.Join(testDir, sID, cID1, rootfsDir)
+	err := os.MkdirAll(testPath1, mountPerm)
+	assert.NoError(err)
+
+	bindUnmountContainerRootfs(context.Background(), filepath.Join(testDir, sID), container)
+
+	// expect testPath1 exist, if not exist, it means this case failed.
+	if _, err := os.Stat(testPath1); err != nil && !os.IsExist(err) {
+		t.Fatal("testPath should not be removed")
+	}
+
+	cID2 := "contIDDevicemapper2"
+	container.state.Fstype = ""
+	container.id = cID2
+	testPath2 := filepath.Join(testDir, sID, cID2, rootfsDir)
+	err = os.MkdirAll(testPath2, mountPerm)
+	assert.NoError(err)
+
+	defer os.RemoveAll(filepath.Join(testDir, sID))
+
+	// expect testPath2 not exist, if exist, it means this case failed.
+	bindUnmountContainerRootfs(context.Background(), filepath.Join(testDir, sID), container)
+	if _, err := os.Stat(testPath2); err == nil || os.IsExist(err) {
+		t.Fatal("testPath should be removed")
+	}
+}