Support recursively read-only (RRO) mounts #45278

AkihiroSuda · 2023-04-05T14:19:23Z

Fixes Support recursively read-only (RRO) bind mounts (kernel >= 5.12) #44978
Fixes Subfolders of read-only volume are writeable docker/for-linux#788
Fixes Subdirectories of read-only bind mount are writeable (when they are rw mountpoints on the host system) #23499

- What I did

Added the support for recursively read-only (RRO) mounts.

docker run -v /foo:/foo:ro is now recursively read-only on kernel >= 5.12.
Automatically falls back to the legacy non-recursively read-only mount mode on kernel < 5.12.

Use ro-non-recursive to disable RRO.
Use ro-force-recursive or rro to explicitly enable RRO. (Fails on kernel < 5.12)
(EDIT: the CLI options are being changed in #46037 and docker/cli#4316 )

It is highly recommended to use RRO in conjunction with rprivate propagation.

- How I did it

By using OCI mount option "rro", which is implemented by calling mount_setattr(2) with AT_RECURSIVE and MOUNT_ATTR_RDONLY.

- How to verify it

Run docker run -v /mnt:/mnt:ro,rprivate on kernel >= 5.12, and make sure /mnt/recursive-mount-dir is read-only.

- Description for the changelog

Support recursively read-only (RRO) mounts

- A picture of a cute animal (not mandatory but encouraged)
🐧

thaJeztah · 2023-04-05T14:27:46Z

daemon/containerfs_linux.go

 					return err
 				}
