This now required after Rust-VMM crate bumps. Also reorder some syscalls
so that they are now in alphabetical order.
Signed-off-by: Rob Bradford <rbradford@meta.com>
Split the data length check into two conditions:
- reject descriptors shorter than one
virtio_blk_discard_write_zeroes segment, and
- reject payloads exceeding MAX_DISCARD_WRITE_ZEROES_SEG segments
Previously only the minimum length was checked and extra segments
were silently dropped.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
Change Block to hold DiskBackend instead of
Box<dyn async_io::DiskFile>. In device_manager, existing formats
(raw, vhd, vhdx) are wrapped in DiskBackend::Legacy while
QcowDiskSync uses DiskBackend::Next. The fuzz target is updated
accordingly.
The Error::DiskResize variant now carries BlockError instead of
DiskFileError, matching the BlockResult return type of
DiskBackend::resize().
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
Set discard_sector_alignment from the logical block size
reported by the backend topology instead of hardcoding it
to 1 sector. This gives the guest accurate alignment hints
so it can avoid sub block discards that the filesystem
might silently ignore.
For example, on a 4K block filesystem the alignment is now
8 sectors (4096/512) instead of 1.
For image formats with their own allocation units (QCOW2
clusters, VHD/VHDX block sizes), the ideal alignment would
be derived from the format cluster/block size. This is
left for a followup that surfaces allocation granularity
through DiskTopology.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
The virtio vhost-user device backend prefers to use externally-provided
eventfds as irqfds. This allows the frontend VM to notify the backend
VM directly, without the need for a userspace proxy process. Since the
frontend can provide irqfds at any time, the backend needs to register
and unregister irqfds dynamically.
This is tricky because the functions that access the irqfd table all
take `&self`, not `&mut self`. The obvious solution to this problem is
to wrap the table in a mutex. Most of these functions are not called on
hot paths, but `.notifier()` is called whenever Cloud Hypervisor needs
to inject an interrupt into a guest. Most devices don't need to
register irqfds at runtime, and for them, slowing down interrupt
injection would be wasteful.
Instead, require devices to opt-in to irqfd registration. The irqfd
table now comes in two forms: one that contains a mutex and one that
does not. The one containing a mutex can be mutated freely, while
attempting to mutate the one that does not will panic.
Right now, no code registeres irqfds at runtime, but this will change in
subsequent commits.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
It is currently left as unimplemented!().
No functional change intended as there are no callers.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
Trigger the interrupts in the guest for the virtio device queues behind
the vhost-user devices when resuming. This avoids a situation where
interrupts from the backend get lost when they are dispatched from the
backend when then guest is paused leading to the guest/backend
effectively waiting for each other to move forward. This is more
reproducible with longer durations between pause and resume as there is
more opportunity for the backend to completely process it's queue and
fire all the interrupts.
It's perfectly safe and allowed by the virtio spec to generate these
interrupts and the performance impact is negligible and is a safe way to
ensure forward progress after a resume.
See: #7850
Signed-off-by: Rob Bradford <rbradford@meta.com>
When VIRTIO_BLK_F_DISCARD or VIRTIO_BLK_F_WRITE_ZEROES features
are advertised, the virtio spec v1.2, sections 5.2.4 and
5.2.6.1, requires the corresponding VirtioBlockConfig fields
to contain valid, non zero values. Leaving them at zero causes
strictly behaved drivers to either reject the features or crash.
Populate max_discard_sectors, max_discard_seg,
discard_sector_alignment, max_write_zeroes_sectors,
max_write_zeroes_seg and write_zeroes_may_unmap after
feature advertisement so drivers can safely negotiate
these features.
Fixes: #7849
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
This device is not called virtio-vhost-user; that's something else.
I don't think the comment really clarifies anything anyway, so just
remove it.
Fixes: 8c618ff5e ("virtio-devices: generic-vhost-user: implement device")
Signed-off-by: Alyssa Ross <hi@alyssa.is>
A buggy or malicious guest may write an inappropriate value into
virtqueue's next_avail field. This will result in an error
when iterating over the queue:
863837ef86/virtio-queue/src/queue.rs (L708)
but this error is (logged and) ignored if pop_descriptor_chain()
is used:
863837ef86/virtio-queue/src/queue.rs (L583)
A reasonable approach, implemented here, is to mark the device as
NEEDS_RESET and ignore further queue events until the guest
reinitializes the device.
How this patch was tested:
Linux kernel was patched to trigger a bad next_avail when the
virtqueue queue counter reaches 5000:
--------------- START OF LINUX KERNEL PATCH ----------
$ git diff
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index b784aab668670..989f2a0c64a77 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -15,6 +15,9 @@
#include <linux/spinlock.h>
#include <xen/xen.h>
+
+void virtqueue_kick_always(struct virtqueue *vq);
+
#ifdef DEBUG
/* For development, we want to crash whenever the ring is screwed. */
#define BAD_RING(_vq, fmt, args...) \
@@ -677,6 +680,12 @@ static inline int virtqueue_add_split(
struct virtqueue *_vq,
* new available array entries. */
virtio_wmb(vq->weak_barriers);
vq->split.avail_idx_shadow++;
+ {
+ if ((vq->split.avail_idx_shadow % 100) == 0)
+ printk(KERN_ERR "avail idx: %d",
+ (int)vq->split.avail_idx_shadow);
+ if (vq->split.avail_idx_shadow == 5000)
+ vq->split.avail_idx_shadow = 0;
+ }
vq->split.vring.avail->idx = cpu_to_virtio16(_vq->vdev,
vq->split.avail_idx_shadow);
vq->num_added++;
@@ -689,6 +698,11 @@ static inline int virtqueue_add_split(
struct virtqueue *_vq,
if (unlikely(vq->num_added == (1 << 16) - 1))
virtqueue_kick(_vq);
+ {
+ if (unlikely(vq->split.avail_idx_shadow == 0))
+ virtqueue_kick_always(_vq);
+ }
+
return 0;
unmap_release:
@@ -2515,6 +2529,11 @@ bool virtqueue_kick(struct virtqueue *vq)
}
EXPORT_SYMBOL_GPL(virtqueue_kick);
+void virtqueue_kick_always(struct virtqueue *vq)
+{
+ virtqueue_kick_prepare(vq);
+ virtqueue_notify(vq);
+}
/**
* virtqueue_get_buf_ctx - get the next used buffer
* @_vq: the struct virtqueue we're talking about.
--------------- END OF LINUX KERNEL PATCH ----------
Then the kernel was booted, and the host pinged until the
nic became unresponsive:
ping -i 0.002 192.168.4.1
Device status was confirmed using
cat /sys/class/net/eth0/device/status
(it was 0x4f).
Then the device was re-initialized:
DEV_NAME=$(basename $(readlink -f /sys/class/net/eth0/device))
echo $DEV_NAME | tee /sys/bus/virtio/drivers/virtio_net/unbind
echo $DEV_NAME | tee /sys/bus/virtio/drivers/virtio_net/bind
ip link set eth0 up
At this point networking became healthly again.
Signed-off-by: Peter Oskolkov <posk@google.com>
Moves the MSRV requirement to the workspace and expands it to all
cloud-hypervisor dependencies and dev-dependencies.
This improves discoverability for new contributors working on crates
other than the cloud-hypervisor itself and creates consistency regarding
the MSRV of cloud-hypervisor dependencies.
Functionally, this doesn't change anything for dependencies of the
cloud-hypervisor crate as the MSRV requirement is already enforced by CI
when building the cloud-hypervisor with the MSRV versioned compiler.
On-behalf-of: SAP julian.schindel@sap.com
Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>
We ran the Microsoft Windows installer in CHV (via network + VNC) and
the installation always failed when the installer wanted to write the
partition table. Since recently, for very good reason, sector 0 writes
are disabled if the image type is not set explicitly and only
implicitly auto-detected as raw [0].
To ease troubleshooting, I've added a descriptive log message. It is a
little spammy, but it is what is required to help users to troubleshoot.
[0] 6ecdf90e22
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com
Signal the guest before processing queue submissions to enable
earlier guest side completion event handling, reducing end-to-end
latency for block device operations.
FIO benchmarks show up to 7.4% bandwidth improvement at 16
iodepth and 4k block size with NVMe devices.
Signed-off-by: wuxinyue <wuxinyue.wxy@antgroup.com>
Both process_input_queue and process_output_queue were only processing
the first descriptor of a chain, leading to data loss if the driver used
chained descriptors.
This change iterates through all descriptors in a chain. It also moves
the flush call out of the descriptor loops to improve performance.
Signed-off-by: Andrei Vagin <avagin@google.com>
For a transmit queue (guest to host), the host only reads from the
guest-provided buffers and does not write to them. According to the
virtio specification (e.g., Section 2.6.8 in Virtio 1.1), the 'len'
field in the used ring has to be set to the number of bytes written to
the buffers. Therefore, it should be 0 for the console transmit queue.
Signed-off-by: Andrei Vagin <avagin@google.com>
The update_console_size method was using fetch_and on the acked_features
atomic, which modified the atomic and cleared other feature bits.
Changed it to use a non-destructive load and bitwise AND.
Signed-off-by: Andrei Vagin <avagin@google.com>
Add a per-disk lock_granularity parameter that lets users choose
between byte-range OFD locks and whole-file OFD locks:
--disk path=/foo.img,lock_granularity=byte-range
--disk path=/bar.img,lock_granularity=full
Byte-range is the default and matches QEMU behavior, working
best with storage backends where whole-file OFD locks are treated
as mandatory. The full option restores the original whole-file
locking for environments that depend on it.
The LockGranularityChoice enum and its FromStr impl live in the
block crate alongside the existing LockGranularity type. The
Block device converts the user-facing choice to the internal
LockGranularity at lock time, keeping device_manager.rs simple.
Closes: #7553
Signed-off-by: Victor Vieux <vieux@repl.it>
Rename the transport feature bit constant from
VIRTIO_F_IOMMU_PLATFORM to VIRTIO_F_ACCESS_PLATFORM across
the entire virtio-devices crate.
The virtio specification as of v1.1 carries bit 33 as
VIRTIO_F_ACCESS_PLATFORM. The Linux kernel UAPI header
<linux/virtio_config.h> carries VIRTIO_F_IOMMU_PLATFORM
only as a backward-compatible alias.
This is a pure rename with no functional or behavioral
change.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
Fix the _reserved field in VirtioIommuConfig from [u8; 7] to
[u8; 3], correcting the struct size from 44 bytes to the
spec-mandated 40 bytes.
The virtio specification v1.2, Section 5.13.4 defines struct
virtio_iommu_config as 40 bytes total.
The kernel UAPI header linux/virtio_iommu.h matches this
layout with __u8 reserved[3] since kernel 5.17. Prior to that,
the struct was 36 bytes with no bypass field at all.
The incorrect [u8; 7] made the packed struct 44 bytes. Since
the struct is exposed to the guest, the guest saw a 44 byte
device specific configuration region instead of 40 bytes.
While well behaved guest drivers only access fields at known
offsets and would not observe data corruption from the extra
4 zero bytes at the tail, the oversized config region is a
spec violation.
The write_config path is not affected because it validates
the exact offset of the bypass field before allowing writes,
and the bypass field sits at offset 36 regardless of the
trailing reserved size.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
This implements a generic vhost-user device. All information about this
device must be provided to Cloud Hypervisor via the command-line or API.
The main use-case is types of vhost-user devices Cloud Hypervisor
doesn't know about, but it can also be used for types it does know
about.
The generic device delegates all configuration space handling to the
backend. This means that the vhost-user backend must support
configuration space access. It also means that the backend has control
of configuration space. For instance, this means that setting the tag
of a virtio-fs device on the virtiofsd command line works as expected.
If the VM is snapshotted or migrated, the backend must write the
configuration space to a separate save file or migration stream.
Similarly, if the VM is restored or migrated, the backend must read the
configuration space from a separate save file or migration stream.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
As well as rejecting writes to sector 0 in the case of raw files where
the user hasn't specified the image_type also reject virtio requests of
type discard and write_zeroes.
Signed-off-by: Rob Bradford <rbradford@meta.com>
If the disk image was autodetected to raw (not specified with image_type
= 0) then in the virtio-block subsystem generate errors for writes to
block 0 (treat as if read-only). This gives an immediate error vs using
the image implementations in the block subsystem.
Signed-off-by: Rob Bradford <rbradford@meta.com>
Unlike most virtio feature bits, VIRTIO_BLK_F_RO is not optional.
It indicates that the host is refusing to permit write operations, and
the guest must not be allowed to override it.
However, the block device currently does not enforce this. If the guest
does not negotiate VIRTIO_BLK_F_RO, the block device will think the
device is writable and forward write requests to the backend.
This is not a security problem right now because the backing device of a
read-only device is always opened read-only. The kernel will thus
reject the write operations with EBADF. If support is added for
receiving the backing device file descriptor via SCM_RIGHTS (#7704),
it will be possible to have a read-only block device backed by a
writable file descriptor. This would make the bug a genuine security
vulnerability.
Fix the bug by explicitly checking if VIRTIO_BLK_F_RO was offered but
not negotiated. In this case, log a warning and proceed as if the guest
did acknowledge the feature. This always indicates a guest driver bug.
Fixes: #7697
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
OVMF sends FLUSH requests to read-only virtio-block devices. Refusing
these requests prevents OVMF from accessing the EFI System Partition and
therefore makes VMs unable to boot. Accept these requests instead.
them.
Ignoring these requests is possible, but inconsistent with fsync(2)
which honors them.
Fixes: #7698
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
Add supports_zero_flag() to DiskFile trait to indicate whether a disk
format can mark clusters/blocks as reading zeros without deallocating
storage.
QCOW2 supports this via the zero flag in L2 entries. VHDX also has
PAYLOAD_BLOCK_ZERO state for this, though it's not yet implemented in
cloud-hypervisor.
This enables DISCARD to be advertised even with sparse=false for formats
with zero-flag support, since they can mark regions as zeros (keeps
storage allocated) instead of requiring full deallocation.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
Add sparse boolean configuration option to DiskConfig with a default
value of true to control disk space allocation behavior.
When sparse is true, the disk uses sparse allocation where deallocated
blocks are returned to the filesystem, and the DISCARD feature is
advertised to the guest.
When sparse is false, disk space is kept fully allocated and DISCARD
is not advertised.
WRITE_ZEROES is always advertised when the backend supports it,
regardless of the sparse setting.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
Add capability query to DiskFile trait to check backend
support for sparse operations (punch hole, write zeroes,
discard). Only advertise VIRTIO_BLK_F_DISCARD and
VIRTIO_BLK_F_WRITE_ZEROES when the backend supports these
operations.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
Instead of closing a file descriptor that belongs to the vhost-user
frontend, drop the vu_common_ctrl::VhostUserHandle and the
vhost::vhost_user::Frontend it contains. This causes the destructor to
drop the file descriptor.
This breaks the last DPDK test, so disable it. See #7689.
Fixes: #7163
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
Since kernel commit 6693731487a8 ("vsock/virtio: Allocate nonlinear SKBs
for handling large transmit buffers"), a large vsock packet can be split
into multiple descriptors.
If we encounter such TX packets, pull the content into an owned buffer.
Fixes: #7672
Signed-off-by: Wei Liu <liuwe@microsoft.com>
This adds some missing features that are useful. In particular it adds
VIRTIO_F_RING_INDIRECT_DESC which gives a performance improvement.
Signed-off-by: Rob Bradford <rbradford@meta.com>
Reported-by: Daniel Farina <daniel@ubicloud.com>
VsockPacket::hdr holds a raw pointer to the address of the VSock packet
header, which is in guest memory. It opens the door to double-fetch
(or TOCTOU) race conditions. Therefore, VSockPacket::hdr content can't
be trusted since it can be arbitrarily changed by the guest, at any
time.
To mitigate this, we can copy the header content to an array in VMM's
memory that the guest can't modify.
Signed-off-by: Thomas Leroy <thomas.leroy.mp@gmail.com>
Based upon the discussion and in
https://github.com/rust-vmm/vhost/issues/29#issue-830820820 and the QEMU
behaviour the get_config offset should be zero. This was not caught by
our integration tests as the vhost-user-blk backend as implemented in
this repository does not use the offset.
Fixes: #7615
Signed-off-by: Rob Bradford <rbradford@meta.com>
It should always succeed and is apparently implicitly called by libc or
some dependency somewhere.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
This change is a prerequisite for live disk resizing. Before this
commit, the epoll-handler threads just got a copy of the sector
size which we cannot update during runtime.
On-behalf-of: SAP thomas.prescher@sap.com
Signed-off-by: Thomas Prescher <thomas.prescher@cyberus-technology.de>
Add fcntl to virtio_block_thread_rules to allow try_clone() on file
descriptors. The try_clone() method uses fcntl(fd, F_DUPFD_CLOEXEC)
to duplicate file descriptors, which is needed for efficient QCOW2
L1 table sync that avoids temporary allocations.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
https://github.com/cloud-hypervisor/cloud-hypervisor/pull/7294 adjusted
the checks for read-only requests made to virtio-blk devices and started
rejecting VIRTIO_BLK_T_GET_ID requests. These requests do not perform
any writes and are needed in order to access device serials from within
the guest.
Signed-off-by: Connor Brewster <cbrewster@hey.com>
This is a follow-up of [0].
# Advantages
- This saves dozens of unneeded clone()s across the whole code base
- Makes it much easier to reason about how parameters are used
(often we passed owned Arc/Rc versions without actually needing
ownership)
# Exceptions
For certain code paths, the alternatives would require awkward or overly
complex code, and in some cases the functions are the logical owners of
the values they take. In those cases, I've added
#[allow(clippy::needless_pass_by_value)].
This does not mean that one should not improve this in the future.
[0] 6a86c157af
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com
This helps to uncover expensive and needless clones in the code base.
For example, I prevented extensive clones in the snapshot path where
(nested) BTreeMap's have been cloned over and over again. Further,
the lint helps devs to much better reason about the ownership of
parameters.
All of these changes have been done manually with the necessary
caution. A few structs that are cheap to clone are now `copy` so that
this lint won't trigger for them.
I didn't enable the lint so far as it is a massive rabbit hole and
needs much more fixes. Nevertheless, it is very useful.
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com
