Signed-off-by: Alyssa Ross <alyssa.ross@unikie.com> --- .../virtualization/crosvm/default.nix | 4 ++ ...-consider-shm-buuffers-when-setting-.patch | 34 +++++++++ ...t_user-loosen-expected-message-order.patch | 71 +++++++++++++++++++ ...ces-vhost_user-remove-spurious-check.patch | 42 +++++++++++ 4 files changed, 151 insertions(+) create mode 100644 pkgs/applications/virtualization/crosvm/devices-properly-consider-shm-buuffers-when-setting-.patch create mode 100644 pkgs/applications/virtualization/crosvm/devices-vhost_user-loosen-expected-message-order.patch create mode 100644 pkgs/applications/virtualization/crosvm/devices-vhost_user-remove-spurious-check.patch diff --git a/pkgs/applications/virtualization/crosvm/default.nix b/pkgs/applications/virtualization/crosvm/default.nix index 311a2a63ae2..40e30dcd819 100644 --- a/pkgs/applications/virtualization/crosvm/default.nix +++ b/pkgs/applications/virtualization/crosvm/default.nix @@ -18,6 +18,10 @@ rustPlatform.buildRustPackage rec { patches = [ ./default-seccomp-policy-dir.diff + + ./devices-properly-consider-shm-buuffers-when-setting-.patch + ./devices-vhost_user-remove-spurious-check.patch + ./devices-vhost_user-loosen-expected-message-order.patch ]; cargoSha256 = "18mj0zc6yfwyrw6v1vl089dhh04kv2pzb99bygnn8nymdlx4fjqa"; diff --git a/pkgs/applications/virtualization/crosvm/devices-properly-consider-shm-buuffers-when-setting-.patch b/pkgs/applications/virtualization/crosvm/devices-properly-consider-shm-buuffers-when-setting-.patch new file mode 100644 index 00000000000..2f0f95c532a --- /dev/null +++ b/pkgs/applications/virtualization/crosvm/devices-properly-consider-shm-buuffers-when-setting-.patch @@ -0,0 +1,34 @@ +From f5afb62e138017f28966d8fa20b44fe00f301d7d Mon Sep 17 00:00:00 2001 +From: Puck Meerburg <puck@puckipedia.com> +Date: Mon, 26 Sep 2022 22:40:53 +0000 +Subject: [PATCH crosvm 1/3] devices: properly consider shm buuffers when + setting gpu_blob flag + +--- + devices/src/virtio/gpu/virtio_gpu.rs | 3 ++- + 1 file changed, 2 insertions(+), 1 deletion(-) + +diff --git a/devices/src/virtio/gpu/virtio_gpu.rs b/devices/src/virtio/gpu/virtio_gpu.rs +index 698215727a..ed97942f7a 100644 +--- a/devices/src/virtio/gpu/virtio_gpu.rs ++++ b/devices/src/virtio/gpu/virtio_gpu.rs +@@ -26,6 +26,7 @@ use rutabaga_gfx::RutabagaHandle; + use rutabaga_gfx::RutabagaIovec; + use rutabaga_gfx::Transfer3D; + use rutabaga_gfx::RUTABAGA_MEM_HANDLE_TYPE_DMABUF; ++use rutabaga_gfx::RUTABAGA_MEM_HANDLE_TYPE_SHM; + use sync::Mutex; + use vm_control::VmMemorySource; + use vm_memory::udmabuf::UdmabufDriver; +@@ -731,7 +732,7 @@ impl VirtioGpu { + descriptor: export.os_handle, + offset: 0, + size: resource.size, +- gpu_blob: true, ++ gpu_blob: export.handle_type != RUTABAGA_MEM_HANDLE_TYPE_SHM, + }, + } + } else { +-- +2.37.1 + diff --git a/pkgs/applications/virtualization/crosvm/devices-vhost_user-loosen-expected-message-order.patch b/pkgs/applications/virtualization/crosvm/devices-vhost_user-loosen-expected-message-order.patch new file mode 100644 index 00000000000..bcfab6eace1 --- /dev/null +++ b/pkgs/applications/virtualization/crosvm/devices-vhost_user-loosen-expected-message-order.patch @@ -0,0 +1,71 @@ +From 6ac11f04ea3344ca0c3873ee9526a8863c9529ae Mon Sep 17 00:00:00 2001 +From: Alyssa Ross <alyssa.ross@unikie.com> +Date: Wed, 28 Sep 2022 15:27:39 +0000 +Subject: [PATCH crosvm 3/3] devices: vhost_user: loosen expected message order + +Prior to this patch, the code assumed get_shared_memory_regions() +would be called before set_slave_req_fd(), since +get_shared_memory_regions() was the only place that set self.shmid, +and set_slave_req_fd() required it to be set. There's nothing in the +vhost-user spec that enforces this ordering though, so it could fail +when used with a non-crosvm frontend. + +To fix this, just don't store the shmid here at all, and fetch it from +the backend when it's required. set_slave_req_fd() should only be +called once, and the two backends that implement +get_shared_memory_region() (Wl and Gpu) both have trivial +implementations, so there shouldn't be any performance reason to cache +shmid here. + +TEST=Run cloud-hypervisor against a crosvm vhost-user backend + +Change-Id: Idb44aeb1dfe1aeff70081987fe6d7d215887d2e4 +--- + devices/src/virtio/vhost/user/device/handler.rs | 10 ++++++---- + 1 file changed, 6 insertions(+), 4 deletions(-) + +diff --git a/devices/src/virtio/vhost/user/device/handler.rs b/devices/src/virtio/vhost/user/device/handler.rs +index 932d948959..27b4eb1619 100644 +--- a/devices/src/virtio/vhost/user/device/handler.rs ++++ b/devices/src/virtio/vhost/user/device/handler.rs +@@ -408,7 +408,6 @@ where + mem: Option<GuestMemory>, + backend: Box<dyn VhostUserBackend>, + ops: O, +- shmid: Option<u8>, + } + + impl DeviceRequestHandler<VhostUserRegularOps> { +@@ -436,7 +435,6 @@ where + mem: None, + backend, + ops, +- shmid: None, + } + } + } +@@ -696,7 +694,12 @@ impl<O: VhostUserPlatformOps> VhostUserSlaveReqHandlerMut for DeviceRequestHandl + } + + fn set_slave_req_fd(&mut self, ep: Box<dyn Endpoint<SlaveReq>>) { +- let shmid = self.shmid.expect("unexpected slave_req_fd"); ++ let shmid = self ++ .backend ++ .get_shared_memory_region() ++ .expect("unexpected slave_req_fd") ++ .id; ++ + let frontend = Slave::new(ep); + self.backend + .set_shared_memory_mapper(Box::new(VhostShmemMapper { +@@ -738,7 +741,6 @@ impl<O: VhostUserPlatformOps> VhostUserSlaveReqHandlerMut for DeviceRequestHandl + + fn get_shared_memory_regions(&mut self) -> VhostResult<Vec<VhostSharedMemoryRegion>> { + Ok(if let Some(r) = self.backend.get_shared_memory_region() { +- self.shmid = Some(r.id); + vec![VhostSharedMemoryRegion::new(r.id, r.length)] + } else { + Vec::new() +-- +2.37.1 + diff --git a/pkgs/applications/virtualization/crosvm/devices-vhost_user-remove-spurious-check.patch b/pkgs/applications/virtualization/crosvm/devices-vhost_user-remove-spurious-check.patch new file mode 100644 index 00000000000..e8ea4db2357 --- /dev/null +++ b/pkgs/applications/virtualization/crosvm/devices-vhost_user-remove-spurious-check.patch @@ -0,0 +1,42 @@ +From e895a064f24d0101a230790bdd6adff6cda898d5 Mon Sep 17 00:00:00 2001 +From: Alyssa Ross <alyssa.ross@unikie.com> +Date: Wed, 28 Sep 2022 15:19:26 +0000 +Subject: [PATCH crosvm 2/3] devices: vhost_user: remove spurious check + +"size" is the amount of data the caller wants to read, not the size of +the data available to read, so this check doesn't make any sense. +It's completely valid to read 4 bytes of a 16 byte config space, +starting at offset 8, but that would fail this check. crosvm doesn't +seem to do this, but cloud-hypervisor does, so this caused crashes +when running cloud-hypervisor against a crosvm vhost-user backend. + +I suspect what this code meant to do is check whether offset + size +would be beyond the end of the config space, but in this part of the +code we don't know the size of the config space, so it's not possible +to check that here. + +TEST=Run cloud-hypervisor against a crosvm vhost-user backend + +Change-Id: I8a3d7960fb67bf8de37cb3f158081d6421859725 +--- + devices/src/virtio/vhost/user/device/handler.rs | 4 ---- + 1 file changed, 4 deletions(-) + +diff --git a/devices/src/virtio/vhost/user/device/handler.rs b/devices/src/virtio/vhost/user/device/handler.rs +index 32e4aaf876..932d948959 100644 +--- a/devices/src/virtio/vhost/user/device/handler.rs ++++ b/devices/src/virtio/vhost/user/device/handler.rs +@@ -680,10 +680,6 @@ impl<O: VhostUserPlatformOps> VhostUserSlaveReqHandlerMut for DeviceRequestHandl + size: u32, + _flags: VhostUserConfigFlags, + ) -> VhostResult<Vec<u8>> { +- if offset >= size { +- return Err(VhostError::InvalidParam); +- } +- + let mut data = vec![0; size as usize]; + self.backend.read_config(u64::from(offset), &mut data); + Ok(data) +-- +2.37.1 + -- 2.37.1