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)
+ }
  }
 }