-
Notifications
You must be signed in to change notification settings - Fork 123
Support ELF core dump creation on guest crash #417
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
d5892ec
to
52c3c71
Compare
cc8a48d
to
0126ce3
Compare
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 support for ELF core dump generation on guest crash by introducing crashdump metadata and integrating it into the hypervisor initialization and crashdump context API. Key changes include:
- Introducing the SandboxMetadata structure and propagating a new crashdump parameter across hypervisor modules.
- Implementing a crashdump_context method that gathers register and memory state from various hypervisors.
- Integrating the elfcore dependency and functionality in a new crashdump module.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/hyperlight_host/src/sandbox/uninitialized_evolve.rs | Passes crashdump metadata to hypervisor handler initialization. |
src/hyperlight_host/src/sandbox/uninitialized.rs | Introduces SandboxMetadata to support crash dump context. |
src/hyperlight_host/src/hypervisor/windows_hypervisor_platform.rs | Adds a get_xsave function and crash dump integration. |
src/hyperlight_host/src/hypervisor/mod.rs | Updates the hypervisor trait to use crashdump_context instead of get_memory_regions. |
src/hyperlight_host/src/hypervisor/kvm.rs | Implements crashdump_context with register mapping and xsave conversion. |
src/hyperlight_host/src/hypervisor/inprocess.rs | Marks crashdump_context as unsupported in in-process mode. |
src/hyperlight_host/src/hypervisor/hyperv_windows.rs | Integrates crashdump_context and replaces the get_memory_regions implementation. |
src/hyperlight_host/src/hypervisor/hyperv_linux.rs | Adds a crashdump_context implementation with register mapping and xsave handling. |
src/hyperlight_host/src/hypervisor/crashdump.rs | Implements core dump creation using elfcore and adds custom memory reading. |
src/hyperlight_host/Cargo.toml | Adds the elfcore dependency for ELF core dump support. |
Comments suppressed due to low confidence (2)
src/hyperlight_host/src/hypervisor/hyperv_windows.rs:535
- Using unwrap() for OsString to String conversion may lead to a runtime panic if the conversion fails; consider handling conversion errors or using to_string_lossy() instead.
Path::new(&path).file_name().map(|name| name.to_os_string().into_string().unwrap()).unwrap()
src/hyperlight_host/src/hypervisor/hyperv_linux.rs:707
- Using unwrap() for converting an OsString to a String here might result in a panic if the conversion fails; consider using a more robust conversion strategy such as to_string_lossy().
Path::new(&path).file_name().map(|name| name.to_os_string().into_string().unwrap()).unwrap()
@@ -46,6 +46,7 @@ tempfile = { version = "3.19", optional = true } | |||
serde_yaml = "0.9" | |||
anyhow = "1.0" | |||
metrics = "0.24.2" | |||
elfcore = { git = "https://github.com/dblnz/elfcore.git", branch = "split-linux-impl-from-elfcore" } |
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.
This needs to be change after the elfcore
PR is merged
0126ce3
to
51ce291
Compare
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 support for generating ELF core dump files when a guest crashes. The changes integrate a new crashdump feature across the codebase by introducing a new SandboxMetadata structure, updating various hypervisor implementations with a crashdump_context function, and adding a dedicated crashdump module along with necessary dependency updates.
- Introduces crashdump support via conditional compilation (#[cfg(crashdump)]) and metadata propagation.
- Implements crashdump_context functions in multiple hypervisor modules.
- Adds a new crashdump module and updates Cargo.toml to include the elfcore dependency.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/hyperlight_host/src/sandbox/uninitialized_evolve.rs | Introduces crashdump metadata propagation in sandbox initialization. |
src/hyperlight_host/src/sandbox/uninitialized.rs | Defines SandboxMetadata and integrates metadata handling in guest binary parsing. |
src/hyperlight_host/src/hypervisor/windows_hypervisor_platform.rs | Adds a new get_xsave function for crashdump support. |
src/hyperlight_host/src/hypervisor/kvm.rs | Implements crashdump_context using vCPU register and XSAVE information. |
src/hyperlight_host/src/hypervisor/inprocess.rs | Marks crashdump_context as unimplemented for in-process mode. |
src/hyperlight_host/src/hypervisor/hypervisor_handler.rs | Updates hypervisor handler functions to accept crashdump metadata. |
src/hyperlight_host/src/hypervisor/hyperv_windows.rs | Adds crashdump_context support and propagates metadata in HypervWindowsDriver. |
src/hyperlight_host/src/hypervisor/hyperv_linux.rs | Implements crashdump_context for HypervLinuxDriver with register extraction and metadata. |
src/hyperlight_host/src/hypervisor/crashdump.rs | Introduces the crashdump module with GuestView, GuestMemReader, and the core dump generation function. |
src/hyperlight_host/Cargo.toml | Adds the elfcore dependency needed for ELF core dump creation. |
println!("Memory dumped to file: {:?}", persist_path); | ||
log::error!("Memory dumped to file: {:?}", persist_path); | ||
println!("Core dump created successfully: {}", path_string); | ||
log::error!("Core dump file: {}", path_string); |
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.
Logging a successful core dump creation as an error may be misleading. Consider using 'log::info' or 'log::debug' to indicate a normal, successful operation.
log::error!("Core dump file: {}", path_string); | |
log::info!("Core dump file: {}", path_string); |
Copilot uses AI. Check for mistakes.
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.
Looks great. Left some nit comments.
By the way (unrelated to your PR), we should simplify each driver to include only the core VM functionality, (like run, set_reg, get_regs, etc.). Then, we can rely on a dyn Hypervisor to avoid duplicating functionality across different implementations.
- the core dump file is an ELF file with special segments that describe the guest's memory when it crashed, the CPU register's values and other special notes that tell the debugger how to set up a debugging session starting from the core dump Signed-off-by: Doru Blânzeanu <[email protected]>
Signed-off-by: Doru Blânzeanu <[email protected]>
…he sandbox - only store the binary path for now Signed-off-by: Doru Blânzeanu <[email protected]>
e345e85
to
45e53bf
Compare
Signed-off-by: Doru Blânzeanu <[email protected]>
…b and lldb using vscode Signed-off-by: Doru Blânzeanu <[email protected]>
45e53bf
to
4d4b87f
Compare
Purpose
Description
This depends on PR.