diff --git a/.github/buildomat/common.sh b/.github/buildomat/common.sh index f8df2081..965e5b78 100644 --- a/.github/buildomat/common.sh +++ b/.github/buildomat/common.sh @@ -1,14 +1,16 @@ #!/bin/bash -# The tofino2 has 20 stages, and the current sidecar.p4 needs all 20 of them. +# The tofino2 has 20 stages, and the current sidecar.p4 needs at most 19 of them, +# depending on the features it's compiled with. +# # Specifying the number of stages isn't strictly necessary, but it allows us to # track when we exceed the current ceiling. The underlying intention is to grow # deliberately and thoughtfully, given the limited space on the ASIC. -# -# Note: this now seems silly since we have maxed out the number of stages, but -# we want to leave this check and note in place should we ever find a way to -# reduce our footprint below 20 stages. -TOFINO_STAGES=20 +if [ "$MULTICAST" == "" ]; then + TOFINO_STAGES=17 +else + TOFINO_STAGES=19 +fi # These describe which version of the SDE to download and where to find it SDE_COMMIT=2a6b33211c9675996dcb99fe939045506667ae94 diff --git a/Cargo.lock b/Cargo.lock index 1ef80514..b2d7ad38 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1154,6 +1154,21 @@ dependencies = [ "thiserror 1.0.69", ] +[[package]] +name = "crc" +version = "3.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5eb8a2a1cd12ab0d987a5d5e825195d372001a4094a0376319d5a0ad71c1ba0d" +dependencies = [ + "crc-catalog", +] + +[[package]] +name = "crc-catalog" +version = "2.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "217698eaf96b4a3f0bc4f3662aaa55bdf913cd54d7204591faa790070c6d0853" + [[package]] name = "crc32fast" version = "1.5.0" @@ -1731,7 +1746,7 @@ dependencies = [ "async-trait", "chrono", "common 0.1.0", - "crc8", + "crc", "dpd-types", "futures", "http", diff --git a/Cargo.toml b/Cargo.toml index a8f11cfc..4629063d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -69,6 +69,7 @@ cfg-if = "1" chrono = "0.4" clap = { version = "4.6", features = [ "derive" ] } colored = "3" +crc = "3.4.0" csv = "1.3" curl = "0.4" dropshot = "0.17.1" diff --git a/dpd-client/Cargo.toml b/dpd-client/Cargo.toml index 7998da4d..a0901737 100644 --- a/dpd-client/Cargo.toml +++ b/dpd-client/Cargo.toml @@ -12,7 +12,6 @@ tofino_asic = ["asic/tofino_asic"] [dependencies] async-trait = "0.1.89" chrono.workspace = true -crc8 = "0.1.1" futures.workspace = true http.workspace = true oxnet.workspace = true @@ -34,6 +33,7 @@ packet = { path = "../packet" } pcap = { path = "../pcap" } asic = { path = "../asic" } anyhow.workspace = true +crc.workspace = true dpd-types.workspace = true lazy_static.workspace = true parking_lot.workspace = true diff --git a/dpd-client/tests/integration_tests/attached_subnet.rs b/dpd-client/tests/integration_tests/attached_subnet.rs index 19f7ff96..43a85180 100644 --- a/dpd-client/tests/integration_tests/attached_subnet.rs +++ b/dpd-client/tests/integration_tests/attached_subnet.rs @@ -63,13 +63,6 @@ async fn test_api() -> TestResult { Ok(()) } -#[derive(Debug)] -enum L4Protocol { - Tcp, - Udp, - Icmp, -} - #[derive(Debug)] struct ExternalTest { // packet source/destination from the vm client's perspective @@ -246,17 +239,25 @@ async fn test_ingress(switch: &Switch, test: &ExternalTest) -> TestResult { // external subnet. let load = vec![0xaau8, 0xbb, 0xcc, 0xdd, 0xee]; let switch_mac = switch.get_port_mac(test.uplink_port).unwrap().to_string(); - let ingress_pkt = common::gen_udp_packet_loaded( - Endpoint::parse( - &test.upstream_router_mac, - &test.pkt_src_ip, - test.pkt_src_port, - ) - .unwrap(), - Endpoint::parse(&switch_mac, &test.pkt_dst_ip, test.pkt_dst_port) - .unwrap(), - &load, - ); + let src = Endpoint::parse( + &test.upstream_router_mac, + &test.pkt_src_ip, + test.pkt_src_port, + ) + .unwrap(); + let dst = Endpoint::parse(&switch_mac, &test.pkt_dst_ip, test.pkt_dst_port) + .unwrap(); + let (ingress_pkt, csum_generated) = match test.l4_protocol { + L4Protocol::Tcp => { + (common::gen_tcp_packet_loaded(src, dst, &load), true) + } + L4Protocol::Udp => { + (common::gen_udp_packet_loaded(src, dst, &load), true) + } + L4Protocol::Icmp => { + (common::gen_icmp_packet_loaded(src, dst, &load), false) + } + }; // Convert the packet into a binary payload that will be encapsulated into a // geneve packet. We also rewrite the destination mac of the packet to @@ -283,7 +284,7 @@ async fn test_ingress(switch: &Switch, test: &ExternalTest) -> TestResult { Endpoint::parse( &backplane_port_mac, switch_port_ip, - geneve::GENEVE_UDP_PORT, + common::tofino_flow_hash(src, dst, test.l4_protocol), ) .unwrap(), Endpoint::parse( @@ -299,7 +300,11 @@ async fn test_ingress(switch: &Switch, test: &ExternalTest) -> TestResult { /* Adjust for transition from switch port to gimlet port */ ipv6::Ipv6Hdr::adjust_hlim(&mut forward_pkt, -1); - udp::UdpHdr::update_checksum(&mut forward_pkt); + if csum_generated { + udp::UdpHdr::update_checksum(&mut forward_pkt); + } else { + forward_pkt.hdrs.udp_hdr.as_mut().unwrap().udp_sum = 0; + } let send = vec![TestPacket { packet: Arc::new(ingress_pkt), diff --git a/dpd-client/tests/integration_tests/common.rs b/dpd-client/tests/integration_tests/common.rs index 3d50f82a..0fcebdc8 100644 --- a/dpd-client/tests/integration_tests/common.rs +++ b/dpd-client/tests/integration_tests/common.rs @@ -50,6 +50,15 @@ const TEST_PCAP_TIMEOUT_MS: i32 = 1; pub struct PhysPort(pub u16); pub const NO_PORT: PhysPort = PhysPort(0xffff); +/// The type of an L4 packet carried over IP +#[repr(u8)] +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +pub enum L4Protocol { + Tcp = ipv4::IPPROTO_TCP, + Udp = ipv4::IPPROTO_UDP, + Icmp = ipv4::IPPROTO_ICMP, +} + // On real hardware the "service port" is the PCI port, which doesn't have a // real physical port - it's just a collection of ringbufs in PCI space. On the // model, we have "physical port" 33 wired to carry the traffic for "tofino @@ -856,15 +865,23 @@ pub fn gen_udp_packet_loaded( Packet::generate(src, dst, udp_stack, Some(body)).unwrap() } -// Construct a single ICMP packet with an optional payload +// Construct a single ICMP echo request packet with an optional payload. pub fn gen_icmp_packet_loaded( src: Endpoint, dst: Endpoint, body: &[u8], ) -> Packet { let icmp_stack = match src.get_ip("src").unwrap() { - IpAddr::V4(_) => vec![ipv4::IPPROTO_ICMP.into(), eth::ETHER_IPV4], - IpAddr::V6(_) => vec![ipv6::IPPROTO_ICMPV6.into(), eth::ETHER_IPV6], + IpAddr::V4(_) => vec![ + u16::from(icmp::ICMP_ECHO) << 8, + ipv4::IPPROTO_ICMP.into(), + eth::ETHER_IPV4, + ], + IpAddr::V6(_) => vec![ + u16::from(icmp::ICMP6_ECHO_REQUEST) << 8, + ipv6::IPPROTO_ICMPV6.into(), + eth::ETHER_IPV6, + ], }; Packet::generate(src, dst, icmp_stack, Some(body)).unwrap() @@ -1441,8 +1458,63 @@ pub fn gen_arp_reply(src: Endpoint, tgt: Endpoint) -> Packet { .unwrap() } +/// Compute an identical flow hash to the `FlowHash` control in ingress +/// processing. +/// +/// This value is currently used in encap header generation and ECMP nexthop +/// selection. +pub fn tofino_flow_hash( + src: Endpoint, + dst: Endpoint, + ip_proto: L4Protocol, +) -> u16 { + let Ok(src_ip) = src.get_ip("src") else { + return 0; + }; + let Ok(dst_ip) = dst.get_ip("dst") else { + return 0; + }; + + let crc = crc::Crc::::new(&crc::CRC_16_ARC); + let mut digest = crc.digest(); + + let proto = match (dst_ip, src_ip) { + (IpAddr::V6(dip), IpAddr::V6(sip)) => { + digest.update(&dip.octets()); + digest.update(&sip.octets()); + if ip_proto == L4Protocol::Icmp { + ipv6::IPPROTO_ICMPV6 + } else { + ip_proto as u8 + } + } + (IpAddr::V4(dip), IpAddr::V4(sip)) => { + digest.update(&dip.octets()); + digest.update(&sip.octets()); + ip_proto as u8 + } + _ => panic!("mismatched src/dst address families"), + }; + + // Ingress processing will compute an L3 hash when no ports + // are available. + let dst_port = dst.get_port("dst").unwrap_or(0); + let src_port = if ip_proto == L4Protocol::Icmp { + // The parser leaves l4_src_port at 0 for ICMP echo. + 0 + } else { + src.get_port("src").unwrap_or(0) + }; + + digest.update(&[proto]); + digest.update(&dst_port.to_be_bytes()); + digest.update(&src_port.to_be_bytes()); + digest.finalize() +} + pub mod prelude { pub use super::ADMIN_LOCAL_MULTICAST_PREFIX; + pub use super::L4Protocol; pub use super::NO_PORT; pub use super::PhysPort; pub use super::SERVICE_PORT; diff --git a/dpd-client/tests/integration_tests/mcast.rs b/dpd-client/tests/integration_tests/mcast.rs index b98b8e76..6c5ac347 100644 --- a/dpd-client/tests/integration_tests/mcast.rs +++ b/dpd-client/tests/integration_tests/mcast.rs @@ -434,7 +434,11 @@ fn prepare_expected_pkt( Endpoint::parse( &switch_port_mac, "::0", - geneve::GENEVE_UDP_PORT, + common::tofino_flow_hash( + send_pkt.src().unwrap(), + send_pkt.dst().unwrap(), + L4Protocol::Udp, + ), ) .unwrap(), Endpoint::parse( diff --git a/dpd-client/tests/integration_tests/nat.rs b/dpd-client/tests/integration_tests/nat.rs index 9d7372a1..f8fba25b 100644 --- a/dpd-client/tests/integration_tests/nat.rs +++ b/dpd-client/tests/integration_tests/nat.rs @@ -161,12 +161,6 @@ async fn test_api() -> TestResult { Ok(()) } -enum L4Protocol { - Tcp, - Udp, - Icmp, -} - struct NatTest { // uplink network info uplink_port: PhysPort, @@ -341,34 +335,30 @@ async fn test_nat_ingress(switch: &Switch, test: &NatTest) -> TestResult { switch.client.route_ipv6_set(&route).await.unwrap(); common::add_neighbor_ipv6(switch, &test.gimlet_ip, gimlet_mac).await?; - let load = vec![0xaau8, 0xbb, 0xcc, 0xdd, 0xee]; // Build a packet coming from an external host via an upstream router, to an // uplinked switch port. The packet is addressed to a nat ip/port pair. + let load = vec![0xaau8, 0xbb, 0xcc, 0xdd, 0xee]; let switch_mac = switch.get_port_mac(test.uplink_port).unwrap().to_string(); - let ingress_pkt = common::gen_udp_packet_loaded( - Endpoint::parse(&test.router_mac, &test.vpc_src_ip, test.vpc_src_port) - .unwrap(), - Endpoint::parse( - &switch_mac, - &test.uplink_port_external, - test.nat_l4_port, - ) - .unwrap(), - &load, - ); - - let icmp_load = &[]; - let ingress_icmp_pkt = common::gen_icmp_packet_loaded( + let src = Endpoint::parse(&test.router_mac, &test.vpc_src_ip, test.vpc_src_port) - .unwrap(), - Endpoint::parse( - &switch_mac, - &test.uplink_port_external, - test.nat_l4_port, - ) - .unwrap(), - icmp_load, - ); + .unwrap(); + let dst = Endpoint::parse( + &switch_mac, + &test.uplink_port_external, + test.nat_l4_port, + ) + .unwrap(); + let (ingress_pkt, csum_generated) = match test.l4_protocol { + L4Protocol::Tcp => { + (common::gen_tcp_packet_loaded(src, dst, &load), true) + } + L4Protocol::Udp => { + (common::gen_udp_packet_loaded(src, dst, &load), true) + } + L4Protocol::Icmp => { + (common::gen_icmp_packet_loaded(src, dst, &load), false) + } + }; // Deparse the incoming packet so we can copy it into the encapsulated // packet @@ -379,13 +369,6 @@ async fn test_nat_ingress(switch: &Switch, test: &NatTest) -> TestResult { eth.eth_dmac = test.vpc_dst_mac.parse().unwrap(); encapped.deparse().unwrap().to_vec() }; - let ingress_icmp_payload = { - let mut encapped = ingress_icmp_pkt.clone(); - let eth = encapped.hdrs.eth_hdr.as_mut().unwrap(); - eth.eth_smac = MacAddr::new(0, 0, 0, 0, 0, 0); - eth.eth_dmac = test.vpc_dst_mac.parse().unwrap(); - encapped.deparse().unwrap().to_vec() - }; // build the encapsulated packet for transporting the NAT packet from the // switch to OPTE @@ -400,7 +383,7 @@ async fn test_nat_ingress(switch: &Switch, test: &NatTest) -> TestResult { Endpoint::parse( &gimlet_port_mac, switch_port_ip, - geneve::GENEVE_UDP_PORT, + common::tofino_flow_hash(src, dst, test.l4_protocol), ) .unwrap(), Endpoint::parse( @@ -413,50 +396,23 @@ async fn test_nat_ingress(switch: &Switch, test: &NatTest) -> TestResult { test.geneve_vni, &ingress_payload, ); - let mut forward_icmp_pkt = common::gen_external_geneve_packet( - Endpoint::parse( - &gimlet_port_mac, - switch_port_ip, - geneve::GENEVE_UDP_PORT, - ) - .unwrap(), - Endpoint::parse( - &test.gimlet_mac, - &test.gimlet_ip, - geneve::GENEVE_UDP_PORT, - ) - .unwrap(), - eth::ETHER_ETHER, - test.geneve_vni, - &ingress_icmp_payload, - ); /* Adjust for transition from switch port to gimlet port */ ipv6::Ipv6Hdr::adjust_hlim(&mut forward_pkt, -1); - ipv6::Ipv6Hdr::adjust_hlim(&mut forward_icmp_pkt, -1); - - udp::UdpHdr::update_checksum(&mut forward_pkt); - // TODO: I cannot convince the tofino to compute this correctly. - // Conveniently, we dont actually need it, see RFC 6935. - // - // udp::UdpHdr::update_checksum(&mut forward_icmp_pkt); - // - forward_icmp_pkt.hdrs.udp_hdr.as_mut().unwrap().udp_sum = 0; - - let send = vec![ - TestPacket { packet: Arc::new(ingress_pkt), port: test.uplink_port }, - TestPacket { - packet: Arc::new(ingress_icmp_pkt), - port: test.uplink_port, - }, - ]; - let expected = vec![ - TestPacket { packet: Arc::new(forward_pkt), port: test.gimlet_port }, - TestPacket { - packet: Arc::new(forward_icmp_pkt), - port: test.gimlet_port, - }, - ]; + if csum_generated { + udp::UdpHdr::update_checksum(&mut forward_pkt); + } else { + forward_pkt.hdrs.udp_hdr.as_mut().unwrap().udp_sum = 0; + } + + let send = vec![TestPacket { + packet: Arc::new(ingress_pkt), + port: test.uplink_port, + }]; + let expected = vec![TestPacket { + packet: Arc::new(forward_pkt), + port: test.gimlet_port, + }]; switch.packet_test(send, expected) } @@ -588,6 +544,13 @@ async fn test_ingress_ipv4_tcp() -> TestResult { test_ingress_ipv4(switch, L4Protocol::Tcp).await } +#[tokio::test] +#[ignore] +async fn test_ingress_ipv4_icmp() -> TestResult { + let switch = &*get_switch().await; + test_ingress_ipv4(switch, L4Protocol::Icmp).await +} + // packet to/from IPv6 addresses, async fn test_egress_ipv6( switch: &Switch, @@ -702,3 +665,10 @@ async fn test_ingress_ipv6_tcp() -> TestResult { let switch = &*get_switch().await; test_ingress_ipv6(switch, L4Protocol::Tcp).await } + +#[tokio::test] +#[ignore] +async fn test_ingress_ipv6_icmp() -> TestResult { + let switch = &*get_switch().await; + test_ingress_ipv6(switch, L4Protocol::Icmp).await +} diff --git a/dpd-client/tests/integration_tests/route_ipv4.rs b/dpd-client/tests/integration_tests/route_ipv4.rs index 37cd8fcc..75f7eceb 100644 --- a/dpd-client/tests/integration_tests/route_ipv4.rs +++ b/dpd-client/tests/integration_tests/route_ipv4.rs @@ -793,23 +793,18 @@ async fn test_multipath(switch: &Switch, routers: &[Router]) -> TestResult { let dst_port: u16 = 4444; // Replicate the path-selection algorithm used in the sidecar p4 code - let mut data = [0u8; 12]; - data[0..4].copy_from_slice(&dst_ip.parse::().unwrap().octets()); - data[4..8].copy_from_slice(&src_ip.parse::().unwrap().octets()); - data[8..10].copy_from_slice(&dst_port.to_be_bytes()); - data[10..12].copy_from_slice(&src_port.to_be_bytes()); + let src = Endpoint::parse("e0:d5:5e:67:89:ab", src_ip, src_port).unwrap(); + let dst = Endpoint::parse("e0:d5:5e:67:89:ac", dst_ip, dst_port).unwrap(); - // The tofino CRC8 implementation uses the default polynomial value of 0x07 - let mut crc8 = crc8::Crc8::create_msb(0x07); - let hash = crc8.calc(&data, 12, 0); - let expected_egress = (hash & 0x3f) as usize % routers.len(); + let hash = common::tofino_flow_hash(src, dst, L4Protocol::Udp); + let expected_egress = usize::from(hash & 0x3f) % routers.len(); let (to_send, mut to_recv) = common::gen_udp_routed_pair( switch, PhysPort(routers[expected_egress].port), routers[expected_egress].mac, - Endpoint::parse("e0:d5:5e:67:89:ab", src_ip, src_port).unwrap(), - Endpoint::parse("e0:d5:5e:67:89:ac", dst_ip, dst_port).unwrap(), + src, + dst, ); let send = diff --git a/dpd-client/tests/integration_tests/route_ipv6.rs b/dpd-client/tests/integration_tests/route_ipv6.rs index dbdf6917..81932cac 100644 --- a/dpd-client/tests/integration_tests/route_ipv6.rs +++ b/dpd-client/tests/integration_tests/route_ipv6.rs @@ -853,23 +853,18 @@ async fn test_multipath(switch: &Switch, routers: &[Router]) -> TestResult { let dst_port: u16 = 4444; // Replicate the path-selection algorithm used in the sidecar p4 code - let mut data = [0u8; 36]; - data[0..16].copy_from_slice(&dst_ip.parse::().unwrap().octets()); - data[16..32].copy_from_slice(&src_ip.parse::().unwrap().octets()); - data[32..34].copy_from_slice(&dst_port.to_be_bytes()); - data[34..36].copy_from_slice(&src_port.to_be_bytes()); + let src = Endpoint::parse("e0:d5:5e:67:89:ab", src_ip, src_port).unwrap(); + let dst = Endpoint::parse("e0:d5:5e:67:89:ac", dst_ip, dst_port).unwrap(); - // The tofino CRC8 implementation uses the default polynomial value of 0x07 - let mut crc8 = crc8::Crc8::create_msb(0x07); - let hash = crc8.calc(&data, 36, 0); - let expected_egress = (hash & 0x3f) as usize % routers.len(); + let hash = common::tofino_flow_hash(src, dst, L4Protocol::Udp); + let expected_egress = usize::from(hash & 0x3f) % routers.len(); let (to_send, mut to_recv) = common::gen_udp_routed_pair( switch, PhysPort(routers[expected_egress].port), routers[expected_egress].mac, - Endpoint::parse("e0:d5:5e:67:89:ab", src_ip, src_port).unwrap(), - Endpoint::parse("e0:d5:5e:67:89:ac", dst_ip, dst_port).unwrap(), + src, + dst, ); let send = diff --git a/dpd-client/tests/integration_tests/table_tests.rs b/dpd-client/tests/integration_tests/table_tests.rs index f764ad15..f0d4c4b8 100644 --- a/dpd-client/tests/integration_tests/table_tests.rs +++ b/dpd-client/tests/integration_tests/table_tests.rs @@ -42,11 +42,11 @@ use crate::integration_tests::common::prelude::*; // investigating. If it only changes by an entry or two, it's fine to just // adjust the constant below to match the observed result. // -// TODO: Multicast drops IPv4 LPM capacity to 7164 (from 8187) due to -// ingress TCAM pressure. Investigate moving MulticastRouter4/6 into the -// egress pipeline to reclaim capacity. +// TODO: Multicast loses some IPv4 LPM capacity due to ingress TCAM pressure. +// Investigate moving MulticastRouter4/6 into the egress pipeline to reclaim +// capacity. #[cfg(feature = "multicast")] -const IPV4_LPM_SIZE: usize = 7164; // ipv4 forwarding table +const IPV4_LPM_SIZE: usize = 8145; // ipv4 forwarding table #[cfg(not(feature = "multicast"))] const IPV4_LPM_SIZE: usize = 8191; // ipv4 forwarding table diff --git a/dpd/p4/metadata.p4 b/dpd/p4/metadata.p4 index c609a269..8fcbda5f 100644 --- a/dpd/p4/metadata.p4 +++ b/dpd/p4/metadata.p4 @@ -31,6 +31,7 @@ struct sidecar_ingress_meta_t { bit<8> drop_reason; // reason a packet was dropped bit<16> l4_src_port; // tcp or udp destination port bit<16> l4_dst_port; // tcp or udp destination port + bit<16> flow_hash; // hash of a packet 5-tuple for ECMP/encap ipv6_addr_t nat_ingress_tgt; // target address for NAT ingress mac_addr_t nat_inner_mac; // inner mac address for NAT ingress geneve_vni_t nat_geneve_vni; // VNI for NAT ingress diff --git a/dpd/p4/parser.p4 b/dpd/p4/parser.p4 index 3a2618d0..07af7074 100644 --- a/dpd/p4/parser.p4 +++ b/dpd/p4/parser.p4 @@ -41,6 +41,7 @@ parser IngressParser( meta.icmp_csum = 0; meta.l4_src_port = 0; meta.l4_dst_port = 0; + meta.flow_hash = 0; meta.l4_length = 0; meta.body_checksum = 0; meta.nexthop_ipv4 = 0; @@ -317,7 +318,7 @@ parser IngressParser( state goto_proto_ipv6 { transition select(hdr.ipv6.next_hdr) { - IPPROTO_ICMPV6: parse_icmp; + IPPROTO_ICMPV6: parse_icmp6; IPPROTO_TCP: parse_tcp; IPPROTO_UDP: parse_udp; IPPROTO_SCTP: drop_sctp; @@ -338,6 +339,39 @@ parser IngressParser( nat_checksum.subtract(hdr.icmp); nat_checksum.subtract_all_and_deposit(meta.body_checksum); + meta.l4_dst_port = hdr.icmp.data[31:16]; + + transition select(hdr.icmp.type) { + ICMP_ECHO: accept; + ICMP_ECHOREPLY: accept; + default: icmp_no_dst_info; + } + } + + state parse_icmp6 { + pkt.extract(hdr.icmp); + meta.pkt_type = meta.pkt_type | PKT_ICMP; + + icmp_checksum.subtract({ + hdr.icmp.hdr_checksum, + hdr.icmp.type, hdr.icmp.code + }); + icmp_checksum.subtract_all_and_deposit(meta.icmp_csum); + + nat_checksum.subtract(hdr.icmp); + nat_checksum.subtract_all_and_deposit(meta.body_checksum); + + meta.l4_dst_port = hdr.icmp.data[31:16]; + + transition select(hdr.icmp.type) { + ICMP6_ECHO: accept; + ICMP6_ECHOREPLY: accept; + default: icmp_no_dst_info; + } + } + + state icmp_no_dst_info { + meta.l4_dst_port = 0; transition accept; } diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index 07a694ae..b06a842a 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -511,8 +511,12 @@ control NatIngress ( hdr.inner_eth.ether_type = hdr.ethernet.ether_type; // 8 bytes + // The Geneve specification suggests that the UDP source port + // SHOULD be calculated as a hash of the inner 5-tuple + // (RFC 8926, ยง3.3). This is fairly crucial for NICs to split + // encapsulated flows across hardware rings. hdr.udp.setValid(); - hdr.udp.src_port = GENEVE_UDP_PORT; + hdr.udp.src_port = meta.flow_hash; hdr.udp.dst_port = GENEVE_UDP_PORT; hdr.udp.hdr_length = udp_len; hdr.udp.checksum = 0; @@ -648,30 +652,6 @@ control NatIngress ( } #endif /* MULTICAST */ - action set_icmp_dst_port() { - meta.l4_dst_port = hdr.icmp.data[31:16]; - } - - table icmp_dst_port { - key = { - hdr.icmp.isValid(): ternary; - hdr.icmp.type: ternary; - } - - actions = { - set_icmp_dst_port; - } - - const entries = { - ( true, ICMP_ECHO ) : set_icmp_dst_port; - ( true, ICMP_ECHOREPLY ) : set_icmp_dst_port; - ( true, ICMP6_ECHO ) : set_icmp_dst_port; - ( true, ICMP6_ECHOREPLY ) : set_icmp_dst_port; - } - - const size = 4; - } - action set_inner_tcp() { hdr.inner_tcp = hdr.tcp; hdr.inner_tcp.setValid(); @@ -714,8 +694,6 @@ control NatIngress ( } apply { - icmp_dst_port.apply(); - // Note: This whole conditional could be simpler as a set of */ // `const entries`, but apply (on tables) cannot be called from actions #ifdef MULTICAST @@ -1233,7 +1211,6 @@ control Router4 ( inout ingress_intrinsic_metadata_for_tm_t ig_tm_md ) { RouterLookupIndex4() lookup_idx; - Hash>(HashAlgorithm_t.CRC8) index_hash; action icmp_error(bit<8> type, bit<8> code) { hdr.sidecar.sc_code = SC_ICMP_NEEDED; @@ -1259,14 +1236,9 @@ control Router4 ( fwd.slot = 0; // Our route selection table is 11 bits wide, and we need 5 bits // of that for our "slot count" index. Thus, we only need 6 - // bits of the 8-bit hash calculated here to complete the 11-bit - // index. - fwd.ecmp_hash = index_hash.get({ - hdr.ipv4.dst_addr, - hdr.ipv4.src_addr, - meta.l4_dst_port, - meta.l4_src_port - }) & 0x3f; + // bits of the 16-bit hash calculated earlier to complete the + // 11-bit index. + fwd.ecmp_hash = 2w0 ++ meta.flow_hash[5:0]; lookup_idx.apply(hdr, fwd); @@ -1380,7 +1352,6 @@ control Router6 ( inout ingress_intrinsic_metadata_for_tm_t ig_tm_md ) { RouterLookupIndex6() lookup_idx; - Hash>(HashAlgorithm_t.CRC8) index_hash; action icmp_error(bit<8> type, bit<8> code) { hdr.sidecar.sc_code = SC_ICMP_NEEDED; @@ -1404,14 +1375,9 @@ control Router6 ( fwd.slot = 0; // Our route selection table is 11 bits wide, and we need 5 bits // of that for our "slot count" index. Thus, we only need 6 - // bits of the 8-bit hash calculated here to complete the 11-bit - // index. - fwd.ecmp_hash = index_hash.get({ - hdr.ipv6.dst_addr, - hdr.ipv6.src_addr, - meta.l4_dst_port, - meta.l4_src_port - }) & 0x3f; + // bits of the 16-bit hash calculated earlier to complete the + // 11-bit index. + fwd.ecmp_hash = 2w0 ++ meta.flow_hash[5:0]; lookup_idx.apply(hdr, fwd); @@ -1687,9 +1653,6 @@ control MulticastIngress ( DirectCounter>(CounterType_t.PACKETS_AND_BYTES) mcast_ipv4_ssm_ctr; DirectCounter>(CounterType_t.PACKETS_AND_BYTES) mcast_ipv6_ssm_ctr; - Hash>(HashAlgorithm_t.CRC16) mcast_hashv6_level1; - Hash>(HashAlgorithm_t.CRC16) mcast_hashv6_level2; - // Drop action for IPv4 multicast packets with no group. // // At this point, We should only allow replication for IPv6 packets that @@ -1760,20 +1723,11 @@ control MulticastIngress ( ig_tm_md.level1_exclusion_id = level1_excl_id; ig_tm_md.level2_exclusion_id = level2_excl_id; - // Set multicast hash based on IPv6 packet fields - ig_tm_md.level1_mcast_hash = (bit<13>)mcast_hashv6_level1.get({ - hdr.ipv6.src_addr, - hdr.ipv6.dst_addr, - hdr.ipv6.next_hdr, - meta.l4_src_port, - meta.l4_dst_port - }); - - // Set secondary multicast hash based on IPv6 packet fields - ig_tm_md.level2_mcast_hash = (bit<13>)mcast_hashv6_level2.get({ - hdr.ipv6.flow_label, - ig_intr_md.ingress_port - }); + // Set L3/ECMP and L2/LAG multicast hashes using the packet's + // flow hash. This will include entropy from the inner flow, see + // the commentary on FlowHash. + ig_tm_md.level1_mcast_hash = meta.flow_hash[12:0]; + ig_tm_md.level2_mcast_hash = meta.flow_hash[15:3]; mcast_ipv6_ctr.count(); } @@ -2112,6 +2066,50 @@ control MulticastEgress ( } #endif /* MULTICAST */ +/* + * Compute the flowhash of any (non-filtered) packet on ingress. + * + * Hashes are used for two purposes in the datapath: + * - ECMP nexthop selection in the Router4/Router6 tables. + * - Providing flow entropy in generated Geneve headers, so that + * destination NICs and transit switches can correctly fanout + * flows over rings and ports. + * + * While this is run at the head of packet processing, this is always + * derived from the inner flow 5-tuple, such that we don't need to re-hash + * the packet after NatEgress processing. OPTE (and other switches in a + * multirack world) store the inner flow hash in the UDP source port of + * encap headers, and as a result the outer flow hash will include that + * entropy. + */ +control FlowHash( + in sidecar_headers_t hdr, + inout sidecar_ingress_meta_t meta) +{ + Hash>(HashAlgorithm_t.CRC16) v4_hash; + Hash>(HashAlgorithm_t.CRC16) v6_hash; + + apply { + if (hdr.ipv4.isValid()) { + meta.flow_hash = v4_hash.get({ + hdr.ipv4.dst_addr, + hdr.ipv4.src_addr, + hdr.ipv4.protocol, + meta.l4_dst_port, + meta.l4_src_port + }); + } else if (hdr.ipv6.isValid()) { + meta.flow_hash = v6_hash.get({ + hdr.ipv6.dst_addr, + hdr.ipv6.src_addr, + hdr.ipv6.next_hdr, + meta.l4_dst_port, + meta.l4_src_port + }); + } + } +} + control Ingress( inout sidecar_headers_t hdr, inout sidecar_ingress_meta_t meta, @@ -2131,6 +2129,7 @@ control Ingress( MulticastIngress() mcast_ingress; #endif /* MULTICAST */ MacRewrite() mac_rewrite; + FlowHash() flow_hash; Counter, PortId_t>(512, CounterType_t.PACKETS_AND_BYTES) ingress_ctr; Counter, PortId_t>(512, CounterType_t.PACKETS_AND_BYTES) egress_ctr; @@ -2152,6 +2151,8 @@ control Ingress( filter.apply(hdr, meta, ig_intr_md); } + flow_hash.apply(hdr, meta); + if (!meta.is_mcast || meta.is_link_local_mcastv6) { attached_subnet_ingress.apply(hdr, meta); } diff --git a/packet/src/icmp.rs b/packet/src/icmp.rs index 1591a1ce..bd81c1fc 100644 --- a/packet/src/icmp.rs +++ b/packet/src/icmp.rs @@ -60,6 +60,8 @@ pub const ICMP6_DST_UNREACH: u8 = 1; pub const ICMP6_PACKET_TOO_BIG: u8 = 2; pub const ICMP6_TIME_EXCEEDED: u8 = 3; pub const ICMP6_PARAM_PROB: u8 = 4; +pub const ICMP6_ECHO_REQUEST: u8 = 128; +pub const ICMP6_ECHO_REPLY: u8 = 129; pub const ICMP6_DST_UNREACH_NOROUTE: u8 = 0; pub const ICMP6_DST_UNREACH_ADMIN: u8 = 1; @@ -132,8 +134,13 @@ impl Protocol for IcmpHdr { let op = protos.pop().unwrap_or_default(); let icmp_type = ((op >> 8) & 0xff) as u8; let icmp_code = (op & 0xff) as u8; - let icmp_id = match crate::L4Endpoint::try_from(dst) { - Ok(ep) => (ep.port as u32) << 16, + let is_v6 = crate::L3Endpoint::try_from(dst).unwrap().ip.is_ipv6(); + let icmp_id = match (is_v6, icmp_type, crate::L4Endpoint::try_from(dst)) + { + (false, ICMP_ECHO, Ok(ep)) + | (false, ICMP_ECHOREPLY, Ok(ep)) + | (true, ICMP6_ECHO_REQUEST, Ok(ep)) + | (true, ICMP6_ECHO_REPLY, Ok(ep)) => u32::from(ep.port) << 16, _ => 0, }; @@ -253,6 +260,15 @@ impl IcmpHdr { let hdr = pkt.hdrs.icmp_hdr.as_mut().unwrap(); hdr.icmp_sum = checksum; } + + /// Return whether this is an ICMP(v6) echo request/reply packet. + pub fn is_echo(&self, is_v6: bool) -> bool { + matches!( + (is_v6, self.icmp_type), + (false, ICMP_ECHO | ICMP_ECHOREPLY) + | (true, ICMP6_ECHO_REQUEST | ICMP6_ECHO_REPLY) + ) + } } #[test] diff --git a/packet/src/lib.rs b/packet/src/lib.rs index a0262c10..689b8e6a 100644 --- a/packet/src/lib.rs +++ b/packet/src/lib.rs @@ -5,8 +5,7 @@ // Copyright 2026 Oxide Computer Company use std::convert::TryFrom; -use std::fmt::Debug; -use std::fmt::{self, Write}; +use std::fmt::{self, Debug, Write}; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; use std::string::ToString; @@ -90,7 +89,7 @@ pub fn parse_ip(ip: &str) -> PacketResult { } } -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug)] pub struct L2Endpoint { pub mac: MacAddr, } @@ -107,7 +106,7 @@ impl fmt::Display for L2Endpoint { } } -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug)] pub struct L3Endpoint { pub mac: MacAddr, pub ip: IpAddr, @@ -129,7 +128,7 @@ impl fmt::Display for L3Endpoint { } } -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug)] pub struct L4Endpoint { pub mac: MacAddr, pub ip: IpAddr, @@ -152,7 +151,7 @@ impl L4Endpoint { } } -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug)] pub enum Endpoint { L2(L2Endpoint), L3(L3Endpoint), @@ -292,9 +291,9 @@ impl TryFrom for L4Endpoint { impl fmt::Display for Endpoint { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Endpoint::L2(e) => e.fmt(f), - Endpoint::L3(e) => e.fmt(f), - Endpoint::L4(e) => e.fmt(f), + Endpoint::L2(e) => fmt::Display::fmt(e, f), + Endpoint::L3(e) => fmt::Display::fmt(e, f), + Endpoint::L4(e) => fmt::Display::fmt(e, f), } } } @@ -462,6 +461,90 @@ impl Packet { Ok(data) } + + /// Collect all source addresses from this packet's protocol stack. + /// + /// This function will return an error if the packet has no Ethernet + /// header, or if multiple protocols are present at the same layer. + pub fn src(&self) -> PacketResult { + let mac = self + .hdrs + .eth_hdr + .ok_or_else(|| { + PacketError::Invalid("packet has no L2 header".into()) + })? + .eth_smac; + + let (ip, v6) = match (&self.hdrs.ipv4_hdr, &self.hdrs.ipv6_hdr) { + (Some(v4), None) => (v4.ipv4_src_ip.into(), false), + (None, Some(v6)) => (v6.ipv6_src_ip.into(), true), + (None, None) => return Ok(L2Endpoint { mac }.into()), + _ => { + return Err(PacketError::Invalid( + "packet has multiple L3 headers".into(), + )); + } + }; + + let port = + match (&self.hdrs.udp_hdr, &self.hdrs.tcp_hdr, &self.hdrs.icmp_hdr) + { + (Some(udp), None, None) => udp.udp_sport, + (None, Some(tcp), None) => tcp.tcp_sport, + (None, None, Some(icmp)) if icmp.is_echo(v6) => 0, + (None, None, _) => return Ok(L3Endpoint { mac, ip }.into()), + _ => { + return Err(PacketError::Invalid( + "packet has multiple L4 headers".into(), + )); + } + }; + + Ok(L4Endpoint { mac, ip, port }.into()) + } + + /// Collect all destination addresses from this packet's protocol stack. + /// + /// This function will return an error if the packet has no Ethernet + /// header, or if multiple protocols are present at the same layer. + pub fn dst(&self) -> PacketResult { + let mac = self + .hdrs + .eth_hdr + .ok_or_else(|| { + PacketError::Invalid("packet has no L2 header".into()) + })? + .eth_dmac; + + let (ip, v6) = match (&self.hdrs.ipv4_hdr, &self.hdrs.ipv6_hdr) { + (Some(v4), None) => (v4.ipv4_dst_ip.into(), false), + (None, Some(v6)) => (v6.ipv6_dst_ip.into(), true), + (None, None) => return Ok(L2Endpoint { mac }.into()), + _ => { + return Err(PacketError::Invalid( + "packet has multiple L3 headers".into(), + )); + } + }; + + let port = + match (&self.hdrs.udp_hdr, &self.hdrs.tcp_hdr, &self.hdrs.icmp_hdr) + { + (Some(udp), None, None) => udp.udp_dport, + (None, Some(tcp), None) => tcp.tcp_dport, + (None, None, Some(icmp)) if icmp.is_echo(v6) => { + (icmp.icmp_data >> 16) as u16 + } + (None, None, _) => return Ok(L3Endpoint { mac, ip }.into()), + _ => { + return Err(PacketError::Invalid( + "packet has multiple L4 headers".into(), + )); + } + }; + + Ok(L4Endpoint { mac, ip, port }.into()) + } } fn collect(