Merge pull request #22110 from giuseppe/fix-default-propagation-for-bind

util: use private propagation with bind

pkg/util/mount_opts.go

@@ -22,15 +22,23 @@ type defaultMountOptions struct {
 	nodev  bool
 }
 
+type getDefaultMountOptionsFn func(path string) (defaultMountOptions, error)
+
 // ProcessOptions parses the options for a bind or tmpfs mount and ensures that
 // they are sensible and follow convention. The isTmpfs variable controls
 // whether extra, tmpfs-specific options will be allowed.
 // The sourcePath variable, if not empty, contains a bind mount source.
 func ProcessOptions(options []string, isTmpfs bool, sourcePath string) ([]string, error) {
+	return processOptionsInternal(options, isTmpfs, sourcePath, getDefaultMountOptions)
+}
+
+func processOptionsInternal(options []string, isTmpfs bool, sourcePath string, getDefaultMountOptions getDefaultMountOptionsFn) ([]string, error) {
 	var (
 		foundWrite, foundSize, foundProp, foundMode, foundExec, foundSuid, foundDev, foundCopyUp, foundBind, foundZ, foundU, foundOverlay, foundIdmap, foundCopy, foundNoSwap, foundNoDereference bool
 	)
 
+	recursiveBind := true
+
 	newOptions := make([]string, 0, len(options))
 	for _, opt := range options {
 		// Some options have parameters - size, mode
@@ -153,7 +161,10 @@ func ProcessOptions(options []string, isTmpfs bool, sourcePath string) ([]string
 				return nil, fmt.Errorf("the 'no-dereference' option can only be set once: %w", ErrDupeMntOption)
 			}
 			foundNoDereference = true
-		case define.TypeBind, "rbind":
+		case define.TypeBind:
+			recursiveBind = false
+			fallthrough
+		case "rbind":
 			if isTmpfs {
 				return nil, fmt.Errorf("the 'bind' and 'rbind' options are not allowed with tmpfs mounts: %w", ErrBadMntOption)
 			}
@@ -184,7 +195,11 @@ func ProcessOptions(options []string, isTmpfs bool, sourcePath string) ([]string
 		newOptions = append(newOptions, "rw")
 	}
 	if !foundProp {
-		newOptions = append(newOptions, "rprivate")
+		if recursiveBind {
+			newOptions = append(newOptions, "rprivate")
+		} else {
+			newOptions = append(newOptions, "private")
+		}
 	}
 	defaults, err := getDefaultMountOptions(sourcePath)
 	if err != nil {

pkg/util/utils_test.go

@@ -3,6 +3,7 @@ package util
 import (
 	"fmt"
 	"math"
+	"sort"
 	"testing"
 	"time"
 
@@ -636,3 +637,130 @@ func TestGetRootlessKeepIDMapping(t *testing.T) {
 		assert.Equal(t, test.expectedGID, gid)
 	}
 }
+
+func getDefaultMountOptionsNoStat(path string) (defaultMountOptions, error) {
+	return defaultMountOptions{false, true, true}, nil
+}
+
+func TestProcessOptions(t *testing.T) {
+	tests := []struct {
+		name       string
+		options    []string
+		isTmpfs    bool
+		sourcePath string
+		expected   []string
+		expectErr  bool
+	}{
+		{
+			name:       "tmpfs",
+			options:    []string{"rw", "size=512m"},
+			isTmpfs:    true,
+			sourcePath: "",
+			expected:   []string{"nodev", "nosuid", "rprivate", "rw", "size=512m", "tmpcopyup"},
+		},
+		{
+			name:       "duplicate idmap option",
+			sourcePath: "/path/to/source",
+			options:    []string{"idmap", "idmap"},
+			expectErr:  true,
+		},
+		{
+			name:       "mode allowed only with tmpfs",
+			sourcePath: "/path/to/source",
+			options:    []string{"rw", "rbind", "mode=0123"},
+			expectErr:  true,
+		},
+		{
+			name:       "noswap allowed only with tmpfs",
+			sourcePath: "/path/to/source",
+			options:    []string{"noswap"},
+			expectErr:  true,
+		},
+		{
+			name:       "tmpcopyup allowed only with tmpfs",
+			sourcePath: "/path/to/source",
+			options:    []string{"tmpcopyup"},
+			expectErr:  true,
+		},
+		{
+			name:       "notmpcopyup allowed only with tmpfs",
+			sourcePath: "/path/to/source",
+			options:    []string{"notmpcopyup"},
+			expectErr:  true,
+		},
+		{
+			name:       "z not allowed with tmpfs",
+			isTmpfs:    true,
+			sourcePath: "/path/to/source",
+			options:    []string{"z"},
+			expectErr:  true,
+		},
+		{
+			name:       "size allowed only with tmpfs",
+			sourcePath: "/path/to/source",
+			options:    []string{"size=123456"},
+			expectErr:  true,
+		},
+		{
+			name:       "conflicting option dev/nodev",
+			sourcePath: "/path/to/source",
+			options:    []string{"dev", "nodev"},
+			expectErr:  true,
+		},
+		{
+			name:       "conflicting option suid/nosuid",
+			sourcePath: "/path/to/source",
+			options:    []string{"suid", "nosuid"},
+			expectErr:  true,
+		},
+		{
+			name:       "conflicting option exec/noexec",
+			sourcePath: "/path/to/source",
+			options:    []string{"noexec", "exec"},
+			expectErr:  true,
+		},
+		{
+			name:       "conflicting option ro/rw",
+			sourcePath: "/path/to/source",
+			options:    []string{"ro", "rw"},
+			expectErr:  true,
+		},
+		{
+			name:       "conflicting option bind/rbind",
+			sourcePath: "/path/to/source",
+			options:    []string{"bind", "rbind"},
+			expectErr:  true,
+		},
+		{
+			name:       "conflicting option bind/rbind",
+			sourcePath: "/path/to/source",
+			options:    []string{"bind", "rbind"},
+			expectErr:  true,
+		},
+		{
+			name:       "default bind mount",
+			sourcePath: "/path/to/source",
+			expected:   []string{"nodev", "nosuid", "rbind", "rprivate", "rw"},
+		},
+		{
+			name:       "default bind mount with bind",
+			sourcePath: "/path/to/source",
+			options:    []string{"bind"},
+			expected:   []string{"nodev", "nosuid", "bind", "private", "rw"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			opts, err := processOptionsInternal(tt.options, tt.isTmpfs, tt.sourcePath, getDefaultMountOptionsNoStat)
+			if tt.expectErr {
+				assert.NotNil(t, err)
+			} else {
+				assert.Nil(t, err)
+				sort.Strings(opts)
+				sort.Strings(tt.expected)
+				assert.Equal(t, opts, tt.expected)
+			}
+		})
+	}
+}