fix(vhost-user-block): honour a readonly backend

[1stvamp]

Description

A vhost-user-block device backed by a readonly backend isn't treated as readonly. read_only is always false.

In VhostUserBlockImpl::new:

let avail_features = acked_features;
let acked_features = acked_features & VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits();
let read_only = acked_features & (1 << VIRTIO_BLK_F_RO) != 0;

The VIRTIO_BLK_F_RO check reads acked_features after it has been reassigned to acked_features & PROTOCOL_FEATURES, so it tests bit 5 against 0x4000_0000 and never matches.

That goes against what the device sets out to do. AVAILABLE_FEATURES includes VIRTIO_BLK_F_RO with 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 the VIRTIO_BLK_F_RO feature, Firecracker will accept it, and the device will act as readonly." So the documented behaviour was never implemented.

read_only feeds Block::read_only(), which attach_block_devices passes to append_root_device_cmdline, so a readonly vhost-user root device is given rw. To be clear about the blast radius: the guest still sees VIRTIO_BLK_F_RO (that bit reaches it via avail_features) so it retries the root mount readonly and does boot, which is why test_vhost_user_block passes today. What you're left with is a /proc/cmdline that disagrees with reality and a later remount,rw that fails. There's no working around it from config either, since VhostUserBlockConfig::try_from rejects is_read_only for vhost-user drives (it must be None), which leaves the backend as the only intended source of the flag.

The fix computes read_only before acked_features is narrowed, rather than reading avail_features, so the shadowing can't swallow the bit again.

test_new_all_features already advertises VIRTIO_BLK_F_RO from its mock backend and asserts avail_features contains it, so it was asserting the broken result. It now asserts read_only is set. test_new_no_features uses a backend with no features and keeps asserting it is not. test_vhost_user_block already boots a readonly-backend root device via the uvm_vhost_user_booted_ro fixture, so it now also asserts ro reaches 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

• 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.

---

This is part 1 of 4 in a stack made with GitButler:

•  4  feat(vhost-user): add a generic vhost-user device #6147
•  3  refactor(vhost-user-block): delegate to the generic vhost-user frontend #6146
•  2  feat(vhost-user): add a device-type agnostic vhost-user frontend #6072
•  1  fix(vhost-user-block): honour a readonly backend #6083 👈

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.82%. Comparing base (40f2ed2) to head (74072c9).
⚠️ Report is 8 commits behind head on main.

⚠️ Current head 74072c9 differs from pull request most recent head cbaeea1

Please upload reports for the commit cbaeea1 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6083      +/-   ##
==========================================
- Coverage   82.98%   82.82%   -0.17%     
==========================================
  Files         277      276       -1     
  Lines       30884    30770     -114     
==========================================
- Hits        25630    25486     -144     
- Misses       5254     5284      +30     

Flag	Coverage Δ
5.10-m5n.metal	83.06% <100.00%> (-0.20%)	⬇️
5.10-m6a.metal	82.42% <100.00%> (-0.20%)	⬇️
5.10-m6g.metal	79.88% <100.00%> (-0.18%)	⬇️
5.10-m6i.metal	83.07% <100.00%> (-0.19%)	⬇️
5.10-m7a.metal-48xl	82.41% <100.00%> (-0.20%)	⬇️
5.10-m7g.metal	79.88% <100.00%> (-0.18%)	⬇️
5.10-m7i.metal-24xl	83.03% <100.00%> (-0.20%)	⬇️
5.10-m7i.metal-48xl	83.03% <100.00%> (-0.20%)	⬇️
5.10-m8g.metal-24xl	79.88% <100.00%> (-0.18%)	⬇️
5.10-m8g.metal-48xl	79.88% <100.00%> (-0.18%)	⬇️
5.10-m8i.metal-48xl	83.04% <100.00%> (-0.19%)	⬇️
5.10-m8i.metal-96xl	83.04% <100.00%> (-0.19%)	⬇️
6.1-m5n.metal	83.09% <100.00%> (?)
6.1-m6a.metal	82.44% <100.00%> (?)
6.1-m6g.metal	79.88% <100.00%> (?)
6.1-m6i.metal	83.09% <100.00%> (?)
6.1-m7a.metal-48xl	82.43% <100.00%> (?)
6.1-m7g.metal	79.88% <100.00%> (?)
6.1-m7i.metal-24xl	83.10% <100.00%> (?)
6.1-m7i.metal-48xl	83.10% <100.00%> (?)
6.1-m8g.metal-24xl	79.87% <100.00%> (?)
6.1-m8g.metal-48xl	79.88% <100.00%> (?)
6.1-m8i.metal-48xl	83.10% <100.00%> (?)
6.1-m8i.metal-96xl	83.10% <100.00%> (?)
6.18-m5n.metal	83.09% <100.00%> (-0.20%)	⬇️
6.18-m6a.metal	82.44% <100.00%> (-0.21%)	⬇️
6.18-m6g.metal	79.88% <100.00%> (-0.18%)	⬇️
6.18-m6i.metal	83.08% <100.00%> (-0.20%)	⬇️
6.18-m7a.metal-48xl	82.43% <100.00%> (-0.20%)	⬇️
6.18-m7g.metal	79.88% <100.00%> (-0.18%)	⬇️
6.18-m7i.metal-24xl	83.10% <100.00%> (-0.20%)	⬇️
6.18-m7i.metal-48xl	83.11% <100.00%> (-0.19%)	⬇️
6.18-m8g.metal-24xl	79.88% <100.00%> (-0.18%)	⬇️
6.18-m8g.metal-48xl	79.88% <100.00%> (-0.18%)	⬇️
6.18-m8i.metal-48xl	83.10% <100.00%> (-0.20%)	⬇️
6.18-m8i.metal-96xl	83.10% <100.00%> (-0.20%)	⬇️

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.

[1stvamp]

The earlier CI failure here was nothing to do with the change. Build #18251 failed on test_balloon.py::test_stats (free_memory off by 0.2%) on x86_64 and on test_shut_down.py::test_reboot (a 10s process-exit timeout) on aarch64, and neither test touches vhost-user. The optional pipeline passed.

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.