fix(vhost-user-block): honour a readonly backend - #6083
Merged
ShadowCurse merged 2 commits intoSep 14, 2026
Merged
ShadowCurse merged 2 commits into
ShadowCurse merged 2 commits into
Conversation
11 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6083 +/- ##
=======================================
Coverage 83.04% 83.04%
=======================================
Files 277 277
Lines 31438 31439 +1
=======================================
+ Hits 26107 26108 +1
Misses 5331 5331
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1stvamp
force-pushed
the
fix/vhost-user-blk-read-only
branch
from
August 17, 2026 12:00
74072c9 to
4a7441f
Compare
Contributor
Author
|
The earlier CI failure here was nothing to do with the change. Build #18251 failed on I've rebased onto current main since then, so there's a fresh build queued. Whenever someone has a moment to unblock it, it'd be good to get a clean run on the record. |
1stvamp
force-pushed
the
fix/vhost-user-blk-read-only
branch
from
August 20, 2026 20:41
4a7441f to
71d04f6
Compare
This was referenced Aug 22, 2026
1stvamp
force-pushed
the
fix/vhost-user-blk-read-only
branch
from
August 25, 2026 01:17
71d04f6 to
cbaeea1
Compare
zulinx86
added
the
Status: Awaiting assignee
1stvamp
force-pushed
the
fix/vhost-user-blk-read-only
branch
from
September 1, 2026 11:16
cbaeea1 to
70e9c01
Compare
1stvamp
force-pushed
the
fix/vhost-user-blk-read-only
branch
from
September 8, 2026 11:05
70e9c01 to
3af1a30
Compare
JamesC1305
reviewed
Sep 8, 2026
JamesC1305
left a comment
JamesC1305
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the PR, the change LGTM. Just one suggestion which could maybe improve readability.
1stvamp
force-pushed
the
fix/vhost-user-blk-read-only
branch
from
September 8, 2026 12:11
3af1a30 to
fe7fda1
Compare
ShadowCurse
reviewed
Sep 8, 2026
The VIRTIO_BLK_F_RO check read acked_features after it had been reassigned to acked_features & PROTOCOL_FEATURES, so it tested bit 5 against 0x4000_0000 and never matched. read_only was therefore always false, even though the device does ask the backend for VIRTIO_BLK_F_RO, and docs/api_requests/block-vhost-user.md documents the backend as the place a readonly vhost-user drive is configured. read_only feeds Block::read_only(), which decides whether a root device gets ro or rw on the guest kernel cmdline, so a readonly vhost-user root device was given rw. The guest still sees VIRTIO_BLK_F_RO and retries the root mount readonly, so it boots, but /proc/cmdline is wrong and a later remount,rw fails. There is no way to correct this from config: VhostUserBlockConfig::try_from rejects is_read_only, which leaves the backend as the only source of the flag. Two different values sharing one name is the actual fault, so they are named apart. negotiate_features now returns backend_acked_features, and acked_features keeps its meaning of what the guest driver has acked, so read_only reads the backend set and no ordering constraint is needed. test_new_all_features already advertises VIRTIO_BLK_F_RO from its mock backend and asserted the broken result, so it now asserts read_only is set, and test_vhost_user_block asserts ro reaches the guest cmdline. Signed-off-by: Wes Mason <wes@1stvamp.org>
1stvamp
force-pushed
the
fix/vhost-user-blk-read-only
branch
from
September 8, 2026 16:47
fe7fda1 to
003c1b5
Compare
JamesC1305
approved these changes
Sep 11, 2026
ShadowCurse
approved these changes
Sep 14, 2026
ShadowCurse
enabled auto-merge
September 14, 2026 09:24
Merged
via the queue into
firecracker-microvm:main
with commit Sep 14, 2026
eaba0ed
6 of 7 checks passed
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
4 participants
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.
Description
A vhost-user-block device backed by a readonly backend isn't treated as readonly.
read_onlyis alwaysfalse.In
VhostUserBlockImpl::new:The
VIRTIO_BLK_F_ROcheck readsacked_featuresafter it has been reassigned toacked_features & PROTOCOL_FEATURES, so it tests bit 5 against0x4000_0000and never matches.That goes against what the device sets out to do.
AVAILABLE_FEATURESincludesVIRTIO_BLK_F_ROwith the comment "We always try to negotiate readonly with the backend. If the backend is configured as readonly, we will accept it.", and the docs say the same: "Whenever the backend advertises theVIRTIO_BLK_F_ROfeature, Firecracker will accept it, and the device will act as readonly." So the documented behaviour was never implemented.read_onlyfeedsBlock::read_only(), whichattach_block_devicespasses toappend_root_device_cmdline, so a readonly vhost-user root device is givenrw. To be clear about the blast radius: the guest still seesVIRTIO_BLK_F_RO(that bit reaches it viaavail_features) so it retries the root mount readonly and does boot, which is whytest_vhost_user_blockpasses today. What you're left with is a/proc/cmdlinethat disagrees with reality and a laterremount,rwthat fails. There's no working around it from config either, sinceVhostUserBlockConfig::try_fromrejectsis_read_onlyfor vhost-user drives (it must beNone), which leaves the backend as the only intended source of the flag.The fix computes
read_onlybeforeacked_featuresis narrowed, rather than readingavail_features, so the shadowing can't swallow the bit again.test_new_all_featuresalready advertisesVIRTIO_BLK_F_ROfrom its mock backend and assertsavail_featurescontains it, so it was asserting the broken result. It now assertsread_onlyis set.test_new_no_featuresuses a backend with no features and keeps asserting it is not.test_vhost_user_blockalready boots a readonly-backend root device via theuvm_vhost_user_booted_rofixture, so it now also assertsroreaches the guest cmdline, which is the part no existing test covered.I found this while working on the generic vhost-user device (#6072), which shares this negotiation code.
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
tools/devtool checkbuild --allto verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.This is part 1 of 4 in a stack made with GitButler: