Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR extracts memory-management into a new crate Changes
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm-mm/src/manager.rs (1)
75-86:⚠️ Potential issue | 🔴 CriticalAdd bounds validation and use checked arithmetic in
copy_from_slice.The
lenparameter is independent frombuf.len(). Iflen > buf.len(), the unsafe copy reads past the buffer bounds. Additionally,offset + len as u64should use checked arithmetic to prevent silent overflow.Suggested fix
pub fn copy_from_slice(&mut self, gpa: u64, buf: &[u8], len: usize) -> Result<(), Error> { let region = self.try_get_region_by_gpa_mut(gpa)?; let hva = region.try_to_hva()?; let offset = gpa - region.gpa; + if len > buf.len() { + return Err(Error::MemoryOverflow); + } + + let end = offset + .checked_add(len as u64) + .ok_or(Error::MemoryOverflow)?; + if end > region.len as u64 { - if offset + len as u64 > region.len as u64 { return Err(Error::MemoryOverflow); } unsafe { hva.add(offset as usize).copy_from(buf.as_ptr(), len); } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm-mm/src/manager.rs` around lines 75 - 86, In copy_from_slice, validate that len does not exceed buf.len() and perform checked arithmetic on offset + len to avoid overflows: check if len <= buf.len() (or return an appropriate Error) before the unsafe block, convert len to u64/usize using try_from/checked conversions, replace the direct offset + len as u64 calculation with checked_add on offset (or use offset.checked_add(len_u64)) and return Error::MemoryOverflow on overflow, and only then perform the unsafe copy using the safely converted usize indices (ref functions/fields: copy_from_slice, try_get_region_by_gpa_mut, try_to_hva, region.gpa, region.len).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/vm-mm/src/manager.rs`:
- Around line 75-86: In copy_from_slice, validate that len does not exceed
buf.len() and perform checked arithmetic on offset + len to avoid overflows:
check if len <= buf.len() (or return an appropriate Error) before the unsafe
block, convert len to u64/usize using try_from/checked conversions, replace the
direct offset + len as u64 calculation with checked_add on offset (or use
offset.checked_add(len_u64)) and return Error::MemoryOverflow on overflow, and
only then perform the unsafe copy using the safely converted usize indices (ref
functions/fields: copy_from_slice, try_get_region_by_gpa_mut, try_to_hva,
region.gpa, region.len).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
Cargo.tomlcrates/vm-bootloader/Cargo.tomlcrates/vm-bootloader/src/boot_loader.rscrates/vm-bootloader/src/boot_loader/arch/aarch64.rscrates/vm-bootloader/src/boot_loader/arch/x86_64.rscrates/vm-bootloader/src/initrd_loader.rscrates/vm-bootloader/src/kernel_loader.rscrates/vm-bootloader/src/kernel_loader/linux/bzimage.rscrates/vm-bootloader/src/kernel_loader/linux/image.rscrates/vm-core/Cargo.tomlcrates/vm-core/src/lib.rscrates/vm-core/src/virt.rscrates/vm-core/src/virt/hvp.rscrates/vm-core/src/virt/hvp/mm.rscrates/vm-core/src/virt/kvm.rscrates/vm-device/Cargo.tomlcrates/vm-device/src/device/virtio/virtio_blk.rscrates/vm-device/src/device/virtio/virtio_mmio_kbd.rscrates/vm-machine/Cargo.tomlcrates/vm-machine/src/device.rscrates/vm-machine/src/firmware/bios.rscrates/vm-machine/src/vm.rscrates/vm-mm/Cargo.tomlcrates/vm-mm/src/allocator.rscrates/vm-mm/src/allocator/mmap_allocator.rscrates/vm-mm/src/error.rscrates/vm-mm/src/lib.rscrates/vm-mm/src/manager.rscrates/vm-mm/src/region.rscrates/vm-virtio/Cargo.tomlcrates/vm-virtio/src/virt_queue.rscrates/vm-virtio/src/virt_queue/virtq_desc_table.rs
💤 Files with no reviewable changes (2)
- crates/vm-core/src/lib.rs
- crates/vm-mm/src/error.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm-core/src/virt/kvm/arch/x86_64.rs (1)
9-13:⚠️ Potential issue | 🟠 MajorAvoid panic in
arch_post_init; propagate PIT creation failure instead.Line 12 uses
unwrap(), which can crash the VM init path despite returningResult<()>.Proposed fix
fn arch_post_init(&mut self) -> Result<()> { - { - let pit_config = kvm_pit_config::default(); - self.vm_fd.create_pit2(pit_config).unwrap(); - } + let pit_config = kvm_pit_config::default(); + self.vm_fd.create_pit2(pit_config)?; Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm-core/src/virt/kvm/arch/x86_64.rs` around lines 9 - 13, The call to self.vm_fd.create_pit2(pit_config).unwrap() in arch_post_init can panic; change it to propagate the error instead (use the ? operator or map_err to convert the kvm error into the function's Result error type) so arch_post_init returns an Err on PIT creation failure; update the create_pit2 invocation inside arch_post_init (referencing pit_config, self.vm_fd.create_pit2, and arch_post_init) to return the error instead of unwrapping.
🧹 Nitpick comments (3)
crates/vm-core/src/virt/kvm.rs (1)
81-90: Potential panic onto_hva().unwrap().If
region.to_hva()returnsNone, this will panic. Since the allocation happened on line 79, it should generally succeed, but theunwrap()silently assumes success. Consider propagating an error instead for defensive programming.Suggested improvement
self.vm_fd .set_user_memory_region(kvm_userspace_memory_region { slot: slot as u32, flags: 0, guest_phys_addr: region.gpa, memory_size: region.len as u64, - userspace_addr: region.to_hva().unwrap() as u64, + userspace_addr: region + .to_hva() + .ok_or_else(|| Error::Internal("memory region not allocated".to_string()))? as u64, })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm-core/src/virt/kvm.rs` around lines 81 - 90, The call to region.to_hva().unwrap() in the unsafe block that calls self.vm_fd.set_user_memory_region can panic if to_hva() returns None; change this to handle the None case and propagate a proper error instead of unwrapping: retrieve the HVA via region.to_hva().ok_or_else(|| /* create a suitable error with context */ )? and pass that value (as u64) to userspace_addr, so set_user_memory_region returns a Result propagated from the enclosing function (preserve the unsafe block and kvm_userspace_memory_region construction but replace the unwrap with an ok_or/ map_err to return an error describing the missing HVA).crates/vm-core/src/virt/hvp.rs (2)
45-122: Helper functionsetup_cpustill usesanyhow::Result.While
setup_cpuis a private helper, it's inconsistent with the migration tocrate::error::Result. Since it's only called from within the thread that already usesanyhow::Result, this is functionally fine but creates maintenance overhead if you later want to call it fromResult-returning code.Consider migrating this when addressing the thread error type inconsistency above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm-core/src/virt/hvp.rs` around lines 45 - 122, The helper function setup_cpu currently returns anyhow::Result, change its signature to return crate::error::Result<()> and update any internal ?-propagated calls to be compatible with crate::error::Error (ensure From<anyhow::Error> or map_err is used if necessary); update the where clause and all call sites (e.g., where setup_cpu(...) is invoked) to accept the new Result type and adjust imports to bring crate::error::Result into scope so compilation and error propagation remain consistent with the crate-wide error type.
329-382: Mixed error types: spawned thread still usesanyhow::Result.The outer
runmethod returnscrate::error::Result<()>, but the spawned thread closure on line 343 returnsanyhow::Result<()>. This inconsistency means:
- Errors from the thread are not propagated to the caller
- The error handling strategy is inconsistent within the same method
Consider either:
- Converting the thread's result to
crate::error::Errorand propagating it- Documenting why
anyhowis intentionally used here (e.g., thread isolation)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm-core/src/virt/hvp.rs` around lines 329 - 382, The spawned thread closure currently returns anyhow::Result<()> which breaks consistency with the outer run signature (crate::error::Result<()>) and prevents thread errors from propagating; change the closure return type to crate::error::Result<()> (or convert its anyhow::Error to crate::error::Error at the closure end), collect each spawn's JoinGuard result from thread::scope and map any thread error (panic or Err) into crate::error::Error and return it from run; update symbols: the closure spawned inside run that creates HvpVcpu, calls setup_cpu, vcpu.run, and handle_vm_exit should return crate::error::Result<()>, and run should join/inspect each spawn result and propagate the first Err as the run error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/vm-core/src/virt/kvm/arch/x86_64.rs`:
- Around line 9-13: The call to self.vm_fd.create_pit2(pit_config).unwrap() in
arch_post_init can panic; change it to propagate the error instead (use the ?
operator or map_err to convert the kvm error into the function's Result error
type) so arch_post_init returns an Err on PIT creation failure; update the
create_pit2 invocation inside arch_post_init (referencing pit_config,
self.vm_fd.create_pit2, and arch_post_init) to return the error instead of
unwrapping.
---
Nitpick comments:
In `@crates/vm-core/src/virt/hvp.rs`:
- Around line 45-122: The helper function setup_cpu currently returns
anyhow::Result, change its signature to return crate::error::Result<()> and
update any internal ?-propagated calls to be compatible with crate::error::Error
(ensure From<anyhow::Error> or map_err is used if necessary); update the where
clause and all call sites (e.g., where setup_cpu(...) is invoked) to accept the
new Result type and adjust imports to bring crate::error::Result into scope so
compilation and error propagation remain consistent with the crate-wide error
type.
- Around line 329-382: The spawned thread closure currently returns
anyhow::Result<()> which breaks consistency with the outer run signature
(crate::error::Result<()>) and prevents thread errors from propagating; change
the closure return type to crate::error::Result<()> (or convert its
anyhow::Error to crate::error::Error at the closure end), collect each spawn's
JoinGuard result from thread::scope and map any thread error (panic or Err) into
crate::error::Error and return it from run; update symbols: the closure spawned
inside run that creates HvpVcpu, calls setup_cpu, vcpu.run, and handle_vm_exit
should return crate::error::Result<()>, and run should join/inspect each spawn
result and propagate the first Err as the run error.
In `@crates/vm-core/src/virt/kvm.rs`:
- Around line 81-90: The call to region.to_hva().unwrap() in the unsafe block
that calls self.vm_fd.set_user_memory_region can panic if to_hva() returns None;
change this to handle the None case and propagate a proper error instead of
unwrapping: retrieve the HVA via region.to_hva().ok_or_else(|| /* create a
suitable error with context */ )? and pass that value (as u64) to
userspace_addr, so set_user_memory_region returns a Result propagated from the
enclosing function (preserve the unsafe block and kvm_userspace_memory_region
construction but replace the unwrap with an ok_or/ map_err to return an error
describing the missing HVA).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
crates/vm-core/src/arch/layout.rscrates/vm-core/src/error.rscrates/vm-core/src/virt.rscrates/vm-core/src/virt/hvp.rscrates/vm-core/src/virt/kvm.rscrates/vm-core/src/virt/kvm/arch/aarch64.rscrates/vm-core/src/virt/kvm/arch/x86_64.rscrates/vm-core/src/virt/kvm/irq_chip.rscrates/vm-machine/src/error.rscrates/vm-machine/src/vm.rs
Merged
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
1 participant
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.
Summary by CodeRabbit