crosvm: add debug labels to devices for improved SIGCHLD logs

Each device (Bus, Pci, Proxy, etc), gets a debug label associated with
it. When a child is spawned, the debug label for it is stored in
a map with the child's pid as the key. If a SIGCHLD is handled, this map
is used to print a more helpful message about exactly which child died.

BUG=None
TEST=run with sandboxing and a faulty child device
     check logs for message about child died
     the child should have a debug label

Change-Id: I61fbbee0a8e701249533a7a3a6a1ad48840f12e5
Reviewed-on: https://chromium-review.googlesource.com/1432835
Commit-Ready: Chih-Yang Hsia <paulhsia@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Tested-by: Zach Reizner <zachr@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>

15 files changed, 114 insertions, 12 deletions

diff --git a/aarch64/src/lib.rs b/aarch64/src/lib.rs
index dda7bfc..d579a17 100644
--- a/aarch64/src/lib.rs
+++ b/aarch64/src/lib.rs
@@ -227,7 +227,7 @@ impl arch::LinuxArch for AArch64 {
         let exit_evt = EventFd::new().map_err(Error::CreateEventFd)?;
 
         let pci_devices = virtio_devs(&mem, &exit_evt)?;
-        let (pci, pci_irqs) =
+        let (pci, pci_irqs, pid_debug_label_map) =
             arch::generate_pci_root(pci_devices, &mut mmio_bus, &mut resources, &mut vm)
                 .map_err(Error::CreatePciRoot)?;
         let pci_bus = Arc::new(Mutex::new(PciConfigMmio::new(pci)));
@@ -277,6 +277,7 @@ impl arch::LinuxArch for AArch64 {
             irq_chip,
             io_bus,
             mmio_bus,
+            pid_debug_label_map,
         })
     }
 }
diff --git a/arch/src/lib.rs b/arch/src/lib.rs
index 057c005..bc326eb 100644
--- a/arch/src/lib.rs
+++ b/arch/src/lib.rs
@@ -11,6 +11,7 @@ extern crate resources;
 extern crate sync;
 extern crate sys_util;
 
+use std::collections::BTreeMap;
 use std::fmt;
 use std::fs::File;
 use std::io::{Read, Seek, SeekFrom};
@@ -52,6 +53,7 @@ pub struct RunnableLinuxVm {
     pub irq_chip: Option<File>,
     pub io_bus: Bus,
     pub mmio_bus: Bus,
+    pub pid_debug_label_map: BTreeMap<u32, String>,
 }
 
 /// The device and optional jail.
