Treat EROFS in cgroups setup as skippable error #1657
Conversation
|
Since it changes your code, @williammartin, can you take a look at this PR? |
|
@rutsky Your summary of why we do the permission check for rootless containers is correct and it doesn't look like this PR will break that. I'm not super sure about the EROFS check logic and the expected behaviour though because I'm not familiar with Docker. If the whole cgroup tree is mounted read-only inside a docker container, how do non-rootless containers work? The only thing I'd note is that |
This PR is about inside a container, where cgroupfs is mounted as ro -- it's rw on the host. |
|
Yeh I got that, but I think I answered my own question which is that the referenced issue #1658 is dealing with unprivileged docker containers, so rootless containers inside are the only thing that would ever work anyways. |
libcontainer/cgroups/fs/apply_raw.go
Outdated
| @@ -144,8 +145,24 @@ func (m *Manager) Apply(pid int) (err error) { | |||
| } | |||
| m.Paths[sys.Name()] = p | |||
|
|
|||
| isPermissionError := func(err error) bool { | |||
There was a problem hiding this comment.
Please define this as an actual function (preferably called isIgnorableError) rather than using the a := func(...) {...} form.
Currently cgroup setup ignores permissions errors. AFAIK this helps with rootless containers: if user have permissions to change cgroups, it will have them setup, otherwise rootless container will use parent process cgroups (as it was with rootless containers before cgroups support was introduced). If cgroup is mounted in read-only mode (e.g. inside Docker container), operations will return not permission error, but EROFS - this patch treats EROFS as skippable error in cgroups setup. Signed-off-by: Vladimir Rutsky <[email protected]>
rutsky
force-pushed
the
treat-EROFS-as-skippable-error
branch
from
7c8b8ec to
05d4d1d
Compare
|
@williammartin thanks for the review! Relatively offtopic to this PR question: what if cgroups setup will fail due to permission error on non-rootless container? E.g. if runc will be run as root and due to some reasons cgroup setup will return permission error (not sure if this is possible, but also I don't see why this is impossible), wouldn't this lead to security issues or unconstrained (in terms of resources) processes inside container? |
|
@rutsky That is something that (I think?) I brought up in #1540 as a point of concern, but the only places where (currently) you'll get an EPERM in However, I do agree we should only use |
|
@cyphar Could you set |
|
Done. I will go through a proper review of all the |
|
This change looks okay in principle, but I think that (harrowing back to my comments in #1540) we should be only applying the @rutsky I can carry this PR for you and do it, if you are no longer interested in updating this. |
|
Closing in favour of #1759 which carries this. |
Currently cgroup setup ignores permissions errors.
AFAIK this helps with rootless containers: if user have permissions to
change cgroups, it will have them setup, otherwise rootless container
will use parent process cgroups (as it was with rootless containers
before cgroups cgroups support was introduced).
If cgroup is mounted in read-only mode (e.g. inside Docker container),
operations will return not permission error, but EROFS - this patch
treats EROFS as skippable error in cgroups setup.
Signed-off-by: Vladimir Rutsky [email protected]