-
Notifications
You must be signed in to change notification settings - Fork 202
Add UFFD write protection #1385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
bugbot run |
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Bugbot reviewed your changes and found no bugs!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds comprehensive UFFD (userfaultfd) write-protection support to track dirty pages, enabling more efficient snapshot creation. The implementation introduces a dual-tracking mechanism: read accesses are tracked via missingRequests and writes via writeRequests, with write-protected pages being unprotected on write faults and marked as dirty.
Key Changes:
- Implements write-protection flow: reads copy with
UFFDIO_COPY_MODE_WP, writes to protected pages triggerWP+WRITEfaults that remove protection and mark pages dirty - Refactors fd handling from
uffdFdto interface-baseduffdiowithFdtype, introducingErrFdExitsentinel for clean termination - Updates
Mapping.GetOffsetreturn type fromuint64touintptrfor page size consistency
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| userfaultfd_write_protection_test.go | Adds comprehensive write-protection tests covering standard/hugepages, serial/parallel operations |
| userfaultfd_test.go | Extensive test suite for UFFD events, request settling, and random operation sequences |
| userfaultfd_process_helpers_test.go | Updates cross-process test infrastructure with separate accessed/dirty offset tracking and improved signal handling |
| userfaultfd_missing_write_test.go | Enhances missing write tests with write-after-write scenarios and dirty offset validation |
| userfaultfd_missing_test.go | Updates missing read tests to use unified operation execution |
| userfaultfd_helpers_test.go | Adds executeOperation helper and accessed utility function |
| userfaultfd.go | Core implementation: adds uffdio interface, dual tracking (missingRequests/writeRequests), handleWriteProtected method |
| fd_helpers_test.go | Introduces mockFd for isolated testing and helpers for fd/API configuration |
| fd.go | Adds WP constants, writeProtect method, refactors to Fd type with interface compliance |
| uffd.go | Updates to use new Fd type and handle ErrFdExit as normal termination |
| mapping_test.go | Updates assertions for uintptr return type change |
| mapping.go | Changes GetOffset page-size return from uint64 to uintptr |
| fdexit.go | Adds ErrFdExit sentinel error for clean exit signaling |
Comments suppressed due to low confidence (3)
packages/orchestrator/internal/sandbox/uffd/userfaultfd/userfaultfd_process_helpers_test.go:287
- Typo in variable name:
accessedOffsestsSignalshould beaccessedOffsetsSignal(missing 't' in 'offsets').
packages/orchestrator/internal/sandbox/uffd/userfaultfd/userfaultfd_process_helpers_test.go:298 - Typo in variable name:
accessedOffsestsSignalshould beaccessedOffsetsSignal(missing 't' in 'offsets'). This is used in signal.Notify and select case, so it should be consistent.
packages/orchestrator/internal/sandbox/uffd/userfaultfd/userfaultfd_missing_write_test.go:13 - Typo in comment: "flakyness" should be "flakiness".
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/orchestrator/internal/sandbox/uffd/userfaultfd/userfaultfd_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
packages/orchestrator/internal/sandbox/uffd/userfaultfd/userfaultfd.go
Outdated
Show resolved
Hide resolved
…nal error in the failure message when reregistering memory region with write protection.
|
I'll test the WP functionality with FC 1.10. |
|
Note
Add UFFD write-protection support (register, handle WP faults, remove WP) and switch dirty tracking to writes; update mapping API and orchestrator integration with graceful fd-exit.
UFFDIO_WRITEPROTECT,UFFDIO_COPY_MODE_WP, flags, andFd.writeProtect.UFFDIO_REGISTER_MODE_WP|MISSINGinNewUserfaultfdFromFdand abstract fd viauffdiointerface.COPYwithMODE_WP.COPY(no WP) and mark dirty.missingRequests,writeRequests);Dirty()now returns write-based tracker.fd(), returnfdexit.ErrFdExiton exit pipe, improved EINTR/EAGAIN handling.Unregister/copy/sizes normalized touintptr; close via interface.uffd/uffd.go):userfaultfd.Fdto constructor; treatfdexit.ErrFdExitas normal shutdown;Stop()signals exit.ErrFdExitand propagate on graceful termination.Mapping.GetOffsetpage size type fromuint64touintptr.Written by Cursor Bugbot for commit baa5ec3. This will update automatically on new commits. Configure here.