From 0c17600995b959e7406b6537ccbc733329546091 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Mon, 3 Mar 2025 10:26:36 -0800 Subject: [PATCH] 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: 732972054 --- pkg/sentry/control/lifecycle.go | 8 +++++++- pkg/sentry/control/proc.go | 12 ++++++++--- pkg/sentry/fdimport/fdimport.go | 28 +++++++++++++++++--------- pkg/sentry/fsimpl/host/host.go | 23 ++++++++++++++++----- pkg/sentry/fsimpl/host/save_restore.go | 8 ++++++++ runsc/boot/loader.go | 9 ++++++++- runsc/boot/restore.go | 2 ++ runsc/container/container_test.go | 21 +++++++++++++++++-- 8 files changed, 90 insertions(+), 21 deletions(-) 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 {