Skip to content

Commit

Permalink
aya: Use Arc<OwnedFd> when loading BTF fd
Browse files Browse the repository at this point in the history 
This fixes an existing file descriptor leak when there is BTF data in
the loaded object.

To avoid lifetime issues while having minimal impact to UX the
`OwnedFd` returned from the BPF_BTF_LOAD syscall will be wrapped in an
`Arc` and shared accross the programs and maps of the loaded BPF
file.
  • Loading branch information
@nrxus
nrxus committed Jul 28, 2023
1 parent 683a1cf commit ea96c29
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 52 deletions.
24 changes: 13 additions & 11 deletions aya/src/bpf.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,12 @@ use std::{
collections::{HashMap, HashSet},
ffi::CString,
fs, io,
os::{fd::RawFd, raw::c_int},
os::{
fd::{OwnedFd, RawFd},
raw::c_int,
},
path::{Path, PathBuf},
sync::Arc,
};

use aya_obj::{
Expand Down Expand Up @@ -390,7 +394,7 @@ impl<'a> BpfLoader<'a> {
let btf_fd = if let Some(features) = &FEATURES.btf() {
if let Some(btf) = obj.fixup_and_sanitize_btf(features)? {
match load_btf(btf.to_bytes(), *verifier_log_level) {
Ok(btf_fd) => Some(btf_fd),
Ok(btf_fd) => Some(Arc::new(btf_fd)),
// Only report an error here if the BTF is truely needed, otherwise proceed without.
Err(err) => {
for program in obj.programs.values() {
Expand Down Expand Up @@ -473,7 +477,7 @@ impl<'a> BpfLoader<'a> {
obj,
fd: None,
pinned: false,
btf_fd,
btf_fd: btf_fd.as_ref().map(Arc::clone),
};
let fd = match map.obj.pinning() {
PinningType::ByName => {
Expand Down Expand Up @@ -543,6 +547,7 @@ impl<'a> BpfLoader<'a> {
let section = prog_obj.section.clone();
let obj = (prog_obj, function_obj);

let btf_fd = btf_fd.as_ref().map(Arc::clone);
let program = if extensions.contains(name.as_str()) {
Program::Extension(Extension {
data: ProgramData::new(prog_name, obj, btf_fd, *verifier_log_level),
Expand Down Expand Up @@ -993,17 +998,14 @@ pub enum BpfError {
ProgramError(#[from] ProgramError),
}

fn load_btf(raw_btf: Vec<u8>, verifier_log_level: VerifierLogLevel) -> Result<RawFd, BtfError> {
fn load_btf(raw_btf: Vec<u8>, verifier_log_level: VerifierLogLevel) -> Result<OwnedFd, BtfError> {
let (ret, verifier_log) = retry_with_verifier_logs(10, |logger| {
bpf_load_btf(raw_btf.as_slice(), logger, verifier_log_level)
});
match ret {
Ok(fd) => Ok(fd as RawFd),
Err((_, io_error)) => Err(BtfError::LoadError {
io_error,
verifier_log,
}),
}
ret.map_err(|(_, io_error)| BtfError::LoadError {
io_error,
verifier_log,
})
}

/// Global data that can be exported to eBPF programs before they are loaded.
Expand Down
35 changes: 20 additions & 15 deletions aya/src/maps/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,10 @@ use std::{
marker::PhantomData,
mem,
ops::Deref,
os::fd::{AsRawFd, RawFd},
os::fd::{AsFd, AsRawFd, OwnedFd, RawFd},
path::Path,
ptr,
sync::Arc,
};

use crate::util::KernelVersion;
Expand Down Expand Up @@ -486,7 +487,7 @@ pub(crate) fn check_v_size<V>(map: &MapData) -> Result<(), MapError> {
pub struct MapData {
pub(crate) obj: obj::Map,
pub(crate) fd: Option<RawFd>,
pub(crate) btf_fd: Option<RawFd>,
pub(crate) btf_fd: Option<Arc<OwnedFd>>,
/// Indicates if this map has been pinned to bpffs
pub pinned: bool,
}
Expand All @@ -504,19 +505,23 @@ impl MapData {
let kernel_version = KernelVersion::current().unwrap();
#[cfg(test)]
let kernel_version = KernelVersion::new(0xff, 0xff, 0xff);
let fd = bpf_create_map(&c_name, &self.obj, self.btf_fd, kernel_version).map_err(
|(code, io_error)| {
if kernel_version < KernelVersion::new(5, 11, 0) {
maybe_warn_rlimit();
}
let fd = bpf_create_map(
&c_name,
&self.obj,
self.btf_fd.as_ref().map(|f| f.as_fd()),
kernel_version,
)
.map_err(|(code, io_error)| {
if kernel_version < KernelVersion::new(5, 11, 0) {
maybe_warn_rlimit();
}

MapError::CreateError {
name: name.into(),
code,
io_error,
}
},
)? as RawFd;
MapError::CreateError {
name: name.into(),
code,
io_error,
}
})? as RawFd;

self.fd = Some(fd);

Expand Down Expand Up @@ -639,7 +644,7 @@ impl Clone for MapData {
MapData {
obj: self.obj.clone(),
fd: self.fd.map(|fd| unsafe { libc::dup(fd) }),
btf_fd: self.btf_fd,
btf_fd: self.btf_fd.as_ref().map(Arc::clone),
pinned: self.pinned,
}
}
Expand Down
9 changes: 5 additions & 4 deletions aya/src/programs/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,9 @@ use libc::ENOSPC;
use std::{
ffi::CString,
io,
os::fd::{AsRawFd, IntoRawFd as _, RawFd},
os::fd::{AsFd, AsRawFd, IntoRawFd as _, OwnedFd, RawFd},
path::{Path, PathBuf},
sync::Arc,
};
use thiserror::Error;

Expand Down Expand Up @@ -413,7 +414,7 @@ pub(crate) struct ProgramData<T: Link> {
pub(crate) attach_btf_obj_fd: Option<u32>,
pub(crate) attach_btf_id: Option<u32>,
pub(crate) attach_prog_fd: Option<RawFd>,
pub(crate) btf_fd: Option<RawFd>,
pub(crate) btf_fd: Option<Arc<OwnedFd>>,
pub(crate) verifier_log_level: VerifierLogLevel,
pub(crate) path: Option<PathBuf>,
pub(crate) flags: u32,
Expand All @@ -423,7 +424,7 @@ impl<T: Link> ProgramData<T> {
pub(crate) fn new(
name: Option<String>,
obj: (obj::Program, obj::Function),
btf_fd: Option<RawFd>,
btf_fd: Option<Arc<OwnedFd>>,
verifier_log_level: VerifierLogLevel,
) -> ProgramData<T> {
ProgramData {
Expand Down Expand Up @@ -613,7 +614,7 @@ fn load_program<T: Link>(
license,
kernel_version: target_kernel_version,
expected_attach_type: *expected_attach_type,
prog_btf_fd: *btf_fd,
prog_btf_fd: btf_fd.as_ref().map(|f| f.as_fd()),
attach_btf_obj_fd: *attach_btf_obj_fd,
attach_btf_id: *attach_btf_id,
attach_prog_fd: *attach_prog_fd,
Expand Down
47 changes: 25 additions & 22 deletions aya/src/sys/bpf.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::{
ffi::{CStr, CString},
io,
mem::{self, MaybeUninit},
os::fd::{FromRawFd as _, OwnedFd, RawFd},
os::fd::{AsRawFd, BorrowedFd, FromRawFd as _, OwnedFd, RawFd},
slice,
};

Expand Down Expand Up @@ -35,7 +35,7 @@ use crate::{
pub(crate) fn bpf_create_map(
name: &CStr,
def: &obj::Map,
btf_fd: Option<RawFd>,
btf_fd: Option<BorrowedFd<'_>>,
kernel_version: KernelVersion,
) -> SysResult<c_long> {
let mut attr = unsafe { mem::zeroed::<bpf_attr>() };
Expand Down Expand Up @@ -75,7 +75,7 @@ pub(crate) fn bpf_create_map(
_ => {
u.btf_key_type_id = m.def.btf_key_type_id;
u.btf_value_type_id = m.def.btf_value_type_id;
u.btf_fd = btf_fd.unwrap_or_default() as u32;
u.btf_fd = btf_fd.map(|fd| fd.as_raw_fd()).unwrap_or_default() as u32;
}
}
}
Expand Down Expand Up @@ -115,7 +115,7 @@ pub(crate) struct BpfLoadProgramAttrs<'a> {
pub(crate) license: &'a CStr,
pub(crate) kernel_version: u32,
pub(crate) expected_attach_type: Option<bpf_attach_type>,
pub(crate) prog_btf_fd: Option<RawFd>,
pub(crate) prog_btf_fd: Option<BorrowedFd<'a>>,
pub(crate) attach_btf_obj_fd: Option<u32>,
pub(crate) attach_btf_id: Option<u32>,
pub(crate) attach_prog_fd: Option<RawFd>,
Expand Down Expand Up @@ -161,7 +161,7 @@ pub(crate) fn bpf_load_program(
let func_info_buf = aya_attr.func_info.func_info_bytes();

if let Some(btf_fd) = aya_attr.prog_btf_fd {
u.prog_btf_fd = btf_fd as u32;
u.prog_btf_fd = btf_fd.as_raw_fd() as u32;
if aya_attr.line_info_rec_size > 0 {
u.line_info = line_info_buf.as_ptr() as *const _ as u64;
u.line_info_cnt = aya_attr.line_info.len() as u32;
Expand Down Expand Up @@ -464,21 +464,8 @@ pub(crate) fn bpf_prog_get_fd_by_id(prog_id: u32) -> Result<OwnedFd, io::Error>
let mut attr = unsafe { mem::zeroed::<bpf_attr>() };

attr.__bindgen_anon_6.__bindgen_anon_1.prog_id = prog_id;

match sys_bpf(bpf_cmd::BPF_PROG_GET_FD_BY_ID, &attr) {
Ok(v) => {
let v = v.try_into().map_err(|_err| {
// _err is std::num::TryFromIntError or std::convert::Infallible depending on
// target, so we can't ascribe.
io::Error::new(
io::ErrorKind::InvalidData,
format!("bpf_prog_get_fd_by_id: invalid fd returned: {}", v),
)
})?;
Ok(unsafe { OwnedFd::from_raw_fd(v) })
}
Err((_, err)) => Err(err),
}
// SAFETY: BPF_PROG_GET_FD_BY_ID returns a new file descriptor.
unsafe { fd_sys_bpf(bpf_cmd::BPF_PROG_GET_FD_BY_ID, &attr).map_err(|(_, e)| e) }
}

pub(crate) fn bpf_prog_get_info_by_fd(prog_fd: RawFd) -> Result<bpf_prog_info, io::Error> {
Expand Down Expand Up @@ -561,7 +548,7 @@ pub(crate) fn bpf_load_btf(
raw_btf: &[u8],
log_buf: &mut [u8],
verifier_log_level: VerifierLogLevel,
) -> SysResult<c_long> {
) -> SysResult<OwnedFd> {
let mut attr = unsafe { mem::zeroed::<bpf_attr>() };
let u = unsafe { &mut attr.__bindgen_anon_7 };
u.btf = raw_btf.as_ptr() as *const _ as u64;
Expand All @@ -571,7 +558,23 @@ pub(crate) fn bpf_load_btf(
u.btf_log_buf = log_buf.as_mut_ptr() as u64;
u.btf_log_size = log_buf.len() as u32;
}
sys_bpf(bpf_cmd::BPF_BTF_LOAD, &attr)
// SAFETY: `BPF_BTF_LOAD` returns a newly created fd.
unsafe { fd_sys_bpf(bpf_cmd::BPF_BTF_LOAD, &attr) }
}

// SAFETY: only use for bpf_cmd that return a new file descriptor on success.
unsafe fn fd_sys_bpf(cmd: bpf_cmd, attr: &bpf_attr) -> SysResult<OwnedFd> {
let fd = sys_bpf(cmd, attr)?;
let fd = fd.try_into().map_err(|_| {
(
fd,
io::Error::new(
io::ErrorKind::InvalidData,
format!("{cmd:?}: invalid fd returned: {fd}"),
),
)
})?;
Ok(OwnedFd::from_raw_fd(fd))
}

pub(crate) fn bpf_btf_get_fd_by_id(id: u32) -> Result<RawFd, io::Error> {
Expand Down

0 comments on commit ea96c29

Please sign in to comment.