From 5a559423c473448588e8f473cecc8d0dd5c7c41b Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Mon, 22 Apr 2024 14:59:55 -0700 Subject: [PATCH] Standardize fd.FD usage across platform and restore This makes the code in controller.go simple, especially for multi-container restore that requires taking ownership of files. PiperOrigin-RevId: 627165958 --- pkg/sentry/kernel/kernel.go | 2 +- pkg/sentry/platform/BUILD | 1 + pkg/sentry/platform/kvm/BUILD | 1 + pkg/sentry/platform/kvm/kvm.go | 16 ++++++------ pkg/sentry/platform/platform.go | 6 ++--- pkg/sentry/platform/ptrace/BUILD | 1 + pkg/sentry/platform/ptrace/ptrace.go | 7 +++-- pkg/sentry/platform/systrap/BUILD | 1 + pkg/sentry/platform/systrap/systrap.go | 5 ++-- pkg/sentry/state/state.go | 2 +- runsc/boot/controller.go | 36 +++++++++++++++----------- runsc/boot/loader.go | 4 +-- runsc/boot/restore.go | 9 ++++--- runsc/cmd/BUILD | 1 + runsc/cmd/boot.go | 3 ++- runsc/sandbox/BUILD | 1 + runsc/sandbox/sandbox.go | 7 ++--- 17 files changed, 59 insertions(+), 44 deletions(-) diff --git a/pkg/sentry/kernel/kernel.go b/pkg/sentry/kernel/kernel.go index 27fff10dd..1e31df435 100644 --- a/pkg/sentry/kernel/kernel.go +++ b/pkg/sentry/kernel/kernel.go @@ -682,7 +682,7 @@ func (k *Kernel) invalidateUnsavableMappings(ctx context.Context) error { } // LoadFrom returns a new Kernel loaded from args. -func (k *Kernel) LoadFrom(ctx context.Context, r io.Reader, pagesFile *os.File, timeReady chan struct{}, net inet.Stack, clocks sentrytime.Clocks, vfsOpts *vfs.CompleteRestoreOptions) error { +func (k *Kernel) LoadFrom(ctx context.Context, r io.Reader, pagesFile io.Reader, timeReady chan struct{}, net inet.Stack, clocks sentrytime.Clocks, vfsOpts *vfs.CompleteRestoreOptions) error { loadStart := time.Now() k.runningTasksCond.L = &k.runningTasksMu diff --git a/pkg/sentry/platform/BUILD b/pkg/sentry/platform/BUILD index 5da0cae31..7871a2ade 100644 --- a/pkg/sentry/platform/BUILD +++ b/pkg/sentry/platform/BUILD @@ -19,6 +19,7 @@ go_library( "//pkg/abi/linux", "//pkg/context", "//pkg/cpuid", + "//pkg/fd", "//pkg/hostarch", "//pkg/seccomp", "//pkg/seccomp/precompiledseccomp", diff --git a/pkg/sentry/platform/kvm/BUILD b/pkg/sentry/platform/kvm/BUILD index d836ec796..a25624d44 100644 --- a/pkg/sentry/platform/kvm/BUILD +++ b/pkg/sentry/platform/kvm/BUILD @@ -65,6 +65,7 @@ go_library( "//pkg/atomicbitops", "//pkg/context", "//pkg/cpuid", + "//pkg/fd", "//pkg/hostarch", "//pkg/hosttid", "//pkg/log", diff --git a/pkg/sentry/platform/kvm/kvm.go b/pkg/sentry/platform/kvm/kvm.go index 1c1a590a8..987d6cb4f 100644 --- a/pkg/sentry/platform/kvm/kvm.go +++ b/pkg/sentry/platform/kvm/kvm.go @@ -17,10 +17,10 @@ package kvm import ( "fmt" - "os" "golang.org/x/sys/unix" pkgcontext "gvisor.dev/gvisor/pkg/context" + "gvisor.dev/gvisor/pkg/fd" "gvisor.dev/gvisor/pkg/hostarch" "gvisor.dev/gvisor/pkg/ring0" "gvisor.dev/gvisor/pkg/ring0/pagetables" @@ -80,11 +80,11 @@ var ( // OpenDevice opens the KVM device and returns the File. // If the devicePath is empty, it will default to /dev/kvm. -func OpenDevice(devicePath string) (*os.File, error) { +func OpenDevice(devicePath string) (*fd.FD, error) { if devicePath == "" { devicePath = "/dev/kvm" } - f, err := os.OpenFile(devicePath, unix.O_RDWR, 0) + f, err := fd.Open(devicePath, unix.O_RDWR, 0) if err != nil { return nil, fmt.Errorf("error opening KVM device file (%s): %v", devicePath, err) } @@ -92,8 +92,8 @@ func OpenDevice(devicePath string) (*os.File, error) { } // New returns a new KVM-based implementation of the platform interface. -func New(deviceFile *os.File) (*KVM, error) { - fd := deviceFile.Fd() +func New(deviceFile *fd.FD) (*KVM, error) { + fd := deviceFile.FD() // Ensure global initialization is done. globalOnce.Do(func() { @@ -109,7 +109,7 @@ func New(deviceFile *os.File) (*KVM, error) { errno unix.Errno ) for { - vm, _, errno = unix.Syscall(unix.SYS_IOCTL, fd, KVM_CREATE_VM, 0) + vm, _, errno = unix.Syscall(unix.SYS_IOCTL, uintptr(fd), KVM_CREATE_VM, 0) if errno == unix.EINTR { continue } @@ -184,11 +184,11 @@ func (k *KVM) NewContext(pkgcontext.Context) platform.Context { type constructor struct{} -func (*constructor) New(f *os.File) (platform.Platform, error) { +func (*constructor) New(f *fd.FD) (platform.Platform, error) { return New(f) } -func (*constructor) OpenDevice(devicePath string) (*os.File, error) { +func (*constructor) OpenDevice(devicePath string) (*fd.FD, error) { return OpenDevice(devicePath) } diff --git a/pkg/sentry/platform/platform.go b/pkg/sentry/platform/platform.go index fd78372cf..c00af4d58 100644 --- a/pkg/sentry/platform/platform.go +++ b/pkg/sentry/platform/platform.go @@ -19,11 +19,11 @@ package platform import ( "fmt" - "os" "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/context" + "gvisor.dev/gvisor/pkg/fd" "gvisor.dev/gvisor/pkg/hostarch" "gvisor.dev/gvisor/pkg/seccomp" "gvisor.dev/gvisor/pkg/seccomp/precompiledseccomp" @@ -547,12 +547,12 @@ type Constructor interface { // Arguments: // // * deviceFile - the device file (e.g. /dev/kvm for the KVM platform). - New(deviceFile *os.File) (Platform, error) + New(deviceFile *fd.FD) (Platform, error) // OpenDevice opens the path to the device used by the platform. // Passing in an empty string will use the default path for the device, // e.g. "/dev/kvm" for the KVM platform. - OpenDevice(devicePath string) (*os.File, error) + OpenDevice(devicePath string) (*fd.FD, error) // Requirements returns platform specific requirements. Requirements() Requirements diff --git a/pkg/sentry/platform/ptrace/BUILD b/pkg/sentry/platform/ptrace/BUILD index 9d89b2571..200782d16 100644 --- a/pkg/sentry/platform/ptrace/BUILD +++ b/pkg/sentry/platform/ptrace/BUILD @@ -30,6 +30,7 @@ go_library( "//pkg/bpf", "//pkg/context", "//pkg/cpuid", + "//pkg/fd", "//pkg/hostarch", "//pkg/hosttid", "//pkg/log", diff --git a/pkg/sentry/platform/ptrace/ptrace.go b/pkg/sentry/platform/ptrace/ptrace.go index 985d480be..b081fff61 100644 --- a/pkg/sentry/platform/ptrace/ptrace.go +++ b/pkg/sentry/platform/ptrace/ptrace.go @@ -45,10 +45,9 @@ package ptrace import ( - "os" - "gvisor.dev/gvisor/pkg/abi/linux" pkgcontext "gvisor.dev/gvisor/pkg/context" + "gvisor.dev/gvisor/pkg/fd" "gvisor.dev/gvisor/pkg/hostarch" "gvisor.dev/gvisor/pkg/sentry/arch" "gvisor.dev/gvisor/pkg/sentry/platform" @@ -262,11 +261,11 @@ func (p *PTrace) NewAddressSpace(any) (platform.AddressSpace, <-chan struct{}, e type constructor struct{} -func (*constructor) New(*os.File) (platform.Platform, error) { +func (*constructor) New(*fd.FD) (platform.Platform, error) { return New() } -func (*constructor) OpenDevice(_ string) (*os.File, error) { +func (*constructor) OpenDevice(_ string) (*fd.FD, error) { return nil, nil } diff --git a/pkg/sentry/platform/systrap/BUILD b/pkg/sentry/platform/systrap/BUILD index 5c9dde337..fadbaa0a8 100644 --- a/pkg/sentry/platform/systrap/BUILD +++ b/pkg/sentry/platform/systrap/BUILD @@ -80,6 +80,7 @@ go_library( "//pkg/bpf", "//pkg/context", "//pkg/cpuid", + "//pkg/fd", "//pkg/hostarch", "//pkg/log", "//pkg/memutil", diff --git a/pkg/sentry/platform/systrap/systrap.go b/pkg/sentry/platform/systrap/systrap.go index 97bdbfda0..0393f95ae 100644 --- a/pkg/sentry/platform/systrap/systrap.go +++ b/pkg/sentry/platform/systrap/systrap.go @@ -56,6 +56,7 @@ import ( "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/abi/linux" pkgcontext "gvisor.dev/gvisor/pkg/context" + "gvisor.dev/gvisor/pkg/fd" "gvisor.dev/gvisor/pkg/hostarch" "gvisor.dev/gvisor/pkg/memutil" "gvisor.dev/gvisor/pkg/sentry/arch" @@ -390,11 +391,11 @@ func (*Systrap) NewContext(ctx pkgcontext.Context) platform.Context { type constructor struct{} -func (*constructor) New(_ *os.File) (platform.Platform, error) { +func (*constructor) New(_ *fd.FD) (platform.Platform, error) { return New() } -func (*constructor) OpenDevice(_ string) (*os.File, error) { +func (*constructor) OpenDevice(_ string) (*fd.FD, error) { return nil, nil } diff --git a/pkg/sentry/state/state.go b/pkg/sentry/state/state.go index 85b6afae1..629d75fdc 100644 --- a/pkg/sentry/state/state.go +++ b/pkg/sentry/state/state.go @@ -115,7 +115,7 @@ type LoadOpts struct { // PagesFile is the file in which all MemoryFile pages are stored if // PagesFile is non-nil. - PagesFile *os.File + PagesFile io.Reader // Key is used for state integrity check. Key []byte diff --git a/runsc/boot/controller.go b/runsc/boot/controller.go index bd587a268..a31ac0d29 100644 --- a/runsc/boot/controller.go +++ b/runsc/boot/controller.go @@ -17,7 +17,6 @@ package boot import ( "errors" "fmt" - "os" "path" gtime "time" @@ -462,32 +461,39 @@ func (cm *containerManager) Restore(o *RestoreOpts, _ *struct{}) error { if len(o.Files) == 0 { return fmt.Errorf("at least one file must be passed to Restore") } - fileIdx := 0 - r := restorer{container: &cm.l.root} - r.stateFile = o.Files[fileIdx] - fileIdx++ - defer r.stateFile.Close() - if info, err := r.stateFile.Stat(); err != nil { + stateFile, err := o.ReleaseFD(0) + if err != nil { return err - } else if info.Size() == 0 { + } + defer stateFile.Close() + + var stat unix.Stat_t + if err := unix.Fstat(stateFile.FD(), &stat); err != nil { + return err + } + if stat.Size == 0 { return fmt.Errorf("statefile cannot be empty") } + r := restorer{container: &cm.l.root, stateFile: stateFile} + + fileIdx := 1 if o.HavePagesFile { - r.pagesFile = o.Files[fileIdx] + pagesFile, err := o.ReleaseFD(fileIdx) + if err != nil { + return err + } + defer pagesFile.Close() fileIdx++ - defer r.pagesFile.Close() + r.pagesFile = pagesFile } if o.HaveDeviceFile { - // The device file is donated to the platform. - // Can't take ownership away from os.File. dup them to get a new FD. - fd, err := unix.Dup(int(o.Files[fileIdx].Fd())) + r.deviceFile, err = o.ReleaseFD(fileIdx) if err != nil { - return fmt.Errorf("failed to dup file: %v", err) + return err } - r.deviceFile = os.NewFile(uintptr(fd), "platform device") fileIdx++ } diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go index fcc33e49f..e5e1da555 100644 --- a/runsc/boot/loader.go +++ b/runsc/boot/loader.go @@ -250,7 +250,7 @@ type Args struct { ControllerFD int // Device is an optional argument that is passed to the platform. The Loader // takes ownership of this file and may close it at any time. - Device *os.File + Device *fd.FD // GoferFDs is an array of FDs used to connect with the Gofer. The Loader // takes ownership of these FDs and may close them at any time. GoferFDs []int @@ -670,7 +670,7 @@ func (l *Loader) Destroy() { refs.OnExit() } -func createPlatform(conf *config.Config, deviceFile *os.File) (platform.Platform, error) { +func createPlatform(conf *config.Config, deviceFile *fd.FD) (platform.Platform, error) { p, err := platform.Lookup(conf.Platform) if err != nil { panic(fmt.Sprintf("invalid platform %s: %s", conf.Platform, err)) diff --git a/runsc/boot/restore.go b/runsc/boot/restore.go index 3ff1c7443..08823cf6c 100644 --- a/runsc/boot/restore.go +++ b/runsc/boot/restore.go @@ -16,10 +16,11 @@ package boot import ( "fmt" - "os" + "io" "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/context" + "gvisor.dev/gvisor/pkg/fd" "gvisor.dev/gvisor/pkg/log" "gvisor.dev/gvisor/pkg/sentry/fsimpl/host" "gvisor.dev/gvisor/pkg/sentry/inet" @@ -46,9 +47,9 @@ const ( type restorer struct { container *containerInfo - stateFile *os.File - pagesFile *os.File - deviceFile *os.File + stateFile io.Reader + pagesFile io.Reader + deviceFile *fd.FD } func createNetworkNamespaceForRestore(l *Loader) (*stack.Stack, *inet.Namespace, error) { diff --git a/runsc/cmd/BUILD b/runsc/cmd/BUILD index f90198e08..1a606f249 100644 --- a/runsc/cmd/BUILD +++ b/runsc/cmd/BUILD @@ -83,6 +83,7 @@ go_library( "//pkg/coretag", "//pkg/coverage", "//pkg/cpuid", + "//pkg/fd", "//pkg/log", "//pkg/metric", "//pkg/prometheus", diff --git a/runsc/cmd/boot.go b/runsc/cmd/boot.go index 6c2ea6b31..1f72e8165 100644 --- a/runsc/cmd/boot.go +++ b/runsc/cmd/boot.go @@ -32,6 +32,7 @@ import ( "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/coretag" "gvisor.dev/gvisor/pkg/cpuid" + "gvisor.dev/gvisor/pkg/fd" "gvisor.dev/gvisor/pkg/log" "gvisor.dev/gvisor/pkg/metric" "gvisor.dev/gvisor/pkg/ring0" @@ -426,7 +427,7 @@ func (b *Boot) Execute(_ context.Context, f *flag.FlagSet, args ...any) subcomma Spec: spec, Conf: conf, ControllerFD: b.controllerFD, - Device: os.NewFile(uintptr(b.deviceFD), "platform device"), + Device: fd.New(b.deviceFD), GoferFDs: b.ioFDs.GetArray(), DevGoferFD: b.devIoFD, StdioFDs: b.stdioFDs.GetArray(), diff --git a/runsc/sandbox/BUILD b/runsc/sandbox/BUILD index c1a5ecfa1..d64384820 100644 --- a/runsc/sandbox/BUILD +++ b/runsc/sandbox/BUILD @@ -25,6 +25,7 @@ go_library( "//pkg/control/client", "//pkg/control/server", "//pkg/coverage", + "//pkg/fd", "//pkg/log", "//pkg/metric:metric_go_proto", "//pkg/prometheus", diff --git a/runsc/sandbox/sandbox.go b/runsc/sandbox/sandbox.go index 02b766cd4..aaf9ccc0e 100644 --- a/runsc/sandbox/sandbox.go +++ b/runsc/sandbox/sandbox.go @@ -41,6 +41,7 @@ import ( "gvisor.dev/gvisor/pkg/control/client" "gvisor.dev/gvisor/pkg/control/server" "gvisor.dev/gvisor/pkg/coverage" + "gvisor.dev/gvisor/pkg/fd" "gvisor.dev/gvisor/pkg/log" metricpb "gvisor.dev/gvisor/pkg/metric/metric_go_proto" "gvisor.dev/gvisor/pkg/prometheus" @@ -474,7 +475,7 @@ func (s *Sandbox) Restore(conf *config.Config, cid string, imagePath string) err } else if deviceFile != nil { defer deviceFile.Close() opt.HaveDeviceFile = true - opt.FilePayload.Files = append(opt.FilePayload.Files, deviceFile) + opt.FilePayload.Files = append(opt.FilePayload.Files, deviceFile.ReleaseToFile("device file")) } conn, err := s.sandboxConnect() @@ -809,7 +810,7 @@ func (s *Sandbox) createSandboxProcess(conf *config.Config, args *Args, startSyn if deviceFile, err := gPlatform.OpenDevice(conf.PlatformDevicePath); err != nil { return fmt.Errorf("opening device file for platform %q: %v", conf.Platform, err) } else if deviceFile != nil { - donations.DonateAndClose("device-fd", deviceFile) + donations.DonateAndClose("device-fd", deviceFile.ReleaseToFile("device file")) } // TODO(b/151157106): syscall tests fail by timeout if asyncpreemptoff @@ -1543,7 +1544,7 @@ func (s *Sandbox) configureStdios(conf *config.Config, stdios []*os.File) error // deviceFileForPlatform opens the device file for the given platform. If the // platform does not need a device file, then nil is returned. // devicePath may be empty to use a sane platform-specific default. -func deviceFileForPlatform(name, devicePath string) (*os.File, error) { +func deviceFileForPlatform(name, devicePath string) (*fd.FD, error) { p, err := platform.Lookup(name) if err != nil { return nil, err