From 69c3e8d63249e8a2cdc2748add98f0a7af97cdd0 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Wed, 26 Jun 2024 20:28:49 -0700 Subject: [PATCH] Move VDSOParamPage out of Timekeeper. We want to decouple the netstack from the kernel. This coupling is causing bugs in restore because netstack needs to be created before the Kernel is restored. So right now, netstack ends up using a "temporary" Kernel which is later destroyed in the restore sequence, but netstack keeps referencing it. Before this change, netstack was being initialized with a `kernel.TimeKeeper`. This `Timekeeper` was being initialized with `VDSOParamPage`, which references the MemoryFile of the kernel. So as a result, on restore the netstack ends up referencing the destroyed kernel's MemoryFile. So instead move out VDSOParamPage from TimeKeeper altogether. The callers of `TimeKeeper.SetClocks()` and `TimeKeeper.ResumeUpdates()` pass VDSOParamPage from the correct kernel being used currently. PiperOrigin-RevId: 647168588 --- pkg/sentry/fsimpl/testutil/kernel.go | 6 ++++-- pkg/sentry/kernel/kernel.go | 9 +++++++-- pkg/sentry/kernel/timekeeper.go | 25 +++++++++---------------- pkg/sentry/kernel/timekeeper_test.go | 23 +++++++++++------------ runsc/boot/loader.go | 6 ++++-- 5 files changed, 35 insertions(+), 34 deletions(-) diff --git a/pkg/sentry/fsimpl/testutil/kernel.go b/pkg/sentry/fsimpl/testutil/kernel.go index 5cf98481f..3d5516a66 100644 --- a/pkg/sentry/fsimpl/testutil/kernel.go +++ b/pkg/sentry/fsimpl/testutil/kernel.go @@ -83,8 +83,9 @@ func Boot() (*kernel.Kernel, error) { } // Create timekeeper. - tk := kernel.NewTimekeeper(k.MemoryFile(), vdso.ParamPage.FileRange()) - tk.SetClocks(time.NewCalibratedClocks()) + tk := kernel.NewTimekeeper() + params := kernel.NewVDSOParamPage(k.MemoryFile(), vdso.ParamPage.FileRange()) + tk.SetClocks(time.NewCalibratedClocks(), params) creds := auth.NewRootCredentials(auth.NewRootUserNamespace()) @@ -96,6 +97,7 @@ func Boot() (*kernel.Kernel, error) { Timekeeper: tk, RootUserNamespace: creds.UserNamespace, Vdso: vdso, + VdsoParams: params, RootUTSNamespace: kernel.NewUTSNamespace("hostname", "domain", creds.UserNamespace), RootIPCNamespace: kernel.NewIPCNamespace(creds.UserNamespace), PIDNamespace: kernel.NewRootPIDNamespace(creds.UserNamespace), diff --git a/pkg/sentry/kernel/kernel.go b/pkg/sentry/kernel/kernel.go index 230ed9742..e30086f54 100644 --- a/pkg/sentry/kernel/kernel.go +++ b/pkg/sentry/kernel/kernel.go @@ -160,6 +160,7 @@ type Kernel struct { useHostCores bool extraAuxv []arch.AuxEntry vdso *loader.VDSO + vdsoParams *VDSOParamPage rootUTSNamespace *UTSNamespace rootIPCNamespace *IPCNamespace @@ -405,6 +406,9 @@ type InitKernelArgs struct { // Vdso holds the VDSO and its parameter page. Vdso *loader.VDSO + // VdsoParams is the VDSO parameter page manager. + VdsoParams *VDSOParamPage + // RootUTSNamespace is the root UTS namespace. RootUTSNamespace *UTSNamespace @@ -466,6 +470,7 @@ func (k *Kernel) Init(args InitKernelArgs) error { } k.extraAuxv = args.ExtraAuxv k.vdso = args.Vdso + k.vdsoParams = args.VdsoParams k.futexes = futex.NewManager() k.netlinkPorts = port.New() k.ptraceExceptions = make(map[*Task]*Task) @@ -777,7 +782,7 @@ func (k *Kernel) LoadFrom(ctx context.Context, r io.Reader, pagesMetadata, pages // Restore the root network stack. k.rootNetworkNamespace.RestoreRootStack(net) - k.Timekeeper().SetClocks(clocks) + k.Timekeeper().SetClocks(clocks, k.vdsoParams) if timeReady != nil { close(timeReady) @@ -1249,7 +1254,7 @@ func (k *Kernel) resumeTimeLocked(ctx context.Context) { // The CPU clock ticker will automatically resume as task goroutines resume // execution. - k.timekeeper.ResumeUpdates() + k.timekeeper.ResumeUpdates(k.vdsoParams) for t := range k.tasks.Root.tids { if t == t.tg.leader { t.tg.itimerRealTimer.Resume() diff --git a/pkg/sentry/kernel/timekeeper.go b/pkg/sentry/kernel/timekeeper.go index 285e52543..23a3c2595 100644 --- a/pkg/sentry/kernel/timekeeper.go +++ b/pkg/sentry/kernel/timekeeper.go @@ -21,8 +21,6 @@ import ( "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/log" ktime "gvisor.dev/gvisor/pkg/sentry/kernel/time" - "gvisor.dev/gvisor/pkg/sentry/memmap" - "gvisor.dev/gvisor/pkg/sentry/pgalloc" sentrytime "gvisor.dev/gvisor/pkg/sentry/time" "gvisor.dev/gvisor/pkg/sync" "gvisor.dev/gvisor/pkg/tcpip" @@ -80,9 +78,6 @@ type Timekeeper struct { // monotonicOffset. saveRealtime int64 - // params manages the parameter page. - params *VDSOParamPage - // mu protects destruction with stop and wg. mu sync.Mutex `state:"nosave"` @@ -97,10 +92,8 @@ type Timekeeper struct { // NewTimekeeper does not take ownership of paramPage. // // SetClocks must be called on the returned Timekeeper before it is usable. -func NewTimekeeper(mf *pgalloc.MemoryFile, paramPage memmap.FileRange) *Timekeeper { - t := Timekeeper{ - params: NewVDSOParamPage(mf, paramPage), - } +func NewTimekeeper() *Timekeeper { + t := Timekeeper{} t.realtimeClock = &timekeeperClock{tk: &t, c: sentrytime.Realtime} t.monotonicClock = &timekeeperClock{tk: &t, c: sentrytime.Monotonic} return &t @@ -113,11 +106,11 @@ func NewTimekeeper(mf *pgalloc.MemoryFile, paramPage memmap.FileRange) *Timekeep // could cause time discontinuities. // // It must also be called after Load. -func (t *Timekeeper) SetClocks(c sentrytime.Clocks) { +func (t *Timekeeper) SetClocks(c sentrytime.Clocks, params *VDSOParamPage) { // Update the params, marking them "not ready", as we may need to // restart calibration on this new machine. if t.restored != nil { - if err := t.params.Write(func() vdsoParams { + if err := params.Write(func() vdsoParams { return vdsoParams{} }); err != nil { panic("unable to reset VDSO params: " + err.Error()) @@ -170,7 +163,7 @@ func (t *Timekeeper) SetClocks(c sentrytime.Clocks) { t.mu.Lock() defer t.mu.Unlock() - t.startUpdater() + t.startUpdater(params) if t.restored != nil { close(t.restored) @@ -206,7 +199,7 @@ func (t *Timekeeper) AfterFunc(d time.Duration, f func()) tcpip.Timer { // startUpdater starts an update goroutine that keeps the clocks updated. // // mu must be held. -func (t *Timekeeper) startUpdater() { +func (t *Timekeeper) startUpdater(params *VDSOParamPage) { if t.stop != nil { // Timekeeper already started return @@ -230,7 +223,7 @@ func (t *Timekeeper) startUpdater() { // Call Update within a Write block to prevent the VDSO // from using the old params between Update and // Write. - if err := t.params.Write(func() vdsoParams { + if err := params.Write(func() vdsoParams { monotonicParams, monotonicOk, realtimeParams, realtimeOk := t.clocks.Update() var p vdsoParams @@ -291,10 +284,10 @@ func (t *Timekeeper) PauseUpdates() { } // ResumeUpdates restarts clock parameter updates stopped by PauseUpdates. -func (t *Timekeeper) ResumeUpdates() { +func (t *Timekeeper) ResumeUpdates(params *VDSOParamPage) { t.mu.Lock() defer t.mu.Unlock() - t.startUpdater() + t.startUpdater(params) } // GetTime returns the current time in nanoseconds. diff --git a/pkg/sentry/kernel/timekeeper_test.go b/pkg/sentry/kernel/timekeeper_test.go index e8720566f..e27da18eb 100644 --- a/pkg/sentry/kernel/timekeeper_test.go +++ b/pkg/sentry/kernel/timekeeper_test.go @@ -51,21 +51,20 @@ func (c *mockClocks) GetTime(id sentrytime.ClockID) (int64, error) { // stateTestClocklessTimekeeper returns a test Timekeeper which has not had // SetClocks called. -func stateTestClocklessTimekeeper(tb testing.TB) *Timekeeper { +func stateTestClocklessTimekeeper(tb testing.TB) (*Timekeeper, *VDSOParamPage) { ctx := contexttest.Context(tb) mf := pgalloc.MemoryFileFromContext(ctx) fr, err := mf.Allocate(hostarch.PageSize, pgalloc.AllocOpts{Kind: usage.Anonymous}) if err != nil { tb.Fatalf("failed to allocate memory: %v", err) } - return &Timekeeper{ - params: NewVDSOParamPage(mf, fr), - } + params := NewVDSOParamPage(mf, fr) + return &Timekeeper{}, params } func stateTestTimekeeper(tb testing.TB) *Timekeeper { - t := stateTestClocklessTimekeeper(tb) - t.SetClocks(sentrytime.NewCalibratedClocks()) + t, params := stateTestClocklessTimekeeper(tb) + t.SetClocks(sentrytime.NewCalibratedClocks(), params) return t } @@ -75,8 +74,8 @@ func TestTimekeeperMonotonicZero(t *testing.T) { monotonic: 100000, } - tk := stateTestClocklessTimekeeper(t) - tk.SetClocks(c) + tk, params := stateTestClocklessTimekeeper(t) + tk.SetClocks(c, params) defer tk.Destroy() now, err := tk.GetTime(sentrytime.Monotonic) @@ -106,11 +105,11 @@ func TestTimekeeperMonotonicForward(t *testing.T) { realtime: 600000, } - tk := stateTestClocklessTimekeeper(t) + tk, params := stateTestClocklessTimekeeper(t) tk.restored = make(chan struct{}) tk.saveMonotonic = 100000 tk.saveRealtime = 400000 - tk.SetClocks(c) + tk.SetClocks(c, params) defer tk.Destroy() // The monotonic clock should jump ahead by 200000 to 300000. @@ -134,11 +133,11 @@ func TestTimekeeperMonotonicJumpBackwards(t *testing.T) { realtime: 400000, } - tk := stateTestClocklessTimekeeper(t) + tk, params := stateTestClocklessTimekeeper(t) tk.restored = make(chan struct{}) tk.saveMonotonic = 100000 tk.saveRealtime = 600000 - tk.SetClocks(c) + tk.SetClocks(c, params) defer tk.Destroy() // The monotonic clock should remain at 100000. diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go index 1aaa1f00c..26e9b1f9a 100644 --- a/runsc/boot/loader.go +++ b/runsc/boot/loader.go @@ -491,8 +491,9 @@ func New(args Args) (*Loader, error) { } // Create timekeeper. - tk := kernel.NewTimekeeper(l.k.MemoryFile(), vdso.ParamPage.FileRange()) - tk.SetClocks(time.NewCalibratedClocks()) + tk := kernel.NewTimekeeper() + params := kernel.NewVDSOParamPage(l.k.MemoryFile(), vdso.ParamPage.FileRange()) + tk.SetClocks(time.NewCalibratedClocks(), params) if err := enableStrace(args.Conf); err != nil { return nil, fmt.Errorf("enabling strace: %w", err) @@ -550,6 +551,7 @@ func New(args Args) (*Loader, error) { RootNetworkNamespace: netns, ApplicationCores: uint(args.NumCPU), Vdso: vdso, + VdsoParams: params, RootUTSNamespace: kernel.NewUTSNamespace(args.Spec.Hostname, args.Spec.Hostname, creds.UserNamespace), RootIPCNamespace: kernel.NewIPCNamespace(creds.UserNamespace), PIDNamespace: kernel.NewRootPIDNamespace(creds.UserNamespace),