kernel: improve tcpip.Timer implementation

- Move ktime.VariableTimer to kernel.timekeeperTcpipTimer, its only use case.
  This allows timekeeperTcpipTimer to use concrete types kernel.timekeeperClock
  and ktime.SampledTimer instead of ktime.Clock and ktime.Timer, saving a tiny
  amount of memory (interface values consist of two pointers) and CPU (for
  interface method calls).

- Fix a bug where timekeeperTcpipTimer expiration can cancel a racing call to
  timekeeperTcpipTimer.Reset() (see use of new field
  timekeeperTcpipTimer.resets).

- Define Listener.NotifyTimer directly on timekeeperTcpipTimer (dropping
  ktime.functionNotifier), and move goroutine spawning from the anonymous
  function in ktime.AfterFunc() into timekeeperTcpipTimer.NotifyTimer(). This
  slightly simplifies the control flow and saves an allocation for the
  anonymous function object.

- Use monotonicClock rather than realtimeClock. It doesn't make sense for
  time-of-day clock adjustments to affect netstack timeouts, and this is
  consistent with tcpip.stdClock => time.AfterFunc => runtime.timer.

PiperOrigin-RevId: 695504159

pkg/sentry/kernel/BUILD

@@ -105,6 +105,13 @@ declare_mutex(
     prefix = "threadGroupTimer",
 )
 
