From 647782519538b2d73f7571ed63081d34ef1f47dc Mon Sep 17 00:00:00 2001 From: Xuewei Niu Date: Thu, 17 Aug 2023 11:58:43 +0800 Subject: [PATCH] agent: Minor changes according to Zhou's comments The changes include: - Change to debug logging level for resources after processed. - Remove a todo for pod cgroup cleanup. - Add an anyhow context to `get_paths_and_mounts()`. - Remove code which denys access to VMROOTFS since it won't take effect. If blackmode is in use, the VMROOTFS will be denyed as default. Otherwise, device cgroups won't be updated in whitelist mode. - Add a unit test for `default_allowed_devices()`. Signed-off-by: Xuewei Niu --- src/agent/rustjail/src/cgroups/fs/mod.rs | 146 +++++++++++++++-------- src/agent/src/device.rs | 3 +- src/agent/src/rpc.rs | 9 +- 3 files changed, 96 insertions(+), 62 deletions(-) diff --git a/src/agent/rustjail/src/cgroups/fs/mod.rs b/src/agent/rustjail/src/cgroups/fs/mod.rs index bb21d04623..9913e945ee 100644 --- a/src/agent/rustjail/src/cgroups/fs/mod.rs +++ b/src/agent/rustjail/src/cgroups/fs/mod.rs @@ -19,7 +19,6 @@ use cgroups::{ }; use crate::cgroups::{is_all_devices_rule, Manager as CgroupManager}; -#[cfg(not(test))] use crate::container::DEFAULT_DEVICES; use anyhow::{anyhow, Context, Result}; use libc::{self, pid_t}; @@ -129,7 +128,7 @@ impl CgroupManager for Manager { if !self.devcg_whitelist { set_devices_resources(&self.cgroup, &r.devices, res, pod_res); } - info!( + debug!( sl(), "Resources after processed, pod_res = {:?}, res = {:?}", pod_res, res ); @@ -204,7 +203,6 @@ impl CgroupManager for Manager { err ); } - // TODO: Clean up pod cgroup if pod cgroup has no children. Ok(()) } @@ -537,28 +535,6 @@ fn build_blk_io_device_throttle_resource( blk_io_device_throttle_resources } -#[allow(dead_code)] -fn linux_device_to_device_resource(d: &LinuxDevice) -> Option { - let dev_type = match DeviceType::from_char(d.r#type.chars().next()) { - Some(t) => t, - None => return None, - }; - - let permissions = vec![ - DevicePermissions::Read, - DevicePermissions::Write, - DevicePermissions::MkNod, - ]; - - Some(DeviceResource { - allow: true, - devtype: dev_type, - major: d.major, - minor: d.minor, - access: permissions, - }) -} - fn linux_device_cgroup_to_device_resource(d: &LinuxDeviceCgroup) -> Option { let dev_type = match DeviceType::from_char(d.r#type.chars().next()) { Some(t) => t, @@ -1038,7 +1014,7 @@ impl Manager { spec: &Spec, devcg_info: Option>>, ) -> Result { - let (paths, mounts) = Self::get_paths_and_mounts(cpath)?; + let (paths, mounts) = Self::get_paths_and_mounts(cpath).context("Get paths and mounts")?; // Do not expect poisoning lock let mut devices_group_info = devcg_info.as_ref().map(|i| i.write().unwrap()); @@ -1053,10 +1029,11 @@ impl Manager { .to_string(); // Create a cgroup for the pod if not exists. - // Note that this step MUST be done before the pause container's - // cgroup created, since children inherit upper nodes' rules, which - // have excessive permission. You'll feel painful to shrink upper - // nodes' permission. + // Note that creating pod cgroup MUST be done before the pause + // container's cgroup created, since the upper node might have + // some excessive permissions, and children inherit upper + // node's rules. You'll feel painful to shrink upper nodes' + // permissions if the new permissions are subset of old. pod_cgroup = Some(load_cgroup(cgroups::hierarchies::auto(), &pod_cpath)); let pod_cg = pod_cgroup.as_ref().unwrap(); @@ -1133,7 +1110,7 @@ impl Manager { /// rules is done by the systemd. That is, the cgroupfs manager runs in /// read-only mode. pub fn new_read_only(cpath: &str) -> Result { - let (paths, mounts) = Self::get_paths_and_mounts(cpath)?; + let (paths, mounts) = Self::get_paths_and_mounts(cpath).context("Get paths and mounts")?; let cg = load_cgroup(cgroups::hierarchies::auto(), cpath); @@ -1224,7 +1201,7 @@ impl Manager { // Do not append default allowed devices for simplicity while // testing. #[cfg(not(test))] - dev_res_list.append(&mut Self::default_allowed_devices()); + dev_res_list.append(&mut default_allowed_devices()); let res = cgroups::Resources { devices: cgroups::DeviceResources { @@ -1237,24 +1214,6 @@ impl Manager { Ok(()) } - /// Generate a list for allowed devices including `DEFAULT_DEVICES` and - /// `DEFAULT_ALLOWED_DEVICES`. - #[cfg(not(test))] - fn default_allowed_devices() -> Vec { - let mut dev_res_list = Vec::new(); - DEFAULT_DEVICES.iter().for_each(|dev| { - if let Some(dev_res) = linux_device_to_device_resource(dev) { - dev_res_list.push(dev_res) - } - }); - DEFAULT_ALLOWED_DEVICES.iter().for_each(|dev| { - if let Some(dev_res) = linux_device_cgroup_to_device_resource(dev) { - dev_res_list.push(dev_res) - } - }); - dev_res_list - } - /// Check if OCI spec contains a whiltlist rule. /// /// # Returns @@ -1279,6 +1238,45 @@ impl Manager { } } +/// Generate a list for allowed devices including `DEFAULT_DEVICES` and +/// `DEFAULT_ALLOWED_DEVICES`. +fn default_allowed_devices() -> Vec { + let mut dev_res_list = Vec::new(); + DEFAULT_DEVICES.iter().for_each(|dev| { + if let Some(dev_res) = linux_device_to_device_resource(dev) { + dev_res_list.push(dev_res) + } + }); + DEFAULT_ALLOWED_DEVICES.iter().for_each(|dev| { + if let Some(dev_res) = linux_device_cgroup_to_device_resource(dev) { + dev_res_list.push(dev_res) + } + }); + dev_res_list +} + +/// Convert LinuxDevice to DeviceResource. +fn linux_device_to_device_resource(d: &LinuxDevice) -> Option { + let dev_type = match DeviceType::from_char(d.r#type.chars().next()) { + Some(t) => t, + None => return None, + }; + + let permissions = vec![ + DevicePermissions::Read, + DevicePermissions::Write, + DevicePermissions::MkNod, + ]; + + Some(DeviceResource { + allow: true, + devtype: dev_type, + major: d.major, + minor: d.minor, + access: permissions, + }) +} + // get the guest's online cpus. pub fn get_guest_cpuset() -> Result { let c = fs::read_to_string(GUEST_CPUS_PATH)?; @@ -1327,13 +1325,20 @@ fn convert_memory_swap_to_v2_value(memory_swap: i64, memory: i64) -> Result #[cfg(test)] mod tests { + use std::collections::HashMap; use std::process::Command; + use std::sync::{Arc, RwLock}; - use oci::Linux; - use oci::LinuxResources; + use cgroups::devices::{DevicePermissions, DeviceType}; + use oci::{Linux, LinuxDeviceCgroup, LinuxResources, Spec}; use test_utils::skip_if_not_root; - use super::*; + use super::default_allowed_devices; + use crate::cgroups::fs::{ + line_to_vec, lines_to_map, Manager, DEFAULT_ALLOWED_DEVICES, WILDCARD, + }; + use crate::cgroups::DevicesCgroupInfo; + use crate::container::DEFAULT_DEVICES; #[test] fn test_line_to_vec() { @@ -1534,4 +1539,39 @@ mod tests { managers[0].pod_cgroup.as_ref().unwrap().delete().unwrap(); } } + + #[test] + fn test_default_allowed_devices() { + let allowed_devices = default_allowed_devices(); + assert_eq!( + allowed_devices.len(), + DEFAULT_DEVICES.len() + DEFAULT_ALLOWED_DEVICES.len() + ); + + let allowed_permissions = vec![ + DevicePermissions::Read, + DevicePermissions::Write, + DevicePermissions::MkNod, + ]; + + let default_devices_0 = &allowed_devices[0]; + assert!(default_devices_0.allow); + assert_eq!(default_devices_0.devtype, DeviceType::Char); + assert_eq!(default_devices_0.major, 1); + assert_eq!(default_devices_0.minor, 3); + assert!(default_devices_0 + .access + .iter() + .all(|&p| allowed_permissions.iter().any(|&ap| ap == p))); + + let default_allowed_devices_0 = &allowed_devices[DEFAULT_DEVICES.len()]; + assert!(default_allowed_devices_0.allow); + assert_eq!(default_allowed_devices_0.devtype, DeviceType::Char); + assert_eq!(default_allowed_devices_0.major, WILDCARD); + assert_eq!(default_allowed_devices_0.minor, WILDCARD); + assert_eq!( + default_allowed_devices_0.access, + vec![DevicePermissions::MkNod] + ); + } } diff --git a/src/agent/src/device.rs b/src/agent/src/device.rs index da11dad339..5587e77991 100644 --- a/src/agent/src/device.rs +++ b/src/agent/src/device.rs @@ -32,7 +32,6 @@ fn sl() -> slog::Logger { slog_scope::logger().new(o!("subsystem" => "device")) } -pub const VM_ROOTFS: &str = "/"; const BLOCK: &str = "block"; pub const DRIVER_9P_TYPE: &str = "9p"; pub const DRIVER_VIRTIOFS_TYPE: &str = "virtio-fs"; @@ -1035,6 +1034,8 @@ mod tests { use std::iter::FromIterator; use tempfile::tempdir; + const VM_ROOTFS: &str = "/"; + #[test] fn test_update_device_cgroup() { let mut spec = Spec { diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index bb7e2a943a..276208bf3f 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -52,10 +52,7 @@ use nix::sys::{stat, statfs}; use nix::unistd::{self, Pid}; use rustjail::process::ProcessOperations; -use crate::device::{ - add_devices, get_virtio_blk_pci_device_name, insert_devices_cgroup_rule, update_env_pci, - DeviceInfo, VM_ROOTFS, -}; +use crate::device::{add_devices, get_virtio_blk_pci_device_name, update_env_pci}; use crate::linux_abi::*; use crate::metrics::get_metrics; use crate::mount::baremount; @@ -239,10 +236,6 @@ impl AgentService { update_container_namespaces(&s, &mut oci, use_sandbox_pidns)?; - // Add the root partition to the device cgroup to prevent access - let dev_info = DeviceInfo::new(VM_ROOTFS, false)?; - insert_devices_cgroup_rule(&mut oci, &dev_info, false, "rw")?; - // Append guest hooks append_guest_hooks(&s, &mut oci)?;