diff --git a/pkg/sentry/control/lifecycle.go b/pkg/sentry/control/lifecycle.go index 926658391..d5d902c8f 100644 --- a/pkg/sentry/control/lifecycle.go +++ b/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 diff --git a/pkg/sentry/control/proc.go b/pkg/sentry/control/proc.go index 1674d01cd..180214aae 100644 --- a/pkg/sentry/control/proc.go +++ b/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 } diff --git a/pkg/sentry/fdimport/fdimport.go b/pkg/sentry/fdimport/fdimport.go index ba4a22df5..87586f213 100644 --- a/pkg/sentry/fdimport/fdimport.go +++ b/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 diff --git a/pkg/sentry/fsimpl/host/host.go b/pkg/sentry/fsimpl/host/host.go index b79f71298..b56485860 100644 --- a/pkg/sentry/fsimpl/host/host.go +++ b/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. diff --git a/pkg/sentry/fsimpl/host/save_restore.go b/pkg/sentry/fsimpl/host/save_restore.go index c4845ed78..13941d019 100644 --- a/pkg/sentry/fsimpl/host/save_restore.go +++ b/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 { diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go index 0dbabcd20..2f9729cd6 100644 --- a/runsc/boot/loader.go +++ b/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 diff --git a/runsc/boot/restore.go b/runsc/boot/restore.go index 9ebb1603d..2d5ca0617 100644 --- a/runsc/boot/restore.go +++ b/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) } diff --git a/runsc/container/container_test.go b/runsc/container/container_test.go index c9e51f1cc..76de1162a 100644 --- a/runsc/container/container_test.go +++ b/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 {