Repository: opencontainers/runc
Pull request #3283 (2021-11-17 02:31:30)

Fix failure with rw bind mount of a ro fuse
As reported in https://github.com/containers/podman/issues/12205, in a case where read-only fuse (sshfs)
mount is used as a volume without specifying ro flag, the kernel fails
to remount it (when adding various flags such as nosuid and nodev),
returning EPERM.

Here's the relevant strace line:

> [pid 333966] mount("/tmp/bats-run-PRVfWc/runc.RbNv8g/bundle/mnt", "/proc/self/fd/7", 0xc0001e9164, MS_NOSUID|MS_NODEV|MS_REMOUNT|MS_BIND|MS_REC, NULL) = -1 EPERM (Operation not permitted)

I was not able to reproduce it with other read-only mounts as the source
(tried tmpfs, read-only bind mount, and an ext2 mount), so somehow this
might be specific to fuse.

The fix is to check whether the source has RDONLY flag, and retry the
remount with this flag added.

A test case (which was kind of hard to write) is added, and it fails
without the fix:

```
not ok 80 runc run [bind mount of a read-only sshfs mount]
# (in test file tests/integration/mounts.bats, line 78)
#   `[ "$status" -eq 0 ]' failed
# runc spec --rootless (status=0):
# 
# runc run test_busybox (status=1):
# time="2021-11-17T20:58:16Z" level=warning msg="unable to get oom kill count" error="no directory specified for memory.oom_control"
# time="2021-11-17T20:58:16Z" level=error msg="runc run failed: unable to start container process: error during container init: error mounting \"/tmp/bats-run-5470/runc.OkQNok/bundle/mnt\" to rootfs at \"/mnt\": mount /tmp/bats-run-5470/runc.OkQNok/bundle/mnt:/mnt (via /proc/self/fd/7), flags: 0x5026: operation not permitted"
```

Note that rootless user need to be able to ssh to rootless@localhost
in order to sshfs to work -- amend setup scripts to make it work.


Patch:
diff --git a/.cirrus.yml b/.cirrus.yml
index bf9975a4..77ce98d2 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -99,7 +99,7 @@ task:
     # Work around dnf mirror failures by retrying a few times.
     for i in $(seq 0 2); do
       sleep $i
-      yum install -y -q gcc git iptables jq glibc-static libseccomp-devel make criu && break
+      yum install -y -q gcc git iptables jq glibc-static libseccomp-devel make criu fuse-sshfs && break
     done
     [ $? -eq 0 ] # fail if yum failed
     # install Go
@@ -113,6 +113,12 @@ task:
     cd -
     # Add a user for rootless tests
     useradd -u2000 -m -d/home/rootless -s/bin/bash rootless
+    # Allow root and rootless itself to execute `ssh rootless@localhost` in tests/rootless.sh
+    ssh-keygen -t ecdsa -N "" -f /root/rootless.key
+    mkdir -m 0700 -p /home/rootless/.ssh
+    cp /root/rootless.key /home/rootless/.ssh/id_ecdsa
+    cat /root/rootless.key.pub >> /home/rootless/.ssh/authorized_keys
+    chown -R rootless.rootless /home/rootless
     # set PATH
     echo 'export PATH=/usr/local/go/bin:/usr/local/bin:$PATH' >> /root/.bashrc
     # Setup ssh localhost for terminal emulation (script -e did not work)
diff --git a/Dockerfile b/Dockerfile
index 03475b8b..2ba1ba12 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -31,6 +31,7 @@ RUN KEYFILE=/usr/share/keyrings/criu-repo-keyring.gpg; \
         kmod \
         pkg-config \
         python3-minimal \
+        sshfs \
         sudo \
         uidmap \
     && apt-get clean \
diff --git a/Vagrantfile.fedora b/Vagrantfile.fedora
index 2c3df9ab..69d36e67 100644
--- a/Vagrantfile.fedora
+++ b/Vagrantfile.fedora
@@ -27,7 +27,7 @@ Vagrant.configure("2") do |config|
       cat << EOF | dnf -y --exclude=kernel,kernel-core,systemd,systemd-* shell && break
 config install_weak_deps false
 update
-install iptables gcc make golang-go glibc-static libseccomp-devel bats jq git-core criu
+install iptables gcc make golang-go glibc-static libseccomp-devel bats jq git-core criu fuse-sshfs
 ts run
 EOF
     done
@@ -36,9 +36,10 @@ EOF
     # Add a user for rootless tests
     useradd -u2000 -m -d/home/rootless -s/bin/bash rootless
 
-    # Allow root to execute `ssh rootless@localhost` in tests/rootless.sh
+    # Allow root and rootless itself to execute `ssh rootless@localhost` in tests/rootless.sh
     ssh-keygen -t ecdsa -N "" -f /root/rootless.key
     mkdir -m 0700 -p /home/rootless/.ssh
+    cp /root/rootless.key /home/rootless/.ssh/id_ecdsa
     cat /root/rootless.key.pub >> /home/rootless/.ssh/authorized_keys
     chown -R rootless.rootless /home/rootless
 
diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go
index 63322d01..2832b829 100644
--- a/libcontainer/rootfs_linux.go
+++ b/libcontainer/rootfs_linux.go
@@ -1065,7 +1065,22 @@ func remount(m *configs.Mount, rootfs string, mountFd *int) error {
 	}
 
 	return utils.WithProcfd(rootfs, m.Destination, func(procfd string) error {
-		return mount(source, m.Destination, procfd, m.Device, uintptr(m.Flags|unix.MS_REMOUNT), "")
+		flags := uintptr(m.Flags | unix.MS_REMOUNT)
+		err := mount(source, m.Destination, procfd, m.Device, flags, "")
+		if err == nil {
+			return nil
+		}
+		// Check if the source has ro flag...
+		var s unix.Statfs_t
+		if err := unix.Statfs(source, &s); err != nil {
+			return &os.PathError{Op: "statfs", Path: source, Err: err}
+		}
+		if s.Flags&unix.MS_RDONLY != unix.MS_RDONLY {
+			return err
+		}
+		// ... and retry the mount with ro flag set.
+		flags |= unix.MS_RDONLY
+		return mount(source, m.Destination, procfd, m.Device, flags, "")
 	})
 }
 
diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go
index 9e39a70b..3676654a 100644
--- a/libcontainer/specconv/spec_linux.go
+++ b/libcontainer/specconv/spec_linux.go
@@ -273,7 +273,7 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) {
 	}
 	spec := opts.Spec
 	if spec.Root == nil {
-		return nil, errors.New("root must be specified")
+		return nil, errors.New("Root must be specified")
 	}
 	rootfsPath := spec.Root.Path
 	if !filepath.IsAbs(rootfsPath) {
@@ -410,20 +410,20 @@ func createLibcontainerMount(cwd string, m specs.Mount) (*configs.Mount, error)
 	return mnt, nil
 }
 
-// checkPropertyName checks if systemd property name is valid. A valid name
-// should consist of latin letters only, and have least 3 of them.
-func checkPropertyName(s string) error {
+// isValidName checks if systemd property name is valid. It should consists
+// of latin letters only, and have least 3 of them.
+func isValidName(s string) bool {
 	if len(s) < 3 {
-		return errors.New("too short")
+		return false
 	}
 	// Check ASCII characters rather than Unicode runes.
 	for _, ch := range s {
 		if (ch >= 'A' && ch <= 'Z') || (ch >= 'a' && ch <= 'z') {
 			continue
 		}
-		return errors.New("contains non-alphabetic character")
+		return false
 	}
-	return nil
+	return true
 }
 
 // Some systemd properties are documented as having "Sec" suffix
@@ -465,12 +465,12 @@ func initSystemdProps(spec *specs.Spec) ([]systemdDbus.Property, error) {
 		if len(name) == len(k) { // prefix not there
 			continue
 		}
-		if err := checkPropertyName(name); err != nil {
-			return nil, fmt.Errorf("annotation %s name incorrect: %w", k, err)
+		if !isValidName(name) {
+			return nil, fmt.Errorf("Annotation %s name incorrect: %s", k, name)
 		}
 		value, err := dbus.ParseVariant(v, dbus.Signature{})
 		if err != nil {
-			return nil, fmt.Errorf("annotation %s=%s value parse error: %w", k, v, err)
+			return nil, fmt.Errorf("Annotation %s=%s value parse error: %w", k, v, err)
 		}
 		// Check for Sec suffix.
 		if trimName := strings.TrimSuffix(name, "Sec"); len(trimName) < len(name) {
@@ -480,7 +480,7 @@ func initSystemdProps(spec *specs.Spec) ([]systemdDbus.Property, error) {
 				name = trimName + "USec"
 				value, err = convertSecToUSec(value)
 				if err != nil {
-					return nil, fmt.Errorf("annotation %s=%s value parse error: %w", k, v, err)
+					return nil, fmt.Errorf("Annotation %s=%s value parse error: %w", k, v, err)
 				}
 			}
 		}


Test patch:
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index c5c29356..72f6eb37 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -46,13 +46,13 @@ jobs:
         curl -fSsl $REPO/Release.key | sudo apt-key add -
         echo "deb $REPO/ /" | sudo tee /etc/apt/sources.list.d/criu.list
         sudo apt update
-        sudo apt install libseccomp-dev criu
+        sudo apt install libseccomp-dev criu sshfs
 
     - name: install deps (criu ${{ matrix.criu }})
       if: matrix.criu != ''
       run: |
         sudo apt -q update
-        sudo apt -q install libseccomp-dev \
+        sudo apt -q install libseccomp-dev sshfs \
           libcap-dev libnet1-dev libnl-3-dev \
           libprotobuf-c-dev libprotobuf-dev protobuf-c-compiler protobuf-compiler
         git clone https://github.com/checkpoint-restore/criu.git ~/criu
@@ -81,9 +81,10 @@ jobs:
       if: matrix.rootless == 'rootless'
       run: |
         sudo useradd -u2000 -m -d/home/rootless -s/bin/bash rootless
-        # Allow root to execute `ssh rootless@localhost` in tests/rootless.sh
+        # Allow root and rootless itself to execute `ssh rootless@localhost` in tests/rootless.sh
         ssh-keygen -t ecdsa -N "" -f $HOME/rootless.key
         sudo mkdir -m 0700 -p /home/rootless/.ssh
+        sudo cp $HOME/rootless.key /home/rootless/.ssh/id_ecdsa
         sudo cp $HOME/rootless.key.pub /home/rootless/.ssh/authorized_keys
         sudo chown -R rootless.rootless /home/rootless
 
diff --git a/libcontainer/specconv/spec_linux_test.go b/libcontainer/specconv/spec_linux_test.go
index 56d80869..ffcac128 100644
--- a/libcontainer/specconv/spec_linux_test.go
+++ b/libcontainer/specconv/spec_linux_test.go
@@ -780,9 +780,9 @@ func TestIsValidName(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		err := checkPropertyName(tc.in)
-		if (err == nil) != tc.valid {
-			t.Errorf("case %q: expected valid: %v, got error: %v", tc.in, tc.valid, err)
+		valid := isValidName(tc.in)
+		if valid != tc.valid {
+			t.Errorf("case %q: expected %v, got %v", tc.in, tc.valid, valid)
 		}
 	}
 }
@@ -790,7 +790,7 @@ func TestIsValidName(t *testing.T) {
 func BenchmarkIsValidName(b *testing.B) {
 	for i := 0; i < b.N; i++ {
 		for _, s := range []string{"", "xx", "xxx", "someValidName", "A name", "Кир", "მადლობა", "合い言葉"} {
-			_ = checkPropertyName(s)
+			_ = isValidName(s)
 		}
 	}
 }
diff --git a/tests/integration/mounts_sshfs.bats b/tests/integration/mounts_sshfs.bats
new file mode 100644
index 00000000..abf82357
--- /dev/null
+++ b/tests/integration/mounts_sshfs.bats
@@ -0,0 +1,40 @@
+#!/usr/bin/env bats
+
+load helpers
+
+function setup() {
+	# Create a ro fuse-sshfs mount; skip the test if it's not working.
+	local sshfs="sshfs
+		-o UserKnownHostsFile=/dev/null
+		-o StrictHostKeyChecking=no
+		-o PasswordAuthentication=no"
+
+	DIR="$BATS_RUN_TMPDIR/fuse-sshfs"
+	mkdir -p "$DIR"
+
+	if ! $sshfs -o ro rootless@localhost: "$DIR"; then
+		skip "test requires working sshfs mounts"
+	fi
+
+	setup_hello
+}
+
+function teardown() {
+	# New distros (Fedora 35) do not have fusermount installed
+	# as a dependency of fuse-sshfs, and good ol' umount works.
+	fusermount -u "$DIR" || umount "$DIR"
+
+	teardown_bundle
+}
+
+@test "runc run [rw bind mount of a ro fuse sshfs mount]" {
+	update_config '	  .mounts += [{
+					type: "bind",
+					source: "'"$DIR"'",
+					destination: "/mnt",
+					options: ["rw", "rprivate", "nosuid", "nodev", "rbind"]
+				}]'
+
+	runc run test_busybox
+	[ "$status" -eq 0 ]
+}