Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

configs: validate: add validation for bind-mount fsflags #3990

Merged
merged 1 commit into from
Nov 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions libcontainer/configs/validate/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be just a warning?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that (at least, according to some other recent issues) most people can't see runc warnings because higher-level runtimes don't provide them to users, so adding a warning doesn't do much. I think we had the same issue with the relative-mount-paths validation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I would prefer if we made this a warning for one release and then upgraded it to an error in runc 1.3, but if warnings are invisible to most users then there's not much point.

}
return nil
}

func checkIDMapMounts(config *configs.Config, m *configs.Mount) error {
if !m.IsIDMapped() {
return nil
Expand Down Expand Up @@ -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)
}
Expand Down
43 changes: 43 additions & 0 deletions libcontainer/configs/validate/validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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: ""},
cyphar marked this conversation as resolved.
Show resolved Hide resolved

{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{
{
Expand Down
Loading