From 204d02022a94dab441029855e5d39d5143444204 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Tue, 29 Aug 2023 17:52:58 -0400 Subject: [PATCH] programs: ProgAttachLink and LircLink hold owned FDs Updates #612. --- aya/src/programs/links.rs | 33 +++++++++++++------------- aya/src/programs/lirc_mode2.rs | 42 +++++++++++++--------------------- aya/src/sys/bpf.rs | 28 ++++++++++++++++++----- 3 files changed, 55 insertions(+), 48 deletions(-) diff --git a/aya/src/programs/links.rs b/aya/src/programs/links.rs index 619ebef6..d06a62e4 100644 --- a/aya/src/programs/links.rs +++ b/aya/src/programs/links.rs @@ -12,10 +12,8 @@ use std::{ use crate::{ generated::bpf_attach_type, pin::PinError, - programs::ProgramError, - sys::{ - bpf_get_object, bpf_pin_object, bpf_prog_attach, bpf_prog_detach, SysResult, SyscallError, - }, + programs::{ProgramError, ProgramFd}, + sys::{bpf_get_object, bpf_pin_object, bpf_prog_attach, bpf_prog_detach, SyscallError}, }; /// A Link. @@ -234,7 +232,7 @@ pub struct ProgAttachLinkId(RawFd, RawFd, bpf_attach_type); /// The Link type used by programs that are attached with `bpf_prog_attach`. #[derive(Debug)] pub struct ProgAttachLink { - prog_fd: RawFd, + prog_fd: ProgramFd, target_fd: OwnedFd, attach_type: bpf_attach_type, } @@ -249,15 +247,11 @@ impl ProgAttachLink { // going to need a duplicate whose lifetime we manage. Let's // duplicate it prior to attaching it so the new file // descriptor is closed at drop in case it fails to attach. + let prog_fd = prog_fd.try_clone_to_owned()?; let target_fd = target_fd.try_clone_to_owned()?; - bpf_prog_attach(prog_fd, target_fd.as_fd(), attach_type).map_err(|(_, io_error)| { - SyscallError { - call: "bpf_prog_attach", - io_error, - } - })?; + bpf_prog_attach(prog_fd.as_fd(), target_fd.as_fd(), attach_type)?; - let prog_fd = prog_fd.as_raw_fd(); + let prog_fd = ProgramFd(prog_fd); Ok(Self { prog_fd, target_fd, @@ -270,13 +264,20 @@ impl Link for ProgAttachLink { type Id = ProgAttachLinkId; fn id(&self) -> Self::Id { - ProgAttachLinkId(self.prog_fd, self.target_fd.as_raw_fd(), self.attach_type) + ProgAttachLinkId( + self.prog_fd.as_fd().as_raw_fd(), + self.target_fd.as_raw_fd(), + self.attach_type, + ) } fn detach(self) -> Result<(), ProgramError> { - let _: SysResult<_> = - bpf_prog_detach(self.prog_fd, self.target_fd.as_fd(), self.attach_type); - Ok(()) + bpf_prog_detach( + self.prog_fd.as_fd(), + self.target_fd.as_fd(), + self.attach_type, + ) + .map_err(Into::into) } } diff --git a/aya/src/programs/lirc_mode2.rs b/aya/src/programs/lirc_mode2.rs index 7b0d886f..6e62ce60 100644 --- a/aya/src/programs/lirc_mode2.rs +++ b/aya/src/programs/lirc_mode2.rs @@ -1,10 +1,10 @@ //! Lirc programs. -use std::os::fd::{AsFd, AsRawFd, BorrowedFd, IntoRawFd as _, OwnedFd, RawFd}; +use std::os::fd::{AsFd, AsRawFd as _, OwnedFd, RawFd}; use crate::{ generated::{bpf_attach_type::BPF_LIRC_MODE2, bpf_prog_type::BPF_PROG_TYPE_LIRC_MODE2}, - programs::{load_program, query, Link, ProgramData, ProgramError, ProgramInfo}, - sys::{bpf_prog_attach, bpf_prog_detach, bpf_prog_get_fd_by_id, SysResult, SyscallError}, + programs::{load_program, query, Link, ProgramData, ProgramError, ProgramFd, ProgramInfo}, + sys::{bpf_prog_attach, bpf_prog_detach, bpf_prog_get_fd_by_id}, }; /// A program used to decode IR into key events for a lirc device. @@ -60,20 +60,15 @@ impl LircMode2 { /// The returned value can be used to detach, see [LircMode2::detach]. pub fn attach(&mut self, lircdev: T) -> Result { let prog_fd = self.fd()?; - let prog_fd = prog_fd.as_fd(); // The link is going to own this new file descriptor so we are // going to need a duplicate whose lifetime we manage. Let's // duplicate it prior to attaching it so the new file // descriptor is closed at drop in case it fails to attach. + let prog_fd = prog_fd.try_clone()?; let lircdev_fd = lircdev.as_fd().try_clone_to_owned()?; - bpf_prog_attach(prog_fd, lircdev_fd.as_fd(), BPF_LIRC_MODE2).map_err(|(_, io_error)| { - SyscallError { - call: "bpf_prog_attach", - io_error, - } - })?; + bpf_prog_attach(prog_fd.as_fd(), lircdev_fd.as_fd(), BPF_LIRC_MODE2)?; self.data.links.insert(LircLink::new(prog_fd, lircdev_fd)) } @@ -103,13 +98,7 @@ impl LircMode2 { .map(|prog_id| { let prog_fd = bpf_prog_get_fd_by_id(prog_id)?; let target_fd = target_fd.try_clone_to_owned()?; - // SAFETY: The file descriptor will stay valid because - // we are leaking it. We cannot use `OwnedFd` in here - // because LircMode2::attach also uses LircLink::new - // but with a borrowed file descriptor (of the loaded - // program) without duplicating. TODO(#612): Fix API - // or internals so this file descriptor isn't leaked - let prog_fd = unsafe { BorrowedFd::borrow_raw(prog_fd.into_raw_fd()) }; + let prog_fd = ProgramFd(prog_fd); Ok(LircLink::new(prog_fd, target_fd)) }) .collect() @@ -123,21 +112,22 @@ pub struct LircLinkId(RawFd, RawFd); #[derive(Debug)] /// An LircMode2 Link pub struct LircLink { - prog_fd: RawFd, + prog_fd: ProgramFd, target_fd: OwnedFd, } impl LircLink { - pub(crate) fn new(prog_fd: BorrowedFd<'_>, target_fd: OwnedFd) -> Self { - let prog_fd = prog_fd.as_raw_fd(); + pub(crate) fn new(prog_fd: ProgramFd, target_fd: OwnedFd) -> Self { Self { prog_fd, target_fd } } /// Get ProgramInfo from this link pub fn info(&self) -> Result { - // SAFETY: TODO(https://github.com/aya-rs/aya/issues/612): make this safe by not holding `RawFd`s. - let prog_fd = unsafe { BorrowedFd::borrow_raw(self.prog_fd) }; - ProgramInfo::new_from_fd(prog_fd) + let Self { + prog_fd, + target_fd: _, + } = self; + ProgramInfo::new_from_fd(prog_fd.as_fd()) } } @@ -145,11 +135,11 @@ impl Link for LircLink { type Id = LircLinkId; fn id(&self) -> Self::Id { - LircLinkId(self.prog_fd, self.target_fd.as_raw_fd()) + LircLinkId(self.prog_fd.as_fd().as_raw_fd(), self.target_fd.as_raw_fd()) } fn detach(self) -> Result<(), ProgramError> { - let _: SysResult<_> = bpf_prog_detach(self.prog_fd, self.target_fd.as_fd(), BPF_LIRC_MODE2); - Ok(()) + bpf_prog_detach(self.prog_fd.as_fd(), self.target_fd.as_fd(), BPF_LIRC_MODE2) + .map_err(Into::into) } } diff --git a/aya/src/sys/bpf.rs b/aya/src/sys/bpf.rs index 41f6fd6f..801ea48d 100644 --- a/aya/src/sys/bpf.rs +++ b/aya/src/sys/bpf.rs @@ -425,28 +425,44 @@ pub(crate) fn bpf_prog_attach( prog_fd: BorrowedFd<'_>, target_fd: BorrowedFd<'_>, attach_type: bpf_attach_type, -) -> SysResult { +) -> Result<(), SyscallError> { let mut attr = unsafe { mem::zeroed::() }; attr.__bindgen_anon_5.attach_bpf_fd = prog_fd.as_raw_fd() as u32; attr.__bindgen_anon_5.target_fd = target_fd.as_raw_fd() as u32; attr.__bindgen_anon_5.attach_type = attach_type as u32; - sys_bpf(bpf_cmd::BPF_PROG_ATTACH, &mut attr) + let ret = sys_bpf(bpf_cmd::BPF_PROG_ATTACH, &mut attr).map_err(|(code, io_error)| { + assert_eq!(code, -1); + SyscallError { + call: "bpf_prog_attach", + io_error, + } + })?; + assert_eq!(ret, 0); + Ok(()) } pub(crate) fn bpf_prog_detach( - prog_fd: RawFd, + prog_fd: BorrowedFd<'_>, target_fd: BorrowedFd<'_>, attach_type: bpf_attach_type, -) -> SysResult { +) -> Result<(), SyscallError> { let mut attr = unsafe { mem::zeroed::() }; - attr.__bindgen_anon_5.attach_bpf_fd = prog_fd as u32; + attr.__bindgen_anon_5.attach_bpf_fd = prog_fd.as_raw_fd() as u32; attr.__bindgen_anon_5.target_fd = target_fd.as_raw_fd() as u32; attr.__bindgen_anon_5.attach_type = attach_type as u32; - sys_bpf(bpf_cmd::BPF_PROG_DETACH, &mut attr) + let ret = sys_bpf(bpf_cmd::BPF_PROG_DETACH, &mut attr).map_err(|(code, io_error)| { + assert_eq!(code, -1); + SyscallError { + call: "bpf_prog_detach", + io_error, + } + })?; + assert_eq!(ret, 0); + Ok(()) } pub(crate) fn bpf_prog_query(