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.

Sign up for GitHub

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

Add device resource update support #3402

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add device resource update support #3402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a5bf5cf
Separate Device handling for default AllowDevices
agners Mar 3, 2022
53506ab
Implement common function to create DeviceCgroup rules
agners Mar 3, 2022
2958e81
Implement Device Resources updates
agners Mar 3, 2022
f17333d
Add integration tests for device updates
agners Aug 5, 2022
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
3 changes: 3 additions & 0 deletions libcontainer/devices/device.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ type Device struct {

// Gid of the device.
Gid uint32 `json:"gid"`

// Is Default Device
IsDefault bool `json:"is_default"`
Copy link
Member

@cyphar cyphar Jul 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This struct is stored on disk and so, on upgrade, any existing containers will have a JSON config that has ever device be "non-default". It seems that this flag is only used for an internal check (and the on-disk flag is never checked) -- in that case, this shouldn't be stored on the filesystem at all and you should hide this field. I'm not sure why we need this flag since we already have a list of the default devices (every item on that list is the default).

(As an aside, we really should have tests to make sure we never break this again -- Docker 17.06.1 was a complete nightmare. I suspect this wouldn't fail tests like that -- since it seems like nothing would actually break -- but it makes me a little worried.)

Copy link
Author

Choose a reason for hiding this comment

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

It seems that this flag is only used for an internal check (and the on-disk flag is never checked)

Afaik, the on-disk flag is used when updating. CreateDefaultDevicesCgroups is now called from update.go (see updateCommand).

From what I remember this was necessary to "clear" allowed devices. Otherwise, the user would have to pass the default devices to get back to default state.

This struct is stored on disk and so, on upgrade, any existing containers will have a JSON config that has ever device be "non-default".

Hm, this is a problem, can we migrate the data?

Copy link
Author

Choose a reason for hiding this comment

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

I've reviewed this again, the reason it is required is because default allowed devices get special treatment for via CreateCgroupConfig. To do this, we need to understand which devices have been added through spec, and which have been added from AllowedDevices. This used to be done in a single function createDevices. With commit 390ac2b de-duplicates this logic so it can be used for updates.

There is no need to store this flag in JSON, so I think we can use json:"-" which we use in other places too.

Copy link
Author

Choose a reason for hiding this comment

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

It turns out, the is_default needs to be stored in the container state to successfully "re-duplicate" on update.

}

// Permissions is a cgroupv1-style string to represent device access. It
Expand Down
180 changes: 112 additions & 68 deletions libcontainer/specconv/spec_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ func KnownMountOptions() []string {
var AllowedDevices = []*devices.Device{
// allow mknod for any device
{
IsDefault: true,
Rule: devices.Rule{
Type: devices.CharDevice,
Major: devices.Wildcard,
Expand All @@ -210,6 +211,7 @@ var AllowedDevices = []*devices.Device{
},
},
{
IsDefault: true,
Rule: devices.Rule{
Type: devices.BlockDevice,
Major: devices.Wildcard,
Expand All @@ -219,10 +221,11 @@ var AllowedDevices = []*devices.Device{
},
},
{
Path: "/dev/null",
FileMode: 0o666,
Uid: 0,
Gid: 0,
Path: "/dev/null",
FileMode: 0o666,
Uid: 0,
Gid: 0,
IsDefault: true,
Rule: devices.Rule{
Type: devices.CharDevice,
Major: 1,
Expand All @@ -232,10 +235,11 @@ var AllowedDevices = []*devices.Device{
},
},
{
Path: "/dev/random",
FileMode: 0o666,
Uid: 0,
Gid: 0,
Path: "/dev/random",
FileMode: 0o666,
Uid: 0,
Gid: 0,
IsDefault: true,
Rule: devices.Rule{
Type: devices.CharDevice,
Major: 1,
Expand All @@ -245,10 +249,11 @@ var AllowedDevices = []*devices.Device{
},
},
{
Path: "/dev/full",
FileMode: 0o666,
Uid: 0,
Gid: 0,
Path: "/dev/full",
FileMode: 0o666,
Uid: 0,
Gid: 0,
IsDefault: true,
Rule: devices.Rule{
Type: devices.CharDevice,
Major: 1,
Expand All @@ -258,10 +263,11 @@ var AllowedDevices = []*devices.Device{
},
},
{
Path: "/dev/tty",
FileMode: 0o666,
Uid: 0,
Gid: 0,
Path: "/dev/tty",
FileMode: 0o666,
Uid: 0,
Gid: 0,
IsDefault: true,
Rule: devices.Rule{
Type: devices.CharDevice,
Major: 5,
Expand All @@ -271,10 +277,11 @@ var AllowedDevices = []*devices.Device{
},
},
{
Path: "/dev/zero",
FileMode: 0o666,
Uid: 0,
Gid: 0,
Path: "/dev/zero",
FileMode: 0o666,
Uid: 0,
Gid: 0,
IsDefault: true,
Rule: devices.Rule{
Type: devices.CharDevice,
Major: 1,
Expand All @@ -284,10 +291,11 @@ var AllowedDevices = []*devices.Device{
},
},
{
Path: "/dev/urandom",
FileMode: 0o666,
Uid: 0,
Gid: 0,
Path: "/dev/urandom",
FileMode: 0o666,
Uid: 0,
Gid: 0,
IsDefault: true,
Rule: devices.Rule{
Type: devices.CharDevice,
Major: 1,
Expand All @@ -298,6 +306,7 @@ var AllowedDevices = []*devices.Device{
},
// /dev/pts/ - pts namespaces are "coming soon"
{
IsDefault: true,
Rule: devices.Rule{
Type: devices.CharDevice,
Major: 136,
Expand All @@ -307,6 +316,7 @@ var AllowedDevices = []*devices.Device{
},
},
{
IsDefault: true,
Rule: devices.Rule{
Type: devices.CharDevice,
Major: 5,
Expand Down Expand Up @@ -380,12 +390,14 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) {
config.Mounts = append(config.Mounts, cm)
}

defaultDevs, err := createDevices(spec, config)
err = createDevices(spec, config)
if err != nil {
return nil, err
}

c, err := CreateCgroupConfig(opts, defaultDevs)
defaultAllowedDevices := CreateDefaultDevicesCgroups(config)

c, err := CreateCgroupConfig(opts, defaultAllowedDevices)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -692,6 +704,48 @@ func initSystemdProps(spec *specs.Spec) ([]systemdDbus.Property, error) {
return sp, nil
}

func CreateCgroupDeviceConfig(r *configs.Resources, specr *specs.LinuxResources, defaultDevs []*devices.Device) error {
if specr != nil {
for i, d := range specr.Devices {
var (
t = "a"
major = int64(-1)
minor = int64(-1)
)
if d.Type != "" {
t = d.Type
}
if d.Major != nil {
major = *d.Major
}
if d.Minor != nil {
minor = *d.Minor
}
if d.Access == "" {
return fmt.Errorf("device access at %d field cannot be empty", i)
}
dt, err := stringToCgroupDeviceRune(t)
if err != nil {
return err
}
r.Devices = append(r.Devices, &devices.Rule{
Type: dt,
Major: major,
Minor: minor,
Permissions: devices.Permissions(d.Access),
Allow: d.Allow,
})
}
}

// Append the default allowed devices to the end of the list.
for _, device := range defaultDevs {
r.Devices = append(r.Devices, &device.Rule)
}

return nil
}

func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*devices.Device) (*configs.Cgroup, error) {
var (
myCgroupPath string
Expand Down Expand Up @@ -748,39 +802,10 @@ func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*devices.Device) (*confi

// In rootless containers, any attempt to make cgroup changes is likely to fail.
// libcontainer will validate this but ignores the error.
var r *specs.LinuxResources
if spec.Linux != nil {
r := spec.Linux.Resources
r = spec.Linux.Resources
if r != nil {
for i, d := range r.Devices {
var (
t = "a"
major = int64(-1)
minor = int64(-1)
)
if d.Type != "" {
t = d.Type
}
if d.Major != nil {
major = *d.Major
}
if d.Minor != nil {
minor = *d.Minor
}
if d.Access == "" {
return nil, fmt.Errorf("device access at %d field cannot be empty", i)
}
dt, err := stringToCgroupDeviceRune(t)
if err != nil {
return nil, err
}
c.Resources.Devices = append(c.Resources.Devices, &devices.Rule{
Type: dt,
Major: major,
Minor: minor,
Permissions: devices.Permissions(d.Access),
Allow: d.Allow,
})
}
if r.Memory != nil {
if r.Memory.Limit != nil {
c.Resources.Memory = *r.Memory.Limit
Expand Down Expand Up @@ -906,12 +931,13 @@ func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*devices.Device) (*confi
}
}
}
}

// Append the default allowed devices to the end of the list.
for _, device := range defaultDevs {
c.Resources.Devices = append(c.Resources.Devices, &device.Rule)
err := CreateCgroupDeviceConfig(c.Resources, r, defaultDevs)
if err != nil {
return nil, err
}
}

return c, nil
}

Expand Down Expand Up @@ -941,21 +967,22 @@ func stringToDeviceRune(s string) (devices.Type, error) {
}
}

func createDevices(spec *specs.Spec, config *configs.Config) ([]*devices.Device, error) {
func createDevices(spec *specs.Spec, config *configs.Config) error {
// If a spec device is redundant with a default device, remove that default
// device (the spec one takes priority).
dedupedAllowDevs := []*devices.Device{}

next:
for _, ad := range AllowedDevices {
if ad.Path != "" && spec.Linux != nil {
if ad.Path == "" {
continue next
}

if spec.Linux != nil {
for _, sd := range spec.Linux.Devices {
if sd.Path == ad.Path {
continue next
}
}
}
dedupedAllowDevs = append(dedupedAllowDevs, ad)
if ad.Path != "" {
config.Devices = append(config.Devices, ad)
}
Expand All @@ -975,7 +1002,7 @@ next:
}
dt, err := stringToDeviceRune(d.Type)
if err != nil {
return nil, err
return err
}
if d.FileMode != nil {
filemode = *d.FileMode &^ unix.S_IFMT
Expand All @@ -995,7 +1022,24 @@ next:
}
}

return dedupedAllowDevs, nil
return nil
}

func CreateDefaultDevicesCgroups(config *configs.Config) []*devices.Device {
defaultAllowedDevices := []*devices.Device{}
next:
for _, ad := range AllowedDevices {
if ad.Path != "" {
for _, device := range config.Devices {
if ad.Path == device.Path && !device.IsDefault {
continue next
}
}
}
defaultAllowedDevices = append(defaultAllowedDevices, ad)
}

return defaultAllowedDevices
}

func setupUserNamespace(spec *specs.Spec, config *configs.Config) error {
Expand Down
8 changes: 4 additions & 4 deletions libcontainer/specconv/spec_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -886,17 +886,17 @@ func TestCreateDevices(t *testing.T) {

conf := &configs.Config{}

defaultDevs, err := createDevices(spec, conf)
err := createDevices(spec, conf)
if err != nil {
t.Errorf("failed to create devices: %v", err)
}

// Verify the returned default devices has the /dev/tty entry deduplicated
// Verify the returned devices has the /dev/tty entry deduplicated
found := false
for _, d := range defaultDevs {
for _, d := range conf.Devices {
if d.Path == "/dev/tty" {
if found {
t.Errorf("createDevices failed: returned a duplicated device entry: %v", defaultDevs)
t.Errorf("createDevices failed: returned a duplicated device entry: %v", conf.Devices)
}
found = true
}
Expand Down
Loading
Loading