support recursive read only (`rro`) option for mounts #25680

danishprakash · 2025-03-25T15:23:57Z

ro on linux kernel < 5.12 doesn't apply MT_RECURSIVE leading to submounts not respecting the readonly option.

crun/runc achieve this by calling mount_setattr(2) when passed the rro mount option. This change check whether the runtime in use supports rro and the kernel supports mount_setattr(2) and passes rro to the runtime. It defaults to rro when supported and when ro is specified. If rro is specified and unsupported, it returns an error.

Closes #24229

Signed-off-by: Danish Prakash contact@danishpraka.sh

`ro` on linux kernel < 5.12 doesn't apply MT_RECURSIVE leading to submounts not respecting the readonly option. crun/runc achieve this by calling mount_setattr(2) when passed the `rro` mount option. This change check whether the runtime in use supports `rro` and the kernel supports mount_setattr(2) and passes `rro` to the runtime. It defaults to `rro` when supported and when `ro` is specified. If `rro` is specified and unsupported, it returns an error. Signed-off-by: Danish Prakash <contact@danishpraka.sh>

openshift-ci · 2025-03-25T15:24:02Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-ci · 2025-03-25T15:24:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: danishprakash
Once this PR has been reviewed and has the lgtm label, please assign mtrmac for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

packit-as-a-service · 2025-03-25T15:55:47Z

Cockpit tests failed for commit 87c34b1. @martinpitt, @jelly, @mvollmer please check.

Luap99

Thanks for working on this. I like to see some more discussion around the features check. I at least have performance concerns with such implementation.

cc @giuseppe @mheon @baude

Luap99 · 2025-03-25T17:19:30Z

libpod/oci_conmon_common.go

+	features, err := runtime.getOCIRuntimeFeatures()
+	if err != nil {
+		return nil, fmt.Errorf("getting %s features: %w", runtime.name, err)
+	}
+	runtime.features = features


IMO this is not acceptable for performance reasons. Every podman command would then have to run this even when the vast majority of them will never need this.
I know the features command seems very fast but it is still a total overhead of a few ms. And we have users on embedded systems where they are much more concerned about every small thing.

Second how many runtimes actually support the features command? Right not this hard error which means it can cause regressions.

I see three options:

don't for any feature checks and assume rro is always supported (that means we would need to bump the kernel baseline and document what runtimes it support)

wrap this into a sync.OnceValues() function so it is only called onca and only when needed.

add a compile time option for podman to specify if rro should be used. Distros that ship podman with a new kernel and runtime then don't have to pay the feature check overhead.

We have also, in the past, added fields to containers.conf to indicate which runtimes supported certain features - see supportsNoCgroups for example. Adding to containers.conf is not perfect (it shifts the burden to the packager to make sure their combination of kernel + runtime version supports RRO) but it is very performant.

Alternatively, I wonder if we could lazy-initialize this

ro is a common option, so we will always end up with the price of running $OCI_RUNTIME features on each invocation. Also there is the risk with podman system service that we will not detect when the OCI runtime was updated.

IMO we could cache the result under the tmpdir using the runtime path as the key, and then use the runtime binary stat (st_dev, st_ino and mtime) information to detect when the cache is out of date.

What do you think?

Alternatively, I wonder if we could lazy-initialize this

That is my suggest 2, using sync.OnceValues()

ro is a common option, so we will always end up with the price of running $OCI_RUNTIME features on each invocation

How so? Any command that does not start a container will certainly never need it. podman stop, podman rm, podman ps, podman logs, etc... How I look at this only the minority will actually need this.

IMO we could cache the result under the tmpdir using the runtime path as the key, and then use the runtime binary stat (st_dev, st_ino and mtime) information to detect when the cache is out of date.

Yes that sounds good to me, there is an overhead for stat and reading the file but compared to the rest we do that is likely not a problem. Should be much better than spawning a new process each time.

Even worse reading the code again we actually call newConmonOCIRuntime() on all runtimes from containers.conf so we pay the overhead multiple times for all runtimes which really doesn't feel right.

ro is a common option, so we will always end up with the price of running $OCI_RUNTIME features on each invocation

How so? Any command that does not start a container will certainly never need it. podman stop, podman rm, podman ps, podman logs, etc... How I look at this only the minority will actually need this.

I was referring only to the commands that run a container, and that is where we care more about performance. sync.OnceValues() won't work with system service as we need to check that the runtime didn't change.

On my system I've:

➜ hyperfine 'crun features' Benchmark 1: crun features Time (mean ± σ): 2.3 ms ± 0.6 ms [User: 0.8 ms, System: 1.6 ms] Range (min … max): 1.6 ms … 6.2 ms 553 runs

a stat() is much faster than that. If we really care about performance we could even consider statx() and query only the information we need. Most of the price will come from dealing with json anyway, so maybe we want to store the information in a different format and store only the bits we care about.

Most of the price will come from dealing with json anyway, so maybe we want to store the information in a different format and store only the bits we care about.

Sounds like a good idea.

(sorry for the multi-week delay in response)

wrap this into a sync.OnceValues() function so it is only called onca and only when needed.

I prefer lazy-initializing the runtime feature over the other two options suggested by @Luap99 as well. As far as caching the output under tmpdir, I'm under the assumption--as @giuseppe pointed out--that it's to avoid a situation where the runtime changes. It shouldn't make too much of a dent in terms of performance.

pkg/specgenutil/volumes.go

Luap99 · 2025-03-25T17:21:37Z

pkg/util/utils_linux.go

+	errKernelDoesNotSupportRRO  = errors.New("kernel does not support recursive readonly mount option `rro`")
+	errRuntimeDoesNotSupportRRO = errors.New("runtime does not support recursive readonly mount option `rro`")
+
+	kernelSupportsRROOnce sync.Once


