libct: prepareCgroupFD: fall back to container init cgroup

[kolyshkin]

Previously, when prepareCgroupFD would not open container's cgroup
(as configured in config.json and saved to state.json), it returned
a fatal error, as we presumed a container can't exist without its own
cgroup.

Apparently, it can. In a case when container is configured without
cgroupns (i.e. it uses hosts cgroups), and /sys/fs/cgroup is mounted
read-write, a rootful container's init can move itself to an entirely
different cgroup (even a new one that it just created), and then the
original container cgroup is removed by the kernel (or systemd?) as
it has no processes left. By the way, from the systemd point of view
the container is gone. And yet it is still there, and users want
runc exec to work!

And it worked, thanks to the "let's try container init's cgroup"
fallback as added by commit c91fe9a ("cgroup2: exec: join the
cgroup of the init process on EBUSY"). The fallback was added for
the entirely different reason, but it happened to work in this very
case, too.

This behavior was broken with the introduction of CLONE_INTO_CGROUP
support.

While it is debatable whether this is a valid scenario when a container
moves itself into a different cgroup, this very setup is used by e.g.
buildkitd running in a privileged kubernetes container (see issue #5089).

To restore the way things are expected to work, add the same "try
container init's cgroup" fallback into prepareCgroupFD.

A test case (reproducing issue in #5089) is added. It fails before
the fix (see #5102) and succeeds here.

Fixes: #5089.

[kolyshkin]

TODO: add an integration test case.

[kolyshkin]

Added a test case. Testing it fails (w/o a fix) in #5102

[rata]

Thanks for tackling this! I'm fine with the general approach, just some nits about the code itself.

[rata]

This mostly LGTM, thanks for so many iterations! :)

[lifubang]

Hi, there’s a minor performance point — feel free to ignore if it’s not worth the change. The fallback path currently triggers two calls to initProcessCgroupPath. Would it make sense to add a small cache to avoid the duplicate initialization?
For example:

diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go
index 7926578d..38a14604 100644
--- a/libcontainer/process_linux.go
+++ b/libcontainer/process_linux.go
@@ -174,6 +174,7 @@ type setnsProcess struct {
        rootlessCgroups bool
        intelRdtPath    string
        initProcessPid  int
+       initCgroupPath  string // cached container init's cgroup path
 }
 
 // tryResetCPUAffinity tries to reset the CPU affinity of the process
@@ -300,6 +301,11 @@ func (p *setnsProcess) addIntoCgroupV1() error {
 //  2. A container init process with no cgroupns and /sys/fs/cgroup rw access
 //     may move itself to any other cgroup, and the original cgroup will disappear.
 func (p *setnsProcess) initProcessCgroupPath() string {
+       // return cached value if available
+       if p.initCgroupPath != "" {
+               return p.initCgroupPath
+       }
+
        if p.initProcessPid == 0 || !cgroups.IsCgroup2UnifiedMode() {
                return ""
        }
@@ -313,7 +319,9 @@ func (p *setnsProcess) initProcessCgroupPath() string {
                return ""
        }
 
-       return fs2.UnifiedMountpoint + cgroup
+       // cache the result
+       p.initCgroupPath = fs2.UnifiedMountpoint + cgroup
+       return p.initCgroupPath
 }
 
 func (p *setnsProcess) addIntoCgroupV2() error {

[kolyshkin]

I think the possibility of using initCgroupPath for the second time is low, as it needs a cgroup v2 host running kernel older than v5.7 (or clone3 being disabled via e.g. seccomp), which should be very rare.

[rata]

LGTM, thanks a lot for fixing this!

Left two very minor cosmetic things. I don't think another review is needed even if you change them :)

[rata]

Super nit and feel free to ignore. But I don't really like the C89 style of declaring variables at the beginning (and since C99 already is not needed, almost 30 years ago :D). I think doing this before the if is more clear:

path := p.initProcessCgroupPath()
if path == "" { ....

And I also think it is clearer to not assign inside the if, when the variable is not scoped to the if. IMHO the code is more readable like that.

But again, this is a complete nit and personal preference, feel free to ignore :)

[kolyshkin]

@rata as to the first point (of not using var path string), alas this won't work because there's a goto above. Here's an error which I'm avoding:

          if sub != "" {
                  goto fail // ERROR: goto fail jumps over variable declaration at line 331
          }
          path := p.initProcessCgroupPath()                                                           
          if path == "" {
                  goto fail
          }
...
fail:
...

Second point is valid but subjective. I see if a = func(); a != nil not as a way to scope a, but as a way to write more compact code. You see it as a way to scope a (while in fact it's not as we're not using := here).

I can see a value though; changed to your liking :)

[rata]

And here too, if you like this pattern more (spaces probably broken, sorry :/):

Suggested change

	if cgroup = p.initProcessCgroupPath(); cgroup == "" {
	cgroup = p.initProcessCgroupPath();
	if cgroup == "" {

Or just ignore too :)

[kolyshkin]

1.4 backport: #5117