From dbfba18dac87cbd837820316d53ad09b27d0c469 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20Medina?= Date: Sun, 30 Jul 2023 20:41:31 -0700 Subject: [PATCH] aya: Return `OwnedFd` for `perf_event_open`. This fixes a file descriptor leak when creating a link of BPF_PERF_EVENT attach type. --- aya/src/maps/perf/perf_buffer.rs | 30 ++++++++------- aya/src/programs/perf_attach.rs | 37 +++++++++--------- aya/src/programs/perf_event.rs | 2 +- aya/src/programs/probe.rs | 17 ++++----- aya/src/programs/trace_point.rs | 4 +- aya/src/sys/mod.rs | 9 +++-- aya/src/sys/perf_event.rs | 64 ++++++++++++++++++-------------- 7 files changed, 88 insertions(+), 75 deletions(-) diff --git a/aya/src/maps/perf/perf_buffer.rs b/aya/src/maps/perf/perf_buffer.rs index 1024152a..2a5bfe3d 100644 --- a/aya/src/maps/perf/perf_buffer.rs +++ b/aya/src/maps/perf/perf_buffer.rs @@ -1,13 +1,13 @@ use std::{ ffi::c_void, io, mem, - os::fd::{AsRawFd, RawFd}, + os::fd::{AsFd, AsRawFd, BorrowedFd, OwnedFd, RawFd}, ptr, slice, sync::atomic::{self, AtomicPtr, Ordering}, }; use bytes::BytesMut; -use libc::{c_int, close, munmap, MAP_FAILED, MAP_SHARED, PROT_READ, PROT_WRITE}; +use libc::{c_int, munmap, MAP_FAILED, MAP_SHARED, PROT_READ, PROT_WRITE}; use thiserror::Error; use crate::{ @@ -15,7 +15,7 @@ use crate::{ perf_event_header, perf_event_mmap_page, perf_event_type::{PERF_RECORD_LOST, PERF_RECORD_SAMPLE}, }, - sys::{perf_event_ioctl, perf_event_open_bpf}, + sys::{perf_event_ioctl, perf_event_open_bpf, SysResult}, PERF_EVENT_IOC_DISABLE, PERF_EVENT_IOC_ENABLE, }; @@ -88,7 +88,7 @@ pub(crate) struct PerfBuffer { buf: AtomicPtr, size: usize, page_size: usize, - fd: RawFd, + fd: OwnedFd, } impl PerfBuffer { @@ -102,8 +102,7 @@ impl PerfBuffer { } let fd = perf_event_open_bpf(cpu_id as i32) - .map_err(|(_, io_error)| PerfBufferError::OpenError { io_error })? - as RawFd; + .map_err(|(_, io_error)| PerfBufferError::OpenError { io_error })?; let size = page_size * page_count; let buf = unsafe { mmap( @@ -111,7 +110,7 @@ impl PerfBuffer { size + page_size, PROT_READ | PROT_WRITE, MAP_SHARED, - fd, + fd.as_fd(), 0, ) }; @@ -128,7 +127,7 @@ impl PerfBuffer { page_size, }; - perf_event_ioctl(fd, PERF_EVENT_IOC_ENABLE, 0) + perf_event_ioctl(perf_buf.fd.as_fd(), PERF_EVENT_IOC_ENABLE, 0) .map_err(|(_, io_error)| PerfBufferError::PerfEventEnableError { io_error })?; Ok(perf_buf) @@ -261,19 +260,24 @@ impl PerfBuffer { impl AsRawFd for PerfBuffer { fn as_raw_fd(&self) -> RawFd { - self.fd + self.fd.as_raw_fd() + } +} + +impl AsFd for PerfBuffer { + fn as_fd(&self) -> BorrowedFd<'_> { + self.fd.as_fd() } } impl Drop for PerfBuffer { fn drop(&mut self) { unsafe { - let _ = perf_event_ioctl(self.fd, PERF_EVENT_IOC_DISABLE, 0); + let _: SysResult<_> = perf_event_ioctl(self.fd.as_fd(), PERF_EVENT_IOC_DISABLE, 0); munmap( self.buf.load(Ordering::SeqCst) as *mut c_void, self.size + self.page_size, ); - close(self.fd); } } } @@ -284,11 +288,11 @@ unsafe fn mmap( len: usize, prot: c_int, flags: c_int, - fd: i32, + fd: BorrowedFd<'_>, offset: libc::off_t, ) -> *mut c_void { #[cfg(not(test))] - return libc::mmap(addr, len, prot, flags, fd, offset); + return libc::mmap(addr, len, prot, flags, fd.as_raw_fd(), offset); #[cfg(test)] use crate::sys::TEST_MMAP_RET; diff --git a/aya/src/programs/perf_attach.rs b/aya/src/programs/perf_attach.rs index 9d5ca24e..f2c7e032 100644 --- a/aya/src/programs/perf_attach.rs +++ b/aya/src/programs/perf_attach.rs @@ -1,11 +1,10 @@ //! Perf attach links. -use libc::close; -use std::os::fd::RawFd; +use std::os::fd::{AsFd as _, AsRawFd as _, OwnedFd, RawFd}; use crate::{ generated::bpf_attach_type::BPF_PERF_EVENT, programs::{probe::detach_debug_fs, FdLink, Link, ProbeKind, ProgramError}, - sys::{bpf_link_create, perf_event_ioctl}, + sys::{bpf_link_create, perf_event_ioctl, SysResult}, FEATURES, PERF_EVENT_IOC_DISABLE, PERF_EVENT_IOC_ENABLE, PERF_EVENT_IOC_SET_BPF, }; @@ -46,7 +45,7 @@ pub struct PerfLinkId(RawFd); /// The attachment type of PerfEvent programs. #[derive(Debug)] pub struct PerfLink { - perf_fd: RawFd, + perf_fd: OwnedFd, probe_kind: Option, event_alias: Option, } @@ -55,16 +54,15 @@ impl Link for PerfLink { type Id = PerfLinkId; fn id(&self) -> Self::Id { - PerfLinkId(self.perf_fd) + PerfLinkId(self.perf_fd.as_raw_fd()) } fn detach(mut self) -> Result<(), ProgramError> { - let _ = perf_event_ioctl(self.perf_fd, PERF_EVENT_IOC_DISABLE, 0); - unsafe { close(self.perf_fd) }; + let _: SysResult<_> = perf_event_ioctl(self.perf_fd.as_fd(), PERF_EVENT_IOC_DISABLE, 0); if let Some(probe_kind) = self.probe_kind.take() { if let Some(event_alias) = self.event_alias.take() { - let _ = detach_debug_fs(probe_kind, &event_alias); + let _: Result<_, _> = detach_debug_fs(probe_kind, &event_alias); } } @@ -72,15 +70,14 @@ impl Link for PerfLink { } } -pub(crate) fn perf_attach(prog_fd: RawFd, fd: RawFd) -> Result { +pub(crate) fn perf_attach(prog_fd: RawFd, fd: OwnedFd) -> Result { if FEATURES.bpf_perf_link() { - let link_fd = - bpf_link_create(prog_fd, fd, BPF_PERF_EVENT, None, 0).map_err(|(_, io_error)| { - ProgramError::SyscallError { - call: "bpf_link_create", - io_error, - } - })? as RawFd; + let link_fd = bpf_link_create(prog_fd, fd.as_raw_fd(), BPF_PERF_EVENT, None, 0).map_err( + |(_, io_error)| ProgramError::SyscallError { + call: "bpf_link_create", + io_error, + }, + )? as RawFd; Ok(PerfLinkInner::FdLink(FdLink::new(link_fd))) } else { perf_attach_either(prog_fd, fd, None, None) @@ -89,7 +86,7 @@ pub(crate) fn perf_attach(prog_fd: RawFd, fd: RawFd) -> Result Result { @@ -98,17 +95,17 @@ pub(crate) fn perf_attach_debugfs( fn perf_attach_either( prog_fd: RawFd, - fd: RawFd, + fd: OwnedFd, probe_kind: Option, event_alias: Option, ) -> Result { - perf_event_ioctl(fd, PERF_EVENT_IOC_SET_BPF, prog_fd).map_err(|(_, io_error)| { + perf_event_ioctl(fd.as_fd(), PERF_EVENT_IOC_SET_BPF, prog_fd).map_err(|(_, io_error)| { ProgramError::SyscallError { call: "PERF_EVENT_IOC_SET_BPF", io_error, } })?; - perf_event_ioctl(fd, PERF_EVENT_IOC_ENABLE, 0).map_err(|(_, io_error)| { + perf_event_ioctl(fd.as_fd(), PERF_EVENT_IOC_ENABLE, 0).map_err(|(_, io_error)| { ProgramError::SyscallError { call: "PERF_EVENT_IOC_ENABLE", io_error, diff --git a/aya/src/programs/perf_event.rs b/aya/src/programs/perf_event.rs index 5de7eeef..5d104499 100644 --- a/aya/src/programs/perf_event.rs +++ b/aya/src/programs/perf_event.rs @@ -168,7 +168,7 @@ impl PerfEvent { .map_err(|(_code, io_error)| ProgramError::SyscallError { call: "perf_event_open", io_error, - })? as i32; + })?; let link = perf_attach(self.data.fd_or_err()?, fd)?; self.data.links.insert(PerfEventLink::new(link)) diff --git a/aya/src/programs/probe.rs b/aya/src/programs/probe.rs index 80a5c088..38cd4d59 100644 --- a/aya/src/programs/probe.rs +++ b/aya/src/programs/probe.rs @@ -3,6 +3,7 @@ use libc::pid_t; use std::{ fs::{self, OpenOptions}, io::{self, Write}, + os::fd::OwnedFd, path::Path, process, sync::atomic::{AtomicUsize, Ordering}, @@ -86,7 +87,7 @@ fn create_as_probe( fn_name: &str, offset: u64, pid: Option, -) -> Result { +) -> Result { use ProbeKind::*; let perf_ty = match kind { @@ -108,14 +109,12 @@ fn create_as_probe( _ => None, }; - let fd = perf_event_open_probe(perf_ty, ret_bit, fn_name, offset, pid).map_err( - |(_code, io_error)| ProgramError::SyscallError { + perf_event_open_probe(perf_ty, ret_bit, fn_name, offset, pid).map_err(|(_code, io_error)| { + ProgramError::SyscallError { call: "perf_event_open", io_error, - }, - )? as i32; - - Ok(fd) + } + }) } fn create_as_trace_point( @@ -123,7 +122,7 @@ fn create_as_trace_point( name: &str, offset: u64, pid: Option, -) -> Result<(i32, String), ProgramError> { +) -> Result<(OwnedFd, String), ProgramError> { use ProbeKind::*; let tracefs = find_tracefs_path()?; @@ -142,7 +141,7 @@ fn create_as_trace_point( call: "perf_event_open", io_error, } - })? as i32; + })?; Ok((fd, event_alias)) } diff --git a/aya/src/programs/trace_point.rs b/aya/src/programs/trace_point.rs index b23594b7..8e4608c1 100644 --- a/aya/src/programs/trace_point.rs +++ b/aya/src/programs/trace_point.rs @@ -82,10 +82,10 @@ impl TracePoint { let id = read_sys_fs_trace_point_id(tracefs, category, name)?; let fd = perf_event_open_trace_point(id, None).map_err(|(_code, io_error)| { ProgramError::SyscallError { - call: "perf_event_open", + call: "perf_event_open_trace_point", io_error, } - })? as i32; + })?; let link = perf_attach(self.data.fd_or_err()?, fd)?; self.data.links.insert(TracePointLink::new(link)) diff --git a/aya/src/sys/mod.rs b/aya/src/sys/mod.rs index 71fd3216..01e33c66 100644 --- a/aya/src/sys/mod.rs +++ b/aya/src/sys/mod.rs @@ -5,7 +5,10 @@ mod perf_event; #[cfg(test)] mod fake; -use std::{io, mem}; +use std::{ + io, mem, + os::fd::{AsRawFd as _, BorrowedFd}, +}; use libc::{c_int, c_long, pid_t, SYS_bpf, SYS_perf_event_open}; @@ -34,7 +37,7 @@ pub(crate) enum Syscall<'a> { flags: u32, }, PerfEventIoctl { - fd: c_int, + fd: BorrowedFd<'a>, request: c_int, arg: c_int, }, @@ -90,7 +93,7 @@ fn syscall(call: Syscall) -> SysResult { flags, } => libc::syscall(SYS_perf_event_open, &attr, pid, cpu, group, flags), Syscall::PerfEventIoctl { fd, request, arg } => { - libc::ioctl(fd, request.try_into().unwrap(), arg) as libc::c_long + libc::ioctl(fd.as_raw_fd(), request.try_into().unwrap(), arg) as libc::c_long } } } { diff --git a/aya/src/sys/perf_event.rs b/aya/src/sys/perf_event.rs index 586bd50a..dad86a45 100644 --- a/aya/src/sys/perf_event.rs +++ b/aya/src/sys/perf_event.rs @@ -1,6 +1,7 @@ use std::{ ffi::{c_long, CString}, - mem, + io, mem, + os::fd::{BorrowedFd, FromRawFd as _, OwnedFd}, }; use libc::{c_int, pid_t}; @@ -25,7 +26,7 @@ pub(crate) fn perf_event_open( sample_frequency: Option, wakeup: bool, flags: u32, -) -> SysResult { +) -> SysResult { let mut attr = unsafe { mem::zeroed::() }; attr.config = config; @@ -42,16 +43,10 @@ pub(crate) fn perf_event_open( attr.__bindgen_anon_1.sample_period = sample_period; } - syscall(Syscall::PerfEventOpen { - attr, - pid, - cpu, - group: -1, - flags, - }) + perf_event_sys(attr, pid, cpu, flags) } -pub(crate) fn perf_event_open_bpf(cpu: c_int) -> SysResult { +pub(crate) fn perf_event_open_bpf(cpu: c_int) -> SysResult { perf_event_open( PERF_TYPE_SOFTWARE as u32, PERF_COUNT_SW_BPF_OUTPUT as u64, @@ -70,7 +65,7 @@ pub(crate) fn perf_event_open_probe( name: &str, offset: u64, pid: Option, -) -> SysResult { +) -> SysResult { let mut attr = unsafe { mem::zeroed::() }; if let Some(ret_bit) = ret_bit { @@ -87,16 +82,10 @@ pub(crate) fn perf_event_open_probe( let cpu = if pid.is_some() { -1 } else { 0 }; let pid = pid.unwrap_or(-1); - syscall(Syscall::PerfEventOpen { - attr, - pid, - cpu, - group: -1, - flags: PERF_FLAG_FD_CLOEXEC, - }) + perf_event_sys(attr, pid, cpu, PERF_FLAG_FD_CLOEXEC) } -pub(crate) fn perf_event_open_trace_point(id: u32, pid: Option) -> SysResult { +pub(crate) fn perf_event_open_trace_point(id: u32, pid: Option) -> SysResult { let mut attr = unsafe { mem::zeroed::() }; attr.size = mem::size_of::() as u32; @@ -106,16 +95,14 @@ pub(crate) fn perf_event_open_trace_point(id: u32, pid: Option) -> SysRes let cpu = if pid.is_some() { -1 } else { 0 }; let pid = pid.unwrap_or(-1); - syscall(Syscall::PerfEventOpen { - attr, - pid, - cpu, - group: -1, - flags: PERF_FLAG_FD_CLOEXEC, - }) + perf_event_sys(attr, pid, cpu, PERF_FLAG_FD_CLOEXEC) } -pub(crate) fn perf_event_ioctl(fd: c_int, request: c_int, arg: c_int) -> SysResult { +pub(crate) fn perf_event_ioctl( + fd: BorrowedFd<'_>, + request: c_int, + arg: c_int, +) -> SysResult { let call = Syscall::PerfEventIoctl { fd, request, arg }; #[cfg(not(test))] return syscall(call); @@ -124,6 +111,29 @@ pub(crate) fn perf_event_ioctl(fd: c_int, request: c_int, arg: c_int) -> SysResu return crate::sys::TEST_SYSCALL.with(|test_impl| unsafe { test_impl.borrow()(call) }); } +fn perf_event_sys(attr: perf_event_attr, pid: pid_t, cpu: i32, flags: u32) -> SysResult { + let fd = syscall(Syscall::PerfEventOpen { + attr, + pid, + cpu, + group: -1, + flags, + })?; + + let fd = fd.try_into().map_err(|_| { + ( + fd, + io::Error::new( + io::ErrorKind::InvalidData, + format!("perf_event_open: invalid fd returned: {fd}"), + ), + ) + })?; + + // SAFETY: perf_event_open returns a new file descriptor on success. + unsafe { Ok(OwnedFd::from_raw_fd(fd)) } +} + /* impl TryFrom for perf_event_type { PERF_RECORD_MMAP = 1,