+declare_mutex(
+    name = "timekeeper_tcpip_timer_mutex",
+    out = "timekeeper_tcpip_timer_mutex.go",
+    package = "kernel",
+    prefix = "timekeeperTcpipTimer",
+)
+
 declare_mutex(
     name = "cgroup_mounts_mutex",
     out = "cgroup_mounts_mutex.go",
@@ -290,6 +297,7 @@ go_library(
         "threads_impl.go",
         "timekeeper.go",
         "timekeeper_state.go",
+        "timekeeper_tcpip_timer_mutex.go",
         "tty.go",
         "user_counters_mutex.go",
         "uts_namespace.go",

pkg/sentry/kernel/kernel.go

@@ -20,6 +20,7 @@
 //
 //	Kernel.extMu
 //	  TTY.mu
+//	  timekeeperTcpipTimer.mu
 //	  ThreadGroup.timerMu
 //	    Locks acquired by ktime.Timer methods
 //	      TaskSet.mu

pkg/sentry/kernel/timekeeper.go

@@ -170,32 +170,6 @@ func (t *Timekeeper) SetClocks(c sentrytime.Clocks, params *VDSOParamPage) {
 	}
 }
 
-var _ tcpip.Clock = (*Timekeeper)(nil)
-
-// Now implements tcpip.Clock.
-func (t *Timekeeper) Now() time.Time {
-	nsec, err := t.GetTime(sentrytime.Realtime)
-	if err != nil {
-		panic("timekeeper.GetTime(sentrytime.Realtime): " + err.Error())
-	}
-	return time.Unix(0, nsec)
-}
-
-// NowMonotonic implements tcpip.Clock.
-func (t *Timekeeper) NowMonotonic() tcpip.MonotonicTime {
-	nsec, err := t.GetTime(sentrytime.Monotonic)
-	if err != nil {
-		panic("timekeeper.GetTime(sentrytime.Monotonic): " + err.Error())
-	}
-	var mt tcpip.MonotonicTime
-	return mt.Add(time.Duration(nsec) * time.Nanosecond)
-}
-
-// AfterFunc implements tcpip.Clock.
-func (t *Timekeeper) AfterFunc(d time.Duration, f func()) tcpip.Timer {
-	return ktime.AfterFunc(t.realtimeClock, d, f)
-}
-
 // startUpdater starts an update goroutine that keeps the clocks updated.
 //
 // mu must be held.
@@ -354,3 +328,116 @@ func (tc *timekeeperClock) Now() ktime.Time {
 func (tc *timekeeperClock) NewTimer(l ktime.Listener) ktime.Timer {
 	return ktime.NewSampledTimer(tc, l)
 }
+
+var _ tcpip.Clock = (*Timekeeper)(nil)
+
+// Now implements tcpip.Clock.
+func (t *Timekeeper) Now() time.Time {
+	nsec, err := t.GetTime(sentrytime.Realtime)
+	if err != nil {
+		panic("timekeeper.GetTime(sentrytime.Realtime): " + err.Error())
+	}
+	return time.Unix(0, nsec)
+}
+
+// NowMonotonic implements tcpip.Clock.
+func (t *Timekeeper) NowMonotonic() tcpip.MonotonicTime {
+	nsec, err := t.GetTime(sentrytime.Monotonic)
+	if err != nil {
+		panic("timekeeper.GetTime(sentrytime.Monotonic): " + err.Error())
+	}
+	var mt tcpip.MonotonicTime
+	return mt.Add(time.Duration(nsec) * time.Nanosecond)
+}
+
+// AfterFunc implements tcpip.Clock.
+func (t *Timekeeper) AfterFunc(d time.Duration, f func()) tcpip.Timer {
+	timer := &timekeeperTcpipTimer{
+		clock: t.monotonicClock,
+		fn:    f,
+	}
+	timer.Reset(d)
+	return timer
+}
+
+// timekeeperTcpipTimer implements tcpip.Timer by wrapping a ktime.SampledTimer.
+// tcpip.Timer does not define a Destroy method, so each timer expiration and
+// each call to Timer.Stop() must release all resources by calling
+// ktime.SampledTimer.Destroy().
+type timekeeperTcpipTimer struct {
+	// immutable
+	clock *timekeeperClock
+	fn    func()
+
+	// mu protects t.
+	mu timekeeperTcpipTimerMutex
+
+	// t stores the latest running Timer. This is replaced whenever Reset is
+	// called since Timer cannot be restarted once it has been Destroyed by Stop.
+	//
+	// This field is nil iff Stop has been called.
+	t *ktime.SampledTimer
+
+	// resets is the number of times Reset has been called. resets is written
+	// with both mu and ktime.SampledTimer locks held, so it may be read with
+	// either or both locks held.
+	resets int
+}
+
+// Stop implements tcpip.Timer.Stop.
+func (r *timekeeperTcpipTimer) Stop() bool {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+
+	if r.t == nil {
+		return false
+	}
+	_, lastSetting := r.t.Set(ktime.Setting{}, nil)
+	r.t.Destroy()
+	r.t = nil
+	return lastSetting.Enabled
+}
+
+// stopExpired is equivalent to Stop, but is called when the timer expires.
+func (r *timekeeperTcpipTimer) stopExpired(reset int) {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+
+	if r.t == nil || r.resets != reset {
+		return
+	}
+	r.t.Destroy()
+	r.t = nil
+}
+
+// Reset implements tcpip.Timer.Reset.
+func (r *timekeeperTcpipTimer) Reset(d time.Duration) {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+
+	if r.t == nil {
+		r.t = ktime.NewSampledTimer(r.clock, r)
+	}
+	r.t.Set(ktime.Setting{
+		Enabled: true,
+		Next:    r.clock.Now().Add(d),
+	}, r.incResets)
+}
+
+func (r *timekeeperTcpipTimer) incResets() {
+	r.resets++
+}
+
+// NotifyTimer implements ktime.Listener.NotifyTimer.
+func (r *timekeeperTcpipTimer) NotifyTimer(exp uint64) {
+	// Implementations of ktime.Listener.NotifyTimer() can't call Timer methods
+	// due to lock ordering, so we must call r.t.Destroy() from another
+	// goroutine. We also must call r.stopExpired() rather than r.Stop(), since
+	// the latter might cancel an unrelated call to r.Reset() that happens
+	// between now and when this goroutine runs.
+	thisReset := r.resets
+	go func() {
+		r.stopExpired(thisReset)
+		r.fn()
+	}()
+}

pkg/sentry/ktime/BUILD

@@ -65,7 +65,6 @@ go_library(
         "synthetic_timer_list.go",
         "synthetic_timer_set.go",
         "uint64_range.go",
-        "util.go",
     ],
     visibility = ["//pkg/sentry:internal"],
     deps = [

pkg/sentry/ktime/util.go

@@ -1,127 +0,0 @@
-// Copyright 2020 The gVisor Authors.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package ktime
-
-import (
-	"sync"
-	"time"
-)
-
-// AfterFunc waits for duration to elapse according to clock then runs fn.
-// The timer is started immediately and will fire exactly once.
-func AfterFunc(clock Clock, duration time.Duration, fn func()) *VariableTimer {
-	timer := &VariableTimer{
-		clock: clock,
-	}
-	timer.notifier = functionNotifier{
-		fn: func() {
-			// tcpip.Timer.Stop() explicitly states that the function is called in a
-			// separate goroutine that Stop() does not synchronize with.
-			// Timer.Destroy() synchronizes with calls to Listener.NotifyTimer().
-			// This is semantically meaningful because, in the former case, it's
-			// legal to call tcpip.Timer.Stop() while holding locks that may also be
-			// taken by the function, but this isn't so in the latter case. Most
-			// immediately, Timer calls Listener.NotifyTimer() while holding
-			// Timer.mu. A deadlock occurs without spawning a goroutine:
-			//   T1: (Timer expires)
-			//     => Timer.Tick()           <- Timer.mu.Lock() called
-			//     => Listener.NotifyTimer()
-			//     => Timer.Stop()
-			//     => Timer.Destroy()        <- Timer.mu.Lock() called, deadlock!
-			//
-			// Spawning a goroutine avoids the deadlock:
-			//   T1: (Timer expires)
-			//     => Timer.Tick()           <- Timer.mu.Lock() called
-			//     => Listener.NotifyTimer() <- Launches T2
-			//   T2:
-			//     => Timer.Stop()
-			//     => Timer.Destroy()        <- Timer.mu.Lock() called, blocks
-			//   T1:
-			//     => (returns)              <- Timer.mu.Unlock() called
-			//   T2:
-			//     => (continues)            <- No deadlock!
-			go func() {
-				timer.Stop()
-				fn()
-			}()
-		},
-	}
-	timer.Reset(duration)
-	return timer
-}
-
-// VariableTimer is a resettable timer with variable duration expirations.
-// Implements tcpip.Timer, which does not define a Destroy method; instead, all
-// resources are released after timer expiration and calls to Timer.Stop.
-//
-// Must be created by AfterFunc.
-type VariableTimer struct {
-	// clock is the time source. clock is immutable.
-	clock Clock
-
-	// notifier is called when the Timer expires. notifier is immutable.
-	notifier functionNotifier
-
-	// mu protects t.
-	mu sync.Mutex
-
-	// t stores the latest running Timer. This is replaced whenever Reset is
-	// called since Timer cannot be restarted once it has been Destroyed by Stop.
-	//
-	// This field is nil iff Stop has been called.
-	t Timer
-}
-
-// Stop implements tcpip.Timer.Stop.
-func (r *VariableTimer) Stop() bool {
-	r.mu.Lock()
-	defer r.mu.Unlock()
-
-	if r.t == nil {
-		return false
-	}
-	_, lastSetting := r.t.Set(Setting{}, nil)
-	r.t.Destroy()
-	r.t = nil
-	return lastSetting.Enabled
-}
-
-// Reset implements tcpip.Timer.Reset.
-func (r *VariableTimer) Reset(d time.Duration) {
-	r.mu.Lock()
-	defer r.mu.Unlock()
-
-	if r.t == nil {
-		r.t = r.clock.NewTimer(&r.notifier)
-	}
-
-	r.t.Set(Setting{
-		Enabled: true,
-		Period:  0,
-		Next:    r.clock.Now().Add(d),
-	}, nil)
-}
-
-// functionNotifier is a TimerListener that runs a function.
-//
-// functionNotifier cannot be saved or loaded.
-type functionNotifier struct {
-	fn func()
-}
-
-// NotifyTimer implements ktime.TimerListener.NotifyTimer.
-func (f *functionNotifier) NotifyTimer(uint64) {
-	f.fn()
-}