linux: restore read-only `/sys` in the sysfs userns fallback by dmytrobe · Pull Request #2179 · containers/crun

dmytrobe · 2026-08-11T19:47:23Z

Related to #2178.

Since 1.29, /sys can end up mounted read-write even though the spec requests it read-only (type: sysfs, "ro").

This happens when a container's user namespace doesn't own its network namespace: creating a fresh sysfs then fails, so crun falls back to bind-mounting the host's /sys instead. If the spec also has a /sys/fs/cgroup mount, that fallback drops the ro flag.

The root cause is a get_bind_mount() call in do_mount(). 9259e891a split it from one call into two and passed rdonly=false to both, even though this fallback exists specifically to preserve a read-only mount. This PR restores rdonly=true.

Also extends mount-ro-cgroup with a userns axis and a /sys read-only assertion — it fails on both 1.28 and 1.29, passes with this change.

I'm not fully confident this is the right fix: it also makes the no-/sys/fs/cgroup-mount case read-only, which was already broken on 1.28 too, so it goes a little beyond a minimal regression fix.

kolyshkin

Both your commits are missing the subject; please fix

kolyshkin · 2026-08-12T02:43:36Z

Both your commits are missing the subject; please fix

... and DCO ("Signed-off-by:" line), too.

giuseppe · 2026-08-12T07:35:07Z

can you try if this is enough for your case?

