Fix restore with pending exec session

Exec'd processes cannot be stitched back to the original caller
and are killed after restore. So ignore failures
to restore host FDs (generally stdio) that belong
to them.

Fixes #11439

PiperOrigin-RevId: 727091752

pkg/sentry/control/lifecycle.go

@@ -276,7 +276,13 @@ func (l *Lifecycle) StartContainer(args *StartContainerArgs, _ *uint32) error {
 		fdMap[appFD] = hostFDs[i]
 	}
 	// Use ContainerID since containers don't have names here.
-	if _, err := fdimport.Import(ctx, fdTable, false, args.KUID, args.KGID, fdMap, initArgs.ContainerID); err != nil {
+	opts := fdimport.ImportOptions{
+		Restorable:    true,
+		UID:           args.KUID,
+		GID:           args.KGID,
+		ContainerName: initArgs.ContainerID,
+	}
+	if _, err := fdimport.Import(ctx, fdTable, fdMap, opts); err != nil {
 		return fmt.Errorf("error importing host files: %w", err)
 	}
 	initArgs.FDTable = fdTable

pkg/sentry/control/proc.go

@@ -265,9 +265,15 @@ func (proc *Proc) execAsync(args *ExecArgs) (*kernel.ThreadGroup, kernel.ThreadI
 		initArgs.Filename = resolved
 	}
 
-	// TODO(gvisor.dev/issue/1956): Container name is not really needed because
-	// exec processes are not restored, but add it for completeness.
-	ttyFile, err := fdimport.Import(ctx, fdTable, args.StdioIsPty, args.KUID, args.KGID, fdMap, "")
+	opts := fdimport.ImportOptions{
+		Console: args.StdioIsPty,
+		// Exec sessions are not restorable because the caller will not be present after the restore.
+		// Exec'd processes are killed after the restore.
+		Restorable: false,
+		UID:        args.KUID,
+		GID:        args.KGID,
+	}
+	ttyFile, err := fdimport.Import(ctx, fdTable, fdMap, opts)
 	if err != nil {
 		return nil, 0, nil, err
 	}

pkg/sentry/fdimport/fdimport.go

@@ -29,11 +29,20 @@ import (
 	"gvisor.dev/gvisor/pkg/sentry/vfs"
 )
 
+// ImportOptions contains options for Import().
+type ImportOptions struct {
+	Console       bool
+	Restorable    bool
+	UID           auth.KUID
+	GID           auth.KGID
+	ContainerName string
+}
+
 // Import imports a map of FDs into the given FDTable. If console is true,
 // sets up TTY for sentry stdin, stdout, and stderr FDs. Used FDs are either
 // closed or released. It's safe for the caller to close any remaining files
 // upon return.
-func Import(ctx context.Context, fdTable *kernel.FDTable, console bool, uid auth.KUID, gid auth.KGID, fds map[int]*fd.FD, containerName string) (*host.TTYFileDescription, error) {
+func Import(ctx context.Context, fdTable *kernel.FDTable, fds map[int]*fd.FD, opts ImportOptions) (*host.TTYFileDescription, error) {
 	k := kernel.KernelFromContext(ctx)
 	if k == nil {
 		return nil, fmt.Errorf("cannot find kernel from context")
@@ -52,23 +61,24 @@ func Import(ctx context.Context, fdTable *kernel.FDTable, console bool, uid auth
 		}
 	}
 
-	// We iterate through the FDs in sorted order for better determinancy
-	// in startup, but it shouldn't matter.
+	// We iterate through the FDs in sorted order to keep it deterministic
+	// during startup, but it shouldn't matter.
 	var ttyFile *vfs.FileDescription
 	for _, appFD := range slices.Sorted(maps.Keys(fds)) {
 		hostFD := fds[appFD]
 		fdOpts := host.NewFDOptions{
-			Savable: true,
+			Savable:    true,
+			Restorable: opts.Restorable,
 		}
-		if uid != auth.NoID || gid != auth.NoID {
+		if opts.UID != auth.NoID || opts.GID != auth.NoID {
 			fdOpts.VirtualOwner = true
-			fdOpts.UID = uid
-			fdOpts.GID = gid
+			fdOpts.UID = opts.UID
+			fdOpts.GID = opts.GID
 		}
 		var appFile *vfs.FileDescription
 
-		fdOpts.RestoreKey = host.MakeRestoreID(containerName, appFD)
-		if console && appFD < 3 {
+		fdOpts.RestoreKey = host.MakeRestoreID(opts.ContainerName, appFD)
+		if opts.Console && appFD < 3 {
 			// Import the file as a host TTY file.
 			if ttyFile == nil {
 				fdOpts.IsTTY = true

pkg/sentry/fsimpl/host/host.go

@@ -150,6 +150,12 @@ type inode struct {
 	// This field is initialized at creation time and is immutable.
 	savable bool
 
+	// restorable is true if hostFD may be restored. This can be set to false
+	// for host FDs that are not going to be present after restore.
+	//
+	// This field is initialized at creation time and is immutable.
+	restorable bool
+
 	// readonly is true if operations that can potentially change the host file
 	// are blocked.
 	//
@@ -221,6 +227,10 @@ type NewFDOptions struct {
 	// restore.
 	RestoreKey vfs.RestoreID
 
+	// Restorable is true if hostFD may be restored. This can be set to false
+	// for host FDs that are not going to be present after restore.
+	Restorable bool
+
 	// If IsTTY is true, the file descriptor is a TTY.
 	IsTTY bool
 
@@ -297,6 +307,7 @@ func NewFD(ctx context.Context, mnt *vfs.Mount, hostFD int, opts *NewFDOptions)
 		i.virtualOwner.gid = atomicbitops.FromUint32(uint32(opts.GID))
 		i.virtualOwner.mode = atomicbitops.FromUint32(stat.Mode)
 	}
+	i.restorable = opts.Restorable
 
 	d := &kernfs.Dentry{}
 	d.Init(&fs.Filesystem, i)
@@ -627,11 +638,13 @@ func (i *inode) SetStat(ctx context.Context, fs *vfs.Filesystem, creds *auth.Cre
 // DecRef implements kernfs.Inode.DecRef.
 func (i *inode) DecRef(ctx context.Context) {
 	i.inodeRefs.DecRef(func() {
-		if i.epollable {
-			fdnotifier.RemoveFD(int32(i.hostFD))
-		}
-		if err := unix.Close(i.hostFD); err != nil {
-			log.Warningf("failed to close host fd %d: %v", i.hostFD, err)
+		if i.hostFD >= 0 {
+			if i.epollable {
+				fdnotifier.RemoveFD(int32(i.hostFD))
+			}
+			if err := unix.Close(i.hostFD); err != nil {
+				log.Warningf("failed to close host fd %d: %v", i.hostFD, err)
+			}
 		}
 		// We can't rely on fdnotifier when closing the fd, because the event may race
 		// with fdnotifier.RemoveFD. Instead, notify the queue explicitly.

pkg/sentry/fsimpl/host/save_restore.go

@@ -22,6 +22,7 @@ import (
 	"golang.org/x/sys/unix"
 	"gvisor.dev/gvisor/pkg/fdnotifier"
 	"gvisor.dev/gvisor/pkg/hostarch"
+	"gvisor.dev/gvisor/pkg/log"
 	"gvisor.dev/gvisor/pkg/safemem"
 	"gvisor.dev/gvisor/pkg/sentry/hostfd"
 	"gvisor.dev/gvisor/pkg/sentry/vfs"
@@ -71,11 +72,18 @@ func (i *inode) beforeSave() {
 
 // afterLoad is invoked by stateify.
 func (i *inode) afterLoad(ctx context.Context) {
+	if !i.restorable {
+		log.Infof("Skipping host FD (%+v) that is not restorable", i.restoreKey)
+		i.hostFD = -1
+		return
+	}
+
 	fdmap := vfs.RestoreFilesystemFDMapFromContext(ctx)
 	fd, ok := fdmap[i.restoreKey]
 	if !ok {
 		panic(fmt.Sprintf("no host FD available for %+v, map: %v", i.restoreKey, fdmap))
 	}
+	log.Debugf("Remapping host FD from %d to %d", i.hostFD, fd)
 	i.hostFD = fd
 
 	if i.epollable {

runsc/boot/loader.go

@@ -1798,7 +1798,14 @@ func createFDTable(ctx context.Context, console bool, stdioFDs []*fd.FD, passFDs
 
 	k := kernel.KernelFromContext(ctx)
 	fdTable := k.NewFDTable()
-	ttyFile, err := fdimport.Import(ctx, fdTable, console, auth.KUID(user.UID), auth.KGID(user.GID), fdMap, containerName)
+	opts := fdimport.ImportOptions{
+		Console:       console,
+		Restorable:    true,
+		UID:           auth.KUID(user.UID),
+		GID:           auth.KGID(user.GID),
+		ContainerName: containerName,
+	}
+	ttyFile, err := fdimport.Import(ctx, fdTable, fdMap, opts)
 	if err != nil {
 		fdTable.DecRef(ctx)
 		return nil, nil, err

runsc/boot/restore.go

@@ -293,8 +293,10 @@ func (r *restorer) restore(l *Loader) error {
 
 	// Kill all processes that have been exec'd since they cannot be properly
 	// restored -- the caller is no longer connected.
+	log.Debugf("Killing any exec session that existed previously")
 	for _, tg := range l.k.RootPIDNamespace().ThreadGroups() {
 		if tg.Leader().Origin == kernel.OriginExec {
+			log.Infof("Killing exec'd process, PID: %d", tg.ID())
 			if err := l.k.SendExternalSignalThreadGroup(tg, &linux.SignalInfo{Signo: int32(linux.SIGKILL)}); err != nil {
 				log.Warningf("Failed to kill exec process after restore: %v", err)
 			}

runsc/container/container_test.go

@@ -37,6 +37,7 @@ import (
 	"golang.org/x/sys/unix"
 	"gvisor.dev/gvisor/pkg/abi/linux"
 	"gvisor.dev/gvisor/pkg/bits"
+	"gvisor.dev/gvisor/pkg/cleanup"
 	"gvisor.dev/gvisor/pkg/log"
 	"gvisor.dev/gvisor/pkg/sentry/control"
 	"gvisor.dev/gvisor/pkg/sentry/fsimpl/erofs"
@@ -1199,11 +1200,11 @@ func TestCheckpointRestore(t *testing.T) {
 // after the container is restored.
 func TestCheckpointRestoreExecKilled(t *testing.T) {
 	spec, conf := sleepSpecConf(t)
-	_, bundleDir, cleanup, err := testutil.SetupContainer(spec, conf)
+	_, bundleDir, cu, err := testutil.SetupContainer(spec, conf)
 	if err != nil {
 		t.Fatalf("error setting up container: %v", err)
 	}
-	defer cleanup()
+	defer cu()
 
 	// Create and start the container.
 	args := Args{
@@ -1228,6 +1229,21 @@ func TestCheckpointRestoreExecKilled(t *testing.T) {
 	if err != nil {
 		t.Fatalf("error executing in container: %v", err)
 	}
+
+	// Test exec process with stdio FDs. FDs will not be present after restore and
+	// should be ignored.
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	stdioCleanup := cleanup.Make(func() {
+		r.Close()
+		w.Close()
+	})
+	defer stdioCleanup.Clean()
+
+	fdMap := map[int]*os.File{0: r, 1: w, 2: w}
+	execArgs.FilePayload = control.NewFilePayload(fdMap, nil)
 	pid2, err := cont.Execute(conf, execArgs)
 	if err != nil {
 		t.Fatalf("error executing in container: %v", err)
@@ -1264,6 +1280,7 @@ func TestCheckpointRestoreExecKilled(t *testing.T) {
 	}
 	cont.Destroy()
 	cont = nil
+	stdioCleanup.Clean()
 
 	cont2, err := New(conf, args)
 	if err != nil {