From ce9ea8c091cf24c3960c78ef15c14d5af4f22f82 Mon Sep 17 00:00:00 2001 From: Nikita Kalyazin Date: Wed, 27 May 2026 14:14:17 +0100 Subject: [PATCH 1/2] fix(uffd): prevent UFFDIO_COPY on closed fd in Prefault Prefault() and Close() could race: Close() freed the uffd fd number while a prefetcher goroutine was about to acquire settleRequests.RLock and call UFFDIO_COPY. If the OS recycled the fd to a non-uffd file between the close and the ioctl, the syscall returned ENOTTY (seen in production from 2026-04-26 onward, worsening after PR #2522 added an extra fd close per session). Fix by making Close() acquire settleRequests.Lock() before closing the fd and setting a `closed` flag. Prefault() checks the flag immediately after acquiring RLock; if set, it returns nil without touching the fd. This ensures the fd is only closed after all in-flight UFFDIO_COPY callers have released the read-lock. Also add faultPhaseBeforePrefaultRLock test hook so a regression test can deterministically park Prefault before the RLock, let Close() run, and verify the closed-check path. Signed-off-by: Nikita Kalyazin --- .../pkg/sandbox/uffd/userfaultfd/prefault.go | 14 ++++++++++ .../sandbox/uffd/userfaultfd/userfaultfd.go | 26 +++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/packages/orchestrator/pkg/sandbox/uffd/userfaultfd/prefault.go b/packages/orchestrator/pkg/sandbox/uffd/userfaultfd/prefault.go index c338943522..97b72e1c5b 100644 --- a/packages/orchestrator/pkg/sandbox/uffd/userfaultfd/prefault.go +++ b/packages/orchestrator/pkg/sandbox/uffd/userfaultfd/prefault.go @@ -13,9 +13,23 @@ import ( // Prefault proactively copies a page to guest memory at the given offset // to speed up sandbox starts. EEXIST (already mapped) is handled gracefully. func (u *Userfaultfd) Prefault(ctx context.Context, offset int64, data []byte) error { + // Test hook: fires before settleRequests.RLock so that a test can park + // the goroutine here, call Close() concurrently, then release and observe + // that the closed check below returns nil without calling UFFDIO_COPY. + if h := u.testFaultHook.Load(); h != nil { + (*h)(0, faultPhaseBeforePrefaultRLock) + } + u.settleRequests.RLock() defer u.settleRequests.RUnlock() + // Close() sets closed under Lock(). Seeing it here under RLock means the + // fd is already closed and its number may have been recycled by the OS — + // skip silently instead of calling UFFDIO_COPY and getting EBADF/ENOTTY. + if u.closed { + return nil + } + ctx, span := tracer.Start(ctx, "prefault page") defer span.End() diff --git a/packages/orchestrator/pkg/sandbox/uffd/userfaultfd/userfaultfd.go b/packages/orchestrator/pkg/sandbox/uffd/userfaultfd/userfaultfd.go index 2c581c9eae..096a53c571 100644 --- a/packages/orchestrator/pkg/sandbox/uffd/userfaultfd/userfaultfd.go +++ b/packages/orchestrator/pkg/sandbox/uffd/userfaultfd/userfaultfd.go @@ -99,6 +99,11 @@ type Userfaultfd struct { // testFaultHook is set only by SetTestFaultHook in test builds. testFaultHook atomic.Pointer[func(uintptr, faultPhase)] + // closed is set by Close() under settleRequests.Lock(). Prefault() checks + // it under settleRequests.RLock() so it never calls UFFDIO_COPY on a fd + // that has already been closed (and potentially recycled by the OS). + closed bool + logger logger.Logger } @@ -108,6 +113,11 @@ type faultPhase uint8 const ( faultPhaseBeforeRLock faultPhase = iota faultPhaseBeforeFaultPage + // faultPhaseBeforePrefaultRLock fires inside Prefault(), before acquiring + // settleRequests.RLock. Used by TestPrefaultConcurrentWithClose to park + // Prefault here, call Close() concurrently, and verify the closed flag is + // checked after the RLock is acquired. + faultPhaseBeforePrefaultRLock ) // faultOutcome is the terminal classification of a faultPage call. @@ -655,6 +665,22 @@ func (u *Userfaultfd) PageSize() int64 { } func (u *Userfaultfd) Close() error { + // Hold the write lock for the entire close sequence so that: + // (a) any Prefault() caller currently holding RLock finishes its + // UFFDIO_COPY before the fd number is freed and potentially + // recycled by the OS; + // (b) Close() is idempotent — a second call sees closed==true and + // returns immediately without touching already-freed fds. + // In production Serve() drains all workers (u.wg.Wait) before + // returning, so this Lock() is always uncontended when Close() fires. + u.settleRequests.Lock() + defer u.settleRequests.Unlock() + + if u.closed { + return nil + } + u.closed = true + syscall.Close(u.wakeupPipe[0]) syscall.Close(u.wakeupPipe[1]) From 64a02ef513497e30287d65bf65c8bf7a398f598c Mon Sep 17 00:00:00 2001 From: Nikita Kalyazin Date: Wed, 27 May 2026 14:14:27 +0100 Subject: [PATCH 2/2] test(uffd): add regression test for Prefault/Close race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestPrefaultConcurrentWithClose deterministically reproduces the race that caused ENOTTY/EBADF in production: a prefetcher goroutine is parked at faultPhaseBeforePrefaultRLock (before acquiring settleRequests.RLock), Close() is called to completion, and then the goroutine is released. The test asserts Prefault returns nil rather than an error from UFFDIO_COPY on a closed or recycled fd. The test is in-process (no cross-process harness needed — Prefault is called directly and short-circuits before any page-fault kernel interaction). The setup uses a real uffd fd so Close() can safely close a valid fd number. Also document BeforePrefaultRLock in the testharness Point constants so the value is named if cross-process tests ever need to park here. Signed-off-by: Nikita Kalyazin --- .../uffd/testutils/testharness/barriers.go | 3 + .../uffd/userfaultfd/close_race_test.go | 121 ++++++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 packages/orchestrator/pkg/sandbox/uffd/userfaultfd/close_race_test.go diff --git a/packages/orchestrator/pkg/sandbox/uffd/testutils/testharness/barriers.go b/packages/orchestrator/pkg/sandbox/uffd/testutils/testharness/barriers.go index 6094f4b74f..830bdce005 100644 --- a/packages/orchestrator/pkg/sandbox/uffd/testutils/testharness/barriers.go +++ b/packages/orchestrator/pkg/sandbox/uffd/testutils/testharness/barriers.go @@ -15,6 +15,9 @@ const ( BeforeRLock Point = iota // BeforeFaultPage parks after settleRequests.RLock, before UFFDIO_COPY. BeforeFaultPage + // BeforePrefaultRLock parks inside Prefault(), before settleRequests.RLock. + // Value must stay in sync with faultPhaseBeforePrefaultRLock in the parent package. + BeforePrefaultRLock ) // Registry is the child-side barrier store consulted by the per-fault hook. diff --git a/packages/orchestrator/pkg/sandbox/uffd/userfaultfd/close_race_test.go b/packages/orchestrator/pkg/sandbox/uffd/userfaultfd/close_race_test.go new file mode 100644 index 0000000000..1c27ce2119 --- /dev/null +++ b/packages/orchestrator/pkg/sandbox/uffd/userfaultfd/close_race_test.go @@ -0,0 +1,121 @@ +//go:build linux + +package userfaultfd + +import ( + "context" + "syscall" + "testing" + "unsafe" + + "github.com/stretchr/testify/require" + + "github.com/e2b-dev/infra/packages/orchestrator/pkg/sandbox/uffd/memory" + "github.com/e2b-dev/infra/packages/shared/pkg/logger" + "github.com/e2b-dev/infra/packages/shared/pkg/storage/header" +) + +// TestPrefaultConcurrentWithClose is a regression test for the race between +// Prefault() and Close() that produced ENOTTY/EBADF in production: +// +// Prefault Close +// (about to acquire RLock) +// acquires Lock +// closed = true +// fd.close() ← fd freed/recycled +// releases Lock +// acquires RLock +// sees closed == true → return nil ✓ +// +// Without the fix, Prefault had no closed check and would call UFFDIO_COPY +// on the closed (and potentially recycled) fd, returning EBADF or ENOTTY. +// +// The test uses faultPhaseBeforePrefaultRLock to park Prefault before the +// RLock acquisition, lets Close() run to completion, then releases the park +// and asserts Prefault returns nil. +func TestPrefaultConcurrentWithClose(t *testing.T) { + t.Parallel() + + withRaceContext(t, func(ctx context.Context) { + // Create a real userfaultfd so Close() can close a valid fd. We do + // NOT call configureApi/register because the test short-circuits via + // the closed flag before any UFFDIO_COPY is attempted. + uffdFd, err := newFd(syscall.O_CLOEXEC | syscall.O_NONBLOCK) + require.NoError(t, err) + + // Minimal page-sized backing store (content doesn't matter; Prefault + // won't reach ReadAt in the short-circuit path). + pageData := make([]byte, header.PageSize) + src := NewMemorySlicer(pageData, int64(header.PageSize)) + + // A single-region mapping anchored at a real heap address. + // GetHostVirtAddr is called after the closed check, so the address + // only needs to satisfy NewUserfaultfdFromFd's region validation. + regionBuf := make([]byte, header.PageSize) + mapping := memory.NewMapping([]memory.Region{{ + BaseHostVirtAddr: uintptr(unsafe.Pointer(®ionBuf[0])), + Size: uintptr(header.PageSize), + Offset: 0, + PageSize: uintptr(header.PageSize), + }}) + + log, err := logger.NewDevelopmentLogger() + require.NoError(t, err) + + u, err := NewUserfaultfdFromFd(uintptr(uffdFd), src, mapping, log) + require.NoError(t, err) + + // Barrier channels: park Prefault at faultPhaseBeforePrefaultRLock so + // Close() can run to completion before the RLock is acquired. + arrived := make(chan struct{}) + release := make(chan struct{}) + + u.SetTestFaultHook(func(_ uintptr, phase faultPhase) { + if phase != faultPhaseBeforePrefaultRLock { + return + } + // Signal arrival exactly once (guard against re-entry). + select { + case <-arrived: + default: + close(arrived) + } + // Wait for the test to release us. + select { + case <-release: + case <-ctx.Done(): + } + }) + + prefaultErrs := make(chan error, 1) + go func() { + prefaultErrs <- u.Prefault(ctx, 0, pageData) + }() + + // Wait for Prefault to park at the pre-RLock hook. + select { + case <-arrived: + case <-ctx.Done(): + t.Fatal("Prefault goroutine did not reach pre-RLock hook within budget") + } + + // Close the uffd while Prefault is parked. + // Pre-fix: fd gets closed and may be recycled; Prefault later calls + // UFFDIO_COPY on it and returns EBADF or ENOTTY. + // Post-fix: Close() holds Lock(); Prefault acquires RLock, sees + // closed==true, returns nil without touching the fd. + require.NoError(t, u.Close()) + + // Release the parked Prefault goroutine. + close(release) + + select { + case err := <-prefaultErrs: + require.NoError(t, err, + "Prefault must return nil after concurrent Close() — "+ + "a non-nil error means UFFDIO_COPY was attempted on the closed fd (EBADF/ENOTTY regression)") + case <-ctx.Done(): + t.Fatal("Prefault goroutine did not complete after hook release") + } + }) +}