From d4ac45d840cc51fc4b2663b4476e656bcb527ecd Mon Sep 17 00:00:00 2001 From: Pavel Mores Date: Tue, 19 Mar 2024 12:39:52 +0100 Subject: [PATCH] runtime-rs: refactor clear_fd_flags() The idea of this function is to make sure O_CLOEXEC is not set on file descriptors that should be inherited by a child (=hypervisor) process. The approach so far is however rather heavy-handed - clearing *all* flags is unjustifiably aggresive for a low-level function with no knowledge of context whatsoever. This commit refactors the function so that it only does what's expected and renames it accordingly. It also clarifies some of its call sites. Signed-off-by: Pavel Mores --- .../hypervisor/src/qemu/cmdline_generator.rs | 4 ++-- .../crates/hypervisor/src/qemu/network.rs | 6 ++--- src/runtime-rs/crates/hypervisor/src/utils.rs | 22 +++++++++++-------- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs b/src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs index 3e3a919ff9..abd974cbdc 100644 --- a/src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs +++ b/src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs @@ -4,7 +4,7 @@ // use super::network::{generate_netdev_fds, NetDevice}; -use crate::utils::clear_fd_flags; +use crate::utils::clear_cloexec; use crate::{kernel_param::KernelParams, HypervisorConfig, NetworkConfig}; use anyhow::{anyhow, Context, Result}; @@ -1056,7 +1056,7 @@ impl<'a> QemuCmdLine<'a> { } pub fn add_vsock(&mut self, vhostfd: RawFd, guest_cid: u32) -> Result<()> { - clear_fd_flags(vhostfd).context("clear flags failed")?; + clear_cloexec(vhostfd).context("clearing O_CLOEXEC failed on vsock fd")?; let mut vhost_vsock_pci = VhostVsock::new(vhostfd, guest_cid, self.bus_type()); diff --git a/src/runtime-rs/crates/hypervisor/src/qemu/network.rs b/src/runtime-rs/crates/hypervisor/src/qemu/network.rs index ce0a0b7e89..3d314e9f54 100644 --- a/src/runtime-rs/crates/hypervisor/src/qemu/network.rs +++ b/src/runtime-rs/crates/hypervisor/src/qemu/network.rs @@ -8,7 +8,7 @@ use std::convert::TryFrom; use std::fs::{File, OpenOptions}; use std::os::fd::RawFd; -use crate::utils::{clear_fd_flags, open_named_tuntap}; +use crate::utils::{clear_cloexec, open_named_tuntap}; use crate::{Address, NetworkConfig}; use anyhow::{anyhow, Context, Result}; @@ -232,7 +232,7 @@ impl NetDevice { netdev_params.push("vhost=on".to_owned()); if let Some(vhost_fds) = self.fds.get("vhostfds") { for fd in vhost_fds.iter() { - clear_fd_flags(*fd)?; + clear_cloexec(*fd).context("clearing O_CLOEXEC failed on vhost fd")?; } let s = vhost_fds .iter() @@ -246,7 +246,7 @@ impl NetDevice { if let Some(tuntap_fds) = self.fds.get("fds") { for fd in tuntap_fds.iter() { - clear_fd_flags(*fd).context("clear flag of fd failed")?; + clear_cloexec(*fd).context("clearing O_CLOEXEC failed on tuntap fd")?; } let s = tuntap_fds .iter() diff --git a/src/runtime-rs/crates/hypervisor/src/utils.rs b/src/runtime-rs/crates/hypervisor/src/utils.rs index 7e3606dabb..95e6554b3d 100644 --- a/src/runtime-rs/crates/hypervisor/src/utils.rs +++ b/src/runtime-rs/crates/hypervisor/src/utils.rs @@ -58,15 +58,19 @@ pub fn get_jailer_root(sid: &str) -> String { [&sandbox_path, JAILER_ROOT].join("/") } -// Clear the O_CLOEXEC which is set by default by Rust standard library -// as it would obviously prevent passing the descriptor to the hypervisor process. -pub fn clear_fd_flags(rawfd: RawFd) -> Result<()> { - if let Err(err) = fcntl::fcntl(rawfd, fcntl::FcntlArg::F_SETFD(fcntl::FdFlag::empty())) { - info!( - sl!(), - "couldn't clear O_CLOEXEC on device's fd, communication with agent will not work: {:?}", - err - ); +// Clear the O_CLOEXEC which is set by default by Rust standard library on +// file descriptors that it opens. This function is mostly meant to be +// called on descriptors to be passed to a child (hypervisor) process as +// O_CLOEXEC would obviously prevent that. +pub fn clear_cloexec(rawfd: RawFd) -> Result<()> { + let cur_flags = fcntl::fcntl(rawfd, fcntl::FcntlArg::F_GETFD)?; + let mut new_flags = fcntl::FdFlag::from_bits(cur_flags).ok_or(anyhow!( + "couldn't construct FdFlag from flags value {:?}", + cur_flags + ))?; + new_flags.remove(fcntl::FdFlag::FD_CLOEXEC); + if let Err(err) = fcntl::fcntl(rawfd, fcntl::FcntlArg::F_SETFD(new_flags)) { + info!(sl!(), "couldn't clear O_CLOEXEC on fd: {:?}", err); return Err(err.into()); }