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..2c77a964 100644 --- a/aya/src/programs/mod.rs +++ b/aya/src/programs/mod.rs @@ -217,6 +217,15 @@ pub enum ProgramError { /// An error occurred while working with IO. #[error(transparent)] IOError(#[from] io::Error), + + /// An error occurred when attmpting to convert a type. + #[error("conversion error from {from} to {to}")] + ConversionError { + /// The type being converted from. + from: String, + /// The type being converted to. + to: String, + }, } /// A [`Program`] file descriptor. @@ -240,13 +249,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,15 +821,14 @@ 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 /// /// The minimum kernel version required to use this feature is 6.6.0. pub trait MultiProgProgram { - /// Returns a borrowed reference to the file descriptor of a given - /// [`Program`] which has support for the kernel's generic multi-prog API. + /// Borrows the file descriptor. fn fd(&self) -> Result, ProgramError>; } @@ -827,7 +836,6 @@ macro_rules! impl_multiprog_fd { ($($struct_name:ident),+ $(,)?) => { $( impl MultiProgProgram for $struct_name { - /// Returns the a borrowed reference file descriptor of this Program. fn fd(&self) -> Result, ProgramError> { Ok(self.fd()?.as_fd()) } @@ -838,14 +846,14 @@ 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 /// /// The minimum kernel version required to use this feature is 6.6.0. pub trait MultiProgLink { - /// Returns a borrowed reference to the file descriptor of a given - /// [`Link`] which has support for the kernel's generic multi-prog API. + /// Borrows the file descriptor. fn fd(&self) -> Result, LinkError>; } @@ -853,7 +861,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/sock_ops.rs b/aya/src/programs/sock_ops.rs index 60f4e08f..0d6d365c 100644 --- a/aya/src/programs/sock_ops.rs +++ b/aya/src/programs/sock_ops.rs @@ -75,6 +75,8 @@ impl SockOps { attach_type, None, mode.into(), + None, + None, ) .map_err(|(_, io_error)| SyscallError { call: "bpf_link_create", diff --git a/aya/src/programs/tc.rs b/aya/src/programs/tc.rs index e6a0a038..1f397789 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; @@ -97,12 +96,12 @@ pub enum TcError { /// the clsact qdisc is already attached #[error("the clsact qdisc is already attached")] AlreadyAttached, - /// tcx links can only be attached to ingress or egress + /// tcx links can only be attached to ingress or egress, custom attachment is not supported #[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, + /// operation not supported for programs loaded via tcx + #[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(tcx_attach_type) => Err(TcError::InvalidTcxAttach(*tcx_attach_type)), } } } @@ -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. @@ -157,7 +156,7 @@ impl SchedClassifier { /// Attaches the program to the given `interface`. /// /// On kernels >= 6.6.0, it will attempt to use the TCX interface and attach as - /// the first program. On older kernels, it will fallback to using the + /// the last TCX program. On older kernels, it will fallback to using the /// legacy netlink interface. /// /// For finer grained control over link ordering use [`SchedClassifier::attach_with_options`]. @@ -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, @@ -272,7 +269,12 @@ impl SchedClassifier { let name = CString::new(name).unwrap(); let (priority, handle) = unsafe { netlink_qdisc_attach( - if_index as i32, + if_index + .try_into() + .map_err(|_| ProgramError::ConversionError { + from: "u32".to_string(), + to: "i32".to_string(), + })?, &attach_type, prog_fd, &name, @@ -296,7 +298,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), @@ -365,15 +367,15 @@ impl Link for NlLink { } fn detach(self) -> Result<(), ProgramError> { - unsafe { - netlink_qdisc_detach( - self.if_index as i32, - &self.attach_type, - self.priority, - self.handle, - ) - } - .map_err(|io_error| TcError::NetlinkError { io_error })?; + let if_index: i32 = + self.if_index + .try_into() + .map_err(|_| ProgramError::ConversionError { + from: "u32".to_string(), + to: "i32".to_string(), + })?; + unsafe { netlink_qdisc_detach(if_index, &self.attach_type, self.priority, self.handle) } + .map_err(|io_error| TcError::NetlinkError { io_error })?; Ok(()) } } @@ -436,7 +438,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 +507,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 +516,7 @@ impl SchedClassifierLink { if let TcLinkInner::NlLink(n) = self.inner() { Ok(n.priority) } else { - Err(TcError::InvalidLink.into()) + Err(TcError::InvalidLinkOperation.into()) } } @@ -524,7 +525,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. diff --git a/test/integration-test/Cargo.toml b/test/integration-test/Cargo.toml index 5e0d81c2..dc9c7667 100644 --- a/test/integration-test/Cargo.toml +++ b/test/integration-test/Cargo.toml @@ -27,6 +27,7 @@ test-case = { workspace = true } test-log = { workspace = true, features = ["log"] } tokio = { workspace = true, features = ["macros", "rt-multi-thread", "time"] } xdpilone = { workspace = true } + [build-dependencies] cargo_metadata = { workspace = true } # TODO(https://github.com/rust-lang/cargo/issues/12375): this should be an artifact dependency, but diff --git a/test/integration-test/src/tests/tcx.rs b/test/integration-test/src/tests/tcx.rs index 6e270a42..bf3c0537 100644 --- a/test/integration-test/src/tests/tcx.rs +++ b/test/integration-test/src/tests/tcx.rs @@ -104,22 +104,17 @@ async fn tcx_ordering() { .unwrap(); prog3.load().unwrap(); - // Test LinkOrder::first() and LinkOrder::set_expected_revision() + // Test LinkOrder::last() with correct expected_revision let mut order: LinkOrder = LinkOrder::last(); - order.set_expected_revision(u64::MAX); - let options = TcAttachOptions::TcxOrder(order); - let result = prog0.attach_with_options("lo", TcAttachType::Ingress, options); - assert!(result.is_err()); - - let mut order: LinkOrder = LinkOrder::last(); - order.set_expected_revision(0); + order.set_expected_revision(1); let options = TcAttachOptions::TcxOrder(order); prog0 .attach_with_options("lo", TcAttachType::Ingress, options) .unwrap(); - // Test LinkOrder::after_program() - let order = LinkOrder::after_program(prog0).unwrap(); + // Test LinkOrder::after_program() with correct expected_revision + let mut order = LinkOrder::after_program(prog0).unwrap(); + order.set_expected_revision(2); let options = TcAttachOptions::TcxOrder(order); let prog1_link_id = prog1 .attach_with_options("lo", TcAttachType::Ingress, options) @@ -127,19 +122,31 @@ async fn tcx_ordering() { let prog1_link = prog1.take_link(prog1_link_id).unwrap(); - // Test LinkOrder::after_link() - let order = LinkOrder::after_link(&prog1_link).unwrap(); + // Test incorrect expected_revision and expect an error + let mut order = LinkOrder::after_link(&prog1_link).unwrap(); + order.set_expected_revision(7); + let options = TcAttachOptions::TcxOrder(order); + let result = prog2.attach_with_options("lo", TcAttachType::Ingress, options); + assert!(result.is_err()); + + // Test LinkOrder::after_link() again with expected_revision == 0 which + // means the expected_revision should be ignored. + let mut order = LinkOrder::after_link(&prog1_link).unwrap(); + order.set_expected_revision(0); let options = TcAttachOptions::TcxOrder(order); prog2 .attach_with_options("lo", TcAttachType::Ingress, options) .unwrap(); - // Test LinkOrder::last() + // Test LinkOrder::last() with no expected_revision let options = TcAttachOptions::TcxOrder(LinkOrder::last()); prog3 .attach_with_options("lo", TcAttachType::Ingress, options) .unwrap(); + // Wait for system to stabilize + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + const PAYLOAD: &str = "hello tcx"; let sock = UdpSocket::bind("127.0.0.1:1778").unwrap();