Fix failure with rw bind mount of a ro fuse by kolyshkin · Pull Request #3283 · opencontainers/runc

kolyshkin · 2021-11-17T02:31:30Z

As reported in containers/podman#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.

1.0 backport: #3292

AkihiroSuda · 2021-11-18T05:25:32Z

tests/integration/mounts.bats

 	[ "$status" -eq 0 ]
 }
+
+@test "runc run [rw bind mount of a ro fuse sshfs mount]" {


Can we move this into mounts_sshfs.bats, as this has huge extra depenedency?

Or maybe just skip the sshfs test by default and require some env var to opt-in to the sshfs test.

Done. If sshfs is not installed or not working for some reason, the test is skipped.

AkihiroSuda · 2021-11-18T05:27:03Z

libcontainer/rootfs_linux.go

-		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 {


Can we rather check errors.Is(err, syscall.EPERM)?

A different kernel might return a different error, and if the original mount is not read-only, we still return that error, and if it is, a retry with added RO flag will most probably return the very same error.

In other words, the error path overhead of a statfs() and (maybe) a mount() is pretty small, and the error returned is most probably the same, so it makes sense to retry on any error.

In short, it doesn't hurt to retry even if there was an error other than EPERM.

As reported in [1], 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. 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, and skip the test if the setup is not working. [1] containers/podman#12205 Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2021-11-19T20:13:58Z

@thaJeztah PTAL

kolyshkin · 2021-11-22T17:14:30Z

This fixes a real bug and we need to backport it to 1.0.3. Can this be merged? PTAL @opencontainers/runc-maintainers

thaJeztah · 2021-11-22T17:26:05Z

Ahm, sorry, I started looking, but got pulled away

thaJeztah

LGTM

mrunalp · 2021-11-22T17:40:55Z

libcontainer/rootfs_linux.go

+		if err == nil {
+			return nil
+		}
+		// Check if the source has ro flag...


Meta question: Can this be solved by requiring the entity configuring the mounts to set the necessary flags?

Yes, but it's not clear who should handle this (e.g. podman thinks it shouldn't, see containers/podman#12205) , and handling it in runc is kind of trivial. Besides, crun already does this, and this makes it hard to convince upper layers should do this.

kolyshkin force-pushed the rootless-ro-bind-rw branch 7 times, most recently from 08915b8 to dd466b3 Compare November 18, 2021 00:12

kolyshkin changed the title ~~WIP test~~ Fix failure with rw bind mount of a ro fuse Nov 18, 2021

kolyshkin force-pushed the rootless-ro-bind-rw branch from dd466b3 to fe6a902 Compare November 18, 2021 01:17

kolyshkin added backport/1.0-todo A PR in main branch which needs to be backported to release-1.0 kind/bug labels Nov 18, 2021

kolyshkin force-pushed the rootless-ro-bind-rw branch from fe6a902 to d7846bc Compare November 18, 2021 02:54

kolyshkin added this to the 1.1.0 milestone Nov 18, 2021

kolyshkin marked this pull request as ready for review November 18, 2021 03:35

kolyshkin requested review from AkihiroSuda and thaJeztah November 18, 2021 03:35

kolyshkin mentioned this pull request Nov 18, 2021

Podman + runc: Cannot mount read-only filesystem on host as a volume containers/podman#12205

Closed

AkihiroSuda reviewed Nov 18, 2021

View reviewed changes

kolyshkin force-pushed the rootless-ro-bind-rw branch 2 times, most recently from d5bf4e0 to 20e15a2 Compare November 18, 2021 16:38

kolyshkin force-pushed the rootless-ro-bind-rw branch from 20e15a2 to 50105de Compare November 18, 2021 21:09

kolyshkin requested review from AkihiroSuda and cyphar November 19, 2021 00:21

AkihiroSuda approved these changes Nov 19, 2021

View reviewed changes

thaJeztah approved these changes Nov 22, 2021

View reviewed changes

thaJeztah merged commit 02fe5f4 into opencontainers:master Nov 22, 2021

mrunalp reviewed Nov 22, 2021

View reviewed changes

kolyshkin mentioned this pull request Nov 22, 2021

[1.0] Fix failure with rw bind mount of a ro fuse #3292

Merged

kolyshkin added backport/1.0-done A PR in main branch which has been backported to release-1.0 and removed backport/1.0-todo A PR in main branch which needs to be backported to release-1.0 labels Nov 30, 2021

kolyshkin mentioned this pull request Dec 12, 2023

Vagrantfile.fedora: upgrade Fedora to 39 #4121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix failure with rw bind mount of a ro fuse#3283

Fix failure with rw bind mount of a ro fuse#3283
thaJeztah merged 1 commit intoopencontainers:masterfrom
kolyshkin:rootless-ro-bind-rw

kolyshkin commented Nov 17, 2021 •

edited

Loading

Uh oh!

AkihiroSuda Nov 18, 2021 •

edited

Loading

Uh oh!

kolyshkin Nov 18, 2021

Uh oh!

kolyshkin Nov 20, 2021

Uh oh!

AkihiroSuda Nov 18, 2021

Uh oh!

kolyshkin Nov 18, 2021

Uh oh!

kolyshkin Nov 18, 2021

Uh oh!

kolyshkin commented Nov 19, 2021

Uh oh!

kolyshkin commented Nov 22, 2021

Uh oh!

thaJeztah commented Nov 22, 2021

Uh oh!

thaJeztah left a comment

Uh oh!

mrunalp Nov 22, 2021

Uh oh!

kolyshkin Nov 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

kolyshkin commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

kolyshkin Nov 20, 2021

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

kolyshkin Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

kolyshkin Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Nov 19, 2021

Uh oh!

kolyshkin commented Nov 22, 2021

Uh oh!

thaJeztah commented Nov 22, 2021

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

mrunalp Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

kolyshkin Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kolyshkin commented Nov 17, 2021 •

edited

Loading

AkihiroSuda Nov 18, 2021 •

edited

Loading