diff --git a/src/libcrun/linux.c b/src/libcrun/linux.c
index e99a53d9..d23f05a6 100644
--- a/src/libcrun/linux.c
+++ b/src/libcrun/linux.c
@@ -1524,6 +1524,8 @@ do_mount (libcrun_container_t *container, const char *source, int targetfd,
                         }
                       else
                         {
+                          cleanup_close int sysfd = -1;
+
                           crun_error_release (err);
                           if (sys_old_root_fd >= 0)
                             {
@@ -1538,6 +1540,17 @@ do_mount (libcrun_container_t *container, const char *source, int targetfd,
                             ret = mount ("/sys", real_target, NULL, MS_BIND | MS_REC, NULL);
                           if (UNLIKELY (ret < 0))
                             return crun_make_error (err, errno, "bind mount `/sys` from the host");
+
+                          sysfd = open_mount_target (container, target, err);
+                          if (UNLIKELY (sysfd < 0))
+                            return sysfd;
+
+                          ret = do_remount (sysfd, target,
+                                            MS_REMOUNT | MS_BIND | (mountflags & ~ALL_PROPAGATIONS),
+                                            NULL, err);
+                          if (UNLIKELY (ret < 0))
+                            return ret;
                         }
                       return do_masked_or_readonly_path (container, "/sys/fs/cgroup", false, false, err);
                     }

dmytrobe · 2026-08-12T08:19:44Z

Hey @giuseppe @kolyshkin Thanks for looking.

@giuseppe Just tried your patch and it doesn't work. One of prerequisites is to have /sys/fs/cgroup mount so it doesn't enter if (!has_mount_for(...)) branch at all.

Squashed commits into 1 and added subject.

giuseppe · 2026-08-12T08:30:50Z

the DCO test is still failing. Could you please fix the author in the git commit itself?

git commit --amend --author='Dmytro Bieliaiev <dimabelyaev27@gmail.com>' should do the trick

giuseppe · 2026-08-12T08:54:11Z

@giuseppe Just tried your patch and it doesn't work. One of prerequisites is to have /sys/fs/cgroup mount so it doesn't enter if (!has_mount_for(...)) branch at all.

thanks for trying that.

I'd still prefer to keep the fix local to the workaround, does the following patch work better?

diff --git a/src/libcrun/linux.c b/src/libcrun/linux.c
index e99a53d9..94166958 100644
--- a/src/libcrun/linux.c
+++ b/src/libcrun/linux.c
@@ -1504,6 +1504,7 @@ do_mount (libcrun_container_t *container, const char *source, int targetfd,
               if (ret > 0)
                 {
                   cleanup_close int mountfd = -1;
+                  cleanup_close int sysfd = -1;
                   int sys_old_root_fd = get_old_root_fd (get_private_data (container));

                   if (sys_old_root_fd >= 0)
@@ -1539,6 +1540,17 @@ do_mount (libcrun_container_t *container, const char *source, int targetfd,
                           if (UNLIKELY (ret < 0))
                             return crun_make_error (err, errno, "bind mount `/sys` from the host");
                         }
+
+                      sysfd = open_mount_target (container, target, err);
+                      if (UNLIKELY (sysfd < 0))
+                        return sysfd;
+
+                      ret = do_remount (sysfd, target,
+                                        MS_REMOUNT | MS_BIND | (mountflags & ~ALL_PROPAGATIONS),
+                                        NULL, err);
+                      if (UNLIKELY (ret < 0))
+                        return ret;
+
                       return do_masked_or_readonly_path (container, "/sys/fs/cgroup", false, false, err);
                     }

@@ -1549,6 +1561,16 @@ do_mount (libcrun_container_t *container, const char *source, int targetfd,
                   if (UNLIKELY (ret < 0))
                     return crun_make_error (err, errno, "move mount to `%s`", real_target);

+                  sysfd = open_mount_target (container, target, err);
+                  if (UNLIKELY (sysfd < 0))
+                    return sysfd;
+
+                  ret = do_remount (sysfd, target,
+                                    MS_REMOUNT | MS_BIND | (mountflags & ~ALL_PROPAGATIONS),
+                                    NULL, err);
+                  if (UNLIKELY (ret < 0))
+                    return ret;
+
                   return 0;
                 }
             }

if it does work for you, just amend it in your commit and keep the test as it is

giuseppe

LGTM

dmytrobe · 2026-08-12T09:36:29Z

@giuseppe Seems to be working, thanks for quick reaction and help.

giuseppe · 2026-08-12T09:42:48Z

there is a test failing locally, are you ok if I push any fix to your branch?

dmytrobe · 2026-08-12T09:44:50Z

Sure, feel free to push

dmytrobe · 2026-08-12T10:35:45Z

@giuseppe I've also noticed /dev/{null,zero,full,tty,random,urandom} also lose their noexec flag on 1.29 when the container has a user namespace (1.28: rw,nosuid,noexec; 1.29: rw,nosuid). Could you please clarify if it's expected or should I file separate issue for it?

giuseppe · 2026-08-12T10:41:55Z

@giuseppe I've also noticed /dev/{null,zero,full,tty,random,urandom} also lose their noexec flag on 1.29 when the container has a user namespace (1.28: rw,nosuid,noexec; 1.29: rw,nosuid). Could you please clarify if it's expected or should I file separate issue for it?

isn't that just "aesthetic"? Is it a real problem?

dmytrobe · 2026-08-12T10:50:01Z

I'm not sure if it's a problem (probably not). I've discovered it the same way a discovered initial problem (mounts diff between versions) so decided to double check if it's expected.

Rename has_mount_for to find_mount_for and return the mount struct (or NULL) instead of a bool, so callers can inspect the matched mount's options. No functional change. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Apply MS_NOSUID | MS_NOEXEC to the cloned mount before moving it, as the other code paths already do, and fall back to recreating the mount from scratch if that fails. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2026-08-12T13:12:36Z

@dmytrobe let me know if you are ok with the last version

When a container has a user namespace but does not own the network namespace, creating a fresh sysfs fails with EPERM and do_mount() falls back to bind-mounting the host's /sys instead. That fallback only runs because MS_RDONLY was requested, so the result has to stay read-only. 9259e89 ("use new mount API in do_mount when available", 1.29) rewrote this fallback around get_bind_mount()/fs_move_mount_to() but never re-applies MS_RDONLY along either of its two success paths, so a spec asking for a read-only /sys silently got a writable one instead: crun 1.28 /sys = ro,nosuid,nodev,noexec,relatime crun 1.29 /sys = rw,nosuid,nodev,noexec,relatime After moving or bind-mounting /sys into place, explicitly remount it with the container's requested mount flags (MS_RDONLY included, via mountflags & ~ALL_PROPAGATIONS) at both call sites. This keeps the fix local to the fallback instead of changing what get_bind_mount() itself requests. Extend mount-ro-cgroup in tests/test_mounts.py with a userns axis and an assertion on /sys's own mount flags (it previously only asserted /sys/fs/cgroup). Without a userns the fallback path was never reached, so the regression went uncovered. Verified against crun built from the 1.28 and 1.29 tags: both fail, each in the branch its own bug lives in; this fix passes all 16 combinations, and mount-ro-cgroup passes in the full test_mounts.py suite. Fixes: containers#2178 Signed-off-by: Dmytro Bieliaiev <dimabelyaev27@gmail.com>

dmytrobe · 2026-08-12T13:40:00Z

Great that it fixes both issues. I only see some linter failure but apart from that LGTM.

Extract the /sys bind-mount workaround used when creating a fresh sysfs mount fails in a user namespace into a dedicated function. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

When do_mount() reopens the target after the mount (because the previous targetfd ends up shadowed underneath the new mountpoint), real_target was left pointing at the old, now-shadowed /proc/self/fd/<fd> path. On kernels that provide mount_setattr() (>= 5.12) this is harmless: the propagation and remount steps operate on the reopened targetfd. On older kernels the code falls back to the classic mount(2) API, which uses real_target, and the shadowed path is no longer a mountpoint, so setting the propagation fails with EINVAL: set propagation for `etc/hosts`: Invalid argument Restore the real_target refresh that used to follow the reopen (dropped in commit 5506b6e) so the mount(2) fallbacks act on the reopened mountpoint. Fixes: containers#2182 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

tom--pollard · 2026-08-12T16:21:44Z

Re linux: refresh real_target after reopening the mount target testing dmytrobe:crun-regression-fix has resolved the issue I reported at #2182 😁

kolyshkin

lgtm

kolyshkin requested changes Aug 12, 2026

View reviewed changes

dmytrobe force-pushed the crun-regression-fix branch from c364918 to e81bf8a Compare August 12, 2026 08:13

dmytrobe requested a review from kolyshkin August 12, 2026 08:20

dmytrobe force-pushed the crun-regression-fix branch from e81bf8a to 381346e Compare August 12, 2026 08:39

dmytrobe force-pushed the crun-regression-fix branch from 381346e to a4bb84b Compare August 12, 2026 09:25

giuseppe approved these changes Aug 12, 2026

View reviewed changes

giuseppe added 2 commits August 12, 2026 13:23

linux: make find_mount_for return the matching mount

8c117e5

Rename has_mount_for to find_mount_for and return the mount struct (or NULL) instead of a bool, so callers can inspect the matched mount's options. No functional change. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

linux: restore nosuid,noexec on precreated devices in userns

40c0f45

Apply MS_NOSUID | MS_NOEXEC to the cloned mount before moving it, as the other code paths already do, and fall back to recreating the mount from scratch if that fails. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the crun-regression-fix branch from a4bb84b to b1e7b85 Compare August 12, 2026 13:12

giuseppe force-pushed the crun-regression-fix branch from b1e7b85 to 18505f0 Compare August 12, 2026 13:43

giuseppe added 2 commits August 12, 2026 16:48

linux: move the sysfs userns fallback into its own function

aa6ceeb

Extract the /sys bind-mount workaround used when creating a fresh sysfs mount fails in a user namespace into a dedicated function. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe mentioned this pull request Aug 12, 2026

crun 1.29 potentially showing issues on Linux <= 4.18 kernels #2182

Closed

kolyshkin approved these changes Aug 12, 2026

View reviewed changes

giuseppe merged commit 393b4b5 into containers:main Aug 12, 2026
47 checks passed

Uh oh!

Conversation

dmytrobe commented Aug 11, 2026

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Aug 12, 2026

Uh oh!

giuseppe commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmytrobe commented Aug 12, 2026

Uh oh!

giuseppe commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giuseppe commented Aug 12, 2026

Uh oh!

giuseppe left a comment

Choose a reason for hiding this comment

Uh oh!

dmytrobe commented Aug 12, 2026

Uh oh!

giuseppe commented Aug 12, 2026

Uh oh!

dmytrobe commented Aug 12, 2026

Uh oh!

dmytrobe commented Aug 12, 2026

Uh oh!

giuseppe commented Aug 12, 2026

Uh oh!

dmytrobe commented Aug 12, 2026

Uh oh!

giuseppe commented Aug 12, 2026

Uh oh!

dmytrobe commented Aug 12, 2026

Uh oh!

tom--pollard commented Aug 12, 2026

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

giuseppe commented Aug 12, 2026 •

edited

Loading

giuseppe commented Aug 12, 2026 •

edited

Loading