Skip to content

Commit a5b7936

Browse files
EtiennePerotEtiennePerot
committed
systrap: Recognize host stalls and don't kill stubs in that case.
This detects cases like SIGSTOP, cgroup `freeze`, etc where the host clock advances but the process doesn't, for legitimate reasons. It treats `futex` sleeps that return more than a second later than the configured timeout as a host-caused stall, moves the deadline back by the stall amount, and gives stub threads at least one more checkup interval. A stub that is really stuck is still killed if the sandbox has actually run for the cumulative duration of stuck timeout. This also removes a `time.Now()` on the common path, saving about 50ns when the state is already changed (only on the slow path but you know, still nice). Fixes #16160.
1 parent 4fd5863 commit a5b7936

3 files changed

Lines changed: 140 additions & 26 deletions

File tree

‎pkg/sentry/platform/systrap/shared_context.go‎

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,7 @@ const (
250250
contextPreemptTimeout = 10 * time.Millisecond
251251
contextCheckupTimeout = 5 * time.Second
252252
stuckContextTimeout = 30 * time.Second
253+
hostStallThreshold = time.Second
253254
)
254255

255256
var (
@@ -279,24 +280,42 @@ func (sc *sharedContext) sleepOnState(state sysmsg.ContextState) error {
279280
}
280281

281282
func (sc *sharedContext) sleepOnStateWithTimeout(state sysmsg.ContextState, stuckTimeout, checkupTimeout time.Duration) error {
282-
timeout := unix.NsecToTimespec(contextPreemptTimeout.Nanoseconds())
283+
sleep := contextPreemptTimeout
284+
timeout := unix.NsecToTimespec(sleep.Nanoseconds())
283285
interruptsSent := 0
284-
deadline := time.Now().Add(stuckTimeout)
286+
var now, deadline time.Time
285287
for sc.state() == state {
286288
if sc.subprocess.dead.Load() {
287289
return errDeadSubprocess
288290
}
289291
errno := sc.shared.SleepOnState(state, &timeout)
290-
if errno == 0 {
291-
continue
292+
if errno == 0 && sc.state() != state {
293+
return nil
292294
}
293-
if errno != unix.ETIMEDOUT {
295+
if errno != 0 && errno != unix.ETIMEDOUT {
294296
panic(fmt.Sprintf("error waiting for state: %v", errno))
295297
}
298+
// The clock is read only on wakeups that do not end the wait, so
299+
// the stuck timeout starts at the first such wakeup.
300+
prev := now
301+
now = time.Now()
302+
if prev.IsZero() {
303+
deadline = now.Add(stuckTimeout)
304+
} else if stall := now.Sub(prev) - sleep; stall > hostStallThreshold {
305+
// Give the stub threads at least one checkup interval to
306+
// respond once the sandbox runs again.
307+
deadline = deadline.Add(stall)
308+
if checkupDeadline := now.Add(checkupTimeout); checkupDeadline.After(deadline) {
309+
deadline = checkupDeadline
310+
}
311+
}
312+
if errno == 0 {
313+
continue
314+
}
296315
if !sc.subprocess.alive() {
297316
return errDeadSubprocess
298317
}
299-
if interruptsSent > 0 && time.Now().After(deadline) {
318+
if interruptsSent > 0 && now.After(deadline) {
300319
// The context can recover between the futex timeout and here.
301320
if sc.state() != state {
302321
return nil
@@ -315,10 +334,11 @@ func (sc *sharedContext) sleepOnStateWithTimeout(state sysmsg.ContextState, stuc
315334
if interruptsSent == 0 {
316335
// Guarantee a full checkup interval if the first interrupt was
317336
// sent close to the deadline.
318-
if checkupDeadline := time.Now().Add(checkupTimeout); checkupDeadline.After(deadline) {
337+
if checkupDeadline := now.Add(checkupTimeout); checkupDeadline.After(deadline) {
319338
deadline = checkupDeadline
320339
}
321-
timeout = unix.NsecToTimespec(checkupTimeout.Nanoseconds())
340+
sleep = checkupTimeout
341+
timeout = unix.NsecToTimespec(sleep.Nanoseconds())
322342
}
323343
interruptsSent++
324344
}

‎pkg/sentry/platform/systrap/shared_context_test.go‎

Lines changed: 91 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,10 @@
1515
package systrap
1616

1717
import (
18+
"bytes"
1819
"errors"
20+
"fmt"
21+
"io"
1922
"os"
2023
"os/exec"
2124
"sync/atomic"
@@ -30,21 +33,36 @@ import (
3033
"gvisor.dev/gvisor/pkg/sentry/platform/systrap/sysmsg"
3134
)
3235

36+
// startStuckChild starts a child process that never responds
37+
// as a stand-in for a stub process.
38+
func startStuckChild() (*exec.Cmd, error) {
39+
cmd := exec.Command(os.Args[0])
40+
cmd.Env = append(os.Environ(), childEnv+"=stuck")
41+
if err := cmd.Start(); err != nil {
42+
return nil, fmt.Errorf("cannot start stuck child: %w", err)
43+
}
44+
return cmd, nil
45+
}
46+
47+
func stopChild(cmd *exec.Cmd) {
48+
cmd.Process.Kill()
49+
cmd.Wait()
50+
}
51+
3352
func newTestSharedContext(t *testing.T) *sharedContext {
3453
t.Helper()
3554

36-
cmd := exec.Command(os.Args[0], "-test.run=TestStuckSubprocessHelper")
37-
cmd.Env = append(os.Environ(), "GVISOR_STUCK_SUBPROCESS_HELPER=1")
38-
if err := cmd.Start(); err != nil {
39-
t.Fatalf("failed to start helper subprocess: %v", err)
55+
cmd, err := startStuckChild()
56+
if err != nil {
57+
t.Fatal(err)
4058
}
41-
t.Cleanup(func() {
42-
cmd.Process.Kill()
43-
cmd.Wait()
44-
})
59+
t.Cleanup(func() { stopChild(cmd) })
60+
return newChildSharedContext(cmd.Process.Pid)
61+
}
4562

63+
func newChildSharedContext(pid int) *sharedContext {
4664
threadID := uint32(1)
47-
child := &thread{tgid: int32(cmd.Process.Pid), tid: int32(cmd.Process.Pid)}
65+
child := &thread{tgid: int32(pid), tid: int32(pid)}
4866
queue := &contextQueue{}
4967
queue.init()
5068
atomic.AddUint32(&queue.end, 1)
@@ -122,6 +140,70 @@ func TestSleepOnStateRecoveredContext(t *testing.T) {
122140
}
123141
}
124142

143+
func TestSleepOnStateHostStall(t *testing.T) {
144+
readyRead, readyWrite, err := os.Pipe()
145+
if err != nil {
146+
t.Fatalf("cannot create pipe: %v", err)
147+
}
148+
defer readyRead.Close()
149+
cmd := exec.Command(os.Args[0])
150+
cmd.Env = append(os.Environ(), childEnv+"=hoststall")
151+
cmd.ExtraFiles = []*os.File{readyWrite}
152+
var out bytes.Buffer
153+
cmd.Stdout = &out
154+
cmd.Stderr = &out
155+
err = cmd.Start()
156+
readyWrite.Close()
157+
if err != nil {
158+
t.Fatalf("cannot start child: %v", err)
159+
}
160+
defer cmd.Process.Kill()
161+
if _, err := io.ReadAll(readyRead); err != nil {
162+
t.Fatalf("cannot wait for child readiness: %v", err)
163+
}
164+
time.Sleep(100 * time.Millisecond)
165+
if err := cmd.Process.Signal(unix.SIGSTOP); err != nil {
166+
t.Fatalf("cannot stop child: %v", err)
167+
}
168+
time.Sleep(hostStallThreshold * 2)
169+
if err := cmd.Process.Signal(unix.SIGCONT); err != nil {
170+
t.Fatalf("cannot continue child: %v", err)
171+
}
172+
if err := cmd.Wait(); err != nil {
173+
t.Fatalf("child failed: %v\n%s", err, out.String())
174+
}
175+
}
176+
177+
// hostStallChild waits on a context whose stub thread responds shortly after
178+
// the host stall ends.
179+
// Closes FD 3 once it is about to wait.
180+
func hostStallChild() error {
181+
unix.CloseOnExec(3)
182+
cmd, err := startStuckChild()
183+
if err != nil {
184+
return err
185+
}
186+
defer stopChild(cmd)
187+
sc := newChildSharedContext(cmd.Process.Pid)
188+
go func() {
189+
for last := time.Now(); ; {
190+
time.Sleep(10 * time.Millisecond)
191+
now := time.Now()
192+
if now.Sub(last) > hostStallThreshold {
193+
break
194+
}
195+
last = now
196+
}
197+
time.Sleep(100 * time.Millisecond)
198+
sc.setState(sysmsg.ContextStateSyscall)
199+
}()
200+
os.NewFile(3, "marker").Close()
201+
if err := sc.sleepOnStateWithTimeout(sysmsg.ContextStateNone, 500*time.Millisecond, time.Second); err != nil {
202+
return fmt.Errorf("sleepOnStateWithTimeout got error %v, want nil", err)
203+
}
204+
return nil
205+
}
206+
125207
func TestSleepOnStateDeadSubprocess(t *testing.T) {
126208
sc := newTestSharedContext(t)
127209
sc.subprocess.dead.Store(true)
@@ -304,10 +386,3 @@ func TestInitSyscallThreadPoolExhausted(t *testing.T) {
304386
t.Fatalf("initSyscallThread succeeded when pool was exhausted, want error")
305387
}
306388
}
307-
308-
func TestStuckSubprocessHelper(t *testing.T) {
309-
if os.Getenv("GVISOR_STUCK_SUBPROCESS_HELPER") == "" {
310-
return
311-
}
312-
select {}
313-
}

‎pkg/sentry/platform/systrap/systrap_test.go‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package systrap
1616

1717
import (
18+
"fmt"
1819
"os"
1920
"sync/atomic"
2021
"testing"
@@ -34,9 +35,27 @@ import (
3435
"gvisor.dev/gvisor/pkg/usermem"
3536
)
3637

38+
const childEnv = "SYSTRAP_TEST_CHILD"
39+
3740
func TestMain(m *testing.M) {
38-
cpuid.Initialize()
39-
os.Exit(m.Run())
41+
switch mode := os.Getenv(childEnv); mode {
42+
case "":
43+
cpuid.Initialize()
44+
os.Exit(m.Run())
45+
case "stuck":
46+
for {
47+
unix.Pause()
48+
}
49+
case "hoststall":
50+
if err := hostStallChild(); err != nil {
51+
fmt.Fprintf(os.Stderr, "%v\n", err)
52+
os.Exit(1)
53+
}
54+
os.Exit(0)
55+
default:
56+
fmt.Fprintf(os.Stderr, "unknown %s: %q\n", childEnv, mode)
57+
os.Exit(1)
58+
}
4059
}
4160

4261
// testMemoryManager implements the parts of platform.MemoryManager that

0 commit comments

Comments
 (0)