Fix a regression when killing and deleting a container with shared(host) pid namespace

checkpoint.go

@@ -70,7 +70,9 @@ checkpointed.`,
 		err = container.Checkpoint(options)
 		if err == nil && !(options.LeaveRunning || options.PreDump) {
 			// Destroy the container unless we tell CRIU to keep it.
-			destroy(container)
+			if err := destroy(container); err != nil {
+				logrus.Warn(err)
+			}
 		}
 		return err
 	},

delete.go

@@ -18,8 +18,7 @@ func killContainer(container *libcontainer.Container) error {
 	for i := 0; i < 100; i++ {
 		time.Sleep(100 * time.Millisecond)
 		if err := container.Signal(unix.Signal(0)); err != nil {
-			destroy(container)
-			return nil
+			return destroy(container)
 		}
 	}
 	return errors.New("container init still running")
@@ -72,7 +71,7 @@ status of "ubuntu01" as "stopped" the following will delete resources held for
 		}
 		switch s {
 		case libcontainer.Stopped:
-			destroy(container)
+			return destroy(container)
 		case libcontainer.Created:
 			return killContainer(container)
 		default:
@@ -81,7 +80,5 @@ status of "ubuntu01" as "stopped" the following will delete resources held for
 			}
 			return fmt.Errorf("cannot delete container %s that is not stopped: %s", id, s)
 		}
-
-		return nil
 	},
 }

libcontainer/cgroups/utils.go

@@ -259,7 +259,7 @@ func RemovePath(path string) error {
 // If after all there are not removed cgroups - appropriate error will be
 // returned.
 func RemovePaths(paths map[string]string) (err error) {
-	const retries = 5
+	const retries = 10
 	delay := 10 * time.Millisecond
 	for i := 0; i < retries; i++ {
 		if i != 0 {

libcontainer/container_linux.go

@@ -368,12 +368,6 @@ func (c *Container) Signal(s os.Signal) error {
 	if err != nil {
 		return err
 	}
-	// To avoid a PID reuse attack, don't kill non-running container.
-	switch status {
-	case Running, Created, Paused:
-	default:
-		return ErrNotRunning
-	}
 
 	// 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,
@@ -383,8 +377,19 @@ func (c *Container) Signal(s os.Signal) error {
 	// 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) {
+		if pids, err := c.cgroupManager.GetAllPids(); c.ignoreCgroupError(err) != nil {
+			return err
+		} else if len(pids) == 0 {
+			return ErrNotRunning
+		}
 		err = signalAllProcesses(c.cgroupManager, unix.SIGKILL)
 	} else {
+		// To avoid a PID reuse attack, don't kill non-running container.
+		switch status {
+		case Running, Created, Paused:
+		default:
+			return ErrNotRunning
+		}
 		err = c.initProcess.signal(s)
 	}
 	if err != nil {
@@ -868,6 +873,26 @@ func (c *Container) newInitConfig(process *Process) *initConfig {
 func (c *Container) Destroy() error {
 	c.m.Lock()
 	defer c.m.Unlock()
+	if !c.config.Namespaces.IsPrivate(configs.NEWPID) {
+		const retries = 10
+		for i := 0; i < retries; i++ {
+			pids, err := c.cgroupManager.GetAllPids()
+			if c.ignoreCgroupError(err) != nil {
+				return err
+			}
+			if len(pids) > 0 {
+				if err := signalAllProcesses(c.cgroupManager, unix.SIGKILL); c.ignoreCgroupError(err) != nil {
+					return err
+				}
+				if i == retries-1 {
+					return errors.New("some processes are still running in the container")
+				}
+				time.Sleep(100 * time.Millisecond)
+			} else {
+				break
+			}
+		}
+	}
 	return c.state.destroy()
 }
 

libcontainer/state_linux.go

@@ -36,6 +36,9 @@ type containerState interface {
 
 func destroy(c *Container) error {
 	err := c.cgroupManager.Destroy()
+	if err != nil {
+		return err
+	}
 	if c.intelRdtManager != nil {
 		if ierr := c.intelRdtManager.Destroy(); err == nil {
 			err = ierr

tests/integration/delete.bats

@@ -195,3 +195,29 @@ EOF
 	# Expect "no such unit" exit code.
 	run -4 systemctl status $user "$SD_UNIT_NAME"
 }
+
+@test "runc delete [with host pid namespace]" {
+	requires root
+
+	update_config '	  .linux.namespaces -= [{"type": "pid"}]
+			| .process.terminal = false
+			| .process.args |= ["/bin/sleep", "1d"]'
+
+	__runc run -d test_host_pidns
+	[ "$status" -eq 0 ]
+	testcontainer test_host_pidns running
+
+	runc state test_host_pidns
+	pid=$(echo $output | jq .pid)
+
+	__runc exec -d test_host_pidns sleep 2d
+	[ "$status" -eq 0 ]
+	testcontainer test_host_pidns running
+
+	kill -9 $pid
+	[ "$status" -eq 0 ]
+	testcontainer test_host_pidns stopped
+
+	runc delete test_host_pidns
+	[ "$status" -eq 0 ]
+}

utils_linux.go

@@ -82,10 +82,8 @@ func newProcess(p specs.Process) (*libcontainer.Process, error) {
 	return lp, nil
 }
 
-func destroy(container *libcontainer.Container) {
-	if err := container.Destroy(); err != nil {
-		logrus.Error(err)
-	}
+func destroy(container *libcontainer.Container) error {
+	return container.Destroy()
 }
 
 // setupIO modifies the given process config according to the options.
@@ -289,7 +287,9 @@ func (r *runner) run(config *specs.Process) (int, error) {
 
 func (r *runner) destroy() {
 	if r.shouldDestroy {
-		destroy(r.container)
+		if err := destroy(r.container); err != nil {
+			logrus.Warn(err)
+		}
 	}
 }