Skip to content

Commit

Permalink
Avoid crashing under Miri
Browse files Browse the repository at this point in the history
  • Loading branch information
tamird committed May 3, 2024
1 parent 7e1666f commit 7a7d168
Show file tree
Hide file tree
Showing 7 changed files with 125 additions and 35 deletions.
72 changes: 72 additions & 0 deletions aya/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<OwnedFd>,
}

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<Self> {
let Self { fd } = self;
let fd = fd.try_clone()?;
Ok(Self { fd })
}

#[cfg(miri)]
fn try_clone(&self) -> std::io::Result<Self> {
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)
}
}
52 changes: 36 additions & 16 deletions aya/src/maps/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,11 +210,26 @@ impl From<InvalidMapTypeError> 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<Self> {
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()
}
}
Expand Down Expand Up @@ -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<P: AsRef<Path>>(
Expand All @@ -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 {
Expand Down Expand Up @@ -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),
})
}

Expand Down Expand Up @@ -972,7 +989,7 @@ impl MapInfo {
pub fn fd(&self) -> Result<MapFd, MapError> {
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.
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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))),
Expand All @@ -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())
);
}

Expand All @@ -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))),
});

Expand All @@ -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())
);
}

Expand Down
17 changes: 7 additions & 10 deletions aya/src/maps/perf/perf_buffer.rs
Original file line number Diff line number Diff line change
@@ -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},
};
Expand Down Expand Up @@ -88,7 +88,7 @@ pub(crate) struct PerfBuffer {
buf: AtomicPtr<perf_event_mmap_page>,
size: usize,
page_size: usize,
fd: OwnedFd,
fd: crate::MiriSafeFd,
}

impl PerfBuffer {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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 _);
Expand Down
4 changes: 2 additions & 2 deletions aya/src/maps/perf/perf_event_array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ impl<T: BorrowMut<MapData>> AsFd for PerfEventArrayBuffer<T> {

impl<T: BorrowMut<MapData>> AsRawFd for PerfEventArrayBuffer<T> {
fn as_raw_fd(&self) -> RawFd {
self.buf.as_raw_fd()
self.buf.as_fd().as_raw_fd()
}
}

Expand Down Expand Up @@ -199,7 +199,7 @@ impl<T: BorrowMut<MapData>> PerfEventArray<T> {
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 {
Expand Down
2 changes: 0 additions & 2 deletions aya/src/maps/sock/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Self> {
let Self(inner) = self;
let super::MapFd(inner) = inner;
let inner = inner.try_clone()?;
let inner = super::MapFd(inner);
Ok(Self(inner))
}
}
Expand Down
11 changes: 7 additions & 4 deletions aya/src/sys/bpf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand All @@ -1110,15 +1113,15 @@ 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);
}

#[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);
Expand Down
2 changes: 1 addition & 1 deletion xtask/public-api/aya.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1758,7 +1758,7 @@ impl<T> core::borrow::BorrowMut<T> for aya::maps::MapData where T: core::marker:
pub fn aya::maps::MapData::borrow_mut(&mut self) -> &mut T
impl<T> core::convert::From<T> 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
Expand Down

0 comments on commit 7a7d168

Please sign in to comment.