Add scratch region #1182
Add scratch region #1182
Conversation
syntactically
requested review from
danbugs,
dblnz,
devigned,
jprendes,
ludfjig,
marosset and
simongdavies
as code owners
syntactically
added
the
kind/enhancement
syntactically
force-pushed
the
lm/add-scratch
branch
4 times, most recently
from
7443d75 to
541f5ed
Compare
Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com>
Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com>
This has to materialise a number of .cargo/config.toml into the tree in order to point Cargo at the Nix-vendored deps. Ideally, the cargo wrapper script would just pass some config information, but unfortunately, due to rust-lang/cargo#11031, external subcommands do not respect --config and will only pick up configuration that's actually in .cargo/config.toml files in the tree. In order to reduce the risk of these getting improperly committed to the repository, the script also adds them to .git/info/exclude whenever it creates them. Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com>
Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com>
Previously, it could not be used after the guests were built, due to some extra generated files. Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com>
This changes the representation of a memory region on Windows to include a file mapping from which it is derived, allowing the WHP code to dynamically map any files necessary into the surrogate. This removes the restriction that all mappings into a hyperlight VM on Windows must come from a single file-backed host mapping. Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com>
This adds a second region of guest physical memory, which is intended to become the only mutable region of memory, but which is currently unused. Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com>
syntactically
force-pushed
the
lm/add-scratch
branch
from
541f5ed to
3066d57
Compare
ludfjig
left a comment
ludfjig
left a comment
There was a problem hiding this comment.
LGTM, and big fan of the cleanup!
|
|
||
| #[derive(Debug)] | ||
| pub(crate) struct HandleMapping { | ||
| pub(crate) use_count: u64, |
There was a problem hiding this comment.
Is this use_count used for anything or could we remove it?
There was a problem hiding this comment.
Shouldn't unmap check use_count before actually unmapping and only unmap when it equals 1? At least this is what I would expect from this pattern.
There was a problem hiding this comment.
Oh, yeah, you are totally correct! Sorry, will fix.
| self.unmap_helper(entry.surrogate_base); | ||
| } else { | ||
| #[cfg(debug_assertions)] | ||
| panic!("Attempted to unmap from surrogate a region that was never mapped") |
There was a problem hiding this comment.
I personally am a fan of panicking on user errors like this instead of slapping Result everywhere, I think maybe there are other places in our codebase where we can consider starting to panic. I know we are supposed to be "panic-free", but I think we can still improve alot
There was a problem hiding this comment.
Yeah, in this case it is a little bit of a cop-out because of the clippy lint, so it silently ignores the error in production, which I don't love. I would like to have another discussion about the use of the anti-panic clippy lint overall / the goal of being mostly panic free. I think that in general there are a large class of internal error things that we currently log or return runtime failure for, but which really are indicative of a bug inside hyperlight (some invariant not being maintained) rather than a mis-use of hyperlight as a library from another application. Being security-focused code, I generally feel that we should check invariants early and often, and indeed take the quickest safe option of "OK just die now" when we find invariant violations, rather than just returning an error but allowing the system to continue to operate in a known-to-be-inconsistent state.
| /// supports at least 36 bits. Almost all of them support at least 40 | ||
| /// bits, so we could consider bumping this in the future if we were | ||
| /// ever memory-constrained. | ||
| pub const MAX_GPA: usize = 0x0000_00ff_ffff_ffff; |
There was a problem hiding this comment.
Is this a typo or I don't understand it correctly?
Why is the MAX_GPA is 0x0000_00ff_ffff_ffff (40 bit) when the comment above says 36 bit?
There was a problem hiding this comment.
Indeed, thanks for catching that.
| // (see https://learn.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-mapviewoffile2#remarks) so we use the same API it uses in the header file here instead of | ||
| // MapViewOfFile2 which does not exist in the rust crate (see https://github.com/microsoft/windows-rs/issues/2595) | ||
| let surrogate_base = unsafe { | ||
| MapViewOfFileNuma2( |
There was a problem hiding this comment.
The docs say the function can fail:
Should we check we get non null Value?
There was a problem hiding this comment.
This code was just pulled over from surrogate_process_manager.rs where it did the same thing, I think? So maybe, but I'm not sure this is the correct venue to fix it?
| begin: r.guest_region.start as u64, | ||
| end: r.guest_region.end as u64, | ||
| offset: r.host_region.start as u64, | ||
| offset: <_ as Into<usize>>::into(r.host_region.start) as u64, |
There was a problem hiding this comment.
This looks like a fix straight from rust-analyzer 😄 can this be just .into()?
This adds the new scratch region, which will eventually become where everything writable is, but is currently not used.
A prerequisite of this is to improve whp support for dynamically mapping file handles into the surrogate process. The same infrastructure could be used to expose (some parts of) the host mapping API on Windows.