new code should use sync.OnceValue(s) instead

mheon · 2025-03-25T17:28:35Z

libpod/oci_missing.go

@@ -194,6 +195,11 @@ func (r *MissingRuntime) SupportsKVM() bool {
 	return false
 }

+// Features returns nil since this is a missing runtime
+func (r *MissingRuntime) Features() *features.Features {
+	return nil


This should still return an error, so I recommend (*features.Features, error) - the missing runtime can have methods called on it and we want to avoid null pointer panics

mheon · 2025-03-25T17:29:43Z

libpod/container_internal_common.go

@@ -374,6 +374,27 @@ func (c *Container) generateSpec(ctx context.Context) (s *spec.Spec, cleanupFunc
 				// Podman decided for --no-dereference as many
 				// bin-utils tools (e..g, touch, chown, cp) do.
 				options = append(options, "copy-symlink")
+			// TODO: this also ends up checking non-user mounts
+			case "ro", "rro":


Why check rro here? It seems like we should always pass it along unmodified, even if unsupported - if the user explicitly requested rro and we silently downgrade, that seems like a problem.

yes, rro must be passed as it is.

Also we need to have a way to restore the previous behavior. If a user really wants a ro, there is no way to achieve it when we always force ro -> rro

Why check rro here? It seems like we should always pass it along unmodified, even if unsupported - if the user explicitly requested rro and we silently downgrade, that seems like a problem.

Downgrade shouldn't happen. The intent for an rro check here is to upgrade ro to rro when supported and to error out when a user explicitly requests rro and it isn't supported.

Also we need to have a way to restore the previous behavior. If a user really wants a ro, there is no way to achieve it when we always force ro -> rro

But why? I can understand for backwards compat but that is wrong from a security point of view. I don't know how bad the impact would be when we roll out this change, but we can introduce a non-recursive (norro) as part of the transition till the buggy ro behaviour fades out. Wdyt?

martinpitt · 2025-03-25T18:08:02Z

The cockpit-podman failure is interesting and relevant. /tmp/ro is mounted read-only, and yet the API has volume.RW == true. (c-podman doesn't know about/test rro yet, of course)

Luap99 · 2025-03-26T14:30:06Z

Another thing I just thought off, this does a kernel feature check but is that actually correct? We might have VM based runtimes such as krun, with the VM runtimes the mount options would be applied in the VM kernel not on the host right? Not sure if this is a real concern.

giuseppe · 2025-03-26T14:37:21Z

pkg/util/utils_linux.go

+		tmpMnt, err := os.MkdirTemp("", "podman-detect-rro")
+		if err != nil {
+			return fmt.Errorf("failed to create a temp directory: %w", err)
+		}
+		for {
+			err = unix.Mount("", tmpMnt, "tmpfs", 0, "")
+			if !errors.Is(err, unix.EINTR) {
+				break
+			}
+		}
+		if err != nil {
+			return fmt.Errorf("failed to mount tmpfs on %q: %w", tmpMnt, err)
+		}
+		defer func() {
+			var umErr error
+			for {
+				umErr = unix.Unmount(tmpMnt, 0)
+				if !errors.Is(umErr, unix.EINTR) {
+					break
+				}
+			}
+			if umErr != nil {
+				logrus.Errorf("Failed to unmount %q: %v", tmpMnt, umErr)
+			}
+		}()


please use open_tree or fsopen so we don't have to create a mount that could be leaked to the host.

kolyshkin · 2025-03-31T19:48:07Z

pkg/util/utils_linux.go

+	kernelSupportsRROOnce.Do(func() {
+		errKernelDoesNotSupportRRO = fn()
+	})
+	return errKernelDoesNotSupportRRO


You can simplify this by using sync.OnceFunc.

openshift-merge-robot · 2025-04-02T14:20:45Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Mar 25, 2025

Luap99 requested changes Mar 25, 2025

View reviewed changes

mheon reviewed Mar 25, 2025

View reviewed changes

giuseppe reviewed Mar 26, 2025

View reviewed changes

kolyshkin reviewed Mar 31, 2025

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support recursive read only (`rro`) option for mounts #25680

support recursive read only (`rro`) option for mounts #25680

danishprakash commented Mar 25, 2025

openshift-ci bot commented Mar 25, 2025

openshift-ci bot commented Mar 25, 2025

packit-as-a-service bot commented Mar 25, 2025

Luap99 left a comment

Luap99 Mar 25, 2025

mheon Mar 25, 2025

mheon Mar 25, 2025

giuseppe Mar 26, 2025

Luap99 Mar 26, 2025

giuseppe Mar 26, 2025

Luap99 Mar 26, 2025

danishprakash Apr 23, 2025

Luap99 Mar 25, 2025

mheon Mar 25, 2025

mheon Mar 25, 2025

giuseppe Mar 26, 2025

danishprakash Apr 28, 2025

danishprakash Apr 28, 2025

martinpitt commented Mar 25, 2025

Luap99 commented Mar 26, 2025

giuseppe Mar 26, 2025

kolyshkin Mar 31, 2025

openshift-merge-robot commented Apr 2, 2025

support recursive read only (rro) option for mounts #25680

Are you sure you want to change the base?

support recursive read only (rro) option for mounts #25680

Conversation

danishprakash commented Mar 25, 2025

openshift-ci bot commented Mar 25, 2025

openshift-ci bot commented Mar 25, 2025

packit-as-a-service bot commented Mar 25, 2025

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt commented Mar 25, 2025

Luap99 commented Mar 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-merge-robot commented Apr 2, 2025

support recursive read only (`rro`) option for mounts #25680

support recursive read only (`rro`) option for mounts #25680