uffd: add support for UFFD_EVENT_REMOVE events#1896
Conversation
bchalios
force-pushed
the
feat/free-page-reporting
branch
from
aef7cd8 to
eab27ea
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
packages/orchestrator/internal/sandbox/uffd/userfaultfd/userfaultfd.go
Outdated
Show resolved
Hide resolved
packages/orchestrator/internal/sandbox/uffd/userfaultfd/userfaultfd.go
Outdated
Show resolved
Hide resolved
bchalios
force-pushed
the
feat/free-page-reporting
branch
2 times, most recently
from
a1d0a02 to
03e2966
Compare
bchalios
force-pushed
the
feat/free-page-reporting
branch
5 times, most recently
from
b766eb3 to
4d197e4
Compare
packages/orchestrator/internal/sandbox/uffd/userfaultfd/userfaultfd.go
Outdated
Show resolved
Hide resolved
packages/orchestrator/internal/sandbox/uffd/userfaultfd/userfaultfd.go
Outdated
Show resolved
Hide resolved
bchalios
force-pushed
the
feat/free-page-reporting
branch
2 times, most recently
from
e639acf to
88420f2
Compare
bchalios
force-pushed
the
feat/free-page-reporting
branch
8 times, most recently
from
d2e5d09 to
8ea97e4
Compare
bchalios
force-pushed
the
feat/free-page-reporting
branch
from
5d7e375 to
6b3dbf3
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| block.Read, | ||
| source, | ||
| fdExit.SignalExit, | ||
| ) |
There was a problem hiding this comment.
Hardcoded block.Read ignores computed access type
High Severity
The accessType variable is correctly computed from pagefault flags (lines 301–307) but then faultPage is called with a hardcoded block.Read instead of the computed accessType. This causes faultPage to always set UFFDIO_COPY_MODE_WP mode, even for write faults. Write page faults need the WP bit cleared so the write can proceed; keeping it set likely triggers an immediate follow-up WP fault, potentially causing an infinite fault loop.
Additional Locations (1)
There was a problem hiding this comment.
that was a leftover. Fixed
bchalios
force-pushed
the
feat/free-page-reporting
branch
from
6b3dbf3 to
5b0c87b
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| } | ||
|
|
||
| return nil | ||
| }) |
There was a problem hiding this comment.
Race: goroutine can overwrite removed state with faulted
High Severity
Worker goroutines from a previous iteration can call pageTracker.setState(addr, ..., faulted) after the main loop has already processed a UFFD_EVENT_REMOVE and set the same address to removed in a subsequent iteration. Since wg.Wait() is only called on exit, not between iterations, the faulted write can overwrite the removed state. A later pagefault then sees faulted and skips serving the page, causing the guest thread to hang indefinitely.
Additional Locations (1)
| span.AddEvent("prefault: page already faulted or write returned EAGAIN") | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Prefault doesn't update pageTracker or prefetchTracker
Medium Severity
The refactored Prefault calls faultPage but never updates pageTracker to faulted or adds the offset to prefetchTracker on success. The old faultPage handled this internally; now callers are responsible. Serve does it correctly, but Prefault doesn't. This means PrefetchData() loses information about prefaulted pages, degrading future prefetch effectiveness since those pages won't appear in the collected prefetch data.
bchalios
force-pushed
the
feat/free-page-reporting
branch
from
5b0c87b to
691a381
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| span.AddEvent("prefault: page already faulted or write returned EAGAIN") | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Prefault doesn't update pageTracker after mapping page
Medium Severity
Prefault calls faultPage but never calls u.pageTracker.setState(addr, addr+u.pageSize, faulted) after a successful mapping. In Serve, the faulted case is explicitly meant to skip already-prefaulted pages (the comment says "during pre-faulting"), but since Prefault never sets the state, subsequent on-demand pagefaults for the same address will see unfaulted, unnecessarily fetch data from the snapshot source, and attempt a redundant UFFDIO_COPY (which returns EEXIST).
Additional Locations (1)
| // would anyways cause clear the write-protection bit. | ||
| if accessType != block.Write { | ||
| copyMode |= UFFDIO_COPY_MODE_WP | ||
| writeErr = u.fd.copy(addr, u.pageSize, b, mode) |
There was a problem hiding this comment.
Nil source with non-standard page size causes crash
Medium Severity
When source is nil (removed page) and u.pageSize is neither header.PageSize nor header.HugepageSize, the switch falls through to the default branch, which calls source.Slice(...) on a nil interface. This would panic with a nil pointer dereference, crashing the UFFD handler and the sandbox. The constructor validates that all regions share the same block size but doesn't restrict it to exactly these two known values.
| } | ||
|
|
||
| return offsets, nil | ||
| } |
There was a problem hiding this comment.
Test faultedOffsets includes removed pages in results
Low Severity
faultedOffsets iterates all entries in pageTracker.m via for addr := range pt.m without filtering by pageState. This returns addresses in any state (faulted, removed, unfaulted), not just faulted ones. Tests that exercise remove events would get incorrect offset lists, as removed addresses would be included in the "faulted" output.
Bump the swagger file for Firecracker to v1.14 and regenerate APIs/models. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Add support for Firecracker v1.14 and make it the default. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
As we're going to handle UFFD_EVENT_REMOVE events triggerred by
Firecracker, we need to keep track of the state of all the guest memory
pages.
Theis commit introduces 3 states:
* Unfaulted - A page that has not been faulted yet.
* Faulted - A page that we have previously faulted in.
* Removed - A page that we have received a remove event for and
haven't faulted in since.
It also adds the necessary book keeping of page state in all the memory
regions of the guest, along with methods for retrieving and setting the
state of pages.
Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Import a few more bindings that we'll need for handling remove events. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Handle UFFD_EVENT_REMOVE events from the file descriptor. These events are triggerred when Firecracker calls madvise() with MADV_DONTNEED on some memory range that we are tracking. This Firecracker behaviour is support with version 1.14.0 onwards using the free page reporting and hinting features of balloon devices. What this means for us is that, we need to track removed pages because subsequent page faults need to be served with a zero page. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Balloon devices provide memory reclamation facilities through free page reporting, as a more efficient mechanism (in terms of latency and CPU time) than traditional ballooning. Free page reporting instructs the guest to periodically report memory that has been freed, so that we can reclaim it in the host side. It is enabled before starting the sandbox and does not require any further host-side orchestration. Enable free page reporting for all new templates using Firecracker versions >= v1.14.0. Also, allow users to optionally disable it for these versions. Older Firecracker versions don't support the feature. Trying to enable it for those, will return an error. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Do that, so we maintain the dirty page tracking on. Prefaulting does not occur due to write faults, so tracking must be maintained. Writing will clear the bit asynchronously in the kernel. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
userfaultfd_zeropage() does not understand UFFD_COPY_MODE_WP. When zeroing a page we should always be passing 0 in mode (since we always want to unblock the faulting thread). For userfaultfd_copy() we want to provide UFFD_COPY_MODE_WP only when we are copying due to a read. This includes read-triggerred page faults from Firecracker and us pre-faulting. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
During pre-faulting we always recorded an error to the span when `faultPage` returned `!handled` for a block, but not all such cases are actually errors. EEXIST errors means that the page has been faulted already, which can potentially happen since we are pre-faulting and handling on-demand page faults at the same time. EAGAIN happens when a UFFD_EVENT_REMOVE event landed in the UFFD while were trying to handle a page fault. Both those cases are fine from the prefaulter's point of view. In the former, there's nothing to do, on-demand handler won the race. In the latter, we need to let the on-demand page fault handler handle the remove event before we proceed. Only record an error when err != nil. Otherwise, add an event. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
bchalios
force-pushed
the
feat/free-page-reporting
branch
from
691a381 to
a537c09
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| case source == nil && u.pageSize == header.PageSize: | ||
| // Here, `mode` is always 0. The only mode `userfaultfd_zeropage()` understands | ||
| // is UFFDIO_ZEROPAGE_DONT_WAKE, which we never use. | ||
| writeErr = u.fd.zero(addr, u.pageSize, 0) |
There was a problem hiding this comment.
Zero-page path loses write-protection for regular pages
Medium Severity
When a removed page is re-faulted by a read access on regular (4KB) pages, UFFDIO_ZEROPAGE is called with mode 0, discarding the write-protection bit. For hugepages, the equivalent path uses fd.copy with mode (which includes UFFDIO_COPY_MODE_WP for reads), correctly preserving WP. This inconsistency means dirty-page tracking is broken for 4KB removed-then-refaulted pages — subsequent writes won't trigger WP faults and may be missed during snapshotting.
Additional Locations (1)
There was a problem hiding this comment.
UFFDIO_ZEROPAGE does not have a COPY_MODE_WP equivalent.
However, in a sense, cursor is right here. If we have 4K pages, we need to register the page for write protection after we first fault it. That is because we can't mark for write-protections anonymous memory (huge pages are backed by hugetlbfs, so that case works fine).
Enabling Firecracker free-page-reporting feature requires us to handle remove events (UFFD_EVENT_REMOVE) in our userfaultfd handler. These events are triggered whenever Firecracker calls madvise(MADV_DONTNEED) (or similar) on a range of guest memory addresses.
The main thing that changes on our logic is that page faults in a page that has previously been removed need to be served with a zero page rather than a page from the snapshot file.
This commit changes the page fault serving logic to:
This is dependent on the part of #1858 that enables free page reporting on the Firecracker side.
Note
High Risk
High risk because it changes core sandbox memory faulting logic (
userfaultfd) and introduces new Firecracker v1.14 behavior (balloon free-page-reporting), which can affect VM stability, memory correctness, and startup reliability.Overview
Updates the sandbox/template pipeline to support Firecracker v1.14 free-page-reporting: bumps the default Firecracker version, adds
FreePageReportingto the template-manager proto/config and builder APIs/CLI (with version-based defaults and an explicit override flag), configures the balloon device to enable the feature at VM startup, and refactors theuserfaultfdimplementation to read and handleUFFD_EVENT_REMOVEevents by tracking per-page state and zero-filling removed pages (including newZEROPAGEioctl support, deferred retries onEAGAIN, and related memory-mapping API adjustments), alongside refreshed generated Firecracker client operations.Written by Cursor Bugbot for commit a537c09. This will update automatically on new commits. Configure here.