@@ -144,9 +146,13 @@ pub fn generate_pci_root(
     mmio_bus: &mut Bus,
     resources: &mut SystemAllocator,
     vm: &mut Vm,
-) -> std::result::Result<(PciRoot, Vec<(u32, PciInterruptPin)>), DeviceRegistrationError> {
+) -> std::result::Result<
+    (PciRoot, Vec<(u32, PciInterruptPin)>, BTreeMap<u32, String>),
+    DeviceRegistrationError,
+> {
     let mut root = PciRoot::new();
     let mut pci_irqs = Vec::new();
+    let mut pid_labels = BTreeMap::new();
     for (dev_idx, (mut device, jail)) in devices.into_iter().enumerate() {
         let mut keep_fds = device.keep_fds();
         syslog::push_fds(&mut keep_fds);
@@ -182,6 +188,7 @@ pub fn generate_pci_root(
         let arced_dev: Arc<Mutex<BusDevice>> = if let Some(jail) = jail {
             let proxy = ProxyDevice::new(device, &jail, keep_fds)
                 .map_err(DeviceRegistrationError::ProxyDeviceCreation)?;
+            pid_labels.insert(proxy.pid() as u32, proxy.debug_label());
             Arc::new(Mutex::new(proxy))
         } else {
             Arc::new(Mutex::new(device))
@@ -193,7 +200,7 @@ pub fn generate_pci_root(
                 .map_err(DeviceRegistrationError::MmioInsert)?;
         }
     }
-    Ok((root, pci_irqs))
+    Ok((root, pci_irqs, pid_labels))
 }
 
 /// Errors for image loading.
diff --git a/devices/src/bus.rs b/devices/src/bus.rs
index 5eb09a2..1c21661 100644
--- a/devices/src/bus.rs
+++ b/devices/src/bus.rs
@@ -17,6 +17,8 @@ use sync::Mutex;
 /// into its allocated portion of address space.
 #[allow(unused_variables)]
 pub trait BusDevice: Send {
+    /// Returns a label suitable for debug output.
+    fn debug_label(&self) -> String;
     /// Reads at `offset` from this device
     fn read(&mut self, offset: u64, data: &mut [u8]) {}
     /// Writes at `offset` into this device
@@ -202,10 +204,18 @@ mod tests {
     use super::*;
 
     struct DummyDevice;
-    impl BusDevice for DummyDevice {}
+    impl BusDevice for DummyDevice {
+        fn debug_label(&self) -> String {
+            "dummy device".to_owned()
+        }
+    }
 
     struct ConstantDevice;
     impl BusDevice for ConstantDevice {
+        fn debug_label(&self) -> String {
+            "constant device".to_owned()
+        }
+
         fn read(&mut self, offset: u64, data: &mut [u8]) {
             for (i, v) in data.iter_mut().enumerate() {
                 *v = (offset as u8) + (i as u8);
diff --git a/devices/src/cmos.rs b/devices/src/cmos.rs
index cf5dd7e..5c04678 100644
--- a/devices/src/cmos.rs
+++ b/devices/src/cmos.rs
@@ -29,6 +29,10 @@ impl Cmos {
 }
 
 impl BusDevice for Cmos {
+    fn debug_label(&self) -> String {
+        "cmos".to_owned()
+    }
+
     fn write(&mut self, offset: u64, data: &[u8]) {
         if data.len() != 1 {
             return;
diff --git a/devices/src/i8042.rs b/devices/src/i8042.rs
index 8981e02..0d0c31d 100644
--- a/devices/src/i8042.rs
+++ b/devices/src/i8042.rs
@@ -22,6 +22,10 @@ impl I8042Device {
 // registers: port 0x61 (I8042_PORT_B_REG, offset 0 from base of 0x61), and
 // port 0x64 (I8042_COMMAND_REG, offset 3 from base of 0x61).
 impl BusDevice for I8042Device {
+    fn debug_label(&self) -> String {
+        "i8042".to_owned()
+    }
+
     fn read(&mut self, offset: u64, data: &mut [u8]) {
         if data.len() == 1 && offset == 3 {
             data[0] = 0x0;
diff --git a/devices/src/pci/pci_device.rs b/devices/src/pci/pci_device.rs
index 4d3a46e..5ccdfef 100644
--- a/devices/src/pci/pci_device.rs
+++ b/devices/src/pci/pci_device.rs
@@ -25,6 +25,8 @@ pub enum Error {
 pub type Result<T> = std::result::Result<T, Error>;
 
 pub trait PciDevice: Send {
+    /// Returns a label suitable for debug output.
+    fn debug_label(&self) -> String;
     /// A vector of device-specific file descriptors that must be kept open
     /// after jailing. Must be called before the process is jailed.
     fn keep_fds(&self) -> Vec<RawFd>;
@@ -66,6 +68,10 @@ pub trait PciDevice: Send {
 }
 
 impl<T: PciDevice> BusDevice for T {
+    fn debug_label(&self) -> String {
+        PciDevice::debug_label(self)
+    }
+
     fn read(&mut self, offset: u64, data: &mut [u8]) {
         self.read_bar(offset, data)
     }
@@ -102,6 +108,10 @@ impl<T: PciDevice> BusDevice for T {
 }
 
 impl<T: PciDevice + ?Sized> PciDevice for Box<T> {
+    /// Returns a label suitable for debug output.
+    fn debug_label(&self) -> String {
+        (**self).debug_label()
+    }
     fn keep_fds(&self) -> Vec<RawFd> {
         (**self).keep_fds()
     }
diff --git a/devices/src/pci/pci_root.rs b/devices/src/pci/pci_root.rs
index 4864ff7..b432be2 100644
--- a/devices/src/pci/pci_root.rs
+++ b/devices/src/pci/pci_root.rs
@@ -20,6 +20,9 @@ struct PciRootConfiguration {
 }
 
 impl PciDevice for PciRootConfiguration {
+    fn debug_label(&self) -> String {
+        "pci root device".to_owned()
+    }
     fn keep_fds(&self) -> Vec<RawFd> {
         Vec::new()
     }
@@ -188,6 +191,10 @@ impl PciConfigIo {
 }
 
 impl BusDevice for PciConfigIo {
+    fn debug_label(&self) -> String {
+        format!("pci config io-port 0x{:03x}", self.config_address)
+    }
+
     fn read(&mut self, offset: u64, data: &mut [u8]) {
         // `offset` is relative to 0xcf8
         let value = match offset {
@@ -245,6 +252,10 @@ impl PciConfigMmio {
 }
 
 impl BusDevice for PciConfigMmio {
+    fn debug_label(&self) -> String {
+        format!("pci config mmio")
+    }
+
     fn read(&mut self, offset: u64, data: &mut [u8]) {
         // Only allow reads to the register boundary.
         let start = offset as usize % 4;
diff --git a/devices/src/pl030.rs b/devices/src/pl030.rs
index 6710b5e..63fedc8 100644
--- a/devices/src/pl030.rs
+++ b/devices/src/pl030.rs
@@ -70,6 +70,10 @@ impl Pl030 {
 }
 
 impl BusDevice for Pl030 {
+    fn debug_label(&self) -> String {
+        "Pl030".to_owned()
+    }
+
     fn write(&mut self, offset: u64, data: &[u8]) {
         if data.len() != 4 {
             warn!("bad write size: {} for pl030", data.len());
diff --git a/devices/src/proxy.rs b/devices/src/proxy.rs
index 0b60736..907f33c 100644
--- a/devices/src/proxy.rs
+++ b/devices/src/proxy.rs
@@ -120,6 +120,7 @@ fn child_proc(sock: UnixDatagram, device: &mut BusDevice) {
 pub struct ProxyDevice {
     sock: MsgSocket<Command, CommandResult>,
     pid: pid_t,
+    debug_label: String,
 }
 
 impl ProxyDevice {
@@ -136,6 +137,7 @@ impl ProxyDevice {
         jail: &Minijail,
         mut keep_fds: Vec<RawFd>,
     ) -> Result<ProxyDevice> {
+        let debug_label = device.debug_label();
         let (child_sock, parent_sock) = UnixDatagram::pair().map_err(Error::Io)?;
 
         keep_fds.push(child_sock.as_raw_fd());
@@ -161,6 +163,7 @@ impl ProxyDevice {
         Ok(ProxyDevice {
             sock: MsgSocket::<Command, CommandResult>::new(parent_sock),
             pid,
+            debug_label,
         })
     }
 
@@ -171,11 +174,17 @@ impl ProxyDevice {
     fn sync_send(&self, cmd: Command) -> Option<CommandResult> {
         let res = self.sock.send(&cmd);
         if let Err(e) = res {
-            error!("failed write to child device process: {:?}", e);
+            error!(
+                "failed write to child device process {}: {:?}",
+                self.debug_label, e
+            );
         };
         match self.sock.recv() {
             Err(e) => {
-                error!("failed read from child device process: {:?}", e);
+                error!(
+                    "failed read from child device process {}: {:?}",
+                    self.debug_label, e
+                );
                 None
             }
             Ok(r) => Some(r),
@@ -184,6 +193,10 @@ impl ProxyDevice {
 }
 
 impl BusDevice for ProxyDevice {
+    fn debug_label(&self) -> String {
+        self.debug_label.clone()
+    }
+
     fn config_register_write(&mut self, reg_idx: usize, offset: u64, data: &[u8]) {
         let len = data.len() as u32;
         let mut buffer = [0u8; 4];
diff --git a/devices/src/serial.rs b/devices/src/serial.rs
index 25911f9..b2a5651 100644
--- a/devices/src/serial.rs
+++ b/devices/src/serial.rs
@@ -185,6 +185,10 @@ impl Serial {
 }
 
 impl BusDevice for Serial {
+    fn debug_label(&self) -> String {
+        "serial".to_owned()
+    }
+
     fn write(&mut self, offset: u64, data: &[u8]) {
         if data.len() != 1 {
             return;
diff --git a/devices/src/virtio/mod.rs b/devices/src/virtio/mod.rs
index 5a46bea..78cfeb2 100644
--- a/devices/src/virtio/mod.rs
+++ b/devices/src/virtio/mod.rs
@@ -64,3 +64,18 @@ const INTERRUPT_STATUS_CONFIG_CHANGED: u32 = 0x2;
 /// Offset from the base MMIO address of a virtio device used by the guest to notify the device of
 /// queue events.
 pub const NOTIFY_REG_OFFSET: u32 = 0x50;
+
+/// Returns a string representation of the given virtio device type number.
+pub fn type_to_str(type_: u32) -> Option<&'static str> {
+    Some(match type_ {
+        TYPE_NET => "net",
+        TYPE_BLOCK => "block",
+        TYPE_RNG => "rng",
+        TYPE_BALLOON => "balloon",
+        TYPE_GPU => "gpu",
+        TYPE_9P => "9p",
+        TYPE_VSOCK => "vsock",
+        TYPE_WL => "wl",
+        _ => return None,
+    })
+}
diff --git a/devices/src/virtio/virtio_device.rs b/devices/src/virtio/virtio_device.rs
index 1bf0281..82ca012 100644
--- a/devices/src/virtio/virtio_device.rs
+++ b/devices/src/virtio/virtio_device.rs
@@ -17,6 +17,14 @@ use sys_util::{EventFd, GuestMemory};
 /// Optionally, a virtio device can implement device reset in which it returns said resources and
 /// resets its internal.
 pub trait VirtioDevice: Send {
+    /// Returns a label suitable for debug output.
+    fn debug_label(&self) -> String {
+        match type_to_str(self.device_type()) {
+            Some(s) => format!("virtio-{}", s),
+            None => format!("virtio (type {})", self.device_type()),
+        }
+    }
+
     /// A vector of device-specific file descriptors that must be kept open
     /// after jailing. Must be called before the process is jailed.
     fn keep_fds(&self) -> Vec<RawFd>;
diff --git a/devices/src/virtio/virtio_pci_device.rs b/devices/src/virtio/virtio_pci_device.rs
index 6a9ca5e..f9cb948 100644
--- a/devices/src/virtio/virtio_pci_device.rs
+++ b/devices/src/virtio/virtio_pci_device.rs
@@ -276,6 +276,10 @@ impl VirtioPciDevice {
 }
 
 impl PciDevice for VirtioPciDevice {
+    fn debug_label(&self) -> String {
+        format!("virtio-pci ({})", self.device.debug_label())
+    }
+
     fn keep_fds(&self) -> Vec<RawFd> {
         let mut fds = self.device.keep_fds();
         if let Some(ref interrupt_evt) = self.interrupt_evt {
diff --git a/src/linux.rs b/src/linux.rs
index 1dcedbe..4b684ca 100644
--- a/src/linux.rs
+++ b/src/linux.rs
@@ -1023,12 +1023,14 @@ fn run_control(
                 Token::ChildSignal => {
                     // Print all available siginfo structs, then exit the loop.
                     while let Some(siginfo) = sigchld_fd.read().map_err(Error::SignalFd)? {
+                        let pid = siginfo.ssi_pid;
+                        let pid_label = match linux.pid_debug_label_map.get(&pid) {
+                            Some(label) => format!("{} (pid {})", label, pid),
+                            None => format!("pid {}", pid),
+                        };
                         error!(
                             "child {} died: signo {}, status {}, code {}",
-                            siginfo.ssi_pid,
-                            siginfo.ssi_signo,
-                            siginfo.ssi_status,
-                            siginfo.ssi_code
+                            pid_label, siginfo.ssi_signo, siginfo.ssi_status, siginfo.ssi_code
                         );
                     }
                     break 'poll;
diff --git a/x86_64/src/lib.rs b/x86_64/src/lib.rs
index 524013c..2857bb7 100644
--- a/x86_64/src/lib.rs
+++ b/x86_64/src/lib.rs
@@ -300,7 +300,7 @@ impl arch::LinuxArch for X8664arch {
         let exit_evt = EventFd::new().map_err(Error::CreateEventFd)?;
 
         let pci_devices = virtio_devs(&mem, &exit_evt)?;
-        let (pci, pci_irqs) =
+        let (pci, pci_irqs, pid_debug_label_map) =
             arch::generate_pci_root(pci_devices, &mut mmio_bus, &mut resources, &mut vm)
                 .map_err(Error::CreatePciRoot)?;
         let pci_bus = Arc::new(Mutex::new(PciConfigIo::new(pci)));
@@ -336,6 +336,7 @@ impl arch::LinuxArch for X8664arch {
             irq_chip,
             io_bus,
             mmio_bus,
+            pid_debug_label_map,
         })
     }
 }
@@ -463,7 +464,11 @@ impl X8664arch {
         pci: Option<Arc<Mutex<devices::PciConfigIo>>>,
     ) -> Result<(devices::Bus, Arc<Mutex<devices::Serial>>)> {
         struct NoDevice;
-        impl devices::BusDevice for NoDevice {}
+        impl devices::BusDevice for NoDevice {
+            fn debug_label(&self) -> String {
+                "no device".to_owned()
+            }
+        }
 
         let mut io_bus = devices::Bus::new();