From 7a7d16885a89af8c10a52e5aba0927784d42f551 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Thu, 2 May 2024 10:19:42 -0400 Subject: [PATCH] Avoid crashing under Miri See https://github.com/rust-lang/rust/pull/124210. --- aya/src/lib.rs | 72 +++++++++++++++++++++++++++ aya/src/maps/mod.rs | 52 +++++++++++++------ aya/src/maps/perf/perf_buffer.rs | 17 +++---- aya/src/maps/perf/perf_event_array.rs | 4 +- aya/src/maps/sock/mod.rs | 2 - aya/src/sys/bpf.rs | 11 ++-- xtask/public-api/aya.txt | 2 +- 7 files changed, 125 insertions(+), 35 deletions(-) diff --git a/aya/src/lib.rs b/aya/src/lib.rs index ece5ebc9..75451e25 100644 --- a/aya/src/lib.rs +++ b/aya/src/lib.rs @@ -88,8 +88,80 @@ pub use programs::loaded_programs; mod sys; pub mod util; +use std::os::fd::{AsFd, BorrowedFd, OwnedFd}; + pub use bpf::*; pub use obj::btf::{Btf, BtfError}; pub use object::Endianness; #[doc(hidden)] pub use sys::netlink_set_link_up; + +// See https://github.com/rust-lang/rust/pull/124210; this structure exists to avoid crashing the +// process when we try to close a fake file descriptor in Miri. +#[derive(Debug)] +struct MiriSafeFd { + #[cfg(not(miri))] + fd: OwnedFd, + #[cfg(miri)] + fd: Option, +} + +impl MiriSafeFd { + #[cfg(any(test, miri))] + const MOCK_FD: u16 = 1337; + + #[cfg(not(miri))] + fn from_fd(fd: OwnedFd) -> Self { + Self { fd } + } + + #[cfg(miri)] + fn from_fd(fd: OwnedFd) -> Self { + Self { fd: Some(fd) } + } + + #[cfg(not(miri))] + fn try_clone(&self) -> std::io::Result { + let Self { fd } = self; + let fd = fd.try_clone()?; + Ok(Self { fd }) + } + + #[cfg(miri)] + fn try_clone(&self) -> std::io::Result { + let Self { fd } = self; + let fd = fd.as_ref().map(OwnedFd::try_clone).transpose()?; + Ok(Self { fd }) + } +} + +impl AsFd for MiriSafeFd { + #[cfg(not(miri))] + fn as_fd(&self) -> BorrowedFd<'_> { + let Self { fd } = self; + fd.as_fd() + } + + #[cfg(miri)] + fn as_fd(&self) -> BorrowedFd<'_> { + let Self { fd } = self; + fd.as_ref().unwrap().as_fd() + } +} + +impl Drop for MiriSafeFd { + #[cfg(not(miri))] + fn drop(&mut self) { + // Intentional no-op. + } + + #[cfg(miri)] + fn drop(&mut self) { + use std::os::fd::AsRawFd as _; + + let Self { fd } = self; + let fd = fd.take().unwrap(); + assert_eq!(fd.as_raw_fd(), Self::MOCK_FD.into()); + std::mem::forget(fd) + } +} diff --git a/aya/src/maps/mod.rs b/aya/src/maps/mod.rs index e781b0c9..d10c02c8 100644 --- a/aya/src/maps/mod.rs +++ b/aya/src/maps/mod.rs @@ -210,11 +210,26 @@ impl From for MapError { /// A map file descriptor. #[derive(Debug)] -pub struct MapFd(OwnedFd); +pub struct MapFd { + fd: crate::MiriSafeFd, +} + +impl MapFd { + fn from_fd(fd: OwnedFd) -> Self { + let fd = crate::MiriSafeFd::from_fd(fd); + Self { fd } + } + + fn try_clone(&self) -> io::Result { + let Self { fd } = self; + let fd = fd.try_clone()?; + Ok(Self { fd }) + } +} impl AsFd for MapFd { fn as_fd(&self) -> BorrowedFd<'_> { - let Self(fd) = self; + let Self { fd } = self; fd.as_fd() } } @@ -558,8 +573,10 @@ impl MapData { io_error, } })?; - let fd = MapFd(fd); - Ok(Self { obj, fd }) + Ok(Self { + obj, + fd: MapFd::from_fd(fd), + }) } pub(crate) fn create_pinned_by_name>( @@ -585,10 +602,10 @@ impl MapData { call: "BPF_OBJ_GET", io_error, }) { - Ok(fd) => { - let fd = MapFd(fd); - Ok(Self { obj, fd }) - } + Ok(fd) => Ok(Self { + obj, + fd: MapFd::from_fd(fd), + }), Err(_) => { let map = Self::create(obj, name, btf_fd)?; map.pin(&path).map_err(|error| MapError::PinError { @@ -658,7 +675,7 @@ impl MapData { let MapInfo(info) = MapInfo::new_from_fd(fd.as_fd())?; Ok(Self { obj: parse_map_info(info, PinningType::None), - fd: MapFd(fd), + fd: MapFd::from_fd(fd), }) } @@ -972,7 +989,7 @@ impl MapInfo { pub fn fd(&self) -> Result { let Self(info) = self; let fd = bpf_map_get_fd_by_id(info.id)?; - Ok(MapFd(fd)) + Ok(MapFd::from_fd(fd)) } /// Loads a map from a pinned path in bpffs. @@ -1035,7 +1052,7 @@ mod test_utils { Syscall::Ebpf { cmd: bpf_cmd::BPF_MAP_CREATE, .. - } => Ok(1337), + } => Ok(crate::MiriSafeFd::MOCK_FD.into()), call => panic!("unexpected syscall {:?}", call), }); MapData::create(obj, "foo", None).unwrap() @@ -1086,13 +1103,16 @@ mod tests { unsafe { attr.__bindgen_anon_6.__bindgen_anon_1.map_id }, 1234 ); - Ok(42) + Ok(crate::MiriSafeFd::MOCK_FD.into()) } Syscall::Ebpf { cmd: bpf_cmd::BPF_OBJ_GET_INFO_BY_FD, attr, } => { - assert_eq!(unsafe { attr.info.bpf_fd }, 42); + assert_eq!( + unsafe { attr.info.bpf_fd }, + crate::MiriSafeFd::MOCK_FD.into() + ); Ok(0) } _ => Err((-1, io::Error::from_raw_os_error(EFAULT))), @@ -1103,7 +1123,7 @@ mod tests { Ok(MapData { obj: _, fd, - }) => assert_eq!(fd.as_fd().as_raw_fd(), 42) + }) => assert_eq!(fd.as_fd().as_raw_fd(), crate::MiriSafeFd::MOCK_FD.into()) ); } @@ -1113,7 +1133,7 @@ mod tests { Syscall::Ebpf { cmd: bpf_cmd::BPF_MAP_CREATE, .. - } => Ok(42), + } => Ok(crate::MiriSafeFd::MOCK_FD.into()), _ => Err((-1, io::Error::from_raw_os_error(EFAULT))), }); @@ -1122,7 +1142,7 @@ mod tests { Ok(MapData { obj: _, fd, - }) => assert_eq!(fd.as_fd().as_raw_fd(), 42) + }) => assert_eq!(fd.as_fd().as_raw_fd(), crate::MiriSafeFd::MOCK_FD.into()) ); } diff --git a/aya/src/maps/perf/perf_buffer.rs b/aya/src/maps/perf/perf_buffer.rs index 72ef5fbd..316438a0 100644 --- a/aya/src/maps/perf/perf_buffer.rs +++ b/aya/src/maps/perf/perf_buffer.rs @@ -1,7 +1,7 @@ use std::{ ffi::c_void, io, mem, - os::fd::{AsFd, AsRawFd, BorrowedFd, OwnedFd, RawFd}, + os::fd::{AsFd, BorrowedFd}, ptr, slice, sync::atomic::{self, AtomicPtr, Ordering}, }; @@ -88,7 +88,7 @@ pub(crate) struct PerfBuffer { buf: AtomicPtr, size: usize, page_size: usize, - fd: OwnedFd, + fd: crate::MiriSafeFd, } impl PerfBuffer { @@ -120,11 +120,12 @@ impl PerfBuffer { }); } + let fd = crate::MiriSafeFd::from_fd(fd); let perf_buf = Self { buf: AtomicPtr::new(buf as *mut perf_event_mmap_page), - fd, size, page_size, + fd, }; perf_event_ioctl(perf_buf.fd.as_fd(), PERF_EVENT_IOC_ENABLE, 0) @@ -259,12 +260,6 @@ impl PerfBuffer { } } -impl AsRawFd for PerfBuffer { - fn as_raw_fd(&self) -> RawFd { - self.fd.as_raw_fd() - } -} - impl AsFd for PerfBuffer { fn as_fd(&self) -> BorrowedFd<'_> { self.fd.as_fd() @@ -307,7 +302,9 @@ mod tests { fn fake_mmap(buf: &MMappedBuf) { override_syscall(|call| match call { - Syscall::PerfEventOpen { .. } | Syscall::PerfEventIoctl { .. } => Ok(42), + Syscall::PerfEventOpen { .. } | Syscall::PerfEventIoctl { .. } => { + Ok(crate::MiriSafeFd::MOCK_FD.into()) + } call => panic!("unexpected syscall: {:?}", call), }); TEST_MMAP_RET.with(|ret| *ret.borrow_mut() = buf as *const _ as *mut _); diff --git a/aya/src/maps/perf/perf_event_array.rs b/aya/src/maps/perf/perf_event_array.rs index d867e659..f48826cc 100644 --- a/aya/src/maps/perf/perf_event_array.rs +++ b/aya/src/maps/perf/perf_event_array.rs @@ -64,7 +64,7 @@ impl> AsFd for PerfEventArrayBuffer { impl> AsRawFd for PerfEventArrayBuffer { fn as_raw_fd(&self) -> RawFd { - self.buf.as_raw_fd() + self.buf.as_fd().as_raw_fd() } } @@ -199,7 +199,7 @@ impl> PerfEventArray { let map_data: &MapData = self.map.deref().borrow(); let map_fd = map_data.fd().as_fd(); let buf = PerfBuffer::open(index, self.page_size, page_count.unwrap_or(2))?; - bpf_map_update_elem(map_fd, Some(&index), &buf.as_raw_fd(), 0) + bpf_map_update_elem(map_fd, Some(&index), &buf.as_fd().as_raw_fd(), 0) .map_err(|(_, io_error)| io_error)?; Ok(PerfEventArrayBuffer { diff --git a/aya/src/maps/sock/mod.rs b/aya/src/maps/sock/mod.rs index 067e9d03..b1015f2c 100644 --- a/aya/src/maps/sock/mod.rs +++ b/aya/src/maps/sock/mod.rs @@ -18,9 +18,7 @@ impl SockMapFd { /// Creates a new instance that shares the same underlying file description as [`self`]. pub fn try_clone(&self) -> io::Result { let Self(inner) = self; - let super::MapFd(inner) = inner; let inner = inner.try_clone()?; - let inner = super::MapFd(inner); Ok(Self(inner)) } } diff --git a/aya/src/sys/bpf.rs b/aya/src/sys/bpf.rs index f1a6d916..add46b67 100644 --- a/aya/src/sys/bpf.rs +++ b/aya/src/sys/bpf.rs @@ -730,6 +730,7 @@ pub(crate) fn is_perf_link_supported() -> bool { u.prog_type = bpf_prog_type::BPF_PROG_TYPE_TRACEPOINT as u32; if let Ok(fd) = bpf_prog_load(&mut attr) { + let fd = crate::MiriSafeFd::from_fd(fd); let fd = fd.as_fd(); matches!( // Uses an invalid target FD so we get EBADF if supported. @@ -828,7 +829,9 @@ pub(crate) fn is_prog_id_supported(map_type: bpf_map_type) -> bool { u.map_flags = 0; // SAFETY: BPF_MAP_CREATE returns a new file descriptor. - unsafe { fd_sys_bpf(bpf_cmd::BPF_MAP_CREATE, &mut attr) }.is_ok() + let fd = unsafe { fd_sys_bpf(bpf_cmd::BPF_MAP_CREATE, &mut attr) }; + let fd = fd.map(crate::MiriSafeFd::from_fd); + fd.is_ok() } pub(crate) fn is_btf_supported() -> bool { @@ -1100,7 +1103,7 @@ mod tests { cmd: bpf_cmd::BPF_LINK_CREATE, .. } => Err((-1, io::Error::from_raw_os_error(EBADF))), - _ => Ok(42), + _ => Ok(crate::MiriSafeFd::MOCK_FD.into()), }); let supported = is_perf_link_supported(); assert!(supported); @@ -1110,7 +1113,7 @@ mod tests { cmd: bpf_cmd::BPF_LINK_CREATE, .. } => Err((-1, io::Error::from_raw_os_error(EINVAL))), - _ => Ok(42), + _ => Ok(crate::MiriSafeFd::MOCK_FD.into()), }); let supported = is_perf_link_supported(); assert!(!supported); @@ -1118,7 +1121,7 @@ mod tests { #[test] fn test_prog_id_supported() { - override_syscall(|_call| Ok(42)); + override_syscall(|_call| Ok(crate::MiriSafeFd::MOCK_FD.into())); // Ensure that the three map types we can check are accepted let supported = is_prog_id_supported(bpf_map_type::BPF_MAP_TYPE_CPUMAP); diff --git a/xtask/public-api/aya.txt b/xtask/public-api/aya.txt index 49ed668e..760c4e08 100644 --- a/xtask/public-api/aya.txt +++ b/xtask/public-api/aya.txt @@ -1758,7 +1758,7 @@ impl core::borrow::BorrowMut for aya::maps::MapData where T: core::marker: pub fn aya::maps::MapData::borrow_mut(&mut self) -> &mut T impl core::convert::From for aya::maps::MapData pub fn aya::maps::MapData::from(t: T) -> T -pub struct aya::maps::MapFd(_) +pub struct aya::maps::MapFd impl core::fmt::Debug for aya::maps::MapFd pub fn aya::maps::MapFd::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl std::os::fd::owned::AsFd for aya::maps::MapFd