Remove HyperlightPEB file mappings#1380
Conversation
Some linkers emit PT_LOAD segments where filesz == memsz but contain .bss sections whose VMA range overlaps with file bytes from unrelated sections. The loader copies the full segment verbatim, leaving .bss with stale data instead of zeros. Collect NOBITS section ranges (excluding .tbss) during ELF parsing and zero-fill them after loading PT_LOAD segments. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> Co-authored-by: danbugs <danilochiarlone@gmail.com>
Remove the nanvix-unstable-gated file_mappings field from HyperlightPEB and all host-side code that wrote to it: - write_file_mapping_entry in mgr.rs - PEB layout calculations (array sizing, heap offset, getter methods) - PEB file_mapping writes in write_peb and map_file_cow - 3 PEB test functions (multiuse, deferred, multiple_entries) - evolve-time write_file_mapping_entry call Embedders that need file mapping metadata can pass it through init_data instead of the PEB struct. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> Co-authored-by: danbugs <danilochiarlone@gmail.com>
ludfjig
requested review from
danbugs,
dblnz,
devigned,
jprendes,
marosset,
simongdavies and
syntactically
as code owners
ludfjig
added
the
kind/refactor
|
LGTM. dev Nanvix today does use |
There was a problem hiding this comment.
Pull request overview
This PR removes the HyperlightPEB-based file mapping metadata plumbing (count/array descriptor + writers + tests), intending to rely on a different mechanism for conveying mappings to the guest. The diff also includes an ELF loader change to zero-fill SHT_NOBITS sections after loading.
Changes:
- Remove writing/reading of file mapping metadata into the PEB (host-side map + evolve path) and delete the associated PEB validation tests.
- Simplify sandbox memory layout by removing the reserved PEB-adjacent
FileMappingInfoarray and its layout offsets/helpers. - Add NOBITS section range collection and post-load zero-filling in the ELF loader.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hyperlight_host/src/sandbox/uninitialized_evolve.rs | Stops recording file mappings into the PEB during deferred mapping apply. |
| src/hyperlight_host/src/sandbox/initialized_multi_use.rs | Removes PEB count pre-check + PEB write, and deletes PEB-entry tests. |
| src/hyperlight_host/src/sandbox/file_mapping.rs | Adjusts the label field linting after PEB metadata removal. |
| src/hyperlight_host/src/mem/mgr.rs | Deletes the helper that wrote FileMappingInfo entries into the PEB. |
| src/hyperlight_host/src/mem/layout.rs | Removes PEB file-mappings offsets/reserved space and related setup code. |
| src/hyperlight_host/src/mem/elf.rs | Collects/zero-fills NOBITS section ranges during ELF load. |
| src/hyperlight_common/src/mem.rs | Removes the file_mappings field from HyperlightPEB. |
| pub struct HyperlightPEB { | ||
| pub input_stack: GuestMemoryRegion, | ||
| pub output_stack: GuestMemoryRegion, | ||
| pub init_data: GuestMemoryRegion, | ||
| pub guest_heap: GuestMemoryRegion, | ||
| /// File mappings array descriptor. | ||
| /// **Note:** `size` holds the **entry count** (number of valid | ||
| /// [`FileMappingInfo`] entries), NOT a byte size. `ptr` holds the | ||
| /// guest address of the preallocated array (immediately after the | ||
| /// PEB struct). | ||
| #[cfg(feature = "nanvix-unstable")] | ||
| pub file_mappings: GuestMemoryRegion, | ||
| } |
There was a problem hiding this comment.
Now that HyperlightPEB no longer contains a file_mappings descriptor, the documentation around file-mapping metadata being stored/registered in the PEB (e.g., MAX_FILE_MAPPINGS / FileMappingInfo comments) is likely stale. Please update or relocate that documentation so it matches the new way mappings are communicated to the guest.
| // Zero-fill NOBITS sections (e.g. .bss) that were not already | ||
| // covered by the filesz < memsz zeroing above. | ||
| for &(addr, size) in &self.nobits_ranges { | ||
| let sh_start = (addr - base_va) as usize; | ||
| let sh_end = sh_start + size as usize; | ||
| if sh_end <= target.len() { | ||
| target[sh_start..sh_end].fill(0); | ||
| } else { | ||
| tracing::warn!( | ||
| "NOBITS section at VA {:#x} (size {:#x}) extends past loaded image, skipping zero-fill", | ||
| addr, | ||
| size | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
addr - base_va can underflow (or wrap in release) for NOBITS sections whose sh_addr is below the first PT_LOAD vaddr (or for non-ALLOC NOBITS sections with sh_addr == 0). This can lead to panics/out-of-bounds slicing and also allows zero-filling the wrong part of target. Filter NOBITS ranges to SHF_ALLOC, and during zero-fill guard addr >= base_va and clamp the [start,end) range to target.len() instead of skipping the whole fill when it extends past the loaded image.
| // Phase 1: host-side OS work (open file, create mapping) | ||
| let mut prepared = prepare_file_cow(file_path, guest_base, label)?; | ||
|
|
There was a problem hiding this comment.
The map_file_cow API docs still say the optional label is recorded in the PEB FileMappingInfo array, but this PR removes PEB file mapping tracking. Update the doc comment to reflect the new source of truth (e.g., init data/blob) or remove the reference to the PEB to avoid misleading API consumers.
| } | ||
|
|
||
| /// Tests that an explicitly provided label exceeding 63 bytes is rejected. | ||
| /// Tests that an explicitly provided label exceeding 63 bytes is rejected. |
There was a problem hiding this comment.
Duplicate doc comment line (same sentence repeated twice). Remove one of them to keep the test module docs clean.
| /// Tests that an explicitly provided label exceeding 63 bytes is rejected. |
| /// The page-aligned size of the mapping in bytes. | ||
| pub(crate) size: usize, | ||
| /// Null-terminated C-style label for this mapping (max 63 chars + null). | ||
| #[cfg_attr(not(feature = "nanvix-unstable"), allow(unused))] | ||
| #[allow(unused)] | ||
| pub(crate) label: [u8; hyperlight_common::mem::FILE_MAPPING_LABEL_MAX_LEN + 1], | ||
| /// Host-side OS resources. `None` after successful consumption |
There was a problem hiding this comment.
PreparedFileMapping::label appears to no longer be read anywhere (and is now silenced with #[allow(unused)]). If the label is no longer used after removing PEB file mappings, consider deleting the field (and related label-building work) or wiring it into the replacement mechanism so this data isn’t computed/stored unnecessarily.
| let guest_code_offset = 0; | ||
| // The following offsets are to the fields of the PEB struct itself! | ||
| let peb_offset = code_size.next_multiple_of(PAGE_SIZE_USIZE); | ||
| let peb_input_data_offset = peb_offset + offset_of!(HyperlightPEB, input_stack); | ||
| let peb_output_data_offset = peb_offset + offset_of!(HyperlightPEB, output_stack); | ||
| let peb_init_data_offset = peb_offset + offset_of!(HyperlightPEB, init_data); | ||
| let peb_heap_data_offset = peb_offset + offset_of!(HyperlightPEB, guest_heap); | ||
| #[cfg(feature = "nanvix-unstable")] | ||
| let peb_file_mappings_offset = peb_offset + offset_of!(HyperlightPEB, file_mappings); | ||
|
|
||
| // The following offsets are the actual values that relate to memory layout, | ||
| // which are written to PEB struct | ||
| let peb_address = Self::BASE_ADDRESS + peb_offset; | ||
| // make sure heap buffer starts at 4K boundary. | ||
| // The FileMappingInfo array is stored immediately after the PEB struct. | ||
| // We statically reserve space for MAX_FILE_MAPPINGS entries so that | ||
| // the heap never overlaps the array, even when all slots are used. | ||
| // The host writes file mapping metadata here via write_file_mapping_entry; | ||
| // the guest only reads the entries. We don't know at layout time how | ||
| // many file mappings the host will register, so we reserve space for | ||
| // the maximum number. | ||
| // The heap starts at the next page boundary after this reserved area. | ||
| #[cfg(feature = "nanvix-unstable")] | ||
| let file_mappings_array_end = peb_offset | ||
| + size_of::<HyperlightPEB>() | ||
| + hyperlight_common::mem::MAX_FILE_MAPPINGS | ||
| * size_of::<hyperlight_common::mem::FileMappingInfo>(); | ||
| #[cfg(feature = "nanvix-unstable")] | ||
| let guest_heap_buffer_offset = file_mappings_array_end.next_multiple_of(PAGE_SIZE_USIZE); | ||
| #[cfg(not(feature = "nanvix-unstable"))] | ||
| let guest_heap_buffer_offset = | ||
| (peb_offset + size_of::<HyperlightPEB>()).next_multiple_of(PAGE_SIZE_USIZE); | ||
|
|
There was a problem hiding this comment.
PR description says file_mappings can be replaced by an "init_blob", but this change removes the PEB-based metadata without any corresponding new mechanism in this diff. Please either update the description to match the actual approach/name used in the codebase, or include the init-blob plumbing in this PR so readers can see how guests discover mappings now.
|
Won't merge rn. Focusing on #1385. |
Removes file_mappings which instead can be replaced by init_blob.
depends on #1379 first, will mark ready after