arm64: copy the master page on CoW instead of zeroing clean pages#84
Merged
Conversation
cow_page() zeroed the new page whenever DESC_DIRTY was clear, treating "not dirty" as "is zero". But split_l2_block() stamps all 512 split 4KB L3 entries with the parent 2MB block's single flag set, so a block that holds non-zero .data whose DIRTY bit does not reflect that content yields !dirty 4KB pages. CoW then zeroed those pages, destroying initialized data: e.g. glibc's stdin FILE struct pointers (_IO_read_ptr) read back NULL, faulting the guest with an EL0 data abort (ESR 0x92000007, L3 translation fault) on the first buffered read after a fork. Layout- sensitive -- only triggers when non-zero .data shares a 2MB block with that wrong DIRTY bit (a guest with a large BSS array shifts it there). The master page is always the CoW source of truth, so duplicate its actual bytes; only honor an explicit zero request (options.zeroes). This drops a fragile "!dirty => zero" optimization in favor of correctness; the page is being written anyway, so set_dirty() follows immediately. The sibling shortcut in the file-backed RO promotion path (writable_page_at) has the same pattern and likely needs the same treatment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds tests/unit/arm64_fork.cpp (the first arm64 fork unit test; fork.cpp and reset.cpp are AMD64-only) covering the cow_page() fix. The bug is layout-sensitive in the field, so the test constructs the triggering state deterministically: a zero-initialized .bss buffer is cloneable with DESC_DIRTY clear after prepare_copy_on_write(), and the host plants non-zero sentinels straight into the master backing via unsafe_memory_at() -- the same dirty-bit-bypassing path the ELF loader uses for .data. A fork then writes one offset of each page (forcing CoW) and reads a sentinel at another offset of the same page. With the bug the CoW zeroes the page and the sentinel is lost (161/1024 survive); with the fix all 1024 survive. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
|
LGTM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On arm64,
cow_page()treated a clearDESC_DIRTYbit as "this page is zero" andzeroed the freshly-allocated copy-on-write page. But
split_l2_block()stamps all512 split 4KB L3 entries with the parent 2MB block's single flag set, so a block
that holds non-zero
.datawhose DIRTY bit doesn't reflect that content yields!dirty4KB pages. CoW then zeroed those pages on first write after a fork,destroying initialized data.
Observed as glibc's stdin
FILEstruct pointers (_IO_read_ptr) reading back NULL,faulting the guest with an EL0 data abort (ESR
0x92000007, L3 translation fault)on the first buffered read in a fork. Layout-sensitive: only triggers when non-zero
.datashares a 2MB block with the wrong DIRTY bit (a guest with a large BSS arrayshifts it there).
Fix
The master page is always the CoW source of truth, so duplicate its actual bytes;
only honor an explicit zero request (
options.zeroes). This drops a fragile"
!dirty⇒ zero" optimization in favor of correctness — the page is being writtenimmediately afterward anyway, so
set_dirty()follows.Follow-ups (not in this PR)
writable_page_at()carries the same"
!dirty⇒ zero" shortcut and likely needs the same treatment.writable_page_at()has a structurally identical guard(
options.zeroes || (pt[e] & PDE64_DIRTY) == 0); not addressed here.Regression test
Added
tests/unit/arm64_fork.cpp— the first arm64 fork unit test (fork.cpp/reset.cppare AMD64-only). The field bug is layout-sensitive, so rather thanchase a fragile
.data/.bsslayout the test constructs the triggering statedeterministically: a zero-initialized
.bssbuffer is cloneable withDESC_DIRTYclear after
prepare_copy_on_write(), and the host plants non-zero sentinelsstraight into the master backing via
unsafe_memory_at()— the samedirty-bit-bypassing path the ELF loader uses for
.data. A fork then writes oneoffset of each page (forcing CoW) and reads a sentinel at another offset of the
same page.
🤖 Generated with Claude Code