Skip to content

Fix differential snapshot corruption on multi-slot VMs - #5705

Merged
zulinx86 merged 9 commits into
firecracker-microvm:mainfrom
ilstam:diff-snapshot-fix
Feb 25, 2026
Merged

Fix differential snapshot corruption on multi-slot VMs#5705
zulinx86 merged 9 commits into
firecracker-microvm:mainfrom
ilstam:diff-snapshot-fix

Conversation

@ilstam

@ilstam ilstam commented Feb 20, 2026

Copy link
Copy Markdown
Contributor

Bug Description

This bug was originally reported by @ejc3 in #5696.

The bug causes Firecracker to corrupt the memory files of differential snapshots for VMs that have multiple memory slots.

The bug is only triggered when the last page of a memory slot is clean (i.e. it haven't been modified since the last snapshot was taken). In practice this only affects VMs that are using memory hot-plugging or any x86 VMs with a memory size larger than 3GiB. VMs of smaller memory size, or VMs running on ARM, are not affected since they have a single KVM memory slot (unless extra memory regions have been added via hot-plugging).

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • I have read and understand CONTRIBUTING.md.
  • I have run tools/devtool checkbuild --all to verify that the PR passes
    build checks on all supported architectures.
  • I have run tools/devtool checkstyle to verify that the PR passes the
    automated style checks.
  • I have described what is done in these changes, why they are needed, and
    how they are solving the problem in a clear and encompassing way.
  • I have updated any relevant documentation (both in code and in the docs)
    in the PR.
  • I have mentioned all user-facing changes in CHANGELOG.md.
  • If a specific issue led to this PR, this PR closes the issue.
  • When making API changes, I have followed the
    Runbook for Firecracker API changes.
  • I have tested all new and changed functionalities in unit tests and/or
    integration tests.
  • I have linked an issue to every new TODO.

  • This functionality cannot be added in rust-vmm.

@ilstam
ilstam requested a review from Manciukic February 20, 2026 11:51
@ilstam ilstam added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Feb 20, 2026

@Manciukic Manciukic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just a nit on the second commit where there's 2 leftover lines. I also verified it against a property test I'm writing for hardening the tests on this logic.

Comment thread src/vmm/src/vstate/memory.rs
@codecov

codecov Bot commented Feb 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.42857% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.16%. Comparing base (e3a99d3) to head (e72c14f).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/vmm/src/vstate/memory.rs 91.42% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5705   +/-   ##
=======================================
  Coverage   83.15%   83.16%           
=======================================
  Files         277      277           
  Lines       29379    29405   +26     
=======================================
+ Hits        24431    24455   +24     
- Misses       4948     4950    +2     
Flag Coverage Δ
5.10-m5n.metal 83.46% <91.42%> (+0.01%) ⬆️
5.10-m6a.metal 82.80% <91.42%> (+0.01%) ⬆️
5.10-m6g.metal 80.17% <91.42%> (+0.01%) ⬆️
5.10-m6i.metal 83.46% <91.42%> (+0.01%) ⬆️
5.10-m7a.metal-48xl 82.78% <91.42%> (+<0.01%) ⬆️
5.10-m7g.metal 80.17% <91.42%> (+0.01%) ⬆️
5.10-m7i.metal-24xl 83.43% <91.42%> (+<0.01%) ⬆️
5.10-m7i.metal-48xl 83.43% <91.42%> (+0.01%) ⬆️
5.10-m8g.metal-24xl 80.16% <91.42%> (+0.01%) ⬆️
5.10-m8g.metal-48xl 80.16% <91.42%> (+0.01%) ⬆️
5.10-m8i.metal-48xl 83.43% <91.42%> (+0.01%) ⬆️
5.10-m8i.metal-96xl 83.43% <91.42%> (+<0.01%) ⬆️
6.1-m5n.metal 83.48% <91.42%> (+<0.01%) ⬆️
6.1-m6a.metal 82.82% <91.42%> (+<0.01%) ⬆️
6.1-m6g.metal 80.16% <91.42%> (+0.01%) ⬆️
6.1-m6i.metal 83.48% <91.42%> (+<0.01%) ⬆️
6.1-m7a.metal-48xl 82.81% <91.42%> (+<0.01%) ⬆️
6.1-m7g.metal 80.17% <91.42%> (+0.01%) ⬆️
6.1-m7i.metal-24xl 83.50% <91.42%> (+0.01%) ⬆️
6.1-m7i.metal-48xl 83.50% <91.42%> (+<0.01%) ⬆️
6.1-m8g.metal-24xl 80.16% <91.42%> (+0.01%) ⬆️
6.1-m8g.metal-48xl 80.16% <91.42%> (+0.01%) ⬆️
6.1-m8i.metal-48xl 83.50% <91.42%> (+0.01%) ⬆️
6.1-m8i.metal-96xl 83.50% <91.42%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Manciukic
Manciukic previously approved these changes Feb 20, 2026
ilstam added a commit to ilstam/firecracker that referenced this pull request Feb 20, 2026
…ecracker-microvm#5707

Update v1.15.0 CHANGELOG mentioning the fix for firecracker-microvm#5707:
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request Feb 20, 2026
Update v1.15.0 CHANGELOG mentioning the fix for firecracker-microvm#5707:
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Comment thread CHANGELOG.md Outdated
ilstam added a commit to ilstam/firecracker that referenced this pull request Feb 20, 2026
Update v1.15.0 CHANGELOG mentioning the fix for firecracker-microvm#5707:
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request Feb 20, 2026
Update v1.15.0 CHANGELOG mentioning the fix for firecracker-microvm#5707:
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request Feb 20, 2026
Update v1.15.0 CHANGELOG mentioning the fix for
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Comment thread src/vmm/src/vstate/memory.rs Outdated
Comment thread src/vmm/src/vstate/memory.rs
zulinx86
zulinx86 previously approved these changes Feb 24, 2026
ilstam added a commit to ilstam/firecracker that referenced this pull request Feb 24, 2026
Update v1.15.0 CHANGELOG mentioning the fix for
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Comment thread src/vmm/src/vstate/memory.rs Outdated
Comment thread src/vmm/src/vstate/memory.rs Outdated
Comment thread src/vmm/src/vstate/memory.rs Outdated
In GuestMemoryExtension::dump_dirty() and GuestMemorySlot::dump_dirty()
there are several unwraps which will cause Firecracker to panic. Instead
of panicking simply propagate errors up the call chain to mark the
snapshot operation as failed. To make error handling less verbose and
more descriptive make GuestMemorySlot::dump_dirty() return MemoryError
instead of GuestMemoryError and define new error types for MemoryError.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
The comment in test_dump_dirty() before the last snapshot is taken says:
// First region pages: [dirty, clean]

However, the following code line updates the second page rather than the
first one, so in reality the state of the bitmap is [clean, dirty]. Fix
the comment.

Additionally, the "Dump only the dirty pages." comment further above is
misleading. Since we write to all pages of all regions, the Firecracker
bitmap (rather than the KVM bitmap) will be all 1s, therefore the first
snapshot taken by this function will dump all memory pages.

Rename 'dirty_bitmap' to 'kvm_dirty_bitmap' to make it clear which
bitmap it refers to and add comments about the state of the Firecracker
bitmap too. To make things more obvious for the reader re-configure the
state of the KVM dirty bitmap next to the relevant comment, just before
we take the second snapshot.

Since we are in the neighbourhood, rename 'expected_first_region' to
'expected_file_contents' since it clearly stores the contents of both
regions rather than the first one only.

No functional change intended.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
It is possible that the dirty bitmap dump_dirty() receives is larger or
smaller than the slot size. Return an error in that case.

Additionally, the inner loop in that function always iterates over the
0..64 range. Typically the region size won't be a multiple of 64, so we
need to make sure that we break after we check the last bit that
corresponds to the last page of the region.

Extend the test_dump_dirty() test case to exercise the new code paths by
supplying wrongly sized  bitmaps to the function.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
GuestMemorySlot::dump_dirty() does not advance the file cursor when the
pages at the end of the memory region are clean. This causes the next
slot to start writing at an incorrect offset and corrupting the contents
of the previous region. Fix this by always advancing the cursor at the
end of dump_dirty().

As per the original report from EJ Ciramella this only affects VMs with
more than one memory slots and it can cause Firecracker or the guest to
crash when loading a corrupted snapshot.

Fixes: 6c4c1bf ("feat(mem): introduce KVM slots per GuestMemoryRegion")
Reported-by: EJ Ciramella <ejc3@meta.com>
Suggested-by: EJ Ciramella <ejc3@meta.com>
Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
The test_dump_dirty() test currently checks the file contents of a
differential snapshot where the last page of both memory regions is
dirty.

Extend this test to also test the case where the last page of both
regions is clean. Additionally, check that the logical size of the
resulting file is different than the physical size due to the holes
representing clean pages. Finally, make sure that if the KVM dirty
bitmap is larger than the region size, the extra bits are ignored.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
The test_diff_snapshot_overlay() case tests differential snapshots on
VMs that have a single memory slot since basic_config() uses a 256MiB
memory size by default.

Parametrize the test so that it's repeated for both 256MiB and 4096MiB
sizes. On x86 this will create 2 memory slots and hence test a different
scenario.

Suggested-by: Riccardo Mancini <mancio@amazon.com>
Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Update v1.15.0 CHANGELOG mentioning the fix for
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>

@zulinx86 zulinx86 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zulinx86
zulinx86 enabled auto-merge (rebase) February 25, 2026 05:22
@zulinx86
zulinx86 merged commit 6cbb44e into firecracker-microvm:main Feb 25, 2026
7 checks passed
ilstam added a commit to ilstam/firecracker that referenced this pull request Feb 25, 2026
Update v1.15.0 CHANGELOG mentioning the fix for
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request Feb 25, 2026
Update v1.14.2 CHANGELOG mentioning the fix for
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit that referenced this pull request Feb 25, 2026
Update v1.15.0 CHANGELOG mentioning the fix for
#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit that referenced this pull request Feb 25, 2026
Update v1.14.2 CHANGELOG mentioning the fix for
#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
@ilstam
ilstam deleted the diff-snapshot-fix branch February 25, 2026 14:01
ilstam added a commit to ilstam/firecracker that referenced this pull request Mar 16, 2026
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request Mar 16, 2026
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request Mar 16, 2026
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request Mar 16, 2026
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request Mar 16, 2026
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request Mar 31, 2026
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request Mar 31, 2026
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request Mar 31, 2026
firecracker-microvm#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit that referenced this pull request Mar 31, 2026
#5705

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Awaiting review Indicates that a pull request is ready to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants