diff --git a/cmd/nerdctl/container/container_run_cgroup_linux_test.go b/cmd/nerdctl/container/container_run_cgroup_linux_test.go index a177448a6e1..5a717007cbf 100644 --- a/cmd/nerdctl/container/container_run_cgroup_linux_test.go +++ b/cmd/nerdctl/container/container_run_cgroup_linux_test.go @@ -498,11 +498,12 @@ func TestParseDevice(t *testing.T) { func TestParseDeviceCgroupRule(t *testing.T) { t.Parallel() + int64Ptr := func(v int64) *int64 { return &v } type testCase struct { s string expectedType string - expectedMajor int64 - expectedMinor int64 + expectedMajor *int64 + expectedMinor *int64 expectedAccess string err string } @@ -510,29 +511,29 @@ func TestParseDeviceCgroupRule(t *testing.T) { { s: "c 189:* rmw", expectedType: "c", - expectedMajor: 189, - expectedMinor: -1, + expectedMajor: int64Ptr(189), + expectedMinor: nil, expectedAccess: "rmw", }, { s: "b 8:0 r", expectedType: "b", - expectedMajor: 8, - expectedMinor: 0, + expectedMajor: int64Ptr(8), + expectedMinor: int64Ptr(0), expectedAccess: "r", }, { s: "a *:* rwm", expectedType: "a", - expectedMajor: -1, - expectedMinor: -1, + expectedMajor: nil, + expectedMinor: nil, expectedAccess: "rwm", }, { s: "c *:5 w", expectedType: "c", - expectedMajor: -1, - expectedMinor: 5, + expectedMajor: nil, + expectedMinor: int64Ptr(5), expectedAccess: "w", }, { @@ -540,8 +541,8 @@ func TestParseDeviceCgroupRule(t *testing.T) { // normalizing them, so the rule has to survive unchanged. s: "a 1:2 rwm", expectedType: "a", - expectedMajor: 1, - expectedMinor: 2, + expectedMajor: int64Ptr(1), + expectedMinor: int64Ptr(2), expectedAccess: "rwm", }, { @@ -587,8 +588,8 @@ func TestParseDeviceCgroupRule(t *testing.T) { assert.NilError(t, err) assert.Equal(t, true, rule.Allow) assert.Equal(t, tc.expectedType, rule.Type) - assert.Equal(t, tc.expectedMajor, *rule.Major) - assert.Equal(t, tc.expectedMinor, *rule.Minor) + assert.DeepEqual(t, tc.expectedMajor, rule.Major) + assert.DeepEqual(t, tc.expectedMinor, rule.Minor) assert.Equal(t, tc.expectedAccess, rule.Access) } else { assert.ErrorContains(t, err, tc.err) @@ -612,6 +613,9 @@ func TestRunDeviceCgroupRule(t *testing.T) { Expected: test.Expects(expect.ExitCodeSuccess, nil, nil), }, { + // Both CLIs reject the missing minor, but with different wording + // (nerdctl: "invalid device cgroup rule format", docker: "invalid + // device cgroup format"), so only the common prefix is matched. Description: "a malformed rule is rejected", Command: func(data test.Data, helpers test.Helpers) test.TestableCommand { return helpers.Command("run", "--rm", @@ -621,7 +625,7 @@ func TestRunDeviceCgroupRule(t *testing.T) { Expected: func(data test.Data, helpers test.Helpers) *test.Expected { return &test.Expected{ ExitCode: expect.ExitCodeGenericFail, - Errors: []error{fmt.Errorf("invalid device cgroup rule format")}, + Errors: []error{fmt.Errorf("invalid device cgroup")}, } }, }, @@ -636,7 +640,7 @@ func TestRunDeviceCgroupRule(t *testing.T) { Expected: func(data test.Data, helpers test.Helpers) *test.Expected { return &test.Expected{ ExitCode: expect.ExitCodeGenericFail, - Errors: []error{fmt.Errorf("invalid device cgroup rule format")}, + Errors: []error{fmt.Errorf("invalid device cgroup")}, } }, }, @@ -645,6 +649,28 @@ func TestRunDeviceCgroupRule(t *testing.T) { testCase.Run(t) } +func TestRunDeviceCgroupRuleDefaultDevices(t *testing.T) { + testCase := nerdtest.Setup() + + testCase.SubTests = []*test.Case{ + { + // The rule only grants access to a non-standard major number (189, + // nvidia GPU), so the container's access to /dev/null, /dev/zero and + // /dev/urandom must come from the default allow list in the spec. + Description: "standard devices remain accessible", + Command: func(data test.Data, helpers test.Helpers) test.TestableCommand { + return helpers.Command("run", "--rm", + "--device-cgroup-rule", "c 189:* rmw", + testutil.AlpineImage, "sh", "-c", + "cat /dev/null && head -c 1 /dev/zero > /dev/null && head -c 1 /dev/urandom > /dev/null && echo ok") + }, + Expected: test.Expects(expect.ExitCodeSuccess, nil, expect.Contains("ok")), + }, + } + + testCase.Run(t) +} + func TestRunCgroupConf(t *testing.T) { testCase := nerdtest.Setup() testCase.Require = require.All( diff --git a/pkg/cmd/container/run_cgroup_linux.go b/pkg/cmd/container/run_cgroup_linux.go index c95165b52e7..97cc9fe77e1 100644 --- a/pkg/cmd/container/run_cgroup_linux.go +++ b/pkg/cmd/container/run_cgroup_linux.go @@ -340,19 +340,25 @@ func validateDeviceMode(mode string) error { var deviceCgroupRuleRegexp = regexp.MustCompile(`^([acb]) ([0-9]+|\*):([0-9]+|\*) ([rwm]{1,3})$`) // ParseDeviceCgroupRule parses the given device cgroup rule into an entry for the -// device allow list of the OCI spec. A "*" major or minor number becomes -1, which -// the runtime expands to "any". +// device allow list of the OCI spec. A "*" major or minor number is left unset, +// which both runc and crun read as "any". It must not be spelled out as -1: runc +// maps a missing field to -1 itself, but crun writes a present -1 verbatim +// ("c -1:-1 m") and the kernel rejects that on cgroup v1. func ParseDeviceCgroupRule(s string) (specs.LinuxDeviceCgroup, error) { matches := deviceCgroupRuleRegexp.FindStringSubmatch(s) if matches == nil { return specs.LinuxDeviceCgroup{}, fmt.Errorf("invalid device cgroup rule format: %q", s) } - parseNumber := func(field string) (int64, error) { + parseNumber := func(field string) (*int64, error) { if field == "*" { - return -1, nil + return nil, nil } - return strconv.ParseInt(field, 10, 64) + value, err := strconv.ParseInt(field, 10, 64) + if err != nil { + return nil, err + } + return &value, nil } major, err := parseNumber(matches[2]) @@ -367,22 +373,110 @@ func ParseDeviceCgroupRule(s string) (specs.LinuxDeviceCgroup, error) { return specs.LinuxDeviceCgroup{ Allow: true, Type: matches[1], - Major: &major, - Minor: &minor, + Major: major, + Minor: minor, Access: matches[4], }, nil } -// withDeviceCgroupRules appends the given rules to the device allow list. -// The list starts with an entry denying every device, so a rule only takes -// effect when it comes after it. +// defaultDeviceCgroupRules holds the entries that runc and crun add to every +// container's device allow list (runc: specconv.AllowedDevices; crun keeps the +// same list for parity with runc). +// +// containerd's oci.WithDefaultUnixDevices already lists most of these devices +// explicitly (/dev/null, /dev/random, /dev/full, /dev/tty, /dev/zero, +// /dev/urandom, /dev/console, /dev/pts, /dev/ptmx), but the mknod wildcards and +// /dev/net/tun are in no spec, and a runtime that applies the spec's device list +// verbatim would deny them. Listing the whole set keeps the allow list +// self-contained; on runc and crun the duplicate grants are no-ops. +// +// Wildcards are deliberately left as nil pointers: runc reads a missing +// major/minor as -1, whereas crun writes a present -1 literally ("c -1:-1 m"), +// which the kernel rejects on cgroup v1. +// +// Keep in sync with runc's specconv.AllowedDevices (unchanged from v1.1.15 +// through v1.5.2, the current stable release). +var defaultDeviceCgroupRules = []specs.LinuxDeviceCgroup{ + // allow mknod for any device + {Allow: true, Type: "c", Access: "m"}, + {Allow: true, Type: "b", Access: "m"}, + {Allow: true, Type: "c", Major: int64Ptr(1), Minor: int64Ptr(3), Access: "rwm"}, // /dev/null + {Allow: true, Type: "c", Major: int64Ptr(1), Minor: int64Ptr(8), Access: "rwm"}, // /dev/random + {Allow: true, Type: "c", Major: int64Ptr(1), Minor: int64Ptr(7), Access: "rwm"}, // /dev/full + {Allow: true, Type: "c", Major: int64Ptr(5), Minor: int64Ptr(0), Access: "rwm"}, // /dev/tty + {Allow: true, Type: "c", Major: int64Ptr(1), Minor: int64Ptr(5), Access: "rwm"}, // /dev/zero + {Allow: true, Type: "c", Major: int64Ptr(1), Minor: int64Ptr(9), Access: "rwm"}, // /dev/urandom + {Allow: true, Type: "c", Major: int64Ptr(136), Access: "rwm"}, // /dev/pts + {Allow: true, Type: "c", Major: int64Ptr(5), Minor: int64Ptr(2), Access: "rwm"}, // /dev/ptmx + {Allow: true, Type: "c", Major: int64Ptr(10), Minor: int64Ptr(200), Access: "rwm"}, // /dev/net/tun +} + +func int64Ptr(v int64) *int64 { + return &v +} + +// withDeviceCgroupRules adds the given rules to the device allow list. +// +// The rules already in the spec stay in front: oci.WithDefaultSpec starts the +// list with a deny-all rule and oci.WithDefaultUnixDevices appends the standard +// device grants, and a device cgroup applies its rules in order, so a deny-all +// rule placed behind a grant cancels it. The defaults and the user rules are +// therefore appended last, and nothing the runtime adds implicitly is needed to +// make the list self-contained. +// +// A default entry is skipped when the spec already grants that device, as +// oci.WithDevices does for --device: mapping /dev/net/tun as read-only must not +// be widened to "rwm" by the fallback grants. The standard devices are already +// listed by oci.WithDefaultUnixDevices, so only the gaps are filled in. func withDeviceCgroupRules(rules []specs.LinuxDeviceCgroup) oci.SpecOpts { return func(_ context.Context, _ oci.Client, _ *containers.Container, s *oci.Spec) error { - s.Linux.Resources.Devices = append(s.Linux.Resources.Devices, rules...) + if len(rules) == 0 { + return nil + } + existing := s.Linux.Resources.Devices + devices := make([]specs.LinuxDeviceCgroup, 0, len(existing)+len(defaultDeviceCgroupRules)+len(rules)) + devices = append(devices, existing...) + for _, d := range defaultDeviceCgroupRules { + if deviceCovered(existing, d) { + continue + } + devices = append(devices, d) + } + devices = append(devices, rules...) + s.Linux.Resources.Devices = devices return nil } } +// deviceCovered reports whether devices already allows the device that rule +// grants, in which case appending rule would be redundant. Only allow entries +// count: the deny-all entry from oci.WithDefaultSpec covers nothing. +func deviceCovered(devices []specs.LinuxDeviceCgroup, rule specs.LinuxDeviceCgroup) bool { + for _, d := range devices { + if !d.Allow { + continue + } + if d.Type != rule.Type && d.Type != "a" { + continue + } + if coversNumber(d.Major, rule.Major) && coversNumber(d.Minor, rule.Minor) { + return true + } + } + return false +} + +// coversNumber reports whether the number already in the allow list covers the +// one a default rule grants. A wildcard covers anything, but a wildcard default +// (the mknod entries) is only covered by another wildcard: a grant for one +// device does not allow creating device nodes. +func coversNumber(existing, want *int64) bool { + if want == nil { + return existing == nil + } + return existing == nil || *existing == *want +} + func withUnified(unified map[string]string) oci.SpecOpts { return func(_ context.Context, _ oci.Client, _ *containers.Container, s *oci.Spec) (err error) { if unified == nil { diff --git a/pkg/cmd/container/run_cgroup_linux_test.go b/pkg/cmd/container/run_cgroup_linux_test.go index 62f0ec2f8c2..31a4deda142 100644 --- a/pkg/cmd/container/run_cgroup_linux_test.go +++ b/pkg/cmd/container/run_cgroup_linux_test.go @@ -26,19 +26,43 @@ import ( "github.com/containerd/containerd/v2/pkg/oci" ) +// specDeviceRules mirrors what the spec already holds when withDeviceCgroupRules +// runs: oci.WithDefaultSpec starts the list with a deny-all rule, and +// oci.WithDefaultUnixDevices appends the standard device grants behind it. +func specDeviceRules() []specs.LinuxDeviceCgroup { + return []specs.LinuxDeviceCgroup{ + {Allow: false, Access: "rwm"}, + {Allow: true, Type: "c", Major: int64Ptr(1), Minor: int64Ptr(3), Access: "rwm"}, // /dev/null + {Allow: true, Type: "c", Major: int64Ptr(1), Minor: int64Ptr(8), Access: "rwm"}, // /dev/random + {Allow: true, Type: "c", Major: int64Ptr(1), Minor: int64Ptr(7), Access: "rwm"}, // /dev/full + {Allow: true, Type: "c", Major: int64Ptr(5), Minor: int64Ptr(0), Access: "rwm"}, // /dev/tty + {Allow: true, Type: "c", Major: int64Ptr(1), Minor: int64Ptr(5), Access: "rwm"}, // /dev/zero + {Allow: true, Type: "c", Major: int64Ptr(1), Minor: int64Ptr(9), Access: "rwm"}, // /dev/urandom + {Allow: true, Type: "c", Major: int64Ptr(5), Minor: int64Ptr(1), Access: "rwm"}, // /dev/console + {Allow: true, Type: "c", Major: int64Ptr(136), Access: "rwm"}, // /dev/pts + {Allow: true, Type: "c", Major: int64Ptr(5), Minor: int64Ptr(2), Access: "rwm"}, // /dev/ptmx + } +} + +// fallbackDefaults lists the entries withDeviceCgroupRules is expected to append +// on top of specDeviceRules: those already covered by the spec are skipped, so +// only the mknod wildcards and /dev/net/tun are left. Written out literally so +// the test does not read its expectations back from the implementation. +func fallbackDefaults() []specs.LinuxDeviceCgroup { + return []specs.LinuxDeviceCgroup{ + {Allow: true, Type: "c", Access: "m"}, // mknod for any char device + {Allow: true, Type: "b", Access: "m"}, // mknod for any block device + {Allow: true, Type: "c", Major: int64Ptr(10), Minor: int64Ptr(200), Access: "rwm"}, // /dev/net/tun + } +} + func TestWithDeviceCgroupRules(t *testing.T) { t.Parallel() - // The spec containerd hands over already denies every device, and allow rules - // take effect only when they come after that entry. newSpec := func() *oci.Spec { return &oci.Spec{ Linux: &specs.Linux{ - Resources: &specs.LinuxResources{ - Devices: []specs.LinuxDeviceCgroup{ - {Allow: false, Access: "rwm"}, - }, - }, + Resources: &specs.LinuxResources{Devices: specDeviceRules()}, }, } } @@ -54,39 +78,174 @@ func TestWithDeviceCgroupRules(t *testing.T) { return parsed } - t.Run("appends the rules after the deny-all entry, in the given order", func(t *testing.T) { + t.Run("appends the defaults and the rules behind the existing spec rules", func(t *testing.T) { s := newSpec() - opt := withDeviceCgroupRules(parse(t, "c 189:* rmw", "b 8:0 r", "a *:* rwm")) - assert.NilError(t, opt(context.Background(), nil, nil, s)) + specRules := specDeviceRules() + rules := parse(t, "c 189:* rmw", "b 8:0 r", "a *:* rwm") + assert.NilError(t, withDeviceCgroupRules(rules)(context.Background(), nil, nil, s)) + + want := make([]specs.LinuxDeviceCgroup, 0, len(specRules)+len(fallbackDefaults())+len(rules)) + want = append(want, specRules...) + want = append(want, fallbackDefaults()...) + want = append(want, rules...) + assert.DeepEqual(t, s.Linux.Resources.Devices, want) devices := s.Linux.Resources.Devices - assert.Equal(t, 4, len(devices)) + // /dev/net/tun is one of the grants that make the list self-contained + // and the spec does not carry it, so it has to come from the defaults. + // The expectation below is written out as a literal instead of being + // read back from defaultDeviceCgroupRules, so dropping or changing the + // grant fails this test. + tun := specs.LinuxDeviceCgroup{ + Allow: true, Type: "c", Major: int64Ptr(10), Minor: int64Ptr(200), Access: "rwm", + } + got := indexOfDevice(devices, tun) + assert.Assert(t, got >= 0, "missing /dev/net/tun grant") + assert.Assert(t, got >= len(specRules), + "/dev/net/tun grant must follow the rules already in the spec, got index %d", got) + assert.Assert(t, got < len(devices)-len(rules), + "/dev/net/tun grant must precede the user rules, got index %d of %d", got, len(devices)) + + // The deny-all rule must stay ahead of every grant: the device cgroup + // applies its rules in order, so a deny-all behind a grant cancels it. assert.Equal(t, false, devices[0].Allow) + assert.Equal(t, "", devices[0].Type) + }) + + t.Run("keeps the --device grants ahead of the appended rules", func(t *testing.T) { + s := newSpec() + deviceGrant := specs.LinuxDeviceCgroup{ + Allow: true, Type: "b", Major: int64Ptr(8), Minor: int64Ptr(0), Access: "rwm", + } + s.Linux.Resources.Devices = append(s.Linux.Resources.Devices, deviceGrant) - assert.Equal(t, true, devices[1].Allow) - assert.Equal(t, "c", devices[1].Type) - assert.Equal(t, int64(189), *devices[1].Major) - assert.Equal(t, int64(-1), *devices[1].Minor) - assert.Equal(t, "rmw", devices[1].Access) + rules := parse(t, "c 189:* rmw") + assert.NilError(t, withDeviceCgroupRules(rules)(context.Background(), nil, nil, s)) - assert.Equal(t, "b", devices[2].Type) - assert.Equal(t, int64(8), *devices[2].Major) - assert.Equal(t, int64(0), *devices[2].Minor) - assert.Equal(t, "r", devices[2].Access) + want := append(specDeviceRules(), deviceGrant) + want = append(want, fallbackDefaults()...) + want = append(want, rules...) + assert.DeepEqual(t, s.Linux.Resources.Devices, want) - assert.Equal(t, "a", devices[3].Type) - assert.Equal(t, int64(-1), *devices[3].Major) - assert.Equal(t, int64(-1), *devices[3].Minor) + // The user rule has to come after the deny-all rule, or it is dropped. + last := s.Linux.Resources.Devices[len(s.Linux.Resources.Devices)-1] + assert.Equal(t, true, last.Allow) + assert.Equal(t, int64(189), *last.Major) }) - t.Run("leaves the device list untouched when no rule is given", func(t *testing.T) { + t.Run("keeps an explicit read-only device mapping", func(t *testing.T) { s := newSpec() - opt := withDeviceCgroupRules(nil) - assert.NilError(t, opt(context.Background(), nil, nil, s)) + // oci.WithDevices maps --device /dev/net/tun:r to exactly this rule. + readOnlyTun := specs.LinuxDeviceCgroup{ + Allow: true, Type: "c", Major: int64Ptr(10), Minor: int64Ptr(200), Access: "r", + } + s.Linux.Resources.Devices = append(s.Linux.Resources.Devices, readOnlyTun) + + rules := parse(t, "c 189:* rmw") + assert.NilError(t, withDeviceCgroupRules(rules)(context.Background(), nil, nil, s)) devices := s.Linux.Resources.Devices - assert.Equal(t, 1, len(devices)) - assert.Equal(t, false, devices[0].Allow) + + // The fallback grants must not widen the mapping to "rwm": the only rule + // for 10:200 has to be the read-only one that was already in the spec. + wideTun := specs.LinuxDeviceCgroup{ + Allow: true, Type: "c", Major: int64Ptr(10), Minor: int64Ptr(200), Access: "rwm", + } + assert.Equal(t, -1, indexOfDevice(devices, wideTun)) + assert.Equal(t, len(specDeviceRules()), indexOfDevice(devices, readOnlyTun)) + + specRules := specDeviceRules() + want := make([]specs.LinuxDeviceCgroup, 0, len(specRules)+1+len(fallbackDefaults())+len(rules)) + want = append(want, specRules...) + want = append(want, readOnlyTun) + // /dev/net/tun is covered by the mapping, so only the mknod wildcards and + // the user rules follow. + want = append(want, fallbackDefaults()[:2]...) + want = append(want, rules...) + assert.DeepEqual(t, devices, want) }) + + t.Run("leaves wildcard major and minor unset", func(t *testing.T) { + s := newSpec() + rules := parse(t, "c *:* rwm", "c 189:* rmw") + assert.NilError(t, withDeviceCgroupRules(rules)(context.Background(), nil, nil, s)) + + // crun writes a present major/minor verbatim, so -1 would reach the kernel + // on cgroup v1; an unset field is what both runtimes read as "any". + devices := s.Linux.Resources.Devices + wildcard := devices[len(devices)-2] + assert.Equal(t, "c", wildcard.Type) + assert.Assert(t, wildcard.Major == nil && wildcard.Minor == nil, + "a fully wildcard rule must not spell out major or minor") + + partial := devices[len(devices)-1] + assert.Equal(t, int64(189), *partial.Major) + assert.Assert(t, partial.Minor == nil, "a wildcard minor must stay unset") + }) + + t.Run("leaves the device list untouched when no rule is given", func(t *testing.T) { + s := newSpec() + assert.NilError(t, withDeviceCgroupRules(nil)(context.Background(), nil, nil, s)) + + assert.DeepEqual(t, s.Linux.Resources.Devices, specDeviceRules()) + }) +} + +// The defaults must not carry an explicit -1 wildcard: crun writes a present +// major/minor verbatim, so "c -1:-1 m" would be rejected by the kernel on +// cgroup v1. Omitted fields are what both runc and crun read as a wildcard. +func TestDefaultDeviceCgroupRulesWildcards(t *testing.T) { + t.Parallel() + + for _, d := range defaultDeviceCgroupRules { + if d.Major != nil { + assert.Assert(t, *d.Major != -1, "unexpected explicit -1 major in %+v", d) + } + if d.Minor != nil { + assert.Assert(t, *d.Minor != -1, "unexpected explicit -1 minor in %+v", d) + } + } + + // mknod for any device: both wildcards omitted. + assert.Assert(t, containsDefault(specs.LinuxDeviceCgroup{Allow: true, Type: "c", Access: "m"}), + "missing mknod char wildcard") + assert.Assert(t, containsDefault(specs.LinuxDeviceCgroup{Allow: true, Type: "b", Access: "m"}), + "missing mknod block wildcard") + + // /dev/pts: wildcard minor omitted. + assert.Assert(t, containsDefault(specs.LinuxDeviceCgroup{ + Allow: true, Type: "c", Major: int64Ptr(136), Access: "rwm", + }), "missing /dev/pts wildcard minor") + + // /dev/net/tun: fixed major/minor, and the spec does not carry it. + assert.Assert(t, containsDefault(specs.LinuxDeviceCgroup{ + Allow: true, Type: "c", Major: int64Ptr(10), Minor: int64Ptr(200), Access: "rwm", + }), "missing /dev/net/tun grant") +} + +func containsDefault(want specs.LinuxDeviceCgroup) bool { + return indexOfDevice(defaultDeviceCgroupRules, want) >= 0 +} + +// indexOfDevice returns the position of want in devices, or -1. +func indexOfDevice(devices []specs.LinuxDeviceCgroup, want specs.LinuxDeviceCgroup) int { + for i, d := range devices { + if sameDevice(d, want) { + return i + } + } + return -1 +} + +func sameDevice(a, b specs.LinuxDeviceCgroup) bool { + return a.Allow == b.Allow && a.Type == b.Type && a.Access == b.Access && + sameInt64(a.Major, b.Major) && sameInt64(a.Minor, b.Minor) +} + +func sameInt64(a, b *int64) bool { + if a == nil || b == nil { + return a == nil && b == nil + } + return *a == *b }