From 03bc89ab0b46328e230092d4b873d0d93f2272ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= Date: Tue, 3 May 2022 16:01:28 +0200 Subject: [PATCH 1/2] clh: Rely on Cloud Hypervisor for generating the device ID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We're currently hitting a race condition on the Cloud Hypervisor's driver code when quickly removing and adding a block device. This happens because the device removal is an asynchronous operation, and we currently do *not* monitor events coming from Cloud Hypervisor to know when the device was actually removed. Together with this, the sandbox code doesn't know about that and when a new device is attached it'll quickly assign what may be the very same ID to the new device, leading to the Cloud Hypervisor's driver trying to hotplug a device with the very same ID of the device that was not yet removed. This is, in a nutshell, why the tests with Cloud Hypervisor and devmapper have been failing every now and then. The workaround taken to solve the issue is basically *not* passing down the device ID to Cloud Hypervisor and simply letting Cloud Hypervisor itself generate those, as Cloud Hypervisor does it in a manner that avoids such conflicts. With this addition we have then to keep a map of the device ID and the Cloud Hypervisor's generated ID, so we can properly remove the device. This workaround will probably stay for a while, at least till someone has enough cycles to implement a way to watch the device removal event and then properly act on that. Spoiler alert, this will be a complex change that may not even be worth it considering the race can be avoided with this commit. Fixes: #4196 Signed-off-by: Fabiano Fidêncio (cherry picked from commit 33a8b705588d6f65163e05bf83119db4360b04ac) Signed-off-by: Fabiano Fidêncio --- src/runtime/virtcontainers/clh.go | 10 +++++++--- src/runtime/virtcontainers/clh_test.go | 2 ++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/runtime/virtcontainers/clh.go b/src/runtime/virtcontainers/clh.go index 95642dd396..b9cb1fb390 100644 --- a/src/runtime/virtcontainers/clh.go +++ b/src/runtime/virtcontainers/clh.go @@ -165,6 +165,7 @@ type cloudHypervisor struct { APIClient clhClient ctx context.Context id string + devicesIds map[string]string vmconfig chclient.VmConfig state CloudHypervisorState config HypervisorConfig @@ -360,6 +361,7 @@ func (clh *cloudHypervisor) CreateVM(ctx context.Context, id string, network Net clh.id = id clh.state.state = clhNotReady + clh.devicesIds = make(map[string]string) clh.Logger().WithField("function", "CreateVM").Info("creating Sandbox") @@ -667,7 +669,6 @@ func (clh *cloudHypervisor) hotplugAddBlockDevice(drive *config.BlockDrive) erro clhDisk := *chclient.NewDiskConfig(drive.File) clhDisk.Readonly = &drive.ReadOnly clhDisk.VhostUser = func(b bool) *bool { return &b }(false) - clhDisk.Id = &driveID pciInfo, _, err := cl.VmAddDiskPut(ctx, clhDisk) @@ -675,6 +676,7 @@ func (clh *cloudHypervisor) hotplugAddBlockDevice(drive *config.BlockDrive) erro return fmt.Errorf("failed to hotplug block device %+v %s", drive, openAPIClientError(err)) } + clh.devicesIds[driveID] = pciInfo.GetId() drive.PCIPath, err = clhPciInfoToPath(pciInfo) return err @@ -688,11 +690,11 @@ func (clh *cloudHypervisor) hotPlugVFIODevice(device *config.VFIODev) error { // Create the clh device config via the constructor to ensure default values are properly assigned clhDevice := *chclient.NewVmAddDevice() clhDevice.Path = &device.SysfsDev - clhDevice.Id = &device.ID pciInfo, _, err := cl.VmAddDevicePut(ctx, clhDevice) if err != nil { return fmt.Errorf("Failed to hotplug device %+v %s", device, openAPIClientError(err)) } + clh.devicesIds[device.ID] = pciInfo.GetId() // clh doesn't use bridges, so the PCI path is simply the slot // number of the device. This will break if clh starts using @@ -753,13 +755,15 @@ func (clh *cloudHypervisor) HotplugRemoveDevice(ctx context.Context, devInfo int ctx, cancel := context.WithTimeout(context.Background(), clhHotPlugAPITimeout*time.Second) defer cancel() + originalDeviceID := clh.devicesIds[deviceID] remove := *chclient.NewVmRemoveDevice() - remove.Id = &deviceID + remove.Id = &originalDeviceID _, err := cl.VmRemoveDevicePut(ctx, remove) if err != nil { err = fmt.Errorf("failed to hotplug remove (unplug) device %+v: %s", devInfo, openAPIClientError(err)) } + delete(clh.devicesIds, deviceID) return nil, err } diff --git a/src/runtime/virtcontainers/clh_test.go b/src/runtime/virtcontainers/clh_test.go index 9bfd2e62ee..47c3640dc9 100644 --- a/src/runtime/virtcontainers/clh_test.go +++ b/src/runtime/virtcontainers/clh_test.go @@ -384,6 +384,7 @@ func TestCloudHypervisorHotplugAddBlockDevice(t *testing.T) { clh := &cloudHypervisor{} clh.config = clhConfig clh.APIClient = &clhClientMock{} + clh.devicesIds = make(map[string]string) clh.config.BlockDeviceDriver = config.VirtioBlock err = clh.hotplugAddBlockDevice(&config.BlockDrive{Pmem: false}) @@ -406,6 +407,7 @@ func TestCloudHypervisorHotplugRemoveDevice(t *testing.T) { clh := &cloudHypervisor{} clh.config = clhConfig clh.APIClient = &clhClientMock{} + clh.devicesIds = make(map[string]string) _, err = clh.HotplugRemoveDevice(context.Background(), &config.BlockDrive{}, BlockDev) assert.NoError(err, "Hotplug remove block device expected no error") From b50b091c87c7724a8ae7c64047e992344ffcc0f2 Mon Sep 17 00:00:00 2001 From: Yibo Zhuang Date: Tue, 3 May 2022 09:57:31 -0700 Subject: [PATCH 2/2] agent: watchers: ensure uid/gid is preserved on copy/mkdir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Today in agent watchers, when we copy files/symlinks or create directories, the ownership of the source path is not preserved which can lead to permission issues. In copy, ensure that we do a chown of the source path uid/gid to the destination file/symlink after copy to ensure that ownership matches the source ownership. fs::copy() takes care of setting the permissions. For directory creation, ensure that we set the permissions of the created directory to the source directory permissions and also perform a chown of the source path uid/gid to ensure directory ownership and permissions matches to the source. Fixes: #4188 Signed-off-by: Yibo Zhuang (cherry picked from commit 70eda2fa6c1bdfdfeb8276c26192e2c1f052465c) Signed-off-by: Fabiano Fidêncio --- src/agent/src/watcher.rs | 56 +++++++++++++++++++++++++++++++++++----- 1 file changed, 49 insertions(+), 7 deletions(-) diff --git a/src/agent/src/watcher.rs b/src/agent/src/watcher.rs index 597c0a8341..e423126613 100644 --- a/src/agent/src/watcher.rs +++ b/src/agent/src/watcher.rs @@ -6,6 +6,7 @@ #![allow(unknown_lints)] use std::collections::HashMap; +use std::os::unix::fs::MetadataExt; use std::path::{Path, PathBuf}; use std::sync::Arc; use std::time::SystemTime; @@ -13,6 +14,7 @@ use std::time::SystemTime; use anyhow::{ensure, Context, Result}; use async_recursion::async_recursion; use nix::mount::{umount, MsFlags}; +use nix::unistd::{Gid, Uid}; use slog::{debug, error, info, warn, Logger}; use thiserror::Error; use tokio::fs; @@ -80,7 +82,8 @@ impl Drop for Storage { } async fn copy(from: impl AsRef, to: impl AsRef) -> Result<()> { - if fs::symlink_metadata(&from).await?.file_type().is_symlink() { + let metadata = fs::symlink_metadata(&from).await?; + if metadata.file_type().is_symlink() { // if source is a symlink, create new symlink with same link source. If // the symlink exists, remove and create new one: if fs::symlink_metadata(&to).await.is_ok() { @@ -88,8 +91,15 @@ async fn copy(from: impl AsRef, to: impl AsRef) -> Result<()> { } fs::symlink(fs::read_link(&from).await?, &to).await?; } else { - fs::copy(from, to).await?; + fs::copy(&from, &to).await?; } + // preserve the source uid and gid to the destination. + nix::unistd::chown( + to.as_ref(), + Some(Uid::from_raw(metadata.uid())), + Some(Gid::from_raw(metadata.gid())), + )?; + Ok(()) } @@ -106,14 +116,29 @@ impl Storage { async fn update_target(&self, logger: &Logger, source_path: impl AsRef) -> Result<()> { let source_file_path = source_path.as_ref(); + let metadata = source_file_path.symlink_metadata()?; // if we are creating a directory: just create it, nothing more to do - if source_file_path.symlink_metadata()?.file_type().is_dir() { + if metadata.file_type().is_dir() { let dest_file_path = self.make_target_path(&source_file_path)?; fs::create_dir_all(&dest_file_path) .await .with_context(|| format!("Unable to mkdir all for {}", dest_file_path.display()))?; + // set the directory permissions to match the source directory permissions + fs::set_permissions(&dest_file_path, metadata.permissions()) + .await + .with_context(|| { + format!("Unable to set permissions for {}", dest_file_path.display()) + })?; + // preserve the source directory uid and gid to the destination. + nix::unistd::chown( + &dest_file_path, + Some(Uid::from_raw(metadata.uid())), + Some(Gid::from_raw(metadata.gid())), + ) + .with_context(|| format!("Unable to set ownership for {}", dest_file_path.display()))?; + return Ok(()); } @@ -504,6 +529,7 @@ mod tests { use super::*; use crate::mount::is_mounted; use crate::skip_if_not_root; + use nix::unistd::{Gid, Uid}; use std::fs; use std::thread; @@ -895,20 +921,28 @@ mod tests { #[tokio::test] async fn test_copy() { + skip_if_not_root!(); + // prepare tmp src/destination let source_dir = tempfile::tempdir().unwrap(); let dest_dir = tempfile::tempdir().unwrap(); + let uid = Uid::from_raw(10); + let gid = Gid::from_raw(200); // verify copy of a regular file let src_file = source_dir.path().join("file.txt"); let dst_file = dest_dir.path().join("file.txt"); fs::write(&src_file, "foo").unwrap(); + nix::unistd::chown(&src_file, Some(uid), Some(gid)).unwrap(); + copy(&src_file, &dst_file).await.unwrap(); // verify destination: - assert!(!fs::symlink_metadata(dst_file) + assert!(!fs::symlink_metadata(&dst_file) .unwrap() .file_type() .is_symlink()); + assert_eq!(fs::metadata(&dst_file).unwrap().uid(), uid.as_raw()); + assert_eq!(fs::metadata(&dst_file).unwrap().gid(), gid.as_raw()); // verify copy of a symlink let src_symlink_file = source_dir.path().join("symlink_file.txt"); @@ -916,7 +950,7 @@ mod tests { tokio::fs::symlink(&src_file, &src_symlink_file) .await .unwrap(); - copy(src_symlink_file, &dst_symlink_file).await.unwrap(); + copy(&src_symlink_file, &dst_symlink_file).await.unwrap(); // verify destination: assert!(fs::symlink_metadata(&dst_symlink_file) .unwrap() @@ -924,6 +958,8 @@ mod tests { .is_symlink()); assert_eq!(fs::read_link(&dst_symlink_file).unwrap(), src_file); assert_eq!(fs::read_to_string(&dst_symlink_file).unwrap(), "foo"); + assert_ne!(fs::metadata(&dst_symlink_file).unwrap().uid(), uid.as_raw()); + assert_ne!(fs::metadata(&dst_symlink_file).unwrap().gid(), gid.as_raw()); } #[tokio::test] @@ -1069,6 +1105,8 @@ mod tests { #[tokio::test] async fn watch_directory() { + skip_if_not_root!(); + // Prepare source directory: // ./tmp/1.txt // ./tmp/A/B/2.txt @@ -1079,7 +1117,9 @@ mod tests { // A/C is an empty directory let empty_dir = "A/C"; - fs::create_dir_all(source_dir.path().join(empty_dir)).unwrap(); + let path = source_dir.path().join(empty_dir); + fs::create_dir_all(&path).unwrap(); + nix::unistd::chown(&path, Some(Uid::from_raw(10)), Some(Gid::from_raw(200))).unwrap(); // delay 20 ms between writes to files in order to ensure filesystem timestamps are unique thread::sleep(Duration::from_millis(20)); @@ -1123,7 +1163,9 @@ mod tests { // create another empty directory A/C/D let empty_dir = "A/C/D"; - fs::create_dir_all(source_dir.path().join(empty_dir)).unwrap(); + let path = source_dir.path().join(empty_dir); + fs::create_dir_all(&path).unwrap(); + nix::unistd::chown(&path, Some(Uid::from_raw(10)), Some(Gid::from_raw(200))).unwrap(); assert_eq!(entry.scan(&logger).await.unwrap(), 1); assert!(dest_dir.path().join(empty_dir).exists()); }