Repository: opencontainers/runc
Pull request #3990 (2023-08-22 16:48:56)

configs: validate: add validation for bind-mount fsflags
Bind-mounts cannot have any filesystem-specific "data" arguments, because the kernel ignores the data argument for MS_BIND and MS_BIND|MS_REMOUNT and we cannot safely try to override the flags because those would affect mounts on the host (these flags affect the superblock).

It should be noted that there are cases where the filesystem-specified flags will also be ignored for non-bind-mounts but those are kernel quirks and there's no real way for us to work around them. And users wouldn't get any real benefit from us adding guardrails to existing kernel behaviour.

See the discussion in opencontainers/runtime-spec#1222. I am a little worried this could break users, but I also would be very surprised if users were specifying flags to bind mounts that are always ignored.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

rata: The code per-se is fine. But I'm not sure I understand is the intent of this PR. In the description is said quite clearly too:

> I am a little worried this could break users, but I also would be very surprised if users were specifying flags to bind mounts that are always ignored.

If this always worked, why do we want to start throwing an error? What would be simpler for us in the future if we do this?

I'd like to think a warning can help as a first step if we want to go on this direction, but no one really looks at the warnings we throw at this part of the stack today, so I don't think it will help :-/
cyphar: At the moment we are ignoring flags the user has explicitly requested and there is no way for us to not ignore them (you by defintion cannot set fs-specific flags on bind-mounts because they are per-superblock). I see this as being related to the work in #3967. The runtime-spec has frustratingly loose wording on how mounts should be handled in these sorts of edge cases.

Perhaps it would be less aggressive to make this a warning and we can return an error in 1.3 if nobody complains.
rata: > Perhaps it would be less aggressive to make this a warning and we can return an error in 1.3 if nobody complains.

I think we already have a bunch of changes in 1.2, I prefer to not add more possibly breaking changes.

But what do we have to win by returning an error? Will something be better for us, or it just has the potential to break users? Can't we even assign data to `""` and throw a warning and consider it done?
lifubang: > I am a little worried this could break users

Is this need to be backported to '1.1'? I am a bit worried too.

> Perhaps it would be less aggressive to make this a warning and we can return an error in 1.3 if nobody complains.

If we change to output a warning msg, I think we can backport it to '1.1'.
kolyshkin: > > I am a little worried this could break users
> 
> Is this need to be backported to '1.1'? I am a bit worried too.

My thinking is, it will help users to find problems in their setups before we release 1.2.0 (meaning, instead of piling everything to 1.2.0, we can use the 1.1.10 opportunity to introduce this change -- and, if it is really breaking users setups, revert it).

A warning seems better, the only problem is, no one sees these warnings, because no one uses runc directly, and (AFAIK) upper level tools do not show runc output to users.

OTOH I don't have a stong opinion, and since this is definitely not a bug fix, let's not backport it.
cyphar: Fixed up the nits.

I agree with @kolyshkin that it would be nice if warnings were useful, but they're not so making it warning-only in 1.2 (or backporting a warning-only version to 1.1) are of limited usefulness...
kolyshkin: Also, in case it causes a major breakage, we can address that in a 1.2.z stable release in a timely manner.

Patch:
diff --git a/libcontainer/configs/validate/validator.go b/libcontainer/configs/validate/validator.go
index a6736f26..0df7242b 100644
--- a/libcontainer/configs/validate/validator.go
+++ b/libcontainer/configs/validate/validator.go
@@ -275,6 +275,26 @@ func cgroupsCheck(config *configs.Config) error {
 	return nil
 }
 
+func checkBindOptions(m *configs.Mount) error {
+	if !m.IsBind() {
+		return nil
+	}
+	// We must reject bind-mounts that also have filesystem-specific mount
+	// options, because the kernel will completely ignore these flags and we
+	// cannot set them per-mountpoint.
+	//
+	// It should be noted that (due to how the kernel caches superblocks), data
+	// options could also silently ignored for other filesystems even when
+	// doing a fresh mount, but there is no real way to avoid this (and it
+	// matches how everything else works). There have been proposals to make it
+	// possible for userspace to detect this caching, but this wouldn't help
+	// runc because the behaviour wouldn't even be desirable for most users.
+	if m.Data != "" {
+		return errors.New("bind mounts cannot have any filesystem-specific options applied")
+	}
+	return nil
+}
+
 func checkIDMapMounts(config *configs.Config, m *configs.Mount) error {
 	if !m.IsIDMapped() {
 		return nil
@@ -313,6 +333,9 @@ func mountsWarn(config *configs.Config) error {
 
 func mountsStrict(config *configs.Config) error {
 	for _, m := range config.Mounts {
+		if err := checkBindOptions(m); err != nil {
+			return fmt.Errorf("invalid mount %+v: %w", m, err)
+		}
 		if err := checkIDMapMounts(config, m); err != nil {
 			return fmt.Errorf("invalid mount %+v: %w", m, err)
 		}


Test patch:
diff --git a/libcontainer/configs/validate/validator_test.go b/libcontainer/configs/validate/validator_test.go
index 7de9b0f9..a882fa64 100644
--- a/libcontainer/configs/validate/validator_test.go
+++ b/libcontainer/configs/validate/validator_test.go
@@ -421,6 +421,49 @@ func TestValidateMounts(t *testing.T) {
 	}
 }
 
+func TestValidateBindMounts(t *testing.T) {
+	testCases := []struct {
+		isErr bool
+		flags int
+		data  string
+	}{
+		{isErr: false, flags: 0, data: ""},
+		{isErr: false, flags: unix.MS_RDONLY | unix.MS_NOSYMFOLLOW, data: ""},
+
+		{isErr: true, flags: 0, data: "idmap"},
+		{isErr: true, flags: unix.MS_RDONLY, data: "custom_ext4_flag"},
+		{isErr: true, flags: unix.MS_NOATIME, data: "rw=foobar"},
+	}
+
+	for _, tc := range testCases {
+		for _, bind := range []string{"bind", "rbind"} {
+			bindFlag := map[string]int{
+				"bind":  unix.MS_BIND,
+				"rbind": unix.MS_BIND | unix.MS_REC,
+			}[bind]
+
+			config := &configs.Config{
+				Rootfs: "/var",
+				Mounts: []*configs.Mount{
+					{
+						Destination: "/",
+						Flags:       tc.flags | bindFlag,
+						Data:        tc.data,
+					},
+				},
+			}
+
+			err := Validate(config)
+			if tc.isErr && err == nil {
+				t.Errorf("%s mount flags:0x%x data:%v, expected error, got nil", bind, tc.flags, tc.data)
+			}
+			if !tc.isErr && err != nil {
+				t.Errorf("%s mount flags:0x%x data:%v, expected nil, got error %v", bind, tc.flags, tc.data, err)
+			}
+		}
+	}
+}
+
 func TestValidateIDMapMounts(t *testing.T) {
 	mapping := []configs.IDMap{
 		{