Use `mount_setattr()` in `bind_mount()` by xxyzz · Pull Request #756 · containers/bubblewrap

xxyzz · 2026-05-31T02:55:18Z

Fix #754, all tests passed. I also update some GitHub Actions.

rusty-snake · 2026-05-31T06:25:03Z

+        .attr_set = MOUNT_ATTR_NOSUID | (devices ? 0 : MOUNT_ATTR_NODEV) |
+                    (readonly ? MOUNT_ATTR_RDONLY : 0),
+      };
+      if (mount_setattr(dest_fd, "",


musl does not have a library function for mount_setattr yet. Also older (before 2024?) glibc versions. We should ifdef-provide our own definition.

Wrapper function added.

charmander · 2026-05-31T09:29:14Z

I also update some GitHub Actions.

Were they related somehow?

xxyzz · 2026-05-31T09:33:52Z

Were they related somehow?

No, these actions are just too outdated so I updated them.

rusty-snake · 2026-05-31T12:57:54Z

What was wrong with has_mount_setattr?

The code changes have some AI smell to me.

xxyzz · 2026-06-01T00:35:13Z

I feel has_mount_setattr is kind of redundant now and the code resembles your previous suggestion in issue 650. I did ask gemini some questions because I'm not sure how to be compatible with musl and the #define lines are modified from the chatbot's answer. Is the change good enough or maybe it need further improvement?

rusty-snake · 2026-06-01T05:07:29Z

Is the change good enough or maybe it need further improvement?

It needs a review from.someone who is actually responsible for the codebase.

ao2 · 2026-06-02T08:28:59Z

I also update some GitHub Actions.

Were they related somehow?

Please move unrelated changes to a separate commit, or even to a separate PR, to have a more focused review, and in general avoid unnecessary space changes.

Also the commit message of the main change should mention the rationale from #754 and also have lines like:

...

Fixes: https://github.com/containers/bubblewrap/issues/754
Signed-off-by: YOUR NAME <email>

Ideally with your real name, but I don't think this is a strict requirement 😄

ao2 · 2026-06-02T08:35:45Z

                                         const char    *src);
+
+#ifndef __NR_mount_setattr
+#define __NR_mount_setattr 442


Please note that syscall numbers in linux are platform specific, so this is not general enough. E.g. on alpha __NR_mount_setattr is 552, see https://github.com/torvalds/linux/blob/6f3ed7fec72fc8979b2a8c7219c0a9fcfc8d07b5/arch/alpha/kernel/syscalls/syscall.tbl#L484

What about having a simpler approach like done for pivot_root? So that we require that __NR_mount_setattr is defined but we support the cases where the system function mount_setattr is not.

Code updated. I think I've read that function before but somehow forgot it...

Yes, I like the approach @ao2 suggested here.

This also means that when compiling on ancient kernels, we would have the option to define __NR_mount_setattr on only the architectures that we expect might genuinely be running ancient kernels (in practice usually x86), like this:

#ifndef __NR_mount_setattr # if defined(__x86_64__) # define __NR_mount_setattr ... # elif defined(__i386__) # define __NR_mount_setattr ... # endif #endif

bubblewrap is used by Steam Runtime version 1, which is very old, so we should check whether its libc/kernel headers define __NR_mount_setattr. (But that's an easy change to add later; and Steam Runtime v1 only supports x86, so it doesn't have to try to cope with other architectures.)

smcv

Thanks for looking at this!

smcv · 2026-06-02T17:08:16Z

        break;

+      case BIND_MOUNT_ERROR_MOUNT_SETATTR:
+        string = xasprintf("mount_setattr() failed at \"%s\"", failing_path);


Nitpick: please follow the whitespace style of the existing code, e.g.

Suggested change

string = xasprintf("mount_setattr() failed at \"%s\"", failing_path);

string = xasprintf ("mount_setattr() failed at \"%s\"", failing_path);

smcv · 2026-06-02T17:16:07Z

+  struct mount_attr attr = {
+    .attr_clr = 0,
+    .attr_set = MOUNT_ATTR_NOSUID | (devices ? 0 : MOUNT_ATTR_NODEV) |
+                (readonly ? MOUNT_ATTR_RDONLY : 0),


This would be easier to review/analyze if you structured it as an early-return on mount_setattr() success, so that the diff wasn't re-indenting all the code that you're avoiding by using mount_setattr():

if (mount_setattr_wrapper (...) == 0) { return BIND_MOUNT_SUCCESS; } else if (errno != ENOSYS) { ... report error ... } /* ... else mount_setattr(2) isn't available, so we'll have to do this the hard way. */ /* If we are in a case-insensitive filesystem, mountinfo might contain a * different case combination of the path we requested to mount. ...

I haven't reviewed all of the diff here, because at the moment it's hard to tell what is genuinely new or changed code and what is just re-indented.

smcv · 2026-06-02T17:18:42Z

+    .attr_set = MOUNT_ATTR_NOSUID | (devices ? 0 : MOUNT_ATTR_NODEV) |
+                (readonly ? MOUNT_ATTR_RDONLY : 0),


Instead of inline ?: operators, I think this would be easier to read in a structure more like this:

struct mount_attr attr = { .attr_clr = 0, .attr_set = MOUNT_ATTR_NOSUID, }; if (!devices) attr.attr_set |= MOUNT_ATTR_NODEV; if (readonly) attr.attr_set |= MOUNT_ATTR_RDONLY;

more like the first commit of #751.

smcv · 2026-06-02T17:23:29Z

+                (readonly ? MOUNT_ATTR_RDONLY : 0),
+  };
+  if (mount_setattr_wrapper (dest_fd, "",
+                             AT_EMPTY_PATH | (recursive ? AT_RECURSIVE : 0), &attr,


Similarly I'd prefer this more like:

unsigned int setattr_flags = AT_EMPTY_PATH; if (recursive) setattr_flags |= AT_RECURSIVE; ... ... mount_setattr_wrapper (dest_fd, "", setattr_flags, ...) ...

smcv · 2026-06-02T17:26:09Z

-
-      return BIND_MOUNT_ERROR_READLINK_DEST_PROC_FD;
-    }
+      if (errno != ENOSYS)


Can mount_setattr() fail with EINVAL if the kernel is new enough to have the syscall, but too old to have one of the flags that we are using?

(It's unfortunate that EINVAL can mean both "you used a flag I didn't understand" and "something about the state of the mount table means I can't carry out your request"...)

AT_RECURSIVE was added in 5.2, before mount_setattr added in 5.12. MOUNT_ATTR_NOSYMFOLLOW was added in 5.14 but it's not used. Should be fine.

smcv · 2026-06-02T17:37:59Z

                                         const char    *src);
+
+#ifndef __NR_mount_setattr
+#define __NR_mount_setattr 442


Yes, I like the approach @ao2 suggested here.

This also means that when compiling on ancient kernels, we would have the option to define __NR_mount_setattr on only the architectures that we expect might genuinely be running ancient kernels (in practice usually x86), like this:

#ifndef __NR_mount_setattr # if defined(__x86_64__) # define __NR_mount_setattr ... # elif defined(__i386__) # define __NR_mount_setattr ... # endif #endif

bubblewrap is used by Steam Runtime version 1, which is very old, so we should check whether its libc/kernel headers define __NR_mount_setattr. (But that's an easy change to add later; and Steam Runtime v1 only supports x86, so it doesn't have to try to cope with other architectures.)

smcv · 2026-06-02T17:51:01Z

[Signed-off-by] Ideally with your real name, but I don't think this is a strict requirement

This was a policy question in the past, and the repo owners concluded that contributions under a pseudonym can be accepted. But...

gitpull at protonmail.com

Is this a real address that is really yours, or a fake/placeholder email address that doesn't really exist?

I think we do need the Signed-off-by to come from an identifiable person, even if they're only identifiable by a pseudonym. Github provides a pseudo-email-address that identifies a Github account which is probably sufficient, although I'd have to check that policy with the repo owners (cc @alexlarsson @cgwalters).

(If this is really your email address then that's fine, it just looks like it might be a placeholder.)

smcv · 2026-06-02T17:58:00Z

the commit message of the main change should mention the rationale from #754

Yes please. Perhaps something like:

Use mount_setattr() to set mount flags

Unlike the old mount(2) API, this lets us apply the same mount flags recursively to a whole tree of mount points in a single operation, avoiding time-of-check/time-of-use race conditions if the mount table changes (#650) and improving performance when there are many mount points (#384).

... and it can probably have Fixes: https://github.com/containers/bubblewrap/issues/384 and Fixes: https://github.com/containers/bubblewrap/issues/650, if it does in fact fix those issues.

Are you using bubblewrap for something? Does this change solve a problem that you were encountering?

xxyzz · 2026-06-03T01:56:16Z

This is my real email.

I use bwrap indirectly via bubblejail. I don't have any problem with it, I guess the commit probably should fix those linked issues.

Unlike the old mount(2) API, this lets us apply the same mount flags recursively to a whole tree of mount points in a single operation, avoiding time-of-check/time-of-use race conditions if the mount table changes (containers#650) and improving performance when there are many mount points (containers#384). Fixes: containers#754 Signed-off-by: xxyzz <gitpull@protonmail.com>

xxyzz force-pushed the mount_setattr branch from ee19906 to 28818f6 Compare May 31, 2026 03:00

rusty-snake reviewed May 31, 2026

View reviewed changes

xxyzz force-pushed the mount_setattr branch 2 times, most recently from 69fdb9e to f65b3ec Compare May 31, 2026 08:38

ao2 suggested changes Jun 2, 2026

View reviewed changes

xxyzz force-pushed the mount_setattr branch from f65b3ec to c4ae75f Compare June 2, 2026 09:00

smcv requested changes Jun 2, 2026

View reviewed changes

xxyzz force-pushed the mount_setattr branch from c4ae75f to ac2e082 Compare June 3, 2026 01:48

xxyzz force-pushed the mount_setattr branch from ac2e082 to 03d25ff Compare June 3, 2026 23:28

	string = xasprintf("mount_setattr() failed at \"%s\"", failing_path);
	string = xasprintf ("mount_setattr() failed at \"%s\"", failing_path);

		.attr_set = MOUNT_ATTR_NOSUID \| (devices ? 0 : MOUNT_ATTR_NODEV) \|
		(readonly ? MOUNT_ATTR_RDONLY : 0),

Conversation

xxyzz commented May 31, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charmander commented May 31, 2026

Uh oh!

xxyzz commented May 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rusty-snake commented May 31, 2026

Uh oh!

xxyzz commented Jun 1, 2026

Uh oh!

rusty-snake commented Jun 1, 2026

Uh oh!

ao2 commented Jun 2, 2026

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!

smcv left a comment

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!

xxyzz Jun 3, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

smcv commented Jun 2, 2026

Uh oh!

smcv commented Jun 2, 2026

Uh oh!

xxyzz commented Jun 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xxyzz commented May 31, 2026 •

edited

Loading

xxyzz Jun 3, 2026 •

edited

Loading