Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aya: fix tc name limit #728

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions aya/src/programs/tc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ use crate::{
},
programs::{define_link_wrapper, load_program, Link, ProgramData, ProgramError},
sys::{
netlink_find_filter_with_name, netlink_qdisc_add_clsact, netlink_qdisc_attach,
netlink_qdisc_detach,
netlink_clsact_qdisc_exists, netlink_find_filter_with_name, netlink_qdisc_add_clsact,
netlink_qdisc_attach, netlink_qdisc_detach,
},
util::{ifindex_from_ifname, tc_handler_make},
VerifierLogLevel,
Expand Down Expand Up @@ -358,3 +358,9 @@ fn qdisc_detach_program_fast(

Ok(())
}

/// Check if the `clasct` qdisc exists on the given interface.
pub fn clsact_qdisc_exists(if_name: &str) -> Result<bool, io::Error> {
let if_index = ifindex_from_ifname(if_name)?;
unsafe { netlink_clsact_qdisc_exists(if_index as i32) }
}
52 changes: 49 additions & 3 deletions aya/src/sys/netlink.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ use libc::{
getsockname, nlattr, nlmsgerr, nlmsghdr, recv, send, setsockopt, sockaddr_nl, socket,
AF_NETLINK, AF_UNSPEC, ETH_P_ALL, IFF_UP, IFLA_XDP, NETLINK_EXT_ACK, NETLINK_ROUTE,
NLA_ALIGNTO, NLA_F_NESTED, NLA_TYPE_MASK, NLMSG_DONE, NLMSG_ERROR, NLM_F_ACK, NLM_F_CREATE,
NLM_F_DUMP, NLM_F_ECHO, NLM_F_EXCL, NLM_F_MULTI, NLM_F_REQUEST, RTM_DELTFILTER, RTM_GETTFILTER,
RTM_NEWQDISC, RTM_NEWTFILTER, RTM_SETLINK, SOCK_RAW, SOL_NETLINK,
NLM_F_DUMP, NLM_F_ECHO, NLM_F_EXCL, NLM_F_MULTI, NLM_F_REQUEST, RTM_DELTFILTER, RTM_GETQDISC,
RTM_GETTFILTER, RTM_NEWQDISC, RTM_NEWTFILTER, RTM_SETLINK, SOCK_RAW, SOL_NETLINK,
};
use thiserror::Error;

Expand Down Expand Up @@ -80,6 +80,46 @@ pub(crate) unsafe fn netlink_set_xdp_fd(
Ok(())
}

pub(crate) unsafe fn netlink_clsact_qdisc_exists(if_index: i32) -> Result<bool, io::Error> {
let sock = NetlinkSocket::open()?;

let mut req = mem::zeroed::<TcRequest>();

let nlmsg_len = mem::size_of::<nlmsghdr>() + mem::size_of::<tcmsg>();

req.header = nlmsghdr {
nlmsg_len: nlmsg_len as u32,
nlmsg_flags: (NLM_F_REQUEST | NLM_F_ACK | NLM_F_DUMP) as u16,
nlmsg_type: RTM_GETQDISC,
nlmsg_pid: 0,
nlmsg_seq: 1,
};
req.tc_info.tcm_family = AF_UNSPEC as u8;

sock.send(&bytes_of(&req)[..req.header.nlmsg_len as usize])?;

for msg in sock.recv()? {
if msg.header.nlmsg_type != RTM_NEWQDISC {
continue;
}

let tc_msg = ptr::read_unaligned(msg.data.as_ptr() as *const tcmsg);
if tc_msg.tcm_ifindex != if_index {
continue;
}

let attrs = parse_attrs(&msg.data[mem::size_of::<tcmsg>()..])?;

if let Some(opts) = attrs.get(&(TCA_KIND as u16)) {
if opts.data == b"clsact\0" {
return Ok(true);
}
}
}

Ok(false)
}

