New issue
Advanced search Search tips

Issue 872973 link

Starred by 3 users

Issue metadata

Status: Verified
Owner: ----
Closed: Aug 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

crosvm virtio-blk device should report VIRTIO_BLK_F_RO for read-only disks

Project Member Reported by dverkamp@chromium.org, Aug 9

Issue description

The virtio block device specification provides a feature bit, VIRTIO_BLK_F_RO, that devices should set for disks that are read-only.

crosvm should provide the read-only status of disk images (--disk/--qcow vs --rwdisk/--rwqcow) to the virtio-blk device model so it can report the VIRTIO_BLK_F_RO flag when applicable. Currently, the RO feature flag is never set, meaning the guest driver considers all devices to be writable.

This behavior can be verified from within the guest using lsblk. Current output looks like this (note the 0 in the RO column for both devices, even though vda is read only):

NAME MAJ:MIN RM   SIZE RO TYPE MOUNTPOINT
vda  254:0    0 244.9M  0 disk /opt/google/cros-containers
vdb  254:16   0  88.9G  0 disk /dev/wl0

Or check /sys/block/$dev/ro (should be 1 for read-only devices).
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosvm/+/5f871eb8f4b152a5f97af0482edefe3a1708f033

commit 5f871eb8f4b152a5f97af0482edefe3a1708f033
Author: Daniel Verkamp <dverkamp@chromium.org>
Date: Mon Aug 13 21:25:11 2018

devices: block: define features as shift counts

Convert the definition of VIRTIO_BLK_F_FLUSH to a shift count instead of
a bitmask.  This matches the way the features are defined in the virtio
spec and makes block consistent with other device models, such as p9.

BUG= chromium:872973 
TEST=cargo test -p devices

Change-Id: Iece974c6f4d826b7bb76622973f08469a7936234
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1170303
Reviewed-by: Zach Reizner <zachr@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/5f871eb8f4b152a5f97af0482edefe3a1708f033/devices/src/virtio/block.rs

Project Member

Comment 2 by bugdroid1@chromium.org, Aug 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosvm/+/de9ae03d522d9f134d76cc60a7c49b7c38714423

commit de9ae03d522d9f134d76cc60a7c49b7c38714423
Author: Daniel Verkamp <dverkamp@chromium.org>
Date: Mon Aug 13 21:25:11 2018

main: replace block writable option with read_only

This is more consistent with the way it will be used in the virtio-blk
device model.

BUG= chromium:872973 
TEST=cargo test

Change-Id: I28c5d007a7f3864ef6e18e9b343d263123302484
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1170304
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Zach Reizner <zachr@chromium.org>

[modify] https://crrev.com/de9ae03d522d9f134d76cc60a7c49b7c38714423/src/linux.rs
[modify] https://crrev.com/de9ae03d522d9f134d76cc60a7c49b7c38714423/src/main.rs

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosvm/+/18fa20569fff7fffcc7e62a37b217be00830be13

commit 18fa20569fff7fffcc7e62a37b217be00830be13
Author: Daniel Verkamp <dverkamp@chromium.org>
Date: Tue Aug 14 04:58:42 2018

devices: block: store features as a u64

This matches other virtio device models (net, p9, vsock) and makes it
easier to add flags conditionally at device creation time.

BUG= chromium:872973 
TEST=cargo test -p devices

Change-Id: I65b3f37c220fae44a3f6b397acc6c0eec2b70bf2
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1170305
Reviewed-by: Zach Reizner <zachr@chromium.org>

[modify] https://crrev.com/18fa20569fff7fffcc7e62a37b217be00830be13/devices/src/virtio/block.rs

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosvm/+/70589a0797115b3df7d42de132f0247ee7727f5d

commit 70589a0797115b3df7d42de132f0247ee7727f5d
Author: Daniel Verkamp <dverkamp@chromium.org>
Date: Tue Aug 14 04:58:42 2018

devices: block: add support for read-only feature

The virtio block specification defines the VIRTIO_BLK_F_RO feature bit
to indicate read-only block devices.  Plumb the read-only status of
block devices from the crosvm frontend into the virtio block device and
populate the flag when appropriate.

BUG= chromium:872973 
TEST=Verify lsblk output in guest has the correct RO values

Change-Id: I23af87cce8020641cd702adca6e8ff9fdd2b8220
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1170306
Reviewed-by: Zach Reizner <zachr@chromium.org>

[modify] https://crrev.com/70589a0797115b3df7d42de132f0247ee7727f5d/src/linux.rs
[modify] https://crrev.com/70589a0797115b3df7d42de132f0247ee7727f5d/devices/src/virtio/block.rs

Status: Verified (was: Unconfirmed)
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosvm/+/a40cbb4a948ea826f69cb83728c31a5a253a3b1c

commit a40cbb4a948ea826f69cb83728c31a5a253a3b1c
Author: Daniel Verkamp <dverkamp@chromium.org>
Date: Fri Sep 21 07:51:15 2018

devices: block: enforce read-only in execute()

To fully meet the requirements laid out by the virtio specification, we
need to fail write commands for devices that expose VIRTIO_BLK_F_RO with
a specific error code of VIRTIO_BLK_S_IOERR.  Pipe the read_only status
down into the worker and the request execute function so that it can be
checked and return the correct error code.

BUG= chromium:872973 
TEST=Attempt to write to read-only /dev/vda in termina

Change-Id: I98c8ad17fde497e5a529d9e65096fb4ef022fd65
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1211062
Reviewed-by: Stephen Barber <smbarber@chromium.org>

[modify] https://crrev.com/a40cbb4a948ea826f69cb83728c31a5a253a3b1c/devices/src/virtio/block.rs

Sign in to add a comment