merge pr #3825

Kir Kolyshkin (7):
  libct: implement support for cgroup.kill
  runc kill: drop -a option
  libct: move killing logic to container.Signal
  libct: fix shared pidns detection
  libct: signalAllProcesses: remove child reaping
  tests/int/kill: add kill -a with host pidns test
  tests/rootless.sh: drop set -x

LGTMs: AkihiroSuda cyphar
Closes #3825

delete.go

@@ -14,10 +14,10 @@ import (
 )
 
 func killContainer(container *libcontainer.Container) error {
-	_ = container.Signal(unix.SIGKILL, false)
+	_ = container.Signal(unix.SIGKILL)
 	for i := 0; i < 100; i++ {
 		time.Sleep(100 * time.Millisecond)
-		if err := container.Signal(unix.Signal(0), false); err != nil {
+		if err := container.Signal(unix.Signal(0)); err != nil {
 			destroy(container)
 			return nil
 		}

kill.go

@@ -1,10 +1,12 @@
 package main
 
 import (
+	"errors"
 	"fmt"
 	"strconv"
 	"strings"
 
+	"github.com/opencontainers/runc/libcontainer"
 	"github.com/urfave/cli"
 	"golang.org/x/sys/unix"
 )
@@ -24,8 +26,9 @@ signal to the init process of the "ubuntu01" container:
        # runc kill ubuntu01 KILL`,
 	Flags: []cli.Flag{
 		cli.BoolFlag{
-			Name:  "all, a",
-			Usage: "send the specified signal to all processes inside the container",
+			Name:   "all, a",
+			Usage:  "(obsoleted, do not use)",
+			Hidden: true,
 		},
 	},
 	Action: func(context *cli.Context) error {
@@ -49,7 +52,11 @@ signal to the init process of the "ubuntu01" container:
 		if err != nil {
 			return err
 		}
-		return container.Signal(signal, context.Bool("all"))
+		err = container.Signal(signal)
+		if errors.Is(err, libcontainer.ErrNotRunning) && context.Bool("all") {
+			err = nil
+		}
+		return err
 	},
 }
 

libcontainer/configs/namespaces_syscall.go

@@ -31,3 +31,15 @@ func (n *Namespaces) CloneFlags() uintptr {
 	}
 	return uintptr(flag)
 }
+
+// IsPrivate tells whether the namespace of type t is configured as private
+// (i.e. it exists and is not shared).
+func (n Namespaces) IsPrivate(t NamespaceType) bool {
+	for _, v := range n {
+		if v.Type == t {
+			return v.Path == ""
+		}
+	}
+	// Not found, so implicitly sharing a parent namespace.
+	return false
+}

libcontainer/container_linux.go

@@ -357,37 +357,47 @@ func (c *Container) start(process *Process) (retErr error) {
 	return nil
 }
 
-func (c *Container) Signal(s os.Signal, all bool) error {
+// Signal sends a specified signal to container's init.
+//
+// When s is SIGKILL and the container does not have its own PID namespace, all
+// the container's processes are killed. In this scenario, the libcontainer
+// user may be required to implement a proper child reaper.
+func (c *Container) Signal(s os.Signal) error {
 	c.m.Lock()
 	defer c.m.Unlock()
 	status, err := c.currentStatus()
 	if err != nil {
 		return err
 	}
-	if all {
-		if status == Stopped && !c.cgroupManager.Exists() {
-			// Avoid calling signalAllProcesses which may print
-			// a warning trying to freeze a non-existing cgroup.
-			return nil
-		}
-		return c.ignoreCgroupError(signalAllProcesses(c.cgroupManager, s))
+	// To avoid a PID reuse attack, don't kill non-running container.
+	switch status {
+	case Running, Created, Paused:
+	default:
+		return ErrNotRunning
 	}
-	// to avoid a PID reuse attack
-	if status == Running || status == Created || status == Paused {
-		if err := c.initProcess.signal(s); err != nil {
-			return fmt.Errorf("unable to signal init: %w", err)
-		}
-		if status == Paused {
-			// For cgroup v1, killing a process in a frozen cgroup
-			// does nothing until it's thawed. Only thaw the cgroup
-			// for SIGKILL.
-			if s, ok := s.(unix.Signal); ok && s == unix.SIGKILL {
-				_ = c.cgroupManager.Freeze(configs.Thawed)
-			}
-		}
-		return nil
+
+	// When a container has its own PID namespace, inside it the init PID
+	// is 1, and thus it is handled specially by the kernel. In particular,
+	// killing init with SIGKILL from an ancestor namespace will also kill
+	// all other processes in that PID namespace (see pid_namespaces(7)).
+	//
+	// OTOH, if PID namespace is shared, we should kill all pids to avoid
+	// leftover processes.
+	if s == unix.SIGKILL && !c.config.Namespaces.IsPrivate(configs.NEWPID) {
+		err = signalAllProcesses(c.cgroupManager, unix.SIGKILL)
+	} else {
+		err = c.initProcess.signal(s)
 	}
-	return ErrNotRunning
+	if err != nil {
+		return fmt.Errorf("unable to signal init: %w", err)
+	}
+	if status == Paused && s == unix.SIGKILL {
+		// For cgroup v1, killing a process in a frozen cgroup
+		// does nothing until it's thawed. Only thaw the cgroup
+		// for SIGKILL.
+		_ = c.cgroupManager.Freeze(configs.Thawed)
+	}
+	return nil
 }
 
 func (c *Container) createExecFifo() error {
@@ -539,7 +549,6 @@ func (c *Container) newInitProcess(p *Process, cmd *exec.Cmd, messageSockPair, l
 			nsMaps[ns.Type] = ns.Path
 		}
 	}
-	_, sharePidns := nsMaps[configs.NEWPID]
 	data, err := c.bootstrapData(c.config.Namespaces.CloneFlags(), nsMaps, initStandard)
 	if err != nil {
 		return nil, err
@@ -584,7 +593,6 @@ func (c *Container) newInitProcess(p *Process, cmd *exec.Cmd, messageSockPair, l
 		container:       c,
 		process:         p,
 		bootstrapData:   data,
-		sharePidns:      sharePidns,
 	}
 	c.initProcess = init
 	return init, nil
@@ -687,8 +695,7 @@ func (c *Container) newInitConfig(process *Process) *initConfig {
 	return cfg
 }
 
-// Destroy destroys the container, if its in a valid state, after killing any
-// remaining running processes.
+// Destroy destroys the container, if its in a valid state.
 //
 // Any event registrations are removed before the container is destroyed.
 // No error is returned if the container is already destroyed.

libcontainer/init_linux.go

@@ -11,7 +11,6 @@ import (
 	"runtime/debug"
 	"strconv"
 	"strings"
-	"unsafe"
 
 	"github.com/containerd/console"
 	"github.com/opencontainers/runtime-spec/specs-go"
@@ -575,39 +574,20 @@ func setupRlimits(limits []configs.Rlimit, pid int) error {
 	return nil
 }
 
-const _P_PID = 1
-
-//nolint:structcheck,unused
-type siginfo struct {
-	si_signo int32
-	si_errno int32
-	si_code  int32
-	// below here is a union; si_pid is the only field we use
-	si_pid int32
-	// Pad to 128 bytes as detailed in blockUntilWaitable
-	pad [96]byte
-}
-
-// isWaitable returns true if the process has exited false otherwise.
-// Its based off blockUntilWaitable in src/os/wait_waitid.go
-func isWaitable(pid int) (bool, error) {
-	si := &siginfo{}
-	_, _, e := unix.Syscall6(unix.SYS_WAITID, _P_PID, uintptr(pid), uintptr(unsafe.Pointer(si)), unix.WEXITED|unix.WNOWAIT|unix.WNOHANG, 0, 0)
-	if e != 0 {
-		return false, &os.SyscallError{Syscall: "waitid", Err: e}
-	}
-
-	return si.si_pid != 0, nil
-}
-
 // signalAllProcesses freezes then iterates over all the processes inside the
 // manager's cgroups sending the signal s to them.
-// If s is SIGKILL and subreaper is not enabled then it will wait for each
-// process to exit.
-// For all other signals it will check if the process is ready to report its
-// exit status and only if it is will a wait be performed.
-func signalAllProcesses(m cgroups.Manager, s os.Signal) error {
-	var procs []*os.Process
+func signalAllProcesses(m cgroups.Manager, s unix.Signal) error {
+	// Use cgroup.kill, if available.
+	if s == unix.SIGKILL {
+		if p := m.Path(""); p != "" { // Either cgroup v2 or hybrid.
+			err := cgroups.WriteFile(p, "cgroup.kill", "1")
+			if err == nil || !errors.Is(err, os.ErrNotExist) {
+				return err
+			}
+			// Fallback to old implementation.
+		}
+	}
+
 	if err := m.Freeze(configs.Frozen); err != nil {
 		logrus.Warn(err)
 	}
@@ -619,55 +599,14 @@ func signalAllProcesses(m cgroups.Manager, s os.Signal) error {
 		return err
 	}
 	for _, pid := range pids {
-		p, err := os.FindProcess(pid)
-		if err != nil {
-			logrus.Warn(err)
-			continue
-		}
-		procs = append(procs, p)
-		if err := p.Signal(s); err != nil {
-			logrus.Warn(err)
+		err := unix.Kill(pid, s)
+		if err != nil && err != unix.ESRCH { //nolint:errorlint // unix errors are bare
+			logrus.Warnf("kill %d: %v", pid, err)
 		}
 	}
 	if err := m.Freeze(configs.Thawed); err != nil {
 		logrus.Warn(err)
 	}
 
-	subreaper, err := system.GetSubreaper()
-	if err != nil {
-		// The error here means that PR_GET_CHILD_SUBREAPER is not
-		// supported because this code might run on a kernel older
-		// than 3.4. We don't want to throw an error in that case,
-		// and we simplify things, considering there is no subreaper
-		// set.
-		subreaper = 0
-	}
-
-	for _, p := range procs {
-		if s != unix.SIGKILL {
-			if ok, err := isWaitable(p.Pid); err != nil {
-				if !errors.Is(err, unix.ECHILD) {
-					logrus.Warn("signalAllProcesses: ", p.Pid, err)
-				}
-				continue
-			} else if !ok {
-				// Not ready to report so don't wait
-				continue
-			}
-		}
-
-		// In case a subreaper has been setup, this code must not
-		// wait for the process. Otherwise, we cannot be sure the
-		// current process will be reaped by the subreaper, while
-		// the subreaper might be waiting for this process in order
-		// to retrieve its exit code.
-		if subreaper == 0 {
-			if _, err := p.Wait(); err != nil {
-				if !errors.Is(err, unix.ECHILD) {
-					logrus.Warn("wait: ", err)
-				}
-			}
-		}
-	}
 	return nil
 }

libcontainer/integration/exec_test.go

@@ -1355,16 +1355,26 @@ func TestPIDHost(t *testing.T) {
 	}
 }
 
-func TestPIDHostInitProcessWait(t *testing.T) {
+func TestHostPidnsInitKill(t *testing.T) {
+	config := newTemplateConfig(t, nil)
+	// Implicitly use host pid ns.
+	config.Namespaces.Remove(configs.NEWPID)
+	testPidnsInitKill(t, config)
+}
+
+func TestSharedPidnsInitKill(t *testing.T) {
+	config := newTemplateConfig(t, nil)
+	// Explicitly use host pid ns.
+	config.Namespaces.Add(configs.NEWPID, "/proc/1/ns/pid")
+	testPidnsInitKill(t, config)
+}
+
+func testPidnsInitKill(t *testing.T, config *configs.Config) {
 	if testing.Short() {
 		return
 	}
 
-	pidns := "/proc/1/ns/pid"
-
 	// Run a container with two long-running processes.
-	config := newTemplateConfig(t, nil)
-	config.Namespaces.Add(configs.NEWPID, pidns)
 	container, err := newContainer(t, config)
 	ok(t, err)
 	defer func() {
@@ -1373,7 +1383,7 @@ func TestPIDHostInitProcessWait(t *testing.T) {
 
 	process1 := &libcontainer.Process{
 		Cwd:  "/",
-		Args: []string{"sleep", "100"},
+		Args: []string{"sleep", "1h"},
 		Env:  standardEnvironment,
 		Init: true,
 	}
@@ -1382,25 +1392,26 @@ func TestPIDHostInitProcessWait(t *testing.T) {
 
 	process2 := &libcontainer.Process{
 		Cwd:  "/",
-		Args: []string{"sleep", "100"},
+		Args: []string{"sleep", "1h"},
 		Env:  standardEnvironment,
 		Init: false,
 	}
 	err = container.Run(process2)
 	ok(t, err)
 
-	// Kill the init process and Wait for it.
-	err = process1.Signal(syscall.SIGKILL)
+	// Kill the container.
+	err = container.Signal(syscall.SIGKILL)
 	ok(t, err)
 	_, err = process1.Wait()
 	if err == nil {
 		t.Fatal("expected Wait to indicate failure")
 	}
 
-	// The non-init process must've been killed.
-	err = process2.Signal(syscall.Signal(0))
-	if err == nil || err.Error() != "no such process" {
-		t.Fatalf("expected process to have been killed: %v", err)
+	// The non-init process must've also been killed. If not,
+	// the test will time out.
+	_, err = process2.Wait()
+	if err == nil {
+		t.Fatal("expected Wait to indicate failure")
 	}
 }
 

libcontainer/process_linux.go

@@ -304,7 +304,6 @@ type initProcess struct {
 	fds             []string
 	process         *Process
 	bootstrapData   io.Reader
-	sharePidns      bool
 }
 
 func (p *initProcess) pid() int {
@@ -616,10 +615,6 @@ func (p *initProcess) start() (retErr error) {
 
 func (p *initProcess) wait() (*os.ProcessState, error) {
 	err := p.cmd.Wait()
-	// we should kill all processes in cgroup when init is died if we use host PID namespace
-	if p.sharePidns {
-		_ = signalAllProcesses(p.manager, unix.SIGKILL)
-	}
 	return p.cmd.ProcessState, err
 }
 

libcontainer/state_linux.go

@@ -7,7 +7,6 @@ import (
 
 	"github.com/opencontainers/runc/libcontainer/configs"
 	"github.com/opencontainers/runtime-spec/specs-go"
-	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
 )
 
@@ -36,12 +35,6 @@ type containerState interface {
 }
 
 func destroy(c *Container) error {
-	if !c.config.Namespaces.Contains(configs.NEWPID) ||
-		c.config.Namespaces.PathOf(configs.NEWPID) != "" {
-		if err := signalAllProcesses(c.cgroupManager, unix.SIGKILL); err != nil {
-			logrus.Warn(err)
-		}
-	}
 	err := c.cgroupManager.Destroy()
 	if c.intelRdtManager != nil {
 		if ierr := c.intelRdtManager.Destroy(); err == nil {

man/runc-kill.8.md

@@ -4,7 +4,7 @@
 **runc-kill** - send a specified signal to container
 
 # SYNOPSIS
-**runc kill** [**--all**|**-a**] _container-id_ [_signal_]
+**runc kill** _container-id_ [_signal_]
 
 # DESCRIPTION
 
@@ -15,15 +15,6 @@ A different signal can be specified either by its name (with or without the
 **SIG** prefix), or its numeric value. Use **kill**(1) with **-l** option
 to list available signals.
 
-# OPTIONS
-**--all**|**-a**
-: Send the signal to all processes inside the container, rather than
-the container's init only. This option is useful when the container does not
-have its own PID namespace.
-
-: When this option is set, no error is returned if the container is stopped
-or does not exist.
-
 # EXAMPLES
 
 The following will send a **KILL** signal to the init process of the

tests/integration/kill.bats

@@ -22,10 +22,63 @@ function teardown() {
 	[ "$status" -eq 0 ]
 	wait_for_container 10 1 test_busybox stopped
 
-	# we should ensure kill work after the container stopped
+	# Check that kill errors on a stopped container.
+	runc kill test_busybox 0
+	[ "$status" -ne 0 ]
+	[[ "$output" == *"container not running"* ]]
+
+	# Check that -a (now obsoleted) makes kill return no error for a stopped container.
 	runc kill -a test_busybox 0
 	[ "$status" -eq 0 ]
 
 	runc delete test_busybox
 	[ "$status" -eq 0 ]
 }
+
+# This is roughly the same as TestPIDHostInitProcessWait in libcontainer/integration.
+@test "kill KILL [host pidns]" {
+	# kill -a currently requires cgroup freezer.
+	requires cgroups_freezer
+
+	update_config '	  .linux.namespaces -= [{"type": "pid"}]'
+	set_cgroups_path
+	if [ $EUID -ne 0 ]; then
+		# kill --all requires working cgroups.
+		requires rootless_cgroup
+		update_config '	  .mounts |= map((select(.type == "proc")
+					| .type = "none"
+					| .source = "/proc"
+					| .options = ["rbind", "nosuid", "nodev", "noexec"]
+				  ) // .)'
+	fi
+
+	runc run -d --console-socket "$CONSOLE_SOCKET" test_busybox
+	[ "$status" -eq 0 ]
+
+	# Start a few more processes.
+	for _ in 1 2 3 4 5; do
+		__runc exec -d test_busybox sleep 1h
+		[ "$status" -eq 0 ]
+	done
+
+	# Get the list of all container processes.
+	path=$(get_cgroup_path "pids")
+	pids=$(cat "$path"/cgroup.procs)
+	echo "pids: $pids"
+	# Sanity check -- make sure all processes exist.
+	for p in $pids; do
+		kill -0 "$p"
+	done
+
+	runc kill test_busybox KILL
+	[ "$status" -eq 0 ]
+	wait_for_container 10 1 test_busybox stopped
+
+	# Make sure all processes are gone.
+	pids=$(cat "$path"/cgroup.procs) || true # OK if cgroup is gone
+	echo "pids: $pids"
+	for p in $pids; do
+		run ! kill -0 "$p"
+		[[ "$output" = *"No such process" ]]
+	done
+}

tests/rootless.sh

@@ -1,4 +1,4 @@
-#!/bin/bash -x
+#!/bin/bash
 # Copyright (C) 2017 SUSE LLC
 #
 # Licensed under the Apache License, Version 2.0 (the "License");