From 125e21cea3ea5e9b38368ffc8fcce408f63b0469 Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Fri, 30 Oct 2020 14:54:49 +0800 Subject: [PATCH 1/3] runtime: readonly mounts should be readonly bindmount on the host So that we get protected at the VM boundary not just the guest kernel. Signed-off-by: Peng Tao --- src/runtime/virtcontainers/container.go | 14 ++------------ src/runtime/virtcontainers/pkg/oci/utils.go | 8 ++++++++ 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/runtime/virtcontainers/container.go b/src/runtime/virtcontainers/container.go index 21f18f35d7..5d317119ff 100644 --- a/src/runtime/virtcontainers/container.go +++ b/src/runtime/virtcontainers/container.go @@ -470,7 +470,7 @@ func (c *Container) shareFiles(m Mount, idx int, hostSharedDir, guestSharedDir s } else { // These mounts are created in the shared dir mountDest := filepath.Join(hostSharedDir, filename) - if err := bindMount(c.ctx, m.Source, mountDest, false, "private"); err != nil { + if err := bindMount(c.ctx, m.Source, mountDest, m.ReadOnly, "private"); err != nil { return "", false, err } // Save HostPath mount value into the mount list of the container. @@ -546,22 +546,12 @@ func (c *Container) mountSharedDirMounts(hostSharedDir, guestSharedDir string) ( continue } - // Check if mount is readonly, let the agent handle the readonly mount - // within the VM. - readonly := false - for _, flag := range m.Options { - if flag == "ro" { - readonly = true - break - } - } - sharedDirMount := Mount{ Source: guestDest, Destination: m.Destination, Type: m.Type, Options: m.Options, - ReadOnly: readonly, + ReadOnly: m.ReadOnly, } sharedDirMounts[sharedDirMount.Destination] = sharedDirMount diff --git a/src/runtime/virtcontainers/pkg/oci/utils.go b/src/runtime/virtcontainers/pkg/oci/utils.go index 755e3c6b45..26c44992e3 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils.go +++ b/src/runtime/virtcontainers/pkg/oci/utils.go @@ -160,11 +160,19 @@ func cmdEnvs(spec specs.Spec, envs []types.EnvVar) []types.EnvVar { } func newMount(m specs.Mount) vc.Mount { + readonly := false + for _, flag := range m.Options { + if flag == "ro" { + readonly = true + break + } + } return vc.Mount{ Source: m.Source, Destination: m.Destination, Type: m.Type, Options: m.Options, + ReadOnly: readonly, } } From a958eaa8d3211ae465c193923de61d3162dc37f0 Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Fri, 30 Oct 2020 17:40:12 +0800 Subject: [PATCH 2/3] runtime: mount shared mountpoint readonly bindmount remount events are not propagated through mount subtrees, so we have to remount the shared dir mountpoint directly. E.g., ``` mkdir -p source dest foo source/foo mount -o bind --make-shared source dest mount -o bind foo source/foo echo bind mount rw mount | grep foo echo remount ro mount -o remount,bind,ro source/foo mount | grep foo ``` would result in: ``` bind mount rw /dev/xvda1 on /home/ubuntu/source/foo type ext4 (rw,relatime,discard,data=ordered) /dev/xvda1 on /home/ubuntu/dest/foo type ext4 (rw,relatime,discard,data=ordered) remount ro /dev/xvda1 on /home/ubuntu/source/foo type ext4 (ro,relatime,discard,data=ordered) /dev/xvda1 on /home/ubuntu/dest/foo type ext4 (rw,relatime,discard,data=ordered) ``` The reason is that bind mount creats new mount structs and attaches them to different mount subtrees. However, MS_REMOUNT only looks for existing mount structs to modify and does not try to propagate the change to mount structs in other subtrees. Fixes: #1061 Signed-off-by: Peng Tao --- src/runtime/virtcontainers/container.go | 15 +++++++++++---- src/runtime/virtcontainers/kata_agent.go | 2 +- src/runtime/virtcontainers/mount.go | 5 +++++ src/runtime/virtcontainers/sandbox_test.go | 2 +- 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/runtime/virtcontainers/container.go b/src/runtime/virtcontainers/container.go index 5d317119ff..87e4cf45e6 100644 --- a/src/runtime/virtcontainers/container.go +++ b/src/runtime/virtcontainers/container.go @@ -435,7 +435,7 @@ func (c *Container) setContainerState(state types.StateString) error { return nil } -func (c *Container) shareFiles(m Mount, idx int, hostSharedDir, guestSharedDir string) (string, bool, error) { +func (c *Container) shareFiles(m Mount, idx int, hostSharedDir, hostMountDir, guestSharedDir string) (string, bool, error) { randBytes, err := utils.GenerateRandomBytes(8) if err != nil { return "", false, err @@ -469,12 +469,19 @@ func (c *Container) shareFiles(m Mount, idx int, hostSharedDir, guestSharedDir s } } else { // These mounts are created in the shared dir - mountDest := filepath.Join(hostSharedDir, filename) + mountDest := filepath.Join(hostMountDir, filename) if err := bindMount(c.ctx, m.Source, mountDest, m.ReadOnly, "private"); err != nil { return "", false, err } // Save HostPath mount value into the mount list of the container. c.mounts[idx].HostPath = mountDest + // bindmount remount event is not propagated to mount subtrees, so we have to remount the shared dir mountpoint directly. + if m.ReadOnly { + mountDest = filepath.Join(hostSharedDir, filename) + if err := remountRo(c.ctx, mountDest); err != nil { + return "", false, err + } + } } return guestDest, false, nil @@ -485,7 +492,7 @@ func (c *Container) shareFiles(m Mount, idx int, hostSharedDir, guestSharedDir s // It also updates the container mount list with the HostPath info, and store // container mounts to the storage. This way, we will have the HostPath info // available when we will need to unmount those mounts. -func (c *Container) mountSharedDirMounts(hostSharedDir, guestSharedDir string) (sharedDirMounts map[string]Mount, ignoredMounts map[string]Mount, err error) { +func (c *Container) mountSharedDirMounts(hostSharedDir, hostMountDir, guestSharedDir string) (sharedDirMounts map[string]Mount, ignoredMounts map[string]Mount, err error) { sharedDirMounts = make(map[string]Mount) ignoredMounts = make(map[string]Mount) var devicesToDetach []string @@ -535,7 +542,7 @@ func (c *Container) mountSharedDirMounts(hostSharedDir, guestSharedDir string) ( var ignore bool var guestDest string - guestDest, ignore, err = c.shareFiles(m, idx, hostSharedDir, guestSharedDir) + guestDest, ignore, err = c.shareFiles(m, idx, hostSharedDir, hostMountDir, guestSharedDir) if err != nil { return nil, nil, err } diff --git a/src/runtime/virtcontainers/kata_agent.go b/src/runtime/virtcontainers/kata_agent.go index b88bec4488..9f5228373f 100644 --- a/src/runtime/virtcontainers/kata_agent.go +++ b/src/runtime/virtcontainers/kata_agent.go @@ -1256,7 +1256,7 @@ func (k *kataAgent) createContainer(sandbox *Sandbox, c *Container) (p *Process, } // Handle container mounts - newMounts, ignoredMounts, err := c.mountSharedDirMounts(getMountPath(sandbox.id), kataGuestSharedDir()) + newMounts, ignoredMounts, err := c.mountSharedDirMounts(getSharePath(sandbox.id), getMountPath(sandbox.id), kataGuestSharedDir()) if err != nil { return nil, err } diff --git a/src/runtime/virtcontainers/mount.go b/src/runtime/virtcontainers/mount.go index 6adf0d40a9..604922dfe8 100644 --- a/src/runtime/virtcontainers/mount.go +++ b/src/runtime/virtcontainers/mount.go @@ -277,6 +277,11 @@ func remount(ctx context.Context, mountflags uintptr, src string) error { return nil } +// remount a mount point as readonly +func remountRo(ctx context.Context, src string) error { + return remount(ctx, syscall.MS_BIND|syscall.MS_RDONLY, src) +} + // bindMountContainerRootfs bind mounts a container rootfs into a 9pfs shared // directory between the guest and the host. func bindMountContainerRootfs(ctx context.Context, shareDir, cid, cRootFs string, readonly bool) error { diff --git a/src/runtime/virtcontainers/sandbox_test.go b/src/runtime/virtcontainers/sandbox_test.go index 6e6dd752c1..9a9d0c1e4a 100644 --- a/src/runtime/virtcontainers/sandbox_test.go +++ b/src/runtime/virtcontainers/sandbox_test.go @@ -1175,7 +1175,7 @@ func TestPreAddDevice(t *testing.T) { }, } - mounts, ignoreMounts, err := container.mountSharedDirMounts("", "") + mounts, ignoreMounts, err := container.mountSharedDirMounts("", "", "") assert.Nil(t, err) assert.Equal(t, len(mounts), 0, "mounts should contain nothing because it only contains a block device") From ff13bde3c1bd03b5ed0c93e7ea315befd24a75b7 Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Wed, 4 Nov 2020 16:59:53 +0800 Subject: [PATCH 3/3] version: revert back to crio 1.8.3 This reverts commit 87848e874e10b692c0efdf0709158fb1f9df72a3 as it is breaking the k8s configMap test. Fixex: #1080 Signed-off-by: Peng Tao --- versions.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/versions.yaml b/versions.yaml index 1ba7d2d85e..8a469c16d9 100644 --- a/versions.yaml +++ b/versions.yaml @@ -179,7 +179,7 @@ externals: description: | OCI-based Kubernetes Container Runtime Interface implementation url: "https://github.com/cri-o/cri-o" - version: "v1.18.4" + version: "v1.18.3" meta: openshift: "6273bea4c9ed788aeb3d051ebf2d030060c05b6c" crictl: 1.0.0-beta.2