From 3f9f4b80149211eddd7f5c3b32bfee99da545007 Mon Sep 17 00:00:00 2001 From: Shukui Yang Date: Wed, 9 Sep 2020 18:34:11 +0800 Subject: [PATCH] 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 --- virtcontainers/container.go | 4 +-- virtcontainers/kata_agent.go | 2 +- virtcontainers/mount.go | 14 +++++---- virtcontainers/mount_test.go | 56 ++++++++++++++++++++++++++++++++++-- 4 files changed, 66 insertions(+), 10 deletions(-) diff --git a/virtcontainers/container.go b/virtcontainers/container.go index 2f5279c866..58f5dfb6e2 100644 --- 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 index 3ba4e3541e..5767e66bea 100644 --- 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 index f7822a2968..ab9e9f8d03 100644 --- 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 index 20b0157a40..4904585d19 100644 --- 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,7 +415,7 @@ 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") @@ -415,3 +423,47 @@ func TestBindUnmountContainerRootfsRemoveRootfsDest(t *testing.T) { 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") + } +}