Skip to content

Minor fixes#5944

Open
ShadowCurse wants to merge 2 commits into
firecracker-microvm:mainfrom
ShadowCurse:bug_fixes
Open

Minor fixes#5944
ShadowCurse wants to merge 2 commits into
firecracker-microvm:mainfrom
ShadowCurse:bug_fixes

Conversation

@ShadowCurse

@ShadowCurse ShadowCurse commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Changes

  • in virtio-mem request handling unmap kvm slots before calling mprotect to prevent a possibility for the guest to quickly re-fault the mprotected memory
  • make Queue::pop functions be bounded to prevent guest from making us process more than queue.size requests

Reason

Fixes a couple of edge-cases where guest actions can lead to unwanted behavior.

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.

@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.83333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.02%. Comparing base (fa586c6) to head (6fe6a25).

Files with missing lines Patch % Lines
src/vmm/src/devices/virtio/pmem/device.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5944      +/-   ##
==========================================
+ Coverage   83.01%   83.02%   +0.01%     
==========================================
  Files         277      277              
  Lines       30129    30147      +18     
==========================================
+ Hits        25012    25031      +19     
+ Misses       5117     5116       -1     
Flag Coverage Δ
5.10-m5n.metal 83.33% <95.83%> (?)
5.10-m6a.metal 82.68% <95.83%> (+0.01%) ⬆️
5.10-m6g.metal 79.97% <95.83%> (+0.01%) ⬆️
5.10-m6i.metal 83.34% <95.83%> (+0.01%) ⬆️
5.10-m7a.metal-48xl 82.67% <95.83%> (+0.01%) ⬆️
5.10-m7g.metal 79.97% <95.83%> (+0.01%) ⬆️
5.10-m7i.metal-24xl 83.30% <95.83%> (+0.01%) ⬆️
5.10-m7i.metal-48xl 83.30% <95.83%> (+0.01%) ⬆️
5.10-m8g.metal-24xl 79.97% <95.83%> (+0.01%) ⬆️
5.10-m8g.metal-48xl 79.97% <95.83%> (+0.01%) ⬆️
5.10-m8i.metal-48xl 83.31% <95.83%> (+0.01%) ⬆️
5.10-m8i.metal-96xl 83.31% <95.83%> (+0.02%) ⬆️
6.1-m5n.metal 83.35% <95.83%> (?)
6.1-m6a.metal 82.70% <95.83%> (+<0.01%) ⬆️
6.1-m6g.metal 79.97% <95.83%> (+0.01%) ⬆️
6.1-m6i.metal 83.35% <95.83%> (+0.01%) ⬆️
6.1-m7a.metal-48xl 82.69% <95.83%> (+0.01%) ⬆️
6.1-m7g.metal 79.97% <95.83%> (+0.01%) ⬆️
6.1-m7i.metal-24xl 83.37% <95.83%> (+0.01%) ⬆️
6.1-m7i.metal-48xl 83.37% <95.83%> (+0.01%) ⬆️
6.1-m8g.metal-24xl 79.97% <95.83%> (+0.01%) ⬆️
6.1-m8g.metal-48xl 79.97% <95.83%> (+0.02%) ⬆️
6.1-m8i.metal-48xl 83.38% <95.83%> (+0.02%) ⬆️
6.1-m8i.metal-96xl 83.37% <95.83%> (+0.01%) ⬆️
6.18-m5n.metal 83.35% <95.83%> (+0.01%) ⬆️
6.18-m6a.metal 82.70% <95.83%> (+0.01%) ⬆️
6.18-m6g.metal 79.97% <95.83%> (+0.01%) ⬆️
6.18-m6i.metal 83.35% <95.83%> (+0.01%) ⬆️
6.18-m7a.metal-48xl 82.69% <95.83%> (+0.01%) ⬆️
6.18-m7g.metal 79.97% <95.83%> (+0.01%) ⬆️
6.18-m7i.metal-24xl 83.37% <95.83%> (+0.01%) ⬆️
6.18-m7i.metal-48xl 83.37% <95.83%> (+<0.01%) ⬆️
6.18-m8g.metal-24xl 79.97% <95.83%> (+0.02%) ⬆️
6.18-m8g.metal-48xl 79.97% <95.83%> (+0.01%) ⬆️
6.18-m8i.metal-48xl 83.37% <95.83%> (+0.01%) ⬆️
6.18-m8i.metal-96xl 83.37% <95.83%> (+0.01%) ⬆️

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

☔ View full report in Codecov by Harness.
📢 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.

In `update_range` function, move kvm slot update before memory discard
to prevent a small time window when the guest can re-fault just
discarded memory before it is removed from kvm view.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Add additional parameter to the `pop`/`pop_or_enable_notification`
methods of the `Queue` to force them to be bounded by that value. In
all cases the initial bound value is the `size` of the queue. This is
the most upper bound we can use. The reason to not use lower values like
`queue.len()` is that the driver is allowed to add new items to the
queue and change the `avail_ring.used_idx` used for notification
suppression. If this happens, we might end up in a situation where we
can process all initial `queue.len()` requests, but since the
`avail_ring.used_idx` is changed, we will not send a notification to the
guest. This will result in a broken device.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse ShadowCurse self-assigned this Jun 10, 2026
@ShadowCurse ShadowCurse marked this pull request as ready for review June 10, 2026 12:49
@ShadowCurse ShadowCurse added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Jun 10, 2026
@ShadowCurse ShadowCurse requested a review from Manciukic June 10, 2026 12:50
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.

1 participant