-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix failure with rw bind mount of a ro fuse #3283
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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... | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Meta question: Can this be solved by requiring the entity configuring the mounts to set the necessary flags?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| 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, "") | ||
| }) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 ] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rather check
errors.Is(err, syscall.EPERM)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In short, it doesn't hurt to retry even if there was an error other than EPERM.