From 1a83204397650511241ad285f7001d76432e852e Mon Sep 17 00:00:00 2001 From: Andre Fredette Date: Wed, 11 Sep 2024 10:21:01 -0400 Subject: [PATCH] Address review comments by alessandrod and tamird. Signed-off-by: Andre Fredette --- aya/src/programs/links.rs | 32 ++++++++++++++++---------------- aya/src/programs/mod.rs | 17 +++++++++-------- aya/src/programs/tc.rs | 28 ++++++++++++---------------- aya/src/sys/bpf.rs | 2 +- 4 files changed, 38 insertions(+), 41 deletions(-) diff --git a/aya/src/programs/links.rs b/aya/src/programs/links.rs index 3fba9398..2cb4c7d5 100644 --- a/aya/src/programs/links.rs +++ b/aya/src/programs/links.rs @@ -397,13 +397,14 @@ pub enum LinkError { SyscallError(#[from] SyscallError), } -/// A [`Link`] identifier, which may or may not be owned by aya. +/// A [`Link`] identifier. pub struct LinkId(u32); impl LinkId { - /// A wrapper for an arbitrary loaded link specified by its kernel id, - /// unsafe because there is no guarantee provided by aya that the link is - /// still loaded or even exists. + /// Create a new [`LinkId`] from its kernel id. + /// + /// This method is unsafe since it doesn't check that the given `id` is a an + /// existing link id. pub unsafe fn new(id: u32) -> Self { Self(id) } @@ -427,8 +428,7 @@ bitflags::bitflags! { } } -/// Struct defining the arguments required for interacting with the kernel's -/// multi-prog API. +/// Arguments required for interacting with the kernel's multi-prog API. /// /// # Minimum kernel version /// @@ -466,7 +466,7 @@ impl Default for LinkOrder { } impl LinkOrder { - /// Ensure the link is created before all other links at a given attachment point. + /// Attach before all other links. pub fn first() -> Self { Self { link_ref: LinkRef::Id(0), @@ -475,7 +475,7 @@ impl LinkOrder { } } - /// Ensure the link is created after all other links for a given attachment point. + /// Attach after all other links. pub fn last() -> Self { Self { link_ref: LinkRef::Id(0), @@ -484,7 +484,7 @@ impl LinkOrder { } } - /// Ensure the link is created before the specified aya-owned link for a given attachment point. + /// Attach before the given link. pub fn before_link(link: &L) -> Result { Ok(Self { link_ref: LinkRef::Fd(link.fd()?.as_raw_fd()), @@ -493,7 +493,7 @@ impl LinkOrder { }) } - /// Ensure the link is created after the specified aya-owned link for a given attachment point. + /// Attach after the given link. pub fn after_link(link: &L) -> Result { Ok(Self { link_ref: LinkRef::Fd(link.fd()?.as_raw_fd()), @@ -502,7 +502,7 @@ impl LinkOrder { }) } - /// Ensure the link is created before a link specified by its kernel id for a given attachment point. + /// Attach before the link with the given id. pub fn before_link_id(id: LinkId) -> Result { Ok(Self { link_ref: LinkRef::Id(id.0), @@ -511,7 +511,7 @@ impl LinkOrder { }) } - /// Ensure the link is created after a link specified by its kernel id for a given attachment point. + /// Attach after the link with the given id. pub fn after_link_id(id: LinkId) -> Result { Ok(Self { link_ref: LinkRef::Id(id.0), @@ -520,7 +520,7 @@ impl LinkOrder { }) } - /// Ensure the link is created before the specified aya-owned program for a given attachment point. + /// Attach before the given program. pub fn before_program(program: &P) -> Result { Ok(Self { link_ref: LinkRef::Fd(program.fd()?.as_raw_fd()), @@ -529,7 +529,7 @@ impl LinkOrder { }) } - /// Ensure the link is created after the specified aya-owned program for a given attachment point. + /// Attach after the given program. pub fn after_program(program: &P) -> Result { Ok(Self { link_ref: LinkRef::Fd(program.fd()?.as_raw_fd()), @@ -538,7 +538,7 @@ impl LinkOrder { }) } - /// Ensure the link is created before a program specified by its kernel id for a given attachment point. + /// Attach before the program with the given id. pub fn before_program_id(id: ProgramId) -> Self { Self { link_ref: LinkRef::Id(id.0), @@ -547,7 +547,7 @@ impl LinkOrder { } } - /// Ensure the link is created after a program specified by its kernel id for a given attachment point. + /// Attach after the program with the given id. pub fn after_program_id(id: ProgramId) -> Self { Self { link_ref: LinkRef::Id(id.0), diff --git a/aya/src/programs/mod.rs b/aya/src/programs/mod.rs index 34ffe52f..0fd37392 100644 --- a/aya/src/programs/mod.rs +++ b/aya/src/programs/mod.rs @@ -240,13 +240,14 @@ impl AsFd for ProgramFd { } /// The various eBPF programs. -/// A [`Program`] identifier, which may or may not be owned by aya. +/// A [`Program`] identifier. pub struct ProgramId(u32); impl ProgramId { - /// A wrapper for an arbitrary loaded program specified by its kernel id, - /// unsafe because there is no guarantee provided by aya that the program is - /// still loaded or even exists. + /// Create a new program id. + /// + /// This method is unsafe since it doesn't check that the given `id` is a + /// valid program id. pub unsafe fn new(id: u32) -> Self { Self(id) } @@ -811,8 +812,8 @@ impl_fd!( CgroupDevice, ); -/// Defines which [`Program`]s support the kernel's -/// generic multi-prog API. +/// Defines the [`Program`] types which support the kernel's +/// [generic multi-prog API](https://github.com/torvalds/linux/commit/053c8e1f235dc3f69d13375b32f4209228e1cb96). /// /// # Minimum kernel version /// @@ -838,7 +839,8 @@ macro_rules! impl_multiprog_fd { impl_multiprog_fd!(SchedClassifier); -/// Defines the [Link] types which support the kernel's [generic multi-prog API](https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=053c8e1f235dc3f69d13375b32f4209228e1cb96). +/// Defines the [`Link`] types which support the kernel's +/// [generic multi-prog API](https://github.com/torvalds/linux/commit/053c8e1f235dc3f69d13375b32f4209228e1cb96). /// /// # Minimum kernel version /// @@ -853,7 +855,6 @@ macro_rules! impl_multiproglink_fd { ($($struct_name:ident),+ $(,)?) => { $( impl MultiProgLink for $struct_name { - /// Returns the a borrowed reference file descriptor of this Program. fn fd(&self) -> Result, LinkError> { let link: &FdLink = self.try_into()?; Ok(link.fd.as_fd()) diff --git a/aya/src/programs/tc.rs b/aya/src/programs/tc.rs index e6a0a038..8ff93b76 100644 --- a/aya/src/programs/tc.rs +++ b/aya/src/programs/tc.rs @@ -6,7 +6,6 @@ use std::{ path::Path, }; -use log::debug; use thiserror::Error; use super::FdLink; @@ -101,8 +100,8 @@ pub enum TcError { #[error("tcx links can only be attached to ingress or egress, custom attachment: {0} is not supported")] InvalidTcxAttach(u32), /// program was loaded via tcx not netlink - #[error("program was loaded via tcx not netlink")] - InvalidLink, + #[error("operation not supported for programs loaded via tcx")] + InvalidLinkOperation, } impl TcAttachType { @@ -114,11 +113,11 @@ impl TcAttachType { } } - pub(crate) fn tcx_attach(&self) -> Result { + pub(crate) fn tcx_attach_type(&self) -> Result { match self { Self::Ingress => Ok(BPF_TCX_INGRESS), Self::Egress => Ok(BPF_TCX_EGRESS), - Self::Custom(i) => Err(TcError::InvalidTcxAttach(i.to_owned())), + Self::Custom(i) => Err(TcError::InvalidTcxAttach(*i)), } } } @@ -127,8 +126,8 @@ impl TcAttachType { /// /// The options vary based on what is supported by the current kernel. Kernels /// older than 6.6.0 must utilize netlink for attachments, while newer kernels -/// can utilize the modern TCX eBPF link type which support's the kernel's -/// multi-prog api. +/// can utilize the modern TCX eBPF link type which supports the kernel's +/// multi-prog API. #[derive(Debug)] pub enum TcAttachOptions { /// Netlink attach options. @@ -137,7 +136,7 @@ pub enum TcAttachOptions { TcxOrder(LinkOrder), } -/// Options for SchedClassifier attach via netlink +/// Options for SchedClassifier attach via netlink. #[derive(Debug, Default, Hash, Eq, PartialEq)] pub struct NlOptions { /// Priority assigned to tc program with lower number = higher priority. @@ -167,7 +166,7 @@ impl SchedClassifier { /// # Errors /// /// When attaching fails, [`ProgramError::SyscallError`] is returned for - /// kernels `>= 6.6.0`, and instead [`TcError::NetlinkError`] is returned for + /// kernels `>= 6.6.0`, and [`TcError::NetlinkError`] is returned for /// older kernels. A common cause of netlink attachment failure is not having added /// the `clsact` qdisc to the given interface, see [`qdisc_add_clsact`] /// @@ -177,14 +176,12 @@ impl SchedClassifier { attach_type: TcAttachType, ) -> Result { if KernelVersion::current().unwrap() >= KernelVersion::new(6, 6, 0) { - debug!("attaching schedClassifier program via tcx link API"); self.attach_with_options( interface, attach_type, TcAttachOptions::TcxOrder(LinkOrder::default()), ) } else { - debug!("attaching SchedClassifier program via netlink API"); self.attach_with_options( interface, attach_type, @@ -296,7 +293,7 @@ impl SchedClassifier { let link_fd = bpf_link_create( prog_fd, LinkTarget::IfIndex(if_index), - attach_type.tcx_attach()?, + attach_type.tcx_attach_type()?, None, options.flags.bits(), Some(&options.link_ref), @@ -436,7 +433,6 @@ impl TryFrom for SchedClassifierLink { type Error = LinkError; fn try_from(fd_link: FdLink) -> Result { - // unwrap of fd_link.fd will not panic since it's only None when being dropped. let info = bpf_link_get_info_by_fd(fd_link.fd.as_fd())?; if info.type_ == (bpf_link_type::BPF_LINK_TYPE_TCX as u32) { return Ok(Self::new(TcLinkInner::FdLink(fd_link))); @@ -506,7 +502,7 @@ impl SchedClassifierLink { if let TcLinkInner::NlLink(n) = self.inner() { Ok(n.attach_type) } else { - Err(TcError::InvalidLink.into()) + Err(TcError::InvalidLinkOperation.into()) } } @@ -515,7 +511,7 @@ impl SchedClassifierLink { if let TcLinkInner::NlLink(n) = self.inner() { Ok(n.priority) } else { - Err(TcError::InvalidLink.into()) + Err(TcError::InvalidLinkOperation.into()) } } @@ -524,7 +520,7 @@ impl SchedClassifierLink { if let TcLinkInner::NlLink(n) = self.inner() { Ok(n.handle) } else { - Err(TcError::InvalidLink.into()) + Err(TcError::InvalidLinkOperation.into()) } } } diff --git a/aya/src/sys/bpf.rs b/aya/src/sys/bpf.rs index 58cd6afb..08129417 100644 --- a/aya/src/sys/bpf.rs +++ b/aya/src/sys/bpf.rs @@ -429,7 +429,7 @@ pub(crate) fn bpf_link_create( .__bindgen_anon_1 .relative_id = id.to_owned(); } - _ => {} + None => {} }; // SAFETY: BPF_LINK_CREATE returns a new file descriptor.