pub(crate) unsafe fn netlink_qdisc_add_clsact(if_index: i32) -> Result<(), io::Error> {
let sock = NetlinkSocket::open()?;

Expand Down Expand Up @@ -289,7 +329,13 @@ struct Request {
struct TcRequest {
header: nlmsghdr,
tc_info: tcmsg,
attrs: [u8; 64],
// The buffer for attributes should be sized to hold at least 256 bytes,
// based on `CLS_BPF_NAME_LEN = 256` from the kernel:
// https://github.com/torvalds/linux/blob/02aee814/net/sched/cls_bpf.c#L28
// We currently use around ~30 bytes of attributes in addition to name.
// Rather than picking a "right sized buffer" for the payload (which is of
// varying length anyway) we use the next largest power of 2.
attrs: [u8; 512],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at least 256, but then why 512?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we currently use around ~30 bytes of attributes in addition to name.
Rather than picking a "right sized buffer" for the payload (which is of varying length anyway) we use the next largest power of 2.
I've got netlink changes planned soon, so will look at making the buffer size variable then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the comment say that please?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will update the comment.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tamird Updated the comment.

}

struct NetlinkSocket {
Expand Down
8 changes: 8 additions & 0 deletions test/integration-ebpf/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,11 @@ path = "src/xdp_sec.rs"
[[bin]]
name = "ring_buf"
path = "src/ring_buf.rs"

[[bin]]
name = "tc_name_limit"
path = "src/tc_name_limit.rs"

[[bin]]
name = "tc_name_limit_exceeded"
path = "src/tc_name_limit_exceeded.rs"
22 changes: 22 additions & 0 deletions test/integration-ebpf/src/tc_name_limit.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#![no_std]
#![no_main]

use aya_bpf::{macros::classifier, programs::TcContext};

// A function with a 256-byte-long name (all 'a's) to be used as the name of
// the ebpf program. This name must match the name passed to userspace side
// of the program (i.e. test/integration-test/src/tests/load.rs).
// 256 is the maximum length allowed by the kernel, so this test should pass.
// https://github.com/torvalds/linux/blob/02aee814/net/sched/cls_bpf.c#L28
#[classifier]
pub fn aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(
_ctx: TcContext,
) -> i32 {
0
}

#[cfg(not(test))]
#[panic_handler]
fn panic(_info: &core::panic::PanicInfo) -> ! {
loop {}
}
22 changes: 22 additions & 0 deletions test/integration-ebpf/src/tc_name_limit_exceeded.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#![no_std]
#![no_main]

use aya_bpf::{macros::classifier, programs::TcContext};

// A function with a 257-byte-long name (all 'a's) to be used as the name of
// the ebpf program. This name must match the name passed to userspace side
// of the program (i.e. test/integration-test/src/tests/load.rs).
// 256 is the maximum length allowed by the kernel, so this test should fail.
// https://github.com/torvalds/linux/blob/02aee814/net/sched/cls_bpf.c#L28
#[classifier]
pub fn aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(
_ctx: TcContext,
) -> i32 {
0
}

#[cfg(not(test))]
#[panic_handler]
fn panic(_info: &core::panic::PanicInfo) -> ! {
loop {}
}
4 changes: 4 additions & 0 deletions test/integration-test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ pub const BPF_PROBE_READ: &[u8] =
pub const REDIRECT: &[u8] = include_bytes_aligned!(concat!(env!("OUT_DIR"), "/redirect"));
pub const XDP_SEC: &[u8] = include_bytes_aligned!(concat!(env!("OUT_DIR"), "/xdp_sec"));
pub const RING_BUF: &[u8] = include_bytes_aligned!(concat!(env!("OUT_DIR"), "/ring_buf"));
pub const TC_NAME_LIMIT_TEST: &[u8] =
include_bytes_aligned!(concat!(env!("OUT_DIR"), "/tc_name_limit"));
pub const TC_NAME_LIMIT_EXCEEDED_TEST: &[u8] =
include_bytes_aligned!(concat!(env!("OUT_DIR"), "/tc_name_limit_exceeded"));

#[cfg(test)]
mod tests;
Expand Down
54 changes: 53 additions & 1 deletion test/integration-test/src/tests/load.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,15 @@ use std::{
time::{Duration, SystemTime},
};

use assert_matches::assert_matches;
use aya::{
maps::Array,
programs::{
links::{FdLink, PinnedLink},
loaded_links, loaded_programs, KProbe, TracePoint, UProbe, Xdp, XdpFlags,
loaded_links, loaded_programs,
tc::{clsact_qdisc_exists, qdisc_add_clsact},
KProbe, ProgramError, SchedClassifier, TcAttachType, TcError, TracePoint, UProbe, Xdp,
XdpFlags,
},
util::KernelVersion,
Bpf,
Expand Down Expand Up @@ -608,3 +612,51 @@ fn pin_lifecycle_uprobe() {
// Make sure the function isn't optimized out.
uprobe_function();
}

#[test]
fn tc_name_limit() {
let clsact_exists = clsact_qdisc_exists("lo").unwrap();
if !clsact_exists {
qdisc_add_clsact("lo").unwrap();
}

let mut bpf = Bpf::load(crate::TC_NAME_LIMIT_TEST).unwrap();

let long_name = "a".repeat(256);

let program: &mut SchedClassifier = bpf
.program_mut(long_name.as_str())
.unwrap()
.try_into()
.unwrap();
program.load().unwrap();
program.attach("lo", TcAttachType::Ingress).unwrap();
}

#[test]
fn tc_name_limit_exceeded() {
let clsact_exists = clsact_qdisc_exists("lo").unwrap();
if !clsact_exists {
qdisc_add_clsact("lo").unwrap();
}

let mut bpf = Bpf::load(crate::TC_NAME_LIMIT_EXCEEDED_TEST).unwrap();

let long_name = "a".repeat(257);

let program: &mut SchedClassifier = bpf
.program_mut(long_name.as_str())
.unwrap()
.try_into()
.unwrap();
program.load().unwrap();

assert_matches!(
program.attach("lo", TcAttachType::Ingress),
Err(ProgramError::TcError(TcError::NetlinkError { io_error })) => {
// An invalid argument error (EINVAL) with code 22 should occur.
// The invalid argument is the tc program name which is too long.
assert_eq!(io_error.raw_os_error(), Some(22))
}
);
}
1 change: 1 addition & 0 deletions xtask/public-api/aya.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4645,6 +4645,7 @@ impl<T> core::borrow::BorrowMut<T> for aya::programs::tc::TcOptions where T: cor
pub fn aya::programs::tc::TcOptions::borrow_mut(&mut self) -> &mut T
impl<T> core::convert::From<T> for aya::programs::tc::TcOptions
pub fn aya::programs::tc::TcOptions::from(t: T) -> T
pub fn aya::programs::tc::clsact_qdisc_exists(if_name: &str) -> core::result::Result<bool, std::io::error::Error>
pub fn aya::programs::tc::qdisc_add_clsact(if_name: &str) -> core::result::Result<(), std::io::error::Error>
pub fn aya::programs::tc::qdisc_detach_program(if_name: &str, attach_type: aya::programs::tc::TcAttachType, name: &str) -> core::result::Result<(), std::io::error::Error>
pub mod aya::programs::tp_btf
Expand Down