+				if m.RecursivelyReadOnly {
+					if err := makeMountRRO(dest); err != nil {


Is this something that should be provided by github.com/moby/sys/mount ? (either through a mount.MakeRecursiveReadOnly() or mount.Mount() accepting this as option?

Not necessary IMHO, as the function just wraps a single MountSetattr call with the EINTR loop.

thaJeztah · 2023-04-05T14:34:16Z

api/swagger.yaml

+            enum:
+              - "if-possible"
+              - "required"


Hm.. bit on the fence here how granular we want to be here (assuming users either know what their system supports, or to get an error that it's not supported).

We could still decide to fall back (and warn) on unsupported platforms, or be explicit and error. (pros/cons to both, I can imagine "error" could mean "works on my machine") OTOH; if you're targeting a specific environment, then perhaps that's the way to go.

FYI: this tri-state granularity was proposed in kubernetes/enhancements#3858 (comment)

thaJeztah · 2023-04-05T14:35:52Z

volume/mounts/linux_parser_test.go

+		{"/tmp:/tmp5:rro", "", mount.TypeBind, "/tmp5", "/tmp", "", "", false, mount.RecursivelyReadOnlyRequired, false},
+		{"/tmp:/tmp6:rro-if-possible", "", mount.TypeBind, "/tmp6", "/tmp", "", "", false, mount.RecursivelyReadOnlyIfPossible, false},
+		{"/tmp:/tmp7:rro-if-possible,rprivate", "", mount.TypeBind, "/tmp7", "/tmp", "", "", false, mount.RecursivelyReadOnlyIfPossible, false},


I wish we could fully freeze (if we didn't not already) the shorthand syntax. To some extend, I can see rro being "reasonable", but (also see my other comment) a bit on the fence on also having rro-if-possible

The --mount CSV form is too long for CLI users, and a huge portion of users still have to stick to older kernels, so I'd prefer to retain rro-if-possible as the "best effort" mode, but I can move it to a separate follow-up PR if you prefer.

thaJeztah · 2023-04-05T14:42:23Z

api/server/router/container/container_routes.go

+		for _, m := range hostConfig.Mounts {
+			if bo := m.BindOptions; bo != nil {
+				bo.RecursivelyReadOnly = ""
+			}
+		}


This is a tricky one; is this hit both when using the shorthand (-v <foo>:<bar>:<options>) and long-form (--mount type=....,opt=...,opt=....) formats?

Because at a glance this may actually result in the "reverse" of the current behaviour. Current APIs will return an error for this case;

docker run -it --rm -v (pwd):/docker:rro alpine docker: Error response from daemon: invalid mode: rro. See 'docker run --help'.

I guess this is different than other options, because "options" itself are supported, but not this specific one.

I also think we should either

fully error (unsupported)

or fallback to ro

Because we should not silently ignore to not mounting read-only if the user expects it to be read-only.

Updated to return an error

cpuguy83 · 2023-04-06T20:09:20Z

We discussed this on the maintainers call today (@thaJeztah @tianon @corhere @neersighted)
There was much debate on how to approach this.

I believe we landed on this (feel free to correct me if I got it wrong):

Change ro to opportunistically try to do a recursive read-only mount (this would be the if-possible variant describe in the PR body). We expect that most, if not all, people are surprised by the current behavior.
We could add warnings when recursive is not possible on docker info and/or on the container create/start endpoints (if ro was requested), but this may be needlessly noisy for cases that don't actually have nested mounts.
Have rro require recursive and error out if not possible.
We may need a daemon option to disable the opportunistic upgrade for ro.

This means users will automatically get the enhanced security where they can and those same specs will continue to work on older daemons or machines that don't support it.

AkihiroSuda · 2023-04-07T00:48:48Z

Change ro to opportunistically try to do a recursive read-only mount (this would be the if-possible variant describe in the PR body). We expect that most, if not all, people are surprised by the current behavior.

We can't do this. Kubernetes needs ro to retain the historical behavior: kubernetes/enhancements#3858 (comment)

AkihiroSuda · 2023-04-07T00:50:50Z

Other usecases of the historical ro:

Mount /src as RO, while leaving /src/_output writable
Mount /lecture_materials as RO, while leaving /lecture_materials/student_assignment_submission writable

thaJeztah · 2023-04-07T00:53:31Z

Wouldn't that case still be supported if the nested mounts are defined as separate mounts?

AkihiroSuda · 2023-04-07T00:55:23Z

Wouldn't that case still be supported if the nested mounts are defined as separate mounts?

Yes, but it's a breaking change.

thaJeztah · 2023-04-07T00:55:41Z

separate mount as in;

- v /src:ro -v /src/_output:rw

thaJeztah · 2023-04-07T00:59:22Z

I expect 99% of those situations to be unintentional (and unexpected; the user asked for the mount to be read only, but another mount ended up in there, and now it's not).

We could consider an advanced (for the --mount extended format) option that explicitly opts-out of recursive.

AkihiroSuda · 2023-04-07T01:34:09Z

I expect 99% of those situations to be unintentional

Yes, but Kubernetes will retain the classic behavior to follow their compatibility policy.

Can we just deprecate ro, and recommend people to switch to rro?

A blocker is that AT_RECURSIVE needs kernel >= 5.12, but I think we can fallback to compose classic read-only mounts to emulate AT_RECURSIVE on an older kernel.
(cc @kolyshkin @cyphar ; Is this safe?)

corhere · 2023-04-07T03:53:59Z

Opportunistic upgrade could be disabled per bind rather than as a daemon option. Then cri-dockerd would be able to map the Kubernetes flags to their semantic equivalents in the engine API.

K8s	Engine
`ro`	`ro,ro-nonrecursive`
`rro`	`rro`
`rro-if-possible`	`ro`

tianon · 2023-04-18T22:42:03Z

When we have ro or readonly in our CSV-style usage, it's technically shorthand for readonly=true -- could we abuse that in a similar way to what util-linux ended up doing to implement something like readonly=nonrecursive? Does our CSV-style flag parsing support that already? (true/false flags that don't require a value but can have an optional string value?)

One thing we discussed about emulating recursive read-only is that mount propagation makes it complex to do safely (for example, if you have a shared mount it'd have to be converted to private and then monitored and sync'd unless we do something clever with fuse which has its own drawbacks).

AkihiroSuda · 2023-05-10T12:52:56Z

Updated PR.
RO mounts are now RRO by default on kernel >= 5.12.

AkihiroSuda · 2023-05-16T10:22:44Z

rebased

`docker run -v /foo:/foo:ro` is now recursively read-only on kernel >= 5.12. Automatically falls back to the legacy non-recursively read-only mount mode on kernel < 5.12. Use `ro-non-recursive` to disable RRO. Use `ro-force-recursive` or `rro` to explicitly enable RRO. (Fails on kernel < 5.12) Fix issue 44978 Fix docker/for-linux issue 788 Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

thaJeztah

"LGTM" - discussed with @corhere and others during the maintainers call, and all concerns should be addressed, so let's get this in.

thaJeztah · 2023-05-25T19:01:55Z

Looks like github sees a conflict, but doesn't show "what" 🤔

If someone can do a quick rebase, then we can get this one in ❤️

AkihiroSuda · 2023-05-25T23:24:32Z

Rebased

thaJeztah · 2023-05-26T00:24:37Z

Thanks!

AkihiroSuda · 2023-05-29T10:05:35Z

SwarmKit PR:

api: sync BindOptions with moby/moby@8d67d0c1a8d2 (v25.0.0-dev) swarmkit#3134

CLI (and docs) PR:

mount: add bind-recursive=<bool|string> and deprecate bind-nonrecursive=<bool> docker/cli#4316

thaJeztah · 2023-07-27T12:12:55Z

volume/mounts/parser.go

+	"rw":                 true,
+	"ro":                 true, // attempts recursive read-only if possible
+	"ro-non-recursive":   true, // makes the mount non-recursively read-only, but still leaves the mount recursive
+	"ro-force-recursive": true, // raises an error if the mount cannot be made recursively read-only
+	"rro":                true, // alias for ro-force-recursive


For future reference; we reverted the new options for the short-hand form, and only keep the advanced options for the advanced (--mount) syntax; see volume: remove the short RRO forms in favor of the long forms #46037

- COS 109 LTS is deprecated and no longer available in certain regions: https://docs.cloud.google.com/container-optimized-os/docs/release-notes#Archived - Due to a change in the upstream docker server (moby/moby#45278) the way of mounting the pre-start hooks into the container needed to be changed. Warning: server and uploader GCE instances will be recreated during a `terraform apply`, but the primed uploader datadisk will be reattached afterwards. PiperOrigin-RevId: 833661682

This was referenced Apr 5, 2023

Support recursively read-only (RRO) bind mounts (kernel >= 5.12) #44978

Closed

Subfolders of read-only volume are writeable docker/for-linux#788

Closed

thaJeztah added area/api API status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. impact/api impact/changelog area/volumes Volumes labels Apr 5, 2023

thaJeztah added this to the 24.0.0 milestone Apr 5, 2023

thaJeztah reviewed Apr 5, 2023

View reviewed changes

thaJeztah added status/1-design-review and removed status/2-code-review labels Apr 5, 2023

thaJeztah reviewed Apr 5, 2023

View reviewed changes

AkihiroSuda force-pushed the rro branch 2 times, most recently from 772e46f to 4733ae1 Compare April 6, 2023 04:56

rumpl modified the milestones: 24.0.0, v-future Apr 24, 2023

AkihiroSuda force-pushed the rro branch from 4733ae1 to 5df682d Compare May 10, 2023 12:50

AkihiroSuda force-pushed the rro branch from 5df682d to b44f438 Compare May 10, 2023 12:56

AkihiroSuda force-pushed the rro branch from f2a0c61 to 93f95c1 Compare May 16, 2023 10:23

neersighted approved these changes May 17, 2023

View reviewed changes

AkihiroSuda requested a review from thaJeztah May 24, 2023 07:09

thaJeztah approved these changes May 25, 2023

View reviewed changes

AkihiroSuda force-pushed the rro branch from 93f95c1 to 5045a2d Compare May 25, 2023 23:23

AkihiroSuda added status/4-merge and removed status/1-design-review labels May 25, 2023

thaJeztah merged commit 0db4174 into moby:master May 26, 2023

AkihiroSuda mentioned this pull request May 29, 2023

api: sync BindOptions with moby/moby@8d67d0c1a8d2 (v25.0.0-dev) moby/swarmkit#3134

Merged

AkihiroSuda mentioned this pull request May 29, 2023

mount: add bind-recursive=<bool|string> and deprecate bind-nonrecursive=<bool> docker/cli#4316

Merged

AkihiroSuda mentioned this pull request Jul 20, 2023

volume: remove the short RRO forms in favor of the long forms #46037

Merged

thaJeztah reviewed Jul 27, 2023

View reviewed changes

thaJeztah mentioned this pull request Aug 28, 2023

Dockerfile: update crun binary to v1.8.7 #46349

Merged

AkihiroSuda mentioned this pull request Oct 3, 2023

docker info: expose runtime features ("rro" mount mode , etc.) #46580

Closed

yankay mentioned this pull request Nov 16, 2023

docs: read-only (RRO) bind-mount have been support by docker containerd/nerdctl#2643

Merged

neersighted mentioned this pull request Nov 16, 2023

[24.0 Backport] docs: add bind-recursive mount option docker/cli#4662

Closed

vvoland mentioned this pull request Jan 22, 2024

Mounting read-only /dev behaviour changed in v25 #47158

Open

This was referenced Jan 30, 2024

[Reverted in #9747] cri: make read-only mounts recursively read-only containerd/containerd#9713

Merged

libdocker.GenerateMountBindings(): Disable RRO mounts when bumping up the Docker API client package to v25 (API v1.44) Mirantis/cri-dockerd#309

Closed

AkihiroSuda mentioned this pull request Feb 7, 2024

Disable upgrading RO mounts to RRO when a client speaks API older than v1.44 (Docker v25) #47355

Closed

thaJeztah mentioned this pull request Feb 15, 2024

Trouble running systemd in CentOS 7 (4.27.x) docker/for-mac#7190

Closed

sairon mentioned this pull request Apr 4, 2024

Add equivalent of bind-recursive option to the Mount type class docker/docker-py#3242

Open

AkihiroSuda mentioned this pull request Feb 6, 2025

Update the list of the unimplemented Docker features (and implement them) containerd/nerdctl#3867

Open

7 tasks

Support recursively read-only (RRO) mounts #45278

Support recursively read-only (RRO) mounts #45278

Uh oh!

Conversation

AkihiroSuda commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Apr 6, 2023

Uh oh!

AkihiroSuda commented Apr 7, 2023

Uh oh!

AkihiroSuda commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Apr 7, 2023

Uh oh!

AkihiroSuda commented Apr 7, 2023

Uh oh!

thaJeztah commented Apr 7, 2023

Uh oh!

thaJeztah commented Apr 7, 2023

Uh oh!

AkihiroSuda commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corhere commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tianon commented Apr 18, 2023

Uh oh!

AkihiroSuda commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented May 16, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 25, 2023

Uh oh!

AkihiroSuda commented May 25, 2023

Uh oh!

thaJeztah commented May 26, 2023

Uh oh!

AkihiroSuda commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AkihiroSuda commented Apr 5, 2023 •

edited

Loading

AkihiroSuda commented Apr 7, 2023 •

edited

Loading

AkihiroSuda commented Apr 7, 2023 •

edited

Loading

corhere commented Apr 7, 2023 •

edited

Loading

AkihiroSuda commented May 10, 2023 •

edited

Loading

AkihiroSuda commented May 29, 2023 •

edited

Loading