From ea96c29ccbae6c59a6a5bfc90f402ad307e22665 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20Medina?= Date: Thu, 27 Jul 2023 22:01:55 -0700 Subject: [PATCH] aya: Use Arc when loading BTF fd 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. --- aya/src/bpf.rs | 24 +++++++++++---------- aya/src/maps/mod.rs | 35 +++++++++++++++++------------- aya/src/programs/mod.rs | 9 ++++---- aya/src/sys/bpf.rs | 47 ++++++++++++++++++++++------------------- 4 files changed, 63 insertions(+), 52 deletions(-) diff --git a/aya/src/bpf.rs b/aya/src/bpf.rs index 818a063b..eed006d4 100644 --- a/aya/src/bpf.rs +++ b/aya/src/bpf.rs @@ -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::{ @@ -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() { @@ -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 => { @@ -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), @@ -993,17 +998,14 @@ pub enum BpfError { ProgramError(#[from] ProgramError), } -fn load_btf(raw_btf: Vec, verifier_log_level: VerifierLogLevel) -> Result { +fn load_btf(raw_btf: Vec, verifier_log_level: VerifierLogLevel) -> Result { 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. diff --git a/aya/src/maps/mod.rs b/aya/src/maps/mod.rs index fca42110..5dce668e 100644 --- a/aya/src/maps/mod.rs +++ b/aya/src/maps/mod.rs @@ -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; @@ -486,7 +487,7 @@ pub(crate) fn check_v_size(map: &MapData) -> Result<(), MapError> { pub struct MapData { pub(crate) obj: obj::Map, pub(crate) fd: Option, - pub(crate) btf_fd: Option, + pub(crate) btf_fd: Option>, /// Indicates if this map has been pinned to bpffs pub pinned: bool, } @@ -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); @@ -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, } } diff --git a/aya/src/programs/mod.rs b/aya/src/programs/mod.rs index f868dfc8..5d25b8cb 100644 --- a/aya/src/programs/mod.rs +++ b/aya/src/programs/mod.rs @@ -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; @@ -413,7 +414,7 @@ pub(crate) struct ProgramData { pub(crate) attach_btf_obj_fd: Option, pub(crate) attach_btf_id: Option, pub(crate) attach_prog_fd: Option, - pub(crate) btf_fd: Option, + pub(crate) btf_fd: Option>, pub(crate) verifier_log_level: VerifierLogLevel, pub(crate) path: Option, pub(crate) flags: u32, @@ -423,7 +424,7 @@ impl ProgramData { pub(crate) fn new( name: Option, obj: (obj::Program, obj::Function), - btf_fd: Option, + btf_fd: Option>, verifier_log_level: VerifierLogLevel, ) -> ProgramData { ProgramData { @@ -613,7 +614,7 @@ fn load_program( 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, diff --git a/aya/src/sys/bpf.rs b/aya/src/sys/bpf.rs index 5a26c57f..858ccc55 100644 --- a/aya/src/sys/bpf.rs +++ b/aya/src/sys/bpf.rs @@ -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, }; @@ -35,7 +35,7 @@ use crate::{ pub(crate) fn bpf_create_map( name: &CStr, def: &obj::Map, - btf_fd: Option, + btf_fd: Option>, kernel_version: KernelVersion, ) -> SysResult { let mut attr = unsafe { mem::zeroed::() }; @@ -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; } } } @@ -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, - pub(crate) prog_btf_fd: Option, + pub(crate) prog_btf_fd: Option>, pub(crate) attach_btf_obj_fd: Option, pub(crate) attach_btf_id: Option, pub(crate) attach_prog_fd: Option, @@ -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; @@ -464,21 +464,8 @@ pub(crate) fn bpf_prog_get_fd_by_id(prog_id: u32) -> Result let mut attr = unsafe { mem::zeroed::() }; 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 { @@ -561,7 +548,7 @@ pub(crate) fn bpf_load_btf( raw_btf: &[u8], log_buf: &mut [u8], verifier_log_level: VerifierLogLevel, -) -> SysResult { +) -> SysResult { let mut attr = unsafe { mem::zeroed::() }; let u = unsafe { &mut attr.__bindgen_anon_7 }; u.btf = raw_btf.as_ptr() as *const _ as u64; @@ -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 { + 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 {