From 7a0302f0d79e32766e28647cfae0fba70e6f9dea Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Sun, 25 Jul 2021 19:20:42 -0700 Subject: [PATCH 1/2] runc init: simplify runc init is special. For one thing, it needs to do a few things before main(), so we have func init() that checks if we're init and does that. What happens next is main() is called, which does some options parsing, figures out it needs to call initCommand.Action and so it does. Now, main() is entirely unnecessary -- we can do everything right from init(). Hopefully the change makes things slightly less complicated. From a user's perspective, the only change is runc help no longer lists 'runc init` (which I think it also good). Signed-off-by: Kir Kolyshkin --- init.go | 11 +++-------- main.go | 6 +----- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/init.go b/init.go index 59ce1e8c739..d39492952a3 100644 --- a/init.go +++ b/init.go @@ -10,11 +10,12 @@ import ( "github.com/opencontainers/runc/libcontainer/logs" _ "github.com/opencontainers/runc/libcontainer/nsenter" "github.com/sirupsen/logrus" - "github.com/urfave/cli" ) func init() { if len(os.Args) > 1 && os.Args[1] == "init" { + // This is the golang entry point for runc init, executed + // before main() but after libcontainer/nsenter's nsexec(). runtime.GOMAXPROCS(1) runtime.LockOSThread() @@ -38,13 +39,7 @@ func init() { panic(fmt.Sprintf("libcontainer: failed to configure logging: %v", err)) } logrus.Debug("child process in init()") - } -} -var initCommand = cli.Command{ - Name: "init", - Usage: `initialize the namespaces and launch the process (do not call it outside of runc)`, - Action: func(context *cli.Context) error { factory, _ := libcontainer.New("") if err := factory.StartInitialization(); err != nil { // as the error is sent back to the parent there is no need to log @@ -52,5 +47,5 @@ var initCommand = cli.Command{ os.Exit(1) } panic("libcontainer: container init failed to exec") - }, + } } diff --git a/main.go b/main.go index 45652ab327d..f141e79b443 100644 --- a/main.go +++ b/main.go @@ -119,7 +119,6 @@ func main() { deleteCommand, eventsCommand, execCommand, - initCommand, killCommand, listCommand, pauseCommand, @@ -149,10 +148,7 @@ func main() { if err := reviseRootDir(context); err != nil { return err } - // let init configure logging on its own - if args := context.Args(); args != nil && args.First() == "init" { - return nil - } + return logs.ConfigureLogging(createLogConfig(context)) } From 5110bd2fc026c0a9849e35322d5b81a3f28c716d Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 26 Jul 2021 10:31:06 -0700 Subject: [PATCH 2/2] nsenter: remove cgroupns sync mechanism As pointed out in TODO item added by commit 64bb59f59, it is not necessary to have a special sync mechanism for cgroupns, as the parent adds runc init to cgroup way earlier (before sending nl bootstrap data. This sync was added by commit df3fa115f974, which was also added a second cgroup manager.Apply() call, later removed in commit d1ba8e39f8c7512. It seems the original author had the idea to wait for that second Apply(). Fixes: df3fa115f974 Signed-off-by: Kir Kolyshkin --- libcontainer/nsenter/nsexec.c | 25 ++----------------------- libcontainer/process_linux.go | 11 ----------- 2 files changed, 2 insertions(+), 34 deletions(-) diff --git a/libcontainer/nsenter/nsexec.c b/libcontainer/nsenter/nsexec.c index 8199faf3a01..49e8f54b6a4 100644 --- a/libcontainer/nsenter/nsexec.c +++ b/libcontainer/nsenter/nsexec.c @@ -41,12 +41,6 @@ enum sync_t { SYNC_CHILD_FINISH = 0x45, /* The child or grandchild has finished. */ }; -/* - * Synchronisation value for cgroup namespace setup. - * The same constant is defined in process_linux.go as "createCgroupns". - */ -#define CREATECGROUPNS 0x80 - #define STAGE_SETUP -1 /* longjmp() arguments. */ #define STAGE_PARENT 0 @@ -1075,24 +1069,9 @@ void nsexec(void) bail("setgroups failed"); } - /* - * Wait until our topmost parent has finished cgroup setup in - * p.manager.Apply(). - * - * TODO(cyphar): Check if this code is actually needed because we - * should be in the cgroup even from stage-0, so - * waiting until now might not make sense. - */ if (config.cloneflags & CLONE_NEWCGROUP) { - uint8_t value; - if (read(pipenum, &value, sizeof(value)) != sizeof(value)) - bail("read synchronisation value failed"); - if (value == CREATECGROUPNS) { - write_log(DEBUG, "unshare cgroup namespace"); - if (unshare(CLONE_NEWCGROUP) < 0) - bail("failed to unshare cgroup namespace"); - } else - bail("received unknown synchronisation value"); + if (unshare(CLONE_NEWCGROUP) < 0) + bail("failed to unshare cgroup namespace"); } write_log(DEBUG, "signal completion to stage-0"); diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go index 490a7c8d702..e1649876b1d 100644 --- a/libcontainer/process_linux.go +++ b/libcontainer/process_linux.go @@ -25,10 +25,6 @@ import ( "golang.org/x/sys/unix" ) -// Synchronisation value for cgroup namespace setup. -// The same constant is defined in nsexec.c as "CREATECGROUPNS". -const createCgroupns = 0x80 - type parentProcess interface { // pid returns the pid for the running process. pid() int @@ -411,13 +407,6 @@ func (p *initProcess) start() (retErr error) { } p.setExternalDescriptors(fds) - // Now it's time to setup cgroup namesapce - if p.config.Config.Namespaces.Contains(configs.NEWCGROUP) && p.config.Config.Namespaces.PathOf(configs.NEWCGROUP) == "" { - if _, err := p.messageSockPair.parent.Write([]byte{createCgroupns}); err != nil { - return fmt.Errorf("error sending synchronization value to init process: %w", err) - } - } - // Wait for our first child to exit if err := p.waitForChildExit(childPid); err != nil { return fmt.Errorf("error waiting for our first child to exit: %w", err)