diff options
Diffstat (limited to 'devices/src')
| -rw-r--r-- | devices/src/irqchip/kvm/mod.rs | 12 | ||||
| -rw-r--r-- | devices/src/irqchip/kvm/x86_64.rs | 216 | ||||
| -rw-r--r-- | devices/src/irqchip/mod.rs | 5 | ||||
| -rw-r--r-- | devices/src/irqchip/x86_64.rs | 38 | ||||
| -rw-r--r-- | devices/src/pci/vfio_pci.rs | 2 | ||||
| -rw-r--r-- | devices/src/virtio/fs/passthrough.rs | 324 | ||||
| -rw-r--r-- | devices/src/virtio/video/decoder/mod.rs | 85 |
7 files changed, 602 insertions, 80 deletions
diff --git a/devices/src/irqchip/kvm/mod.rs b/devices/src/irqchip/kvm/mod.rs index 56794a3..a5fd89f 100644 --- a/devices/src/irqchip/kvm/mod.rs +++ b/devices/src/irqchip/kvm/mod.rs @@ -8,21 +8,29 @@ use std::sync::Arc; use sync::Mutex; use sys_util::{EventFd, Result}; +#[cfg(any(target_arch = "x86", target_arch = "x86_64"))] +mod x86_64; +#[cfg(any(target_arch = "x86", target_arch = "x86_64"))] +pub use x86_64::*; + use crate::IrqChip; /// IrqChip implementation where the entire IrqChip is emulated by KVM. /// /// This implementation will use the KVM API to create and configure the in-kernel irqchip. pub struct KvmKernelIrqChip { - _vm: KvmVm, + vm: KvmVm, vcpus: Arc<Mutex<Vec<Option<KvmVcpu>>>>, } impl KvmKernelIrqChip { /// Construct a new KvmKernelIrqchip. pub fn new(vm: KvmVm, num_vcpus: usize) -> Result<KvmKernelIrqChip> { + // TODO (colindr): this constructor needs aarch64 vs x86_64 implementations because we + // want to use vm.create_device instead of create_irq_chip on aarch64 + vm.create_irq_chip()?; Ok(KvmKernelIrqChip { - _vm: vm, + vm, vcpus: Arc::new(Mutex::new((0..num_vcpus).map(|_| None).collect())), }) } diff --git a/devices/src/irqchip/kvm/x86_64.rs b/devices/src/irqchip/kvm/x86_64.rs new file mode 100644 index 0000000..acede53 --- /dev/null +++ b/devices/src/irqchip/kvm/x86_64.rs @@ -0,0 +1,216 @@ +// Copyright 2020 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +use hypervisor::kvm::KvmVcpu; +use hypervisor::{IoapicState, LapicState, PicSelect, PicState, PitState}; +use kvm_sys::*; +use sys_util::Result; + +use crate::{Bus, IrqChipX86_64, KvmKernelIrqChip}; + +impl IrqChipX86_64<KvmVcpu> for KvmKernelIrqChip { + /// Get the current state of the PIC + fn get_pic_state(&self, select: PicSelect) -> Result<PicState> { + Ok(PicState::from(&self.vm.get_pic_state(select)?)) + } + + /// Set the current state of the PIC + fn set_pic_state(&mut self, select: PicSelect, state: &PicState) -> Result<()> { + self.vm.set_pic_state(select, &kvm_pic_state::from(state)) + } + + /// Get the current state of the IOAPIC + fn get_ioapic_state(&self) -> Result<IoapicState> { + Ok(IoapicState::from(&self.vm.get_ioapic_state()?)) + } + + /// Set the current state of the IOAPIC + fn set_ioapic_state(&mut self, state: &IoapicState) -> Result<()> { + self.vm.set_ioapic_state(&kvm_ioapic_state::from(state)) + } + + /// Get the current state of the specified VCPU's local APIC + fn get_lapic_state(&self, _vcpu_id: usize) -> Result<LapicState> { + unimplemented!("get_lapic_state for KvmKernelIrqChip is not yet implemented"); + } + + /// Set the current state of the specified VCPU's local APIC + fn set_lapic_state(&mut self, _vcpu_id: usize, _state: &LapicState) -> Result<()> { + unimplemented!("set_lapic_state for KvmKernelIrqChip is not yet implemented"); + } + + /// Create a PIT (Programmable Interval Timer) for this VM. + /// The KvmKernelIrqchip creates the PIT by calling the KVM_CREATE_PIT2 KVM API. The + /// io_bus is not used in this case because KVM handles intercepting port-mapped io intended + /// for the PIT. + fn create_pit(&mut self, _io_bus: &mut Bus) -> Result<()> { + self.vm.create_pit() + } + + /// Retrieves the state of the PIT. Gets the pit state via the KVM API. + fn get_pit(&self) -> Result<PitState> { + Ok(PitState::from(&self.vm.get_pit_state()?)) + } + + /// Sets the state of the PIT. Sets the pit state via the KVM API. + fn set_pit(&mut self, state: &PitState) -> Result<()> { + self.vm.set_pit_state(&kvm_pit_state2::from(state)) + } +} + +#[cfg(test)] +mod tests { + + use hypervisor::kvm::{Kvm, KvmVm}; + use sys_util::GuestMemory; + + use crate::irqchip::{IrqChip, IrqChipX86_64, KvmKernelIrqChip}; + use crate::Bus; + + use hypervisor::{PicSelect, Vm, VmX86_64}; + + fn get_chip() -> (KvmKernelIrqChip, KvmVm) { + let kvm = Kvm::new().expect("failed to instantiate Kvm"); + let mem = GuestMemory::new(&[]).unwrap(); + let vm = KvmVm::new(&kvm, mem).expect("failed tso instantiate vm"); + let vcpu = vm.create_vcpu(0).expect("failed to instantiate vcpu"); + + let mut chip = KvmKernelIrqChip::new(vm.try_clone().expect("failed to clone vm"), 1) + .expect("failed to instantiate KvmKernelIrqChip"); + + chip.add_vcpu(0, vcpu).expect("failed to add vcpu"); + + (chip, vm) + } + + #[test] + fn get_pic() { + let (chip, vm) = get_chip(); + + let state = chip + .get_pic_state(PicSelect::Primary) + .expect("could not get pic state"); + + // Default is that no irq lines are asserted + assert_eq!(state.irr, 0); + + // Assert Irq Line 0 + vm.set_irq_line(0, true).expect("could not set irq line"); + + let state = chip + .get_pic_state(PicSelect::Primary) + .expect("could not get pic state"); + + // Bit 0 should now be 1 + assert_eq!(state.irr, 1); + } + + #[test] + fn set_pic() { + let (mut chip, _) = get_chip(); + + let mut state = chip + .get_pic_state(PicSelect::Primary) + .expect("could not get pic state"); + + // set bits 0 and 1 + state.irr = 3; + + chip.set_pic_state(PicSelect::Primary, &state) + .expect("could not set the pic state"); + + let state = chip + .get_pic_state(PicSelect::Primary) + .expect("could not get pic state"); + + // Bits 1 and 0 should now be 1 + assert_eq!(state.irr, 3); + } + + #[test] + fn get_ioapic() { + let (chip, vm) = get_chip(); + + let state = chip.get_ioapic_state().expect("could not get ioapic state"); + + // Default is that no irq lines are asserted + assert_eq!(state.current_interrupt_level_bitmap, 0); + + // Default routing entries has routes 0..24 routed to vectors 0..24 + for i in 0..24 { + // when the ioapic is reset by kvm, it defaults to all zeroes except the + // interrupt mask is set to 1, which is bit 16 + assert_eq!(state.redirect_table[i].get(0, 64), 1 << 16); + } + + // Assert Irq Line 1 + vm.set_irq_line(1, true).expect("could not set irq line"); + + let state = chip.get_ioapic_state().expect("could not get ioapic state"); + + // Bit 1 should now be 1 + assert_eq!(state.current_interrupt_level_bitmap, 2); + } + + #[test] + fn set_ioapic() { + let (mut chip, _) = get_chip(); + + let mut state = chip.get_ioapic_state().expect("could not get ioapic state"); + + // set a vector in the redirect table + state.redirect_table[2].set_vector(15); + // set the irq line status on that entry + state.current_interrupt_level_bitmap = 4; + + chip.set_ioapic_state(&state) + .expect("could not set the ioapic state"); + + let state = chip.get_ioapic_state().expect("could not get ioapic state"); + + // verify that get_ioapic_state returns what we set + assert_eq!(state.redirect_table[2].get_vector(), 15); + assert_eq!(state.current_interrupt_level_bitmap, 4); + } + + #[test] + fn get_pit() { + let (mut chip, _) = get_chip(); + let mut io_bus = Bus::new(); + chip.create_pit(&mut io_bus).expect("failed to create pit"); + + let state = chip.get_pit().expect("failed to get pit state"); + + assert_eq!(state.flags, 0); + // assert reset state of pit + for i in 0..3 { + // initial count of 0 sets it to 0x10000; + assert_eq!(state.channels[i].count, 0x10000); + assert_eq!(state.channels[i].mode, 0xff); + assert_eq!(state.channels[i].gate, i != 2); + } + } + + #[test] + fn set_pit() { + let (mut chip, _) = get_chip(); + let mut io_bus = Bus::new(); + chip.create_pit(&mut io_bus).expect("failed to create pit"); + + let mut state = chip.get_pit().expect("failed to get pit state"); + + // set some values + state.channels[0].count = 500; + state.channels[0].mode = 1; + + // Setting the pit should initialize the one-shot timer + chip.set_pit(&state).expect("failed to set pit state"); + + let state = chip.get_pit().expect("failed to get pit state"); + + // check the values we set + assert_eq!(state.channels[0].count, 500); + assert_eq!(state.channels[0].mode, 1); + } +} diff --git a/devices/src/irqchip/mod.rs b/devices/src/irqchip/mod.rs index a8023e9..3284bc6 100644 --- a/devices/src/irqchip/mod.rs +++ b/devices/src/irqchip/mod.rs @@ -10,6 +10,11 @@ use std::marker::{Send, Sized}; use hypervisor::{IrqRoute, Vcpu}; use sys_util::{EventFd, Result}; +#[cfg(any(target_arch = "x86", target_arch = "x86_64"))] +mod x86_64; +#[cfg(any(target_arch = "x86", target_arch = "x86_64"))] +pub use x86_64::*; + /// Trait that abstracts interactions with interrupt controllers. /// /// Each VM will have one IrqChip instance which is responsible for routing IRQ lines and diff --git a/devices/src/irqchip/x86_64.rs b/devices/src/irqchip/x86_64.rs new file mode 100644 index 0000000..4b50427 --- /dev/null +++ b/devices/src/irqchip/x86_64.rs @@ -0,0 +1,38 @@ +// Copyright 2020 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +use crate::Bus; +use hypervisor::{IoapicState, LapicState, PicSelect, PicState, PitState, VcpuX86_64}; +use sys_util::Result; + +use crate::IrqChip; + +pub trait IrqChipX86_64<V: VcpuX86_64>: IrqChip<V> { + /// Get the current state of the PIC + fn get_pic_state(&self, select: PicSelect) -> Result<PicState>; + + /// Set the current state of the PIC + fn set_pic_state(&mut self, select: PicSelect, state: &PicState) -> Result<()>; + + /// Get the current state of the IOAPIC + fn get_ioapic_state(&self) -> Result<IoapicState>; + + /// Set the current state of the IOAPIC + fn set_ioapic_state(&mut self, state: &IoapicState) -> Result<()>; + + /// Get the current state of the specified VCPU's local APIC + fn get_lapic_state(&self, vcpu_id: usize) -> Result<LapicState>; + + /// Set the current state of the specified VCPU's local APIC + fn set_lapic_state(&mut self, vcpu_id: usize, state: &LapicState) -> Result<()>; + + /// Create a PIT (Programmable Interval Timer) for this VM. + fn create_pit(&mut self, io_bus: &mut Bus) -> Result<()>; + + /// Retrieves the state of the PIT. + fn get_pit(&self) -> Result<PitState>; + + /// Sets the state of the PIT. + fn set_pit(&mut self, state: &PitState) -> Result<()>; +} diff --git a/devices/src/pci/vfio_pci.rs b/devices/src/pci/vfio_pci.rs index 671e8cd..5f6998e 100644 --- a/devices/src/pci/vfio_pci.rs +++ b/devices/src/pci/vfio_pci.rs @@ -9,7 +9,7 @@ use std::u32; use kvm::Datamatch; use msg_socket::{MsgReceiver, MsgSender}; use resources::{Alloc, MmioType, SystemAllocator}; -use sys_util::{error, EventFd, MemoryMapping}; +use sys_util::{error, EventFd, MappedRegion, MemoryMapping}; use vfio_sys::*; use vm_control::{ diff --git a/devices/src/virtio/fs/passthrough.rs b/devices/src/virtio/fs/passthrough.rs index bcc7c5d..4833bb7 100644 --- a/devices/src/virtio/fs/passthrough.rs +++ b/devices/src/virtio/fs/passthrough.rs @@ -9,6 +9,7 @@ use std::fs::File; use std::io; use std::mem::{self, size_of, MaybeUninit}; use std::os::unix::io::{AsRawFd, FromRawFd, RawFd}; +use std::ptr; use std::str::FromStr; use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; use std::sync::Arc; @@ -765,6 +766,99 @@ fn forget_one( } } +// A temporary directory that is automatically deleted when dropped unless `into_inner()` is called. +// This isn't a general-purpose temporary directory and is only intended to be used to ensure that +// there are no leaks when initializing a newly created directory with the correct metadata (see the +// implementation of `mkdir()` below). The directory is removed via a call to `unlinkat` so callers +// are not allowed to actually populate this temporary directory with any entries (or else deleting +// the directory will fail). +struct TempDir { + path: CString, + file: File, +} + +impl TempDir { + // Creates a new temporary directory in `path` with a randomly generated name. `path` must be a + // directory. + fn new(path: CString) -> io::Result<TempDir> { + let mut template = path.into_bytes(); + + // mkdtemp requires that the last 6 bytes are XXXXXX. We're not using PathBuf here because + // the round-trip from Vec<u8> to PathBuf and back is really tedious due to Windows/Unix + // differences. + template.extend(b"/.XXXXXX"); + + // The only way this can cause an error is if the caller passed in a malformed CString. + let ptr = CString::new(template) + .map(CString::into_raw) + .map_err(|e| io::Error::new(io::ErrorKind::InvalidInput, e))?; + + // Safe because this will only modify `ptr`. + let ret = unsafe { libc::mkdtemp(ptr) }; + + // Safe because this pointer was originally created by a call to `CString::into_raw`. + let tmpdir = unsafe { CString::from_raw(ptr) }; + + if ret.is_null() { + return Err(io::Error::last_os_error()); + } + + // Safe because this doesn't modify any memory and we check the return value. + let fd = unsafe { + libc::openat( + libc::AT_FDCWD, + tmpdir.as_ptr(), + libc::O_DIRECTORY | libc::O_CLOEXEC, + ) + }; + if fd < 0 { + return Err(io::Error::last_os_error()); + } + + Ok(TempDir { + path: tmpdir, + // Safe because we just opened this fd. + file: unsafe { File::from_raw_fd(fd) }, + }) + } + + fn path(&self) -> &CStr { + &self.path + } + + // Consumes the `TempDir`, returning the inner `File` without deleting the temporary + // directory. + fn into_inner(self) -> (CString, File) { + // Safe because this is a valid pointer and we are going to call `mem::forget` on `self` so + // we will not be aliasing memory. + let path = unsafe { ptr::read(&self.path) }; + let file = unsafe { ptr::read(&self.file) }; + mem::forget(self); + + (path, file) + } +} + +impl AsRawFd for TempDir { + fn as_raw_fd(&self) -> RawFd { + self.file.as_raw_fd() + } +} + +impl Drop for TempDir { + fn drop(&mut self) { + // There is no `AT_EMPTY_PATH` equivalent for `unlinkat` and using "." as the path returns + // EINVAL. So we have no choice but to use the real path. Safe because this doesn't modify + // any memory and we check the return value. + let ret = + unsafe { libc::unlinkat(libc::AT_FDCWD, self.path().as_ptr(), libc::AT_REMOVEDIR) }; + if ret < 0 { + println!("Failed to remove tempdir: {}", io::Error::last_os_error()); + error!("Failed to remove tempdir: {}", io::Error::last_os_error()); + } + } +} + impl FileSystem for PassthroughFs { type Inode = Inode; type Handle = Handle; @@ -892,7 +986,14 @@ impl FileSystem for PassthroughFs { mode: u32, umask: u32, ) -> io::Result<Entry> { - let (_uid, _gid) = set_creds(ctx.uid, ctx.gid)?; + // This method has the same issues as `create()`: namely that the kernel may have allowed a + // process to make a directory due to one of its supplementary groups but that information + // is not forwarded to us. However, there is no `O_TMPDIR` equivalent for directories so + // instead we create a "hidden" directory with a randomly generated name in the parent + // directory, modify the uid/gid and mode to the proper values, and then rename it to the + // requested name. This ensures that even in the case of a power loss the directory is not + // visible in the filesystem with the requested name but incorrect metadata. The only thing + // left would be a empty hidden directory with a random name. let data = self .inodes .lock() @@ -900,13 +1001,43 @@ impl FileSystem for PassthroughFs { .map(Arc::clone) .ok_or_else(ebadf)?; - // Safe because this doesn't modify any memory and we check the return value. - let res = unsafe { libc::mkdirat(data.file.as_raw_fd(), name.as_ptr(), mode & !umask) }; - if res == 0 { - self.do_lookup(parent, name) - } else { - Err(io::Error::last_os_error()) + let tmpdir = self.get_path(parent).and_then(TempDir::new)?; + + // Set the uid and gid for the directory. Safe because this doesn't modify any memory and we + // check the return value. + let ret = unsafe { libc::fchown(tmpdir.as_raw_fd(), ctx.uid, ctx.gid) }; + if ret < 0 { + return Err(io::Error::last_os_error()); } + + // Set the mode. Safe because this doesn't modify any memory and we check the return value. + let ret = unsafe { libc::fchmod(tmpdir.as_raw_fd(), mode & !umask) }; + if ret < 0 { + return Err(io::Error::last_os_error()); + } + + // Now rename it into place. Safe because this doesn't modify any memory and we check the + // return value. TODO: Switch to libc::renameat2 once + // https://github.com/rust-lang/libc/pull/1508 lands and we have glibc 2.28. + let ret = unsafe { + libc::syscall( + libc::SYS_renameat2, + libc::AT_FDCWD, + tmpdir.path().as_ptr(), + data.file.as_raw_fd(), + name.as_ptr(), + libc::RENAME_NOREPLACE, + ) + }; + if ret < 0 { + return Err(io::Error::last_os_error()); + } + + // Now that we've moved the directory make sure we don't try to delete the now non-existent + // `tmpdir`. + tmpdir.into_inner(); + + self.do_lookup(parent, name) } fn rmdir(&self, _ctx: Context, parent: Inode, name: &CStr) -> io::Result<()> { @@ -1007,7 +1138,16 @@ impl FileSystem for PassthroughFs { flags: u32, umask: u32, ) -> io::Result<(Entry, Option<Handle>, OpenOptions)> { - let (_uid, _gid) = set_creds(ctx.uid, ctx.gid)?; + // The `Context` may not contain all the information we need to create the file here. For + // example, a process may be part of several groups, one of which gives it permission to + // create a file in `parent`, but is not the gid of the process. This information is not + // forwarded to the server so we don't know when this is happening. Instead, we just rely on + // the access checks in the kernel driver: if we received this request then the kernel has + // determined that the process is allowed to create the file and we shouldn't reject it now + // based on acls. + // + // To ensure that the file is created atomically with the proper uid/gid we use `O_TMPFILE` + // + `linkat` as described in the `open(2)` manpage. let data = self .inodes .lock() @@ -1015,15 +1155,26 @@ impl FileSystem for PassthroughFs { .map(Arc::clone) .ok_or_else(ebadf)?; - // Safe because this doesn't modify any memory and we check the return value. We don't - // really check `flags` because if the kernel can't handle poorly specified flags then we - // have much bigger problems. + // We don't want to use `O_EXCL` with `O_TMPFILE` as it has a different meaning when used in + // that combination. + let mut tmpflags = (flags as i32 | libc::O_TMPFILE | libc::O_CLOEXEC | libc::O_NOFOLLOW) + & !(libc::O_EXCL | libc::O_CREAT); + + // O_TMPFILE requires that we use O_RDWR or O_WRONLY. + if flags as i32 & libc::O_ACCMODE == libc::O_RDONLY { + tmpflags &= !libc::O_ACCMODE; + tmpflags |= libc::O_RDWR; + } + + // Safe because this is a valid c string. + let current_dir = unsafe { CStr::from_bytes_with_nul_unchecked(b".\0") }; + + // Safe because this doesn't modify any memory and we check the return value. let fd = unsafe { libc::openat( data.file.as_raw_fd(), - name.as_ptr(), - (flags as i32 | libc::O_CREAT | libc::O_CLOEXEC | libc::O_NOFOLLOW) - & !libc::O_DIRECT, + current_dir.as_ptr(), + tmpflags, mode & !(umask & 0o777), ) }; @@ -1032,26 +1183,49 @@ impl FileSystem for PassthroughFs { } // Safe because we just opened this fd. - let file = Mutex::new(unsafe { File::from_raw_fd(fd) }); + let tmpfile = unsafe { File::from_raw_fd(fd) }; - let entry = self.do_lookup(parent, name)?; + // Now set the uid and gid for the file. Safe because this doesn't modify any memory and we + // check the return value. + let ret = unsafe { libc::fchown(tmpfile.as_raw_fd(), ctx.uid, ctx.gid) }; + if ret < 0 { + return Err(io::Error::last_os_error()); + } - let handle = self.next_handle.fetch_add(1, Ordering::Relaxed); - let data = HandleData { - inode: entry.inode, - file, + let proc_path = CString::new(format!("self/fd/{}", tmpfile.as_raw_fd())) + .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?; + + // Finally link it into the file system tree so that it's visible to other processes. Safe + // because this doesn't modify any memory and we check the return value. + let ret = unsafe { + libc::linkat( + self.proc.as_raw_fd(), + proc_path.as_ptr(), + data.file.as_raw_fd(), + name.as_ptr(), + libc::AT_SYMLINK_FOLLOW, + ) }; + if ret < 0 { + return Err(io::Error::last_os_error()); + } - self.handles.lock().insert(handle, Arc::new(data)); + // We no longer need the tmpfile. + mem::drop(tmpfile); - let mut opts = OpenOptions::empty(); - match self.cfg.cache_policy { - CachePolicy::Never => opts |= OpenOptions::DIRECT_IO, - CachePolicy::Always => opts |= OpenOptions::KEEP_CACHE, - _ => {} - }; + let entry = self.do_lookup(parent, name)?; + let (handle, opts) = self + .do_open( + entry.inode, + flags & !((libc::O_CREAT | libc::O_EXCL | libc::O_NOCTTY) as u32), + ) + .map_err(|e| { + // Don't leak the entry. + self.forget(ctx, entry.inode, 1); + e + })?; - Ok((entry, Some(handle), opts)) + Ok((entry, handle, opts)) } fn unlink(&self, _ctx: Context, parent: Inode, name: &CStr) -> io::Result<()> { @@ -1694,34 +1868,38 @@ impl FileSystem for PassthroughFs { out_size: u32, r: R, ) -> io::Result<IoctlReply> { + const GET_ENCRYPTION_POLICY: u32 = FS_IOC_GET_ENCRYPTION_POLICY() as u32; + const SET_ENCRYPTION_POLICY: u32 = FS_IOC_SET_ENCRYPTION_POLICY() as u32; + // Normally, we wouldn't need to retry the FS_IOC_GET_ENCRYPTION_POLICY and // FS_IOC_SET_ENCRYPTION_POLICY ioctls. Unfortunately, the I/O directions for both of them // are encoded backwards so they can only be handled as unrestricted fuse ioctls. - if cmd == FS_IOC_GET_ENCRYPTION_POLICY() as u32 { - if out_size < size_of::<fscrypt_policy_v1>() as u32 { - let input = Vec::new(); - let output = vec![IoctlIovec { - base: arg, - len: size_of::<fscrypt_policy_v1>() as u64, - }]; - Ok(IoctlReply::Retry { input, output }) - } else { - self.get_encryption_policy(handle) + match cmd { + GET_ENCRYPTION_POLICY => { + if out_size < size_of::<fscrypt_policy_v1>() as u32 { + let input = Vec::new(); + let output = vec![IoctlIovec { + base: arg, + len: size_of::<fscrypt_policy_v1>() as u64, + }]; + Ok(IoctlReply::Retry { input, output }) + } else { + self.get_encryption_policy(handle) + } } - } else if cmd == FS_IOC_SET_ENCRYPTION_POLICY() as u32 { - if in_size < size_of::<fscrypt_policy_v1>() as u32 { - let input = vec![IoctlIovec { - base: arg, - len: size_of::<fscrypt_policy_v1>() as u64, - }]; - let output = Vec::new(); - Ok(IoctlReply::Retry { input, output }) - } else { - self.set_encryption_policy(handle, r) + SET_ENCRYPTION_POLICY => { + if in_size < size_of::<fscrypt_policy_v1>() as u32 { + let input = vec![IoctlIovec { + base: arg, + len: size_of::<fscrypt_policy_v1>() as u64, + }]; + let output = Vec::new(); + Ok(IoctlReply::Retry { input, output }) + } else { + self.set_encryption_policy(handle, r) + } } - } else { - // Did you know that a file/directory is not a TTY? - Err(io::Error::from_raw_os_error(libc::ENOTTY)) + _ => Err(io::Error::from_raw_os_error(libc::ENOTTY)), } } @@ -1782,6 +1960,10 @@ impl FileSystem for PassthroughFs { mod tests { use super::*; + use std::env; + use std::os::unix::ffi::OsStringExt; + use std::path::{Path, PathBuf}; + #[test] fn padded_cstrings() { assert_eq!(strip_padding(b".\0\0\0\0\0\0\0").to_bytes(), b"."); @@ -1802,4 +1984,44 @@ mod tests { fn no_nul_byte() { strip_padding(b"no nul bytes in string"); } + + #[test] + fn create_temp_dir() { + let t = TempDir::new( + CString::new(env::temp_dir().into_os_string().into_vec()) + .expect("env::temp_dir() is not a valid c-string"), + ) + .expect("Failed to create temporary directory"); + + let cstr = t.path().to_string_lossy(); + let path = Path::new(&*cstr); + assert!(path.exists()); + assert!(path.is_dir()); + } + + #[test] + fn remove_temp_dir() { + let t = TempDir::new( + CString::new(env::temp_dir().into_os_string().into_vec()) + .expect("env::temp_dir() is not a valid c-string"), + ) + .expect("Failed to create temporary directory"); + let cstr = t.path().to_string_lossy(); + let path = PathBuf::from(&*cstr); + mem::drop(t); + assert!(!path.exists()); + } + + #[test] + fn temp_dir_into_inner() { + let t = TempDir::new( + CString::new(env::temp_dir().into_os_string().into_vec()) + .expect("env::temp_dir() is not a valid c-string"), + ) + .expect("Failed to create temporary directory"); + let (cstr, _) = t.into_inner(); + let cow_str = cstr.to_string_lossy(); + let path = Path::new(&*cow_str); + assert!(path.exists()); + } } diff --git a/devices/src/virtio/video/decoder/mod.rs b/devices/src/virtio/video/decoder/mod.rs index 3516cad..1dfd35c 100644 --- a/devices/src/virtio/video/decoder/mod.rs +++ b/devices/src/virtio/video/decoder/mod.rs @@ -5,7 +5,7 @@ //! Implementation of a virtio video decoder device backed by LibVDA. use std::collections::btree_map::Entry; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use std::convert::TryInto; use std::fs::File; use std::os::unix::io::{AsRawFd, IntoRawFd}; @@ -42,6 +42,8 @@ type FrameBufferId = i32; type ResourceHandle = u32; type Timestamp = u64; +const OUTPUT_BUFFER_COUNT: usize = 32; + // Represents queue types of pending Clear commands if exist. #[derive(Default)] struct PendingClearCmds { @@ -64,9 +66,14 @@ struct Context { res_id_to_res_handle: BTreeMap<u32, ResourceHandle>, // OutputResourceId <-> FrameBufferId + // TODO: Encapsulate the following two maps as a type of a bijective map. res_id_to_frame_buf_id: BTreeMap<OutputResourceId, FrameBufferId>, frame_buf_id_to_res_id: BTreeMap<FrameBufferId, OutputResourceId>, + // Stores free FrameBufferIds which `use_output_buffer()` or `reuse_output_buffer()` can be + // called with. + available_frame_buf_ids: BTreeSet<FrameBufferId>, + keep_resources: Vec<File>, // Stores queue types of pending Clear commands if exist. @@ -127,12 +134,31 @@ impl Context { self.res_id_to_res_handle.insert(resource_id, handle); } - fn register_queued_frame_buffer(&mut self, resource_id: OutputResourceId) -> FrameBufferId { - // Generate a new FrameBufferId - let id = self.res_id_to_frame_buf_id.len() as FrameBufferId; + fn register_queued_frame_buffer( + &mut self, + resource_id: OutputResourceId, + ) -> VideoResult<FrameBufferId> { + // Use the first free frame buffer id. + let id = *self.available_frame_buf_ids.iter().next().ok_or_else(|| { + error!( + "no frame buffer ID is available for resource_id {}", + resource_id + ); + VideoError::InvalidOperation + })?; + self.available_frame_buf_ids.remove(&id); + + // Invalidate old entries for `resource_id` and `id` from two maps to keep them bijective. + if let Some(old_frame_buf_id) = self.res_id_to_frame_buf_id.remove(&resource_id) { + self.frame_buf_id_to_res_id.remove(&old_frame_buf_id); + } + if let Some(old_res_id) = self.frame_buf_id_to_res_id.remove(&id) { + self.res_id_to_frame_buf_id.remove(&old_res_id); + } + self.res_id_to_frame_buf_id.insert(resource_id, id); self.frame_buf_id_to_res_id.insert(id, resource_id); - id + Ok(id) } fn reset(&mut self) { @@ -200,6 +226,9 @@ impl Context { right: i32, bottom: i32, ) -> Option<ResourceId> { + // `buffer_id` becomes available for another frame. + self.available_frame_buf_ids.insert(buffer_id); + let plane_size = ((right - left) * (bottom - top)) as u32; for fmt in self.out_params.plane_formats.iter_mut() { fmt.plane_size = plane_size; @@ -239,6 +268,10 @@ impl Context { } else if self.pending_clear_cmds.output { self.pending_clear_cmds.output = false; + // Reinitialize `available_frame_buf_ids`. + self.available_frame_buf_ids.clear(); + self.available_frame_buf_ids = (0..OUTPUT_BUFFER_COUNT as i32).collect(); + Some(QueueType::Output) } else { error!("unexpected ResetResponse"); @@ -426,8 +459,6 @@ impl<'a> Decoder<'a> { // first time. let mut ctx = self.contexts.get_mut(&stream_id)?; if !ctx.set_output_buffer_count { - const OUTPUT_BUFFER_COUNT: usize = 32; - // Set the buffer count to the maximum value. // TODO(b/1518105): This is a hack due to the lack of way of telling a number of // frame buffers explictly in virtio-video v3 RFC. Once we have the way, @@ -436,20 +467,18 @@ impl<'a> Decoder<'a> { .get(&stream_id)? .set_output_buffer_count(OUTPUT_BUFFER_COUNT) .map_err(VideoError::VdaError)?; + + // FrameBufferId must be in the range of 0.. ((# of buffers) - 1). + ctx.available_frame_buf_ids = (0..OUTPUT_BUFFER_COUNT as i32).collect(); + ctx.set_output_buffer_count = true; } - // We assume ResourceCreate is not called to an output resource that is already - // imported to Chrome for now. - // TODO(keiichiw): We need to support this case for a guest client who may use - // arbitrary numbers of buffers. (e.g. C2V4L2Component in ARCVM) - // Such a client is valid as long as it uses at most 32 buffers at the same time. + // If the given resource_id was associated with an old frame_buf_id, + // dissociate them. if let Some(frame_buf_id) = ctx.res_id_to_frame_buf_id.get(&resource_id) { - error!( - "resource {} has already been imported to Chrome as a frame buffer {}", - resource_id, frame_buf_id - ); - return Err(VideoError::InvalidOperation); + ctx.frame_buf_id_to_res_id.remove(&frame_buf_id); + ctx.res_id_to_frame_buf_id.remove(&resource_id); } Ok(()) @@ -545,14 +574,18 @@ impl<'a> Decoder<'a> { // In case a given resource has been imported to VDA, call // `session.reuse_output_buffer()` and return a response. // Otherwise, `session.use_output_buffer()` will be called below. - match ctx.res_id_to_frame_buf_id.get(&resource_id) { - Some(buffer_id) => { - session - .reuse_output_buffer(*buffer_id) - .map_err(VideoError::VdaError)?; - return Ok(()); - } - None => (), + if let Some(buffer_id) = ctx.res_id_to_frame_buf_id.get(&resource_id) { + if !ctx.available_frame_buf_ids.remove(&buffer_id) { + error!( + "resource_id {} is associated with VDA's frame_buffer id {}, which is in use or out of range.", + resource_id, buffer_id + ); + return Err(VideoError::InvalidOperation); + } + session + .reuse_output_buffer(*buffer_id) + .map_err(VideoError::VdaError)?; + return Ok(()); }; let resource_info = ctx.get_resource_info(resource_bridge, resource_id)?; @@ -579,7 +612,7 @@ impl<'a> Decoder<'a> { let buffer_id = self .contexts .get_mut(&stream_id)? - .register_queued_frame_buffer(resource_id); + .register_queued_frame_buffer(resource_id)?; session .use_output_buffer(buffer_id as i32, libvda::PixelFormat::NV12, fd, &planes) |