Don't deny all devices when update cgroup resource #2205
Conversation
libcontainer/cgroups/fs/devices.go
Outdated
| @@ -78,3 +119,22 @@ func (s *DevicesGroup) Remove(d *cgroupData) error { | |||
| func (s *DevicesGroup) GetStats(path string, stats *cgroups.Stats) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func readDevicesExceptDefault(path string) (allowed map[string]Empty, err error) { | |||
| cgroupData, err := readFile(path, "devices.list") | |||
There was a problem hiding this comment.
the same check won't be possible with cgroup v2.
Would it be possible to detect whether it is updating an existing cgroup in another way and return early if cgroup.Resources.Devices is not defined?
There was a problem hiding this comment.
the same check won't be possible with cgroup v2.
Would it be possible to detect whether it is updating an existing cgroup in another way and return early if
cgroup.Resources.Devicesis not defined?
Cgroup v2 will use devices_v2.go to set configuration, so readDevicesExceptDefault won't affect cgroup v2. I'll check if cgroup v2 has interface to query allowed devices.
For current design, I think the answer is no. Because the update function only set cpu, disk or memory, but cgroup manager only support Set interface. This will set and replace all subsystem result.
There was a problem hiding this comment.
@giuseppe I checked cgroup v2 and found that cgroup v2 will not raise Operation not permitted when invoking update function. So I think this patch is only used for cgroup v1
Signed-off-by: mYmNeo <[email protected]>
|
@giuseppe PTAL again? |
| if len(file) > 0 { | ||
| if err := fscommon.WriteFile(path, file, dev.CgroupString()); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
| if cgroup.Resources.AllowAllDevices != nil { | ||
| if *cgroup.Resources.AllowAllDevices == false { |
There was a problem hiding this comment.
This isn't directly related to this patch (and is something we should fix separately), but I've hit this wall while trying to review it.
I've spent a while now trying to understand what the hell this whole AllowAllDevices, AllowedDevices and DeniedDevices stuff is supposed to do and how it interacts with Devices. It's been around forever in runc, isn't exposed in the runtime-spec, and isn't even generated by specconv. It appears to be only used for unit testing itself, which seems like a senseless thing to test. Honestly, this patch and many others would be much nicer if we just nuked this from orbit and just had Devices. When the hell would you ever not want AllowAllDevices == false!?
In the context of this patch, I'm not even sure how to review whether the semantics of the below code are correct -- if I'm correct in my understanding that nothing uses the code, how do we know if the changes make sense...
@crosbymichael Am I missing something? Will something really bad happen if we get rid of it? I remember the initProcessTime nightmare when we changed the type of a field in the libcontainer state JSON (which resulted in the whole "migration" fun) -- would something similar happen if we just purged this concept from the codebase?
@AkihiroSuda WDYT? (Since you were the last person to touch it, and I think @crosbymichael and I last looked at this code in 2016.)
| // For the second time set, we don't deny all devices | ||
| if dev.Type == defaultDevice.Type && len(oldAllowedDevices) != 0 { | ||
| file = "" | ||
| } |
There was a problem hiding this comment.
I really don't like this, and after looking at it for a bit I think I'm convinced it isn't right. This code will ignore any update that contains But even if this check was a -- but there are valid use-cases for a other than "deny everything" (you can allow both character and block devices for a particular major number, for instance).dev == defaultDevice it'd still be wrong since you can also allow all devices (though you shouldn't) -- and that would be ignored.
I think the issue is that the caller shouldn't be specifying a default deny rule in the list which we then need to filter out -- that should be done as part of this module. So we figure out what rules we need to specify.
EDIT: My comment about a was wrong, I forgot that a always means everything, and that you can't specify both block and character devices.
| newAllowedDevices := make(map[string]Empty) | ||
| for _, dev := range cgroup.AllowedDevices { | ||
| newAllowedDevices[dev.CgroupString()] = Empty{} | ||
| } |
There was a problem hiding this comment.
I think this should loop over cgroup.Devices given my previous point about how AllowedDevices is actually not used by anything. But as I said, most of this logic needs to be torched -- I just wrote a very long block of text about how this patch doesn't handle denying no longer allowed devices, only to realise that this code block actually is used, and in theory does handle that problem...
|
After looking at this patch and the cgroup code, I've decided I'm just going to do a big cleanup and include a fix for this problem as part of it. Thanks for the head-start @mYmNeo -- and my apologies, as this really isn't the best bit of the codebase to touch if you want to have a fun time. |
Looking forward to the fix patch. |
|
#2391 is the follow-up for this PR. |
Resolve #2204.
Signed-off-by: mYmNeo [email protected]