From 8feedbb02a304c526b21179ee309d078e31cfd0c Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Tue, 13 Oct 2026 06:11:37 +0000 Subject: [PATCH 1/4] [multicast] check inner TTL on decap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parser's TTL/hop limit checks cover only the outer header thus far. A zero inner TTL could wrap to 255 on decap, while on bifurcated egress, we could forward TTL 1 as 0. This work drops expired multicast packets before decrementing the TTL/hop limit, occurring both at ingress for external-only groups and at egress for bifurcated (replicated) groups. Underlay replicas do retain their inner packet. Drop counting now adds a `multicast_ttl_exceeded` reason to account for expiry. Also, this includes: - zero TTL/hop limit on unicast NAT egress now goes to Time Exceeded, along with TTL=1 (it used to wrap to 255) - no ICMP error requests for multicast route misses or expiry (RFC 1812 §4.3.2.7, RFC 4443 §2.4[e.3]) - `sc_egress` is set to a constant 0 on the sidecar paths that read `ucast_egress_port` before anything actually sets it (see P4-16 §8.25, unspecified value) - the mcast MAC rewrite table no longer runs on replicas we're dropping due to TTL (its direct counter had been counting them) - fix spelling: `nat_engress_blocked` -> `nat_egress_blocked` on the drop reason label (typo from #197) --- dpd-client/tests/integration_tests/common.rs | 16 +- dpd-client/tests/integration_tests/mcast.rs | 720 ++++++++++++++++++ dpd-client/tests/integration_tests/nat.rs | 142 +++- dpd-client/tests/integration_tests/service.rs | 2 +- dpd/p4/constants.p4 | 6 +- dpd/p4/sidecar.p4 | 113 +-- dpd/src/counters.rs | 7 +- 7 files changed, 917 insertions(+), 89 deletions(-) diff --git a/dpd-client/tests/integration_tests/common.rs b/dpd-client/tests/integration_tests/common.rs index d52eb08e..f70bd34b 100644 --- a/dpd-client/tests/integration_tests/common.rs +++ b/dpd-client/tests/integration_tests/common.rs @@ -680,10 +680,19 @@ impl Switch { } pub fn packet_test( + &self, + send: Vec, + expect: Vec, + ) -> TestResult { + self.packet_test_matched(send, expect)?; + Ok(()) + } + + pub fn packet_test_matched( &self, mut send: Vec, mut expect: Vec, - ) -> TestResult { + ) -> anyhow::Result> { // NOTE: This timeout is critical. // // The tests are normally run against the Tofino simulator, a software @@ -745,11 +754,12 @@ impl Switch { } // We should also see the packets we expected to see + let mut matched = Vec::with_capacity(expect.len()); for (e, expected) in expect.iter().enumerate() { for (g, got) in captured.iter().enumerate() { if expected == got { found.push(e); - captured.remove(g); + matched.push(captured.remove(g)); break; } } @@ -790,7 +800,7 @@ impl Switch { } match errors.len() { - 0 => Ok(()), + 0 => Ok(matched), _ => Err(anyhow!( errors .iter() diff --git a/dpd-client/tests/integration_tests/mcast.rs b/dpd-client/tests/integration_tests/mcast.rs index 1d8a003b..3982735e 100644 --- a/dpd-client/tests/integration_tests/mcast.rs +++ b/dpd-client/tests/integration_tests/mcast.rs @@ -2633,6 +2633,726 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_underlay_and_external_membe cleanup_test_group(switch, MULTICAST_NAT_IP.into(), TEST_TAG).await } +fn build_inner_ttl_geneve_pkt( + external_ip: IpAddr, + src_mac: MacAddr, + vlan: Option, + nat_target: &types::NatTarget, + inner_ttl: u8, + mcast_tag: u8, + inner_eth: bool, +) -> (packet::Packet, packet::Packet) { + let src_port = 3333; + let dst_port = 4444; + + let og_pkt = match external_ip { + IpAddr::V4(_) => { + let mut pkt = create_ipv4_multicast_packet( + external_ip, + src_mac, + "192.168.1.10", + src_port, + dst_port, + vlan, + ); + let current = pkt.hdrs.ipv4_hdr.as_ref().unwrap().ipv4_ttl; + ipv4::Ipv4Hdr::adjust_ttl( + &mut pkt, + i16::from(inner_ttl) - i16::from(current), + ); + pkt + } + IpAddr::V6(_) => { + let mut pkt = create_ipv6_multicast_packet( + external_ip, + src_mac, + "2001:db8::1", + src_port, + dst_port, + vlan, + ); + let current = pkt.hdrs.ipv6_hdr.as_ref().unwrap().ipv6_hop_lim; + ipv6::Ipv6Hdr::adjust_hlim( + &mut pkt, + i16::from(inner_ttl) - i16::from(current), + ); + pkt + } + }; + + let (payload, ether_type) = if inner_eth { + let mut inner = og_pkt.clone(); + inner.hdrs.eth_hdr.as_mut().unwrap().eth_8021q = None; + (inner.deparse().unwrap().to_vec(), eth::ETHER_ETHER) + } else { + let eth_hdr_len = og_pkt.hdrs.eth_hdr.as_ref().unwrap().hdr_len(); + let ether_type = match external_ip { + IpAddr::V4(_) => eth::ETHER_IPV4, + IpAddr::V6(_) => eth::ETHER_IPV6, + }; + (og_pkt.deparse().unwrap()[eth_hdr_len..].to_vec(), ether_type) + }; + + let geneve_pkt = common::gen_geneve_packet_with_mcast_tag( + Endpoint::parse( + GIMLET_MAC, + &GIMLET_IP.to_string(), + geneve::GENEVE_UDP_PORT, + ) + .unwrap(), + Endpoint::parse( + &MacAddr::from(nat_target.internal_ip.derive_multicast_mac()) + .to_string(), + &nat_target.internal_ip.to_string(), + geneve::GENEVE_UDP_PORT, + ) + .unwrap(), + ether_type, + nat_target.vni.clone().into(), + mcast_tag, + &payload, + ); + + (og_pkt, geneve_pkt) +} + +async fn run_bifurcated_inner_ttl_test( + switch: &Switch, + external_ip: IpAddr, + inner_ttl: u8, + drop_label: Option<&str>, + inner_eth: bool, +) -> TestResult { + let ingress = PhysPort(10); + let egress_external = PhysPort(15); + let egress_underlay = PhysPort(19); + + let internal_multicast_ip = IpAddr::V6(MULTICAST_NAT_IP); + create_test_multicast_group( + switch, + internal_multicast_ip, + Some(TEST_TAG), + &[ + (egress_external, types::Direction::External), + (egress_underlay, types::Direction::Underlay), + ], + types::InternalForwarding { nat_target: None }, + types::ExternalForwarding { vlan_id: None }, + None, + ) + .await; + + let vlan = Some(10); + let nat_target = match external_ip { + IpAddr::V4(_) => create_nat_target_ipv4(), + IpAddr::V6(_) => create_nat_target_ipv6(), + }; + + let created_group = create_test_multicast_group( + switch, + external_ip, + Some(TEST_TAG), + &[], + types::InternalForwarding { nat_target: Some(nat_target.clone()) }, + types::ExternalForwarding { vlan_id: vlan }, + None, + ) + .await; + + let src_mac = switch.get_port_mac(ingress).unwrap(); + let (og_pkt, geneve_pkt) = build_inner_ttl_geneve_pkt( + external_ip, + src_mac, + vlan, + &nat_target, + inner_ttl, + 2, + inner_eth, + ); + + let test_pkt = + TestPacket { packet: Arc::new(geneve_pkt.clone()), port: ingress }; + + let underlay_pkt = prepare_expected_pkt( + switch, + &geneve_pkt, + None, + None, + Some(egress_underlay), + ); + + let mut expected_pkts = Vec::new(); + if drop_label.is_none() { + let external_pkt = prepare_expected_pkt( + switch, + &og_pkt, + vlan, + None, + Some(egress_external), + ); + expected_pkts.push(TestPacket { + packet: Arc::new(external_pkt), + port: egress_external, + }); + } + expected_pkts.push(TestPacket { + packet: Arc::new(underlay_pkt), + port: egress_underlay, + }); + + let port_label_external = switch.port_label(egress_external).unwrap(); + let port_label_underlay = switch.port_label(egress_underlay).unwrap(); + + let ctr_baseline_external = switch + .get_counter(&port_label_external, Some("multicast_external")) + .await + .unwrap(); + let ctr_baseline_underlay = switch + .get_counter(&port_label_underlay, Some("multicast_underlay")) + .await + .unwrap(); + let ctr_baseline_drop_port = switch + .get_counter(&port_label_external, Some("egress_drop_port")) + .await + .unwrap(); + let ctr_baseline_drop_reason = match drop_label { + Some(label) => { + switch.get_counter(label, Some("egress_drop_reason")).await.unwrap() + } + None => 0, + }; + + let matched = + switch.packet_test_matched(vec![test_pkt], expected_pkts).unwrap(); + let underlay_got = + matched.iter().find(|p| p.port == egress_underlay).unwrap(); + assert_eq!( + underlay_got.packet.body, geneve_pkt.body, + "underlay replica must carry the unmodified inner frame" + ); + + check_counter_incremented( + switch, + &port_label_underlay, + ctr_baseline_underlay, + 1, + Some("multicast_underlay"), + ) + .await + .unwrap(); + + match drop_label { + Some(label) => { + check_counter_incremented( + switch, + label, + ctr_baseline_drop_reason, + 1, + Some("egress_drop_reason"), + ) + .await + .unwrap(); + check_counter_incremented( + switch, + &port_label_external, + ctr_baseline_drop_port, + 1, + Some("egress_drop_port"), + ) + .await + .unwrap(); + + let ctr_external = switch + .get_counter(&port_label_external, Some("multicast_external")) + .await + .unwrap(); + assert_eq!( + ctr_external, ctr_baseline_external, + "dropped external replica must not hit the external counter" + ); + } + None => { + check_counter_incremented( + switch, + &port_label_external, + ctr_baseline_external, + 1, + Some("multicast_external"), + ) + .await + .unwrap(); + + let ctr_drop_port = switch + .get_counter(&port_label_external, Some("egress_drop_port")) + .await + .unwrap(); + assert_eq!( + ctr_drop_port, ctr_baseline_drop_port, + "forwarded external replica must not be counted as dropped" + ); + } + } + + cleanup_test_group(switch, get_group_ip(&created_group), TEST_TAG) + .await + .unwrap(); + cleanup_test_group(switch, MULTICAST_NAT_IP.into(), TEST_TAG).await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_bifurcated_v4_zero() -> TestResult { + let switch = &*get_switch().await; + run_bifurcated_inner_ttl_test( + switch, + IpAddr::V4(MULTICAST_TEST_IPV4), + 0, + Some("multicast_ttl_exceeded"), + false, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_bifurcated_v4_one() -> TestResult { + let switch = &*get_switch().await; + run_bifurcated_inner_ttl_test( + switch, + IpAddr::V4(MULTICAST_TEST_IPV4), + 1, + Some("multicast_ttl_exceeded"), + false, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_bifurcated_v4_two() -> TestResult { + let switch = &*get_switch().await; + run_bifurcated_inner_ttl_test( + switch, + IpAddr::V4(MULTICAST_TEST_IPV4), + 2, + None, + false, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_bifurcated_v6_zero() -> TestResult { + let switch = &*get_switch().await; + run_bifurcated_inner_ttl_test( + switch, + IpAddr::V6(MULTICAST_TEST_IPV6), + 0, + Some("multicast_ttl_exceeded"), + false, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_bifurcated_v6_one() -> TestResult { + let switch = &*get_switch().await; + run_bifurcated_inner_ttl_test( + switch, + IpAddr::V6(MULTICAST_TEST_IPV6), + 1, + Some("multicast_ttl_exceeded"), + false, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_bifurcated_v6_two() -> TestResult { + let switch = &*get_switch().await; + run_bifurcated_inner_ttl_test( + switch, + IpAddr::V6(MULTICAST_TEST_IPV6), + 2, + None, + false, + ) + .await +} + +async fn run_external_only_inner_ttl_test( + switch: &Switch, + external_ip: IpAddr, + inner_ttl: u8, + expect_drop: bool, + inner_eth: bool, +) -> TestResult { + let ingress = PhysPort(10); + let egress1 = PhysPort(15); + let egress2 = PhysPort(17); + + let internal_multicast_ip = IpAddr::V6(MULTICAST_NAT_IP); + create_test_multicast_group( + switch, + internal_multicast_ip, + Some(TEST_TAG), + &[ + (egress1, types::Direction::External), + (egress2, types::Direction::External), + ], + types::InternalForwarding { nat_target: None }, + types::ExternalForwarding { vlan_id: None }, + None, + ) + .await; + + let vlan = Some(10); + let nat_target = match external_ip { + IpAddr::V4(_) => create_nat_target_ipv4(), + IpAddr::V6(_) => create_nat_target_ipv6(), + }; + + let created_group = create_test_multicast_group( + switch, + external_ip, + Some(TEST_TAG), + &[], + types::InternalForwarding { nat_target: Some(nat_target.clone()) }, + types::ExternalForwarding { vlan_id: vlan }, + None, + ) + .await; + + let src_mac = switch.get_port_mac(ingress).unwrap(); + let (og_pkt, geneve_pkt) = build_inner_ttl_geneve_pkt( + external_ip, + src_mac, + vlan, + &nat_target, + inner_ttl, + 0, + inner_eth, + ); + + let test_pkt = TestPacket { packet: Arc::new(geneve_pkt), port: ingress }; + + let expected_pkts = if expect_drop { + Vec::new() + } else { + vec![ + TestPacket { + packet: Arc::new(prepare_expected_pkt( + switch, + &og_pkt, + vlan, + None, + Some(egress1), + )), + port: egress1, + }, + TestPacket { + packet: Arc::new(prepare_expected_pkt( + switch, + &og_pkt, + vlan, + None, + Some(egress2), + )), + port: egress2, + }, + ] + }; + + let port_label_ingress = switch.port_label(ingress).unwrap(); + let port_label_egress1 = switch.port_label(egress1).unwrap(); + let port_label_egress2 = switch.port_label(egress2).unwrap(); + + let ctr_baseline_drop_reason = + switch.get_counter("multicast_ttl_exceeded", None).await.unwrap(); + let ctr_baseline_drop_port = switch + .get_counter(&port_label_ingress, Some("ingress_drop_port")) + .await + .unwrap(); + let ctr_baseline_external_egress1 = switch + .get_counter(&port_label_egress1, Some("multicast_external")) + .await + .unwrap(); + let ctr_baseline_external_egress2 = switch + .get_counter(&port_label_egress2, Some("multicast_external")) + .await + .unwrap(); + + switch.packet_test(vec![test_pkt], expected_pkts).unwrap(); + + if expect_drop { + check_counter_incremented( + switch, + "multicast_ttl_exceeded", + ctr_baseline_drop_reason, + 1, + None, + ) + .await + .unwrap(); + check_counter_incremented( + switch, + &port_label_ingress, + ctr_baseline_drop_port, + 1, + Some("ingress_drop_port"), + ) + .await + .unwrap(); + + let ctr_external_egress1 = switch + .get_counter(&port_label_egress1, Some("multicast_external")) + .await + .unwrap(); + let ctr_external_egress2 = switch + .get_counter(&port_label_egress2, Some("multicast_external")) + .await + .unwrap(); + assert_eq!( + ctr_external_egress1, ctr_baseline_external_egress1, + "dropped packet must not be replicated to external members" + ); + assert_eq!( + ctr_external_egress2, ctr_baseline_external_egress2, + "dropped packet must not be replicated to external members" + ); + } else { + check_counter_incremented( + switch, + &port_label_egress1, + ctr_baseline_external_egress1, + 1, + Some("multicast_external"), + ) + .await + .unwrap(); + check_counter_incremented( + switch, + &port_label_egress2, + ctr_baseline_external_egress2, + 1, + Some("multicast_external"), + ) + .await + .unwrap(); + + let ctr_drop_reason = + switch.get_counter("multicast_ttl_exceeded", None).await.unwrap(); + assert_eq!( + ctr_drop_reason, ctr_baseline_drop_reason, + "forwarded packet must not be counted as a TTL drop" + ); + } + + cleanup_test_group(switch, get_group_ip(&created_group), TEST_TAG) + .await + .unwrap(); + cleanup_test_group(switch, MULTICAST_NAT_IP.into(), TEST_TAG).await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_external_v4_zero() -> TestResult { + let switch = &*get_switch().await; + run_external_only_inner_ttl_test( + switch, + IpAddr::V4(MULTICAST_TEST_IPV4), + 0, + true, + false, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_external_v4_one() -> TestResult { + let switch = &*get_switch().await; + run_external_only_inner_ttl_test( + switch, + IpAddr::V4(MULTICAST_TEST_IPV4), + 1, + true, + false, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_external_v4_two() -> TestResult { + let switch = &*get_switch().await; + run_external_only_inner_ttl_test( + switch, + IpAddr::V4(MULTICAST_TEST_IPV4), + 2, + false, + false, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_external_v6_zero() -> TestResult { + let switch = &*get_switch().await; + run_external_only_inner_ttl_test( + switch, + IpAddr::V6(MULTICAST_TEST_IPV6), + 0, + true, + false, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_external_v6_one() -> TestResult { + let switch = &*get_switch().await; + run_external_only_inner_ttl_test( + switch, + IpAddr::V6(MULTICAST_TEST_IPV6), + 1, + true, + false, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_external_v6_two() -> TestResult { + let switch = &*get_switch().await; + run_external_only_inner_ttl_test( + switch, + IpAddr::V6(MULTICAST_TEST_IPV6), + 2, + false, + false, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_bifurcated_v4_eth_one() -> TestResult { + let switch = &*get_switch().await; + run_bifurcated_inner_ttl_test( + switch, + IpAddr::V4(MULTICAST_TEST_IPV4), + 1, + Some("multicast_ttl_exceeded"), + true, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_bifurcated_v6_eth_one() -> TestResult { + let switch = &*get_switch().await; + run_bifurcated_inner_ttl_test( + switch, + IpAddr::V6(MULTICAST_TEST_IPV6), + 1, + Some("multicast_ttl_exceeded"), + true, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_external_v4_eth_one() -> TestResult { + let switch = &*get_switch().await; + run_external_only_inner_ttl_test( + switch, + IpAddr::V4(MULTICAST_TEST_IPV4), + 1, + true, + true, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_external_v6_eth_one() -> TestResult { + let switch = &*get_switch().await; + run_external_only_inner_ttl_test( + switch, + IpAddr::V6(MULTICAST_TEST_IPV6), + 1, + true, + true, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_bifurcated_v4_eth_two() -> TestResult { + let switch = &*get_switch().await; + run_bifurcated_inner_ttl_test( + switch, + IpAddr::V4(MULTICAST_TEST_IPV4), + 2, + None, + true, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_bifurcated_v6_eth_two() -> TestResult { + let switch = &*get_switch().await; + run_bifurcated_inner_ttl_test( + switch, + IpAddr::V6(MULTICAST_TEST_IPV6), + 2, + None, + true, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_external_v4_eth_two() -> TestResult { + let switch = &*get_switch().await; + run_external_only_inner_ttl_test( + switch, + IpAddr::V4(MULTICAST_TEST_IPV4), + 2, + false, + true, + ) + .await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_external_v6_eth_two() -> TestResult { + let switch = &*get_switch().await; + run_external_only_inner_ttl_test( + switch, + IpAddr::V6(MULTICAST_TEST_IPV6), + 2, + false, + true, + ) + .await +} + /// Link-local replicas reach egress with a zero replication id, so the /// counter block reaches them through `is_link_local_ipv6_mcast` rather /// than `egress_rid`. Both the underlay and external conditions exclude diff --git a/dpd-client/tests/integration_tests/nat.rs b/dpd-client/tests/integration_tests/nat.rs index 9d7372a1..0b4c9e9c 100644 --- a/dpd-client/tests/integration_tests/nat.rs +++ b/dpd-client/tests/integration_tests/nat.rs @@ -195,7 +195,11 @@ struct NatTest { geneve_vni: u32, } -async fn test_nat_egress(switch: &Switch, test: &NatTest) -> TestResult { +async fn test_nat_egress( + switch: &Switch, + test: &NatTest, + inner_ttl: Option, +) -> TestResult { let router_mac = test.router_mac.parse()?; // set up the switch internals so we can route the packet from the gimlet @@ -247,10 +251,10 @@ async fn test_nat_egress(switch: &Switch, test: &NatTest) -> TestResult { // responsibility, then this needs to be done on the 'expected' packet // rather than the 'to_send' packet. let inner = { - if let Some(mut ipv4) = payload_pkt.hdrs.ipv4_hdr { + if let Some(ipv4) = payload_pkt.hdrs.ipv4_hdr.as_mut() { ipv4.ipv4_src_ip = test.uplink_port_external.parse()?; Ok(eth::ETHER_IPV4) - } else if let Some(mut ipv6) = payload_pkt.hdrs.ipv6_hdr { + } else if let Some(ipv6) = payload_pkt.hdrs.ipv6_hdr.as_mut() { ipv6.ipv6_src_ip = test.uplink_port_external.parse()?; Ok(eth::ETHER_IPV6) } else { @@ -276,6 +280,14 @@ async fn test_nat_egress(switch: &Switch, test: &NatTest) -> TestResult { } }; + if let Some(ttl) = inner_ttl { + if let Some(ipv4) = payload_pkt.hdrs.ipv4_hdr.as_mut() { + ipv4.ipv4_ttl = ttl; + } else if let Some(ipv6) = payload_pkt.hdrs.ipv6_hdr.as_mut() { + ipv6.ipv6_hop_lim = ttl; + } + } + if inner == eth::ETHER_IPV4 { ipv4::Ipv4Hdr::update_checksum(&mut payload_pkt); } @@ -296,25 +308,57 @@ async fn test_nat_egress(switch: &Switch, test: &NatTest) -> TestResult { &payload[14..], ); - let expected = match test.uplink_port_registered { - true => { - let mut to_recv = common::gen_packet_routed( - switch, - test.uplink_port, - &payload_pkt, - ); - eth::EthHdr::rewrite_dmac(&mut to_recv, router_mac); - vec![TestPacket { - packet: Arc::new(to_recv), - port: test.uplink_port, - }] - } - false => Vec::new(), + let ttl_expired = matches!(inner_ttl, Some(0 | 1)); + let expected = if ttl_expired { + let mut punt = payload_pkt.clone(); + let eth_hdr = punt.hdrs.eth_hdr.as_mut().unwrap(); + eth_hdr.eth_smac = test.gimlet_mac.parse()?; + eth_hdr.eth_dmac = switch_mac.parse()?; + let icmp_type = match inner { + eth::ETHER_IPV4 => icmp::ICMP_TIME_EXCEEDED, + _ => icmp::ICMP6_TIME_EXCEEDED, + }; + common::set_icmp_needed( + switch, + &mut punt, + test.gimlet_port, + NO_PORT, + icmp_type, + 0, + ); + vec![TestPacket { packet: Arc::new(punt), port: SERVICE_PORT }] + } else if test.uplink_port_registered { + let mut to_recv = + common::gen_packet_routed(switch, test.uplink_port, &payload_pkt); + eth::EthHdr::rewrite_dmac(&mut to_recv, router_mac); + vec![TestPacket { packet: Arc::new(to_recv), port: test.uplink_port }] + } else { + // Egress to a non-uplink port is blocked by NatEgressFilter, and the + // counter check below confirms the drop reason. + Vec::new() }; let send = TestPacket { packet: Arc::new(to_send), port: test.gimlet_port }; - switch.packet_test(vec![send], expected) + if ttl_expired || test.uplink_port_registered { + return switch.packet_test(vec![send], expected); + } + + let blocked = "nat_egress_blocked"; + let ctr_baseline = + switch.get_counter(blocked, Some("egress_drop_reason")).await?; + + switch.packet_test(vec![send], expected)?; + + check_counter_incremented( + switch, + blocked, + ctr_baseline, + 1, + Some("egress_drop_reason"), + ) + .await?; + Ok(()) } async fn test_nat_ingress(switch: &Switch, test: &NatTest) -> TestResult { @@ -466,6 +510,7 @@ async fn test_egress_ipv4( switch: &Switch, l4_protocol: L4Protocol, uplink_port_registered: bool, + inner_ttl: Option, ) -> TestResult { let test = NatTest { uplink_port: PhysPort(14), @@ -492,7 +537,7 @@ async fn test_egress_ipv4( geneve_vni: 1, // not used on egress tests }; - test_nat_egress(switch, &test).await + test_nat_egress(switch, &test, inner_ttl).await } // UDP packet to/from IPv4 addresses @@ -500,7 +545,7 @@ async fn test_egress_ipv4( #[ignore] async fn test_egress_ipv4_udp() -> TestResult { let switch = &*get_switch().await; - test_egress_ipv4(switch, L4Protocol::Udp, true).await + test_egress_ipv4(switch, L4Protocol::Udp, true, None).await } // TCP packet to/from IPv4 addresses @@ -508,7 +553,7 @@ async fn test_egress_ipv4_udp() -> TestResult { #[ignore] async fn test_egress_ipv4_tcp() -> TestResult { let switch = &*get_switch().await; - test_egress_ipv4(switch, L4Protocol::Tcp, true).await + test_egress_ipv4(switch, L4Protocol::Tcp, true, None).await } // ICMP packet to/from IPv4 addresses @@ -516,7 +561,7 @@ async fn test_egress_ipv4_tcp() -> TestResult { #[ignore] async fn test_egress_ipv4_icmp() -> TestResult { let switch = &*get_switch().await; - test_egress_ipv4(switch, L4Protocol::Icmp, true).await + test_egress_ipv4(switch, L4Protocol::Icmp, true, None).await } // UDP packet to an IPv4 address, egressing a backplane port. @@ -524,7 +569,7 @@ async fn test_egress_ipv4_icmp() -> TestResult { #[ignore] async fn test_backplane_egress_ipv4_udp() -> TestResult { let switch = &*get_switch().await; - test_egress_ipv4(switch, L4Protocol::Udp, false).await + test_egress_ipv4(switch, L4Protocol::Udp, false, None).await } async fn test_ingress_ipv4( @@ -593,6 +638,7 @@ async fn test_egress_ipv6( switch: &Switch, l4_protocol: L4Protocol, uplink_port_registered: bool, + inner_ttl: Option, ) -> TestResult { let test = NatTest { uplink_port: PhysPort(14), @@ -619,28 +665,70 @@ async fn test_egress_ipv6( geneve_vni: 1, // not used on egress tests }; - test_nat_egress(switch, &test).await + test_nat_egress(switch, &test, inner_ttl).await } #[tokio::test] #[ignore] async fn test_egress_ipv6_udp() -> TestResult { let switch = &*get_switch().await; - test_egress_ipv6(switch, L4Protocol::Udp, true).await + test_egress_ipv6(switch, L4Protocol::Udp, true, None).await } #[tokio::test] #[ignore] async fn test_egress_ipv6_tcp() -> TestResult { let switch = &*get_switch().await; - test_egress_ipv6(switch, L4Protocol::Tcp, true).await + test_egress_ipv6(switch, L4Protocol::Tcp, true, None).await } #[tokio::test] #[ignore] async fn test_backplane_egress_ipv6_tcp() -> TestResult { let switch = &*get_switch().await; - test_egress_ipv6(switch, L4Protocol::Tcp, false).await + test_egress_ipv6(switch, L4Protocol::Tcp, false, None).await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_egress_v4_zero() -> TestResult { + let switch = &*get_switch().await; + test_egress_ipv4(switch, L4Protocol::Udp, true, Some(0)).await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_egress_v4_one() -> TestResult { + let switch = &*get_switch().await; + test_egress_ipv4(switch, L4Protocol::Udp, true, Some(1)).await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_egress_v4_two() -> TestResult { + let switch = &*get_switch().await; + test_egress_ipv4(switch, L4Protocol::Udp, true, Some(2)).await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_egress_v6_zero() -> TestResult { + let switch = &*get_switch().await; + test_egress_ipv6(switch, L4Protocol::Udp, true, Some(0)).await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_egress_v6_one() -> TestResult { + let switch = &*get_switch().await; + test_egress_ipv6(switch, L4Protocol::Udp, true, Some(1)).await +} + +#[tokio::test] +#[ignore] +async fn test_inner_ttl_egress_v6_two() -> TestResult { + let switch = &*get_switch().await; + test_egress_ipv6(switch, L4Protocol::Udp, true, Some(2)).await } async fn test_ingress_ipv6( diff --git a/dpd-client/tests/integration_tests/service.rs b/dpd-client/tests/integration_tests/service.rs index 2aeff4a5..bdf99cac 100644 --- a/dpd-client/tests/integration_tests/service.rs +++ b/dpd-client/tests/integration_tests/service.rs @@ -260,7 +260,7 @@ async fn test_ttl_exceeded() -> TestResult { switch, &mut recv_pkt, ingress, - SERVICE_PORT, + NO_PORT, icmp::ICMP_TIME_EXCEEDED, 0, ); diff --git a/dpd/p4/constants.p4 b/dpd/p4/constants.p4 index 225301b6..d565722a 100644 --- a/dpd/p4/constants.p4 +++ b/dpd/p4/constants.p4 @@ -96,5 +96,9 @@ const bit<8> DROP_GENEVE_OPTIONS_TOO_LONG = 0x18; const bit<8> DROP_GENEVE_OPTION_MALFORMED = 0x19; const bit<8> DROP_GENEVE_OPTION_UNKNOWN = 0x1A; const bit<8> DROP_SCTP = 0x1B; +// No Time Exceeded for multicast +// (see RFC 1812 §4.3.2.7, §5.3.1; +// RFC 4443 §2.4(e.3)) +const bit<8> DROP_MULTICAST_TTL_EXCEEDED = 0x1C; // MAX(DROP_xxx) + 1 -const bit<32> DROP_REASON_MAX = 0x1C; +const bit<32> DROP_REASON_MAX = 0x1D; diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index 9cd96b8d..5e8cba49 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -342,7 +342,10 @@ control Services( action mcast_inbound_link_local() { hdr.sidecar.sc_code = SC_FWD_TO_USERSPACE; hdr.sidecar.sc_ingress = (bit<16>)ig_intr_md.ingress_port; - hdr.sidecar.sc_egress = (bit<16>)ig_tm_md.ucast_egress_port; + // Inbound packets do not have an egress port yet, and reading + // the unset ucast_egress_port yields an unspecified value: + // https://p4.org/wp-content/uploads/sites/53/p4-spec/docs/P4-16-v1.2.4.html#sec-uninitialized-values-and-writing-invalid-headers + hdr.sidecar.sc_egress = 0; hdr.sidecar.sc_ether_type = hdr.ethernet.ether_type; hdr.sidecar.sc_payload = 0; hdr.sidecar.setValid(); @@ -1235,7 +1238,10 @@ control Router4 ( action icmp_error(bit<8> type, bit<8> code) { hdr.sidecar.sc_code = SC_ICMP_NEEDED; hdr.sidecar.sc_ingress = (bit<16>)ig_intr_md.ingress_port; - hdr.sidecar.sc_egress = (bit<16>)ig_tm_md.ucast_egress_port; + // No path into this action has yet assigned ucast_egress_port, + // and reading it before assignment yields an unspecified value: + // https://p4.org/wp-content/uploads/sites/53/p4-spec/docs/P4-16-v1.2.4.html#sec-uninitialized-values-and-writing-invalid-headers + hdr.sidecar.sc_egress = 0; hdr.sidecar.sc_ether_type = hdr.ethernet.ether_type; hdr.sidecar.sc_payload = (bit<128>)type << 8 | (bit<128>)code; hdr.sidecar.setValid(); @@ -1272,7 +1278,7 @@ control Router4 ( // Dont set meta.dropped because we want an error packet // to go out. meta.drop_reason = DROP_IPV4_UNROUTEABLE; - } else if (hdr.ipv4.ttl == 1 && !IS_SERVICE(fwd.port)) { + } else if (hdr.ipv4.ttl <= 8w1 && !IS_SERVICE(fwd.port)) { icmp_error(ICMP_TIME_EXCEEDED, ICMP_EXC_TTL); // Dont set meta.dropped because we want an error packet // to go out. @@ -1291,23 +1297,10 @@ control Router4 ( control MulticastRouter4( inout sidecar_headers_t hdr, inout sidecar_ingress_meta_t meta, - in ingress_intrinsic_metadata_t ig_intr_md, inout ingress_intrinsic_metadata_for_tm_t ig_tm_md ) { DirectCounter>(CounterType_t.PACKETS_AND_BYTES) ctr; - action icmp_error(bit<8> type, bit<8> code) { - hdr.sidecar.sc_code = SC_ICMP_NEEDED; - hdr.sidecar.sc_ingress = (bit<16>)ig_intr_md.ingress_port; - hdr.sidecar.sc_egress = (bit<16>)ig_tm_md.ucast_egress_port; - hdr.sidecar.sc_ether_type = hdr.ethernet.ether_type; - hdr.sidecar.sc_payload = (bit<128>)type << 8 | (bit<128>)code; - hdr.sidecar.setValid(); - hdr.ethernet.ether_type = ETHERTYPE_SIDECAR; - meta.service_routed = true; - ig_tm_md.ucast_egress_port = USER_SPACE_SERVICE_PORT; - } - action unreachable() { ctr.count(); } @@ -1348,16 +1341,15 @@ control MulticastRouter4( hdr.vlan.setInvalid(); } + // ICMP errors must not be sent for multicast destinations: + // https://www.rfc-editor.org/rfc/rfc1812.html#section-4.3.2.7 if (!tbl.apply().hit) { - icmp_error(ICMP_DEST_UNREACH, ICMP_DST_UNREACH_NET); meta.drop_reason = DROP_IPV4_UNROUTEABLE; - // Dont set meta.dropped because we want an error packet - // to go out. - } else if (hdr.ipv4.ttl == 1 && !meta.service_routed) { - icmp_error(ICMP_TIME_EXCEEDED, ICMP_EXC_TTL); - meta.drop_reason = DROP_IPV4_TTL_INVALID; - // Dont set meta.dropped because we want an error packet - // to go out. + meta.dropped = true; + } else if (hdr.ipv4.ttl <= 8w1) { + // https://www.rfc-editor.org/rfc/rfc1812.html#section-5.3.1 + meta.drop_reason = DROP_MULTICAST_TTL_EXCEEDED; + meta.dropped = true; } else { // Set the destination port to an invalid value ig_tm_md.ucast_egress_port = (PortId_t)0x1ff; @@ -1380,7 +1372,10 @@ control Router6 ( action icmp_error(bit<8> type, bit<8> code) { hdr.sidecar.sc_code = SC_ICMP_NEEDED; hdr.sidecar.sc_ingress = (bit<16>)ig_intr_md.ingress_port; - hdr.sidecar.sc_egress = (bit<16>)ig_tm_md.ucast_egress_port; + // No path into this action has yet assigned ucast_egress_port, + // and reading it before assignment yields an unspecified value: + // https://p4.org/wp-content/uploads/sites/53/p4-spec/docs/P4-16-v1.2.4.html#sec-uninitialized-values-and-writing-invalid-headers + hdr.sidecar.sc_egress = 0; hdr.sidecar.sc_ether_type = hdr.ethernet.ether_type; hdr.sidecar.sc_payload = (bit<128>)type << 8 | (bit<128>)code; hdr.sidecar.setValid(); @@ -1415,7 +1410,7 @@ control Router6 ( meta.drop_reason = DROP_IPV6_UNROUTEABLE; // Dont set meta.dropped because we want an error packet // to go out. - } else if (hdr.ipv6.hop_limit == 1 && !IS_SERVICE(fwd.port)) { + } else if (hdr.ipv6.hop_limit <= 8w1 && !IS_SERVICE(fwd.port)) { icmp_error(ICMP6_TIME_EXCEEDED, ICMP_EXC_TTL); meta.drop_reason = DROP_IPV6_TTL_EXCEEDED; // Dont set meta.dropped because we want an error packet @@ -1432,23 +1427,10 @@ control Router6 ( control MulticastRouter6 ( inout sidecar_headers_t hdr, inout sidecar_ingress_meta_t meta, - in ingress_intrinsic_metadata_t ig_intr_md, inout ingress_intrinsic_metadata_for_tm_t ig_tm_md ) { DirectCounter>(CounterType_t.PACKETS_AND_BYTES) ctr; - action icmp_error(bit<8> type, bit<8> code) { - hdr.sidecar.sc_code = SC_ICMP_NEEDED; - hdr.sidecar.sc_ingress = (bit<16>)ig_intr_md.ingress_port; - hdr.sidecar.sc_egress = (bit<16>)ig_tm_md.ucast_egress_port; - hdr.sidecar.sc_ether_type = hdr.ethernet.ether_type; - hdr.sidecar.sc_payload = (bit<128>)type << 8 | (bit<128>)code; - hdr.sidecar.setValid(); - hdr.ethernet.ether_type = ETHERTYPE_SIDECAR; - meta.service_routed = true; - ig_tm_md.ucast_egress_port = USER_SPACE_SERVICE_PORT; - } - action unreachable() { ctr.count(); } @@ -1488,16 +1470,17 @@ control MulticastRouter6 ( hdr.vlan.setInvalid(); } + // ICMPv6 errors must not be originated for multicast + // destinations; neither exception in RFC 4443 §2.4(e.3) + // (Packet Too Big, Parameter Problem code 2) applies here: + // https://www.rfc-editor.org/rfc/rfc4443.html#section-2.4 if (!tbl.apply().hit) { - icmp_error(ICMP6_DST_UNREACH, ICMP6_DST_UNREACH_NOROUTE); meta.drop_reason = DROP_IPV6_UNROUTEABLE; - // Dont set meta.dropped because we want an error packet - // to go out. - } else if (hdr.ipv6.hop_limit == 1) { - icmp_error(ICMP6_TIME_EXCEEDED, ICMP_EXC_TTL); - meta.drop_reason = DROP_IPV6_TTL_EXCEEDED; - // Dont set meta.dropped because we want an error packet - // to go out. + meta.dropped = true; + } else if (hdr.ipv6.hop_limit <= 8w1) { + // https://www.rfc-editor.org/rfc/rfc8200.html#section-3 + meta.drop_reason = DROP_MULTICAST_TTL_EXCEEDED; + meta.dropped = true; } else { // Set the destination port to an invalid value ig_tm_md.ucast_egress_port = (PortId_t)0x1ff; @@ -1515,13 +1498,13 @@ control L3Router( apply { if (hdr.ipv4.isValid()) { if (meta.is_mcast && !meta.is_link_local_mcastv6) { - MulticastRouter4.apply(hdr, meta, ig_intr_md, ig_tm_md); + MulticastRouter4.apply(hdr, meta, ig_tm_md); } else { Router4.apply(hdr, meta, ig_intr_md, ig_tm_md); } } else if (hdr.ipv6.isValid()) { if (meta.is_mcast && !meta.is_link_local_mcastv6) { - MulticastRouter6.apply(hdr, meta, ig_intr_md, ig_tm_md); + MulticastRouter6.apply(hdr, meta, ig_tm_md); } else { Router6.apply(hdr, meta, ig_intr_md, ig_tm_md); } @@ -1864,7 +1847,10 @@ control MulticastIngress ( * 3. asic_id_to_port : Map ASIC port ID to logical port number (0-255) * 4. port_bitmap_check : Test port's bit in bitmap (see port_bitmap_check.p4) * 5. modify_hdr : If bitmap_result != 0, decapsulate packet (strip - * outer headers, decrement TTL/hop limit, handle VLAN) + * outer headers, decrement TTL/hop limit, handle VLAN), + * unless the inner TTL/hop limit is <= 1, in which + * case the replica is dropped with + * DROP_MULTICAST_TTL_EXCEEDED * * The bitmap marks which egress ports require decapsulation (typically external * customer-facing ports) vs which keep encapsulation (underlay/sled-bound). @@ -1872,8 +1858,7 @@ control MulticastIngress ( control MulticastEgress ( inout sidecar_headers_t hdr, inout sidecar_egress_meta_t meta, - in egress_intrinsic_metadata_t eg_intr_md, - in egress_intrinsic_metadata_for_deparser_t eg_dprsr_md + in egress_intrinsic_metadata_t eg_intr_md ) { action set_decap_ports( @@ -2055,8 +2040,22 @@ control MulticastEgress ( port_bitmap_check.apply(); } if (meta.bitmap_result != 0) { - meta.ipv4_checksum_recalc = true; - modify_hdr.apply(); + // A decapsulated packet is forwarded + // as a normal hop, which requires the + // inner TTL check before modify_hdr + // actually decrements it: + // https://www.rfc-editor.org/rfc/rfc2003.html#section-3.1 + // https://www.rfc-editor.org/rfc/rfc2473.html#section-3.3 + if (hdr.inner_ipv4.isValid() && + hdr.inner_ipv4.ttl <= 8w1) { + meta.drop_reason = DROP_MULTICAST_TTL_EXCEEDED; + } else if (hdr.inner_ipv6.isValid() && + hdr.inner_ipv6.hop_limit <= 8w1) { + meta.drop_reason = DROP_MULTICAST_TTL_EXCEEDED; + } else { + meta.ipv4_checksum_recalc = true; + modify_hdr.apply(); + } } } } @@ -2337,8 +2336,10 @@ control Egress( // the packet meta.drop_reason = DROP_MULTICAST_PATH_FILTERED; } else { - mcast_egress.apply(hdr, meta, eg_intr_md, eg_dprsr_md); - mac_rewrite.apply(hdr, eg_intr_md.egress_port); + mcast_egress.apply(hdr, meta, eg_intr_md); + if (meta.drop_reason == 0) { + mac_rewrite.apply(hdr, eg_intr_md.egress_port); + } } } else if (meta.bridge_hdr.nat_egress_hit) { egress_filter.apply(meta, eg_intr_md); diff --git a/dpd/src/counters.rs b/dpd/src/counters.rs index b189cb6f..7ae3ec65 100644 --- a/dpd/src/counters.rs +++ b/dpd/src/counters.rs @@ -184,6 +184,7 @@ enum DropReason { GeneveOptionMalformed, GeneveOptionUnknown, DropSctp, + MulticastTtlExceeded, } impl TryFrom for DropReason { @@ -219,6 +220,7 @@ impl TryFrom for DropReason { 25 => Ok(DropReason::GeneveOptionMalformed), 26 => Ok(DropReason::GeneveOptionUnknown), 27 => Ok(DropReason::DropSctp), + 28 => Ok(DropReason::MulticastTtlExceeded), x => Err(format!("Unrecognized drop reason: {x}")), } } @@ -248,7 +250,7 @@ fn reason_label(ctr: u8) -> Result, String> { DropReason::Ipv4Unrouteable => "ipv4_unrouteable".to_string(), DropReason::Ipv6Unrouteable => "ipv6_unrouteable".to_string(), DropReason::NatIngressMiss => "nat_ingress_miss".to_string(), - DropReason::NatEgressBlocked => "nat_engress_blocked".to_string(), + DropReason::NatEgressBlocked => "nat_egress_blocked".to_string(), DropReason::MulticastNoGroup => "multicast_no_group".to_string(), DropReason::MulticastInvalidMac => "multicast_invalid_mac".to_string(), DropReason::MulticastCpuCopy => "multicast_cpu_copy".to_string(), @@ -266,6 +268,9 @@ fn reason_label(ctr: u8) -> Result, String> { } DropReason::GeneveOptionUnknown => "geneve_option_unknown".to_string(), DropReason::DropSctp => "drop_sctp".to_string(), + DropReason::MulticastTtlExceeded => { + "multicast_ttl_exceeded".to_string() + } }; Ok(Some(label)) } From 7e10cfbe43c8370cbb0a53bde42ba2fa19fe0b7e Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Mon, 19 Oct 2026 07:27:55 +0000 Subject: [PATCH 2/4] [p4] receive ttl/hop limit 0 when the switch is the destination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ry flagged this on oxidecomputer/sidecar-lite#152 (its handling of ff02 reject was emulating this sidecar). Ever since #14, the parser dropped ttl/hl (hop-limit) 0 before `services` was involved, which meant that a unicast packet headed to a switch address and `ff02::/16` headed to the scrimlet would both be dropped. RFC 1812 §4.2.2.9 and RFC 8200 §3 mention that the destination should still accept it, while the forwarding pass still has to drop (per RFC 1812 §5.3.1). This work updates the sidecar parser to record the reason via meta and then continue parsing. For unicast and IPv6 link-local multicast, the drop has been moved to the forwarding side. Note: some of the arrangement here is to make this change while operating at our 20-stage max. Includes: - mcast packets at ttl/hl 1 now get counted as `multicast_ttl_exceeded` - Geneve encapped packets headed to a switch address with an outer hop limit of 0 get decapped again, as it was before #14 --- .../tests/integration_tests/counters.rs | 38 +++++++ dpd-client/tests/integration_tests/mcast.rs | 8 +- dpd-client/tests/integration_tests/nat.rs | 37 +++++-- .../tests/integration_tests/route_ipv6.rs | 39 ++++++++ dpd-client/tests/integration_tests/service.rs | 99 ++++++++++++++++++- dpd/p4/parser.p4 | 42 +++++--- dpd/p4/sidecar.p4 | 28 +++++- 7 files changed, 260 insertions(+), 31 deletions(-) diff --git a/dpd-client/tests/integration_tests/counters.rs b/dpd-client/tests/integration_tests/counters.rs index f793f2a6..b9801d47 100644 --- a/dpd-client/tests/integration_tests/counters.rs +++ b/dpd-client/tests/integration_tests/counters.rs @@ -18,9 +18,11 @@ use crate::integration_tests::common; use crate::integration_tests::common::prelude::*; use crate::integration_tests::icmp_ipv4; use ::common::network::MacAddr; +use ::common::network::Vni; use dpd_client::ClientInfo; use dpd_client::types::Ipv4Entry; use dpd_client::types::Ipv6Entry; +use dpd_client::types::NatTarget; // Run a single drop test. This sends a packet that we expect to be dropped, // and verifies that the expected drop counter is bumped by one. If the test @@ -181,6 +183,42 @@ async fn test_ipv4_ttl_invalid() -> TestResult { test_ipv4_ttl_test(0, "ipv4_ttl_invalid").await } +#[tokio::test] +#[ignore] +async fn test_ipv4_ttl_invalid_nat_ingress() -> TestResult { + let switch = &*get_switch().await; + let ingress = PhysPort(14); + let nat_ip = "192.168.1.2"; + let nat_port = 2000; + + let tgt = NatTarget { + internal_ip: "fd00:1122:7788:0101::4".parse().unwrap(), + inner_mac: "04:01:01:01:01:01".parse::()?.into(), + vni: Vni::new(7654).unwrap().into(), + }; + switch + .client + .nat_ipv4_create(&nat_ip.parse().unwrap(), 1024, 2048, &tgt) + .await + .unwrap(); + + let mut packet = common::gen_udp_packet( + Endpoint::parse("02:aa:bb:cc:dd:ee", "10.10.10.32", 4444).unwrap(), + Endpoint::parse( + &switch.get_port_mac(ingress).unwrap().to_string(), + nat_ip, + nat_port, + ) + .unwrap(), + ); + packet.hdrs.ipv4_hdr.as_mut().unwrap().ipv4_ttl = 0; + packet::ipv4::Ipv4Hdr::update_checksum(&mut packet); + + assert!(one_drop_test(switch, ingress, packet, "ipv4_ttl_invalid").await?); + + Ok(()) +} + /// Set a port to accept only inbound traffic with a corresponding NAT mapping. /// Send in a packet that has no corresponding NAT entry, and check the /// nat_ingress_miss counter. diff --git a/dpd-client/tests/integration_tests/mcast.rs b/dpd-client/tests/integration_tests/mcast.rs index 9b70b791..119e040f 100644 --- a/dpd-client/tests/integration_tests/mcast.rs +++ b/dpd-client/tests/integration_tests/mcast.rs @@ -1858,13 +1858,13 @@ async fn test_multicast_ttl_one() -> TestResult { let expected_pkts = vec![]; let ctr_baseline = - switch.get_counter("ipv4_ttl_invalid", None).await.unwrap(); + switch.get_counter("multicast_ttl_exceeded", None).await.unwrap(); switch.packet_test(vec![test_pkt], expected_pkts).unwrap(); check_counter_incremented( switch, - "ipv4_ttl_invalid", + "multicast_ttl_exceeded", ctr_baseline, 1, None, @@ -3716,13 +3716,13 @@ async fn test_ipv6_multicast_hop_limit_one() -> TestResult { let expected_pkts = vec![]; let ctr_baseline = - switch.get_counter("ipv6_ttl_invalid", None).await.unwrap(); + switch.get_counter("multicast_ttl_exceeded", None).await.unwrap(); switch.packet_test(vec![test_pkt], expected_pkts).unwrap(); check_counter_incremented( switch, - "ipv6_ttl_invalid", + "multicast_ttl_exceeded", ctr_baseline, 1, None, diff --git a/dpd-client/tests/integration_tests/nat.rs b/dpd-client/tests/integration_tests/nat.rs index 0b4c9e9c..391a4c13 100644 --- a/dpd-client/tests/integration_tests/nat.rs +++ b/dpd-client/tests/integration_tests/nat.rs @@ -199,6 +199,7 @@ async fn test_nat_egress( switch: &Switch, test: &NatTest, inner_ttl: Option, + outer_hop_limit: Option, ) -> TestResult { let router_mac = test.router_mac.parse()?; @@ -294,7 +295,7 @@ async fn test_nat_egress( let switch_mac = switch.get_port_mac(test.gimlet_port).unwrap().to_string(); let payload = payload_pkt.deparse().unwrap().to_vec(); - let to_send = common::gen_geneve_packet( + let mut to_send = common::gen_geneve_packet( Endpoint::parse(&test.gimlet_mac, &test.gimlet_ip, 3333).unwrap(), Endpoint::parse( &switch_mac, @@ -308,6 +309,10 @@ async fn test_nat_egress( &payload[14..], ); + if let Some(hop_limit) = outer_hop_limit { + to_send.hdrs.ipv6_hdr.as_mut().unwrap().ipv6_hop_lim = hop_limit; + } + let ttl_expired = matches!(inner_ttl, Some(0 | 1)); let expected = if ttl_expired { let mut punt = payload_pkt.clone(); @@ -506,13 +511,11 @@ async fn test_nat_ingress(switch: &Switch, test: &NatTest) -> TestResult { } // packet to/from IPv4 addresses, with an IPv6 address for the OPTE host -async fn test_egress_ipv4( - switch: &Switch, +fn egress_ipv4_test( l4_protocol: L4Protocol, uplink_port_registered: bool, - inner_ttl: Option, -) -> TestResult { - let test = NatTest { +) -> NatTest { + NatTest { uplink_port: PhysPort(14), uplink_port_external: "192.168.1.2".to_string(), uplink_port_registered, @@ -535,9 +538,25 @@ async fn test_egress_ipv4( nat_l4_port: 10, l4_protocol, geneve_vni: 1, // not used on egress tests - }; + } +} - test_nat_egress(switch, &test, inner_ttl).await +async fn test_egress_ipv4( + switch: &Switch, + l4_protocol: L4Protocol, + uplink_port_registered: bool, + inner_ttl: Option, +) -> TestResult { + let test = egress_ipv4_test(l4_protocol, uplink_port_registered); + test_nat_egress(switch, &test, inner_ttl, None).await +} + +#[tokio::test] +#[ignore] +async fn test_egress_ipv4_outer_hop_limit_zero() -> TestResult { + let switch = &*get_switch().await; + let test = egress_ipv4_test(L4Protocol::Udp, true); + test_nat_egress(switch, &test, None, Some(0)).await } // UDP packet to/from IPv4 addresses @@ -665,7 +684,7 @@ async fn test_egress_ipv6( geneve_vni: 1, // not used on egress tests }; - test_nat_egress(switch, &test, inner_ttl).await + test_nat_egress(switch, &test, inner_ttl, None).await } #[tokio::test] diff --git a/dpd-client/tests/integration_tests/route_ipv6.rs b/dpd-client/tests/integration_tests/route_ipv6.rs index dbdf6917..8d9a6e88 100644 --- a/dpd-client/tests/integration_tests/route_ipv6.rs +++ b/dpd-client/tests/integration_tests/route_ipv6.rs @@ -543,6 +543,45 @@ async fn test_ipv6_link_local_multicast_hop_limit_one() -> TestResult { result } +#[tokio::test] +#[ignore] +async fn test_ipv6_link_local_mcast_hop_limit_zero() -> TestResult { + let switch = &*get_switch().await; + + let ingress = PhysPort(10); + + let src = Endpoint::parse("e0:d5:5e:67:89:ab", "fe80::1", 3333).unwrap(); + let dst = Endpoint::parse("33:33:00:00:00:01", "ff02::1", 4444).unwrap(); + + let mut send = common::gen_udp_packet(src, dst); + ipv6::Ipv6Hdr::adjust_hlim(&mut send, -255); + + let test_pkt = TestPacket { packet: Arc::new(send.clone()), port: ingress }; + + let mut recv = send.clone(); + common::add_sidecar_hdr( + switch, + &mut recv, + sidecar::SC_FWD_TO_USERSPACE, + ingress, + NO_PORT, + None, + ); + + let expected = + vec![TestPacket { packet: Arc::new(recv), port: SERVICE_PORT }]; + + let ctr_baseline = + switch.get_counter("ipv6_ttl_invalid", None).await.unwrap(); + + let result = switch.packet_test(vec![test_pkt], expected); + + let ctr_final = switch.get_counter("ipv6_ttl_invalid", None).await.unwrap(); + assert_eq!(ctr_final, ctr_baseline, "link-local mcast at hop limit 0"); + + result +} + #[tokio::test] #[ignore] async fn test_reset() -> TestResult { diff --git a/dpd-client/tests/integration_tests/service.rs b/dpd-client/tests/integration_tests/service.rs index bdf99cac..4ebe39bd 100644 --- a/dpd-client/tests/integration_tests/service.rs +++ b/dpd-client/tests/integration_tests/service.rs @@ -9,7 +9,8 @@ use std::sync::Arc; use ::common::network::MacAddr; use dpd_client::ClientInfo; use dpd_client::types::Ipv4Entry; -use packet::{Endpoint, eth, icmp, ipv4, sidecar}; +use dpd_client::types::Ipv6Entry; +use packet::{Endpoint, eth, icmp, ipv4, ipv6, sidecar}; use crate::integration_tests::common; use crate::integration_tests::common::prelude::*; @@ -57,6 +58,102 @@ async fn test_service_ipv4_unicast() -> TestResult { switch.packet_test(vec![send], vec![expected]) } +#[tokio::test] +#[ignore] +async fn test_service_ipv4_unicast_ttl_zero() -> TestResult { + let switch = &*get_switch().await; + + let ingress = PhysPort(12); + let router_ip = "10.10.10.1"; + let router_mac = "02:78:39:45:b9:00"; + + let (port_id, link_id) = switch.link_id(ingress).unwrap(); + let entry = Ipv4Entry { + addr: router_ip.parse().unwrap(), + tag: switch.client.inner().tag.clone(), + }; + switch.client.link_ipv4_create(&port_id, &link_id, &entry).await.unwrap(); + + let mut send_pkt = common::gen_udp_packet( + Endpoint::parse("e0:d5:5e:67:89:ab", "10.10.10.10", 3333).unwrap(), + Endpoint::parse(router_mac, router_ip, 4444).unwrap(), + ); + send_pkt.hdrs.ipv4_hdr.as_mut().unwrap().ipv4_ttl = 0; + ipv4::Ipv4Hdr::update_checksum(&mut send_pkt); + let mut recv_pkt = send_pkt.clone(); + + let send = TestPacket { packet: Arc::new(send_pkt), port: ingress }; + + common::add_sidecar_hdr( + switch, + &mut recv_pkt, + sidecar::SC_FWD_TO_USERSPACE, + ingress, + NO_PORT, + None, + ); + let expected = + TestPacket { packet: Arc::new(recv_pkt), port: SERVICE_PORT }; + + let ctr_baseline = + switch.get_counter("ipv4_ttl_invalid", None).await.unwrap(); + + let result = switch.packet_test(vec![send], vec![expected]); + + let ctr_final = switch.get_counter("ipv4_ttl_invalid", None).await.unwrap(); + assert_eq!(ctr_final, ctr_baseline, "ttl 0 to a switch address"); + + result +} + +#[tokio::test] +#[ignore] +async fn test_service_ipv6_unicast_hop_limit_zero() -> TestResult { + let switch = &*get_switch().await; + + let ingress = PhysPort(12); + let router_ip = "fd00:1122:3344:0100::1"; + let router_mac = "02:78:39:45:b9:00"; + + let (port_id, link_id) = switch.link_id(ingress).unwrap(); + let entry = Ipv6Entry { + addr: router_ip.parse().unwrap(), + tag: switch.client.inner().tag.clone(), + }; + switch.client.link_ipv6_create(&port_id, &link_id, &entry).await.unwrap(); + + let mut send_pkt = common::gen_udp_packet( + Endpoint::parse("e0:d5:5e:67:89:ab", "fd00:1122:3344:0200::1", 3333) + .unwrap(), + Endpoint::parse(router_mac, router_ip, 4444).unwrap(), + ); + ipv6::Ipv6Hdr::adjust_hlim(&mut send_pkt, -255); + let mut recv_pkt = send_pkt.clone(); + + let send = TestPacket { packet: Arc::new(send_pkt), port: ingress }; + + common::add_sidecar_hdr( + switch, + &mut recv_pkt, + sidecar::SC_FWD_TO_USERSPACE, + ingress, + NO_PORT, + None, + ); + let expected = + TestPacket { packet: Arc::new(recv_pkt), port: SERVICE_PORT }; + + let ctr_baseline = + switch.get_counter("ipv6_ttl_invalid", None).await.unwrap(); + + let result = switch.packet_test(vec![send], vec![expected]); + + let ctr_final = switch.get_counter("ipv6_ttl_invalid", None).await.unwrap(); + assert_eq!(ctr_final, ctr_baseline, "hop limit 0 to a switch address"); + + result +} + // Packets sent to an IP address assigned to a switch port should be forwarded // to the service port with an added sidecar header. #[tokio::test] diff --git a/dpd/p4/parser.p4 b/dpd/p4/parser.p4 index 7de38b6b..b9974c23 100644 --- a/dpd/p4/parser.p4 +++ b/dpd/p4/parser.p4 @@ -151,11 +151,24 @@ parser IngressParser( state validate_ttl { transition select(hdr.ipv4.ttl) { - 8w0: invalidate_ttl; + 8w0: defer_ttl_drop; default: goto_proto_ipv4; } } + // Records the TTL drop reason without setting `meta.dropped`. A packet + // addressed to the switch must still be received (RFC 1812 §4.2.2.9), and + // whether the destination is a switch address depends on Filter's switch + // address tables, which run after the parser. + // + // NatIngress and Router4/Router6 drop on the forwarding paths, although + // the NAT egress (Geneve decap) path is exempt from a Router4/Router6 drop + // and still punts for an inner TTL of 0. + state defer_ttl_drop { + meta.drop_reason = DROP_IPV4_TTL_INVALID; + transition goto_proto_ipv4; + } + // IPv4 Multicast Address Validation (RFC 1112, RFC 7042). // // This validates that IPv4 multicast packets (224.0.0.0/4) use the proper @@ -204,7 +217,7 @@ parser IngressParser( state validate_mcast_ttl { transition select(hdr.ipv4.ttl) { 8w0: invalidate_ttl; - 8w1: invalidate_ttl; + 8w1: mcast_ttl_exceeded; default: goto_proto_ipv4; } } @@ -217,6 +230,12 @@ parser IngressParser( transition accept; } + state mcast_ttl_exceeded { + meta.drop_reason = DROP_MULTICAST_TTL_EXCEEDED; + meta.dropped = true; + transition accept; + } + state invalidate_ipv4_mcast { meta.is_mcast = true; meta.drop_reason = DROP_MULTICAST_INVALID_MAC; @@ -267,7 +286,7 @@ parser IngressParser( state set_link_local_mcast { meta.is_link_local_mcastv6 = true; meta.is_mcast = true; - transition validate_mcast_link_local_hop_limit; + transition validate_hop_limit; } state check_ipv6_mcast { @@ -281,26 +300,21 @@ parser IngressParser( state validate_hop_limit { transition select(hdr.ipv6.hop_limit) { - 8w0: invalidate_hop_limit; + 8w0: defer_hop_limit_drop; default: goto_proto_ipv6; } } - state validate_mcast_link_local_hop_limit { - // For link-local multicast, we allow a hop limit of 1. - // This is to ensure that link-local multicast packets - // are not forwarded beyond the local link. - transition select(hdr.ipv6.hop_limit) { - 8w0: invalidate_hop_limit; - 8w1: goto_proto_ipv6; - default: goto_proto_ipv6; - } + // Similar to `defer_ttl_drop`, but for IPv6 (RFC 8200 §3). + state defer_hop_limit_drop { + meta.drop_reason = DROP_IPV6_TTL_INVALID; + transition goto_proto_ipv6; } state validate_mcast_hop_limit { transition select(hdr.ipv6.hop_limit) { 8w0: invalidate_hop_limit; - 8w1: invalidate_hop_limit; + 8w1: mcast_ttl_exceeded; default: goto_proto_ipv6; } } diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index c44e1b4e..18a62d04 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -726,10 +726,20 @@ control NatIngress ( if (ingress_hit.apply().hit) { if (hdr.ipv4.isValid()) { - CalculateIPv4Len.apply(hdr, meta); - encap_ipv4(); + if (hdr.ipv4.ttl == 0) { + meta.drop_reason = DROP_IPV4_TTL_INVALID; + meta.dropped = true; + } else { + CalculateIPv4Len.apply(hdr, meta); + encap_ipv4(); + } } else if (hdr.ipv6.isValid()) { - encap_ipv6(); + if (hdr.ipv6.hop_limit == 0) { + meta.drop_reason = DROP_IPV6_TTL_INVALID; + meta.dropped = true; + } else { + encap_ipv6(); + } } if (hdr.vlan.isValid()) { @@ -1251,6 +1261,12 @@ control Router4 ( } apply { + if (hdr.ipv4.ttl == 0 && !meta.nat_egress_hit) { + meta.dropped = true; + meta.drop_reason = DROP_IPV4_TTL_INVALID; + return; + } + route4_result_t fwd; fwd.is_v6 = false; fwd.nexthop6 = 0; @@ -1385,6 +1401,12 @@ control Router6 ( } apply { + if (hdr.ipv6.hop_limit == 0 && !meta.nat_egress_hit) { + meta.dropped = true; + meta.drop_reason = DROP_IPV6_TTL_INVALID; + return; + } + route6_result_t fwd; fwd.nexthop = 0; fwd.port = 0; From dc269d190020c5db1cbd59c2623a16e7c769d0ce Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Fri, 30 Oct 2026 00:10:46 +0000 Subject: [PATCH 3/4] [review] address review feedback --- dpd-client/tests/integration_tests/common.rs | 11 +- .../tests/integration_tests/counters.rs | 150 ++++++++------ .../tests/integration_tests/route_ipv6.rs | 72 ++----- dpd-client/tests/integration_tests/service.rs | 189 +++++++++++------- dpd/p4/constants.p4 | 20 +- dpd/p4/sidecar.p4 | 62 +++--- packet/src/lib.rs | 8 + packet/src/sidecar.rs | 157 ++++++++++++++- 8 files changed, 450 insertions(+), 219 deletions(-) diff --git a/dpd-client/tests/integration_tests/common.rs b/dpd-client/tests/integration_tests/common.rs index 6a02650c..9c3243e1 100644 --- a/dpd-client/tests/integration_tests/common.rs +++ b/dpd-client/tests/integration_tests/common.rs @@ -1363,7 +1363,15 @@ pub fn add_sidecar_hdr( payload: Option<&[u8]>, ) { let eth = pkt.hdrs.eth_hdr.as_mut().unwrap(); - let sc_ether_type = eth.eth_type; + // sidecar.p4 emits a tagged packet's VLAN header after the sidecar header, + // with `sc_ether_type` mapping to the VLAN instead of the L3 protocol. + let (sc_ether_type, sidecar_vlan_hdr) = match eth.eth_8021q.take() { + Some(tag) => ( + eth::ETHER_VLAN, + Some(sidecar::SidecarVlanHdr { tag, ether_type: eth.eth_type }), + ), + None => (eth.eth_type, None), + }; let sc_payload = [0u8; 16]; let mut s = sidecar::SidecarHdr { @@ -1380,6 +1388,7 @@ pub fn add_sidecar_hdr( } pkt.hdrs.sidecar_hdr = Some(s); + pkt.hdrs.sidecar_vlan_hdr = sidecar_vlan_hdr; eth.eth_type = eth::ETHER_SIDECAR; } diff --git a/dpd-client/tests/integration_tests/counters.rs b/dpd-client/tests/integration_tests/counters.rs index a9f8ccff..89f9b850 100644 --- a/dpd-client/tests/integration_tests/counters.rs +++ b/dpd-client/tests/integration_tests/counters.rs @@ -76,6 +76,30 @@ pub async fn check_counter_incremented( ) } +/// Assert that a counter holds at `baseline` for the full polling cycle. +/// +/// Counter updates can lag the packet; a single occurrence of an unchanged +/// read does not demonstrate that a late increment will not arrive. +pub async fn check_counter_unchanged( + switch: &Switch, + counter_name: &str, + baseline: u64, + client_name: Option<&str>, +) -> anyhow::Result<()> { + let table = client_name.unwrap_or("ingress_drop_reason"); + for poll in 1..=COUNTER_POLL_ATTEMPTS { + tokio::time::sleep(COUNTER_POLL_INTERVAL).await; + let value = switch.get_counter(counter_name, client_name).await?; + if value != baseline { + bail!( + "counter '{counter_name}' in {table} expected to stay at \ + {baseline}, but reached {value} on poll {poll}" + ); + } + } + Ok(()) +} + /// Process a single packet, asserting that a given counter is incremented by /// one and a packet is appropriately transformed (or dropped).. pub async fn one_counted_test( @@ -204,41 +228,41 @@ async fn test_ipv4_checksum_err() -> TestResult { Ok(()) } -async fn test_ipv4_ttl_test(ttl: u8, reason: &str) -> TestResult { - let switch = &*get_switch().await; - let ingress = PhysPort(10); - let subnet = "10.10.10.0/24"; - let nexthop_ip = "10.10.10.1"; - let src_ip = "10.10.9.10"; - let dst_ip = "10.10.10.11"; - - common::set_route_ipv4(switch, subnet, ingress, nexthop_ip).await?; - - let mut packet = common::gen_udp_packet( - Endpoint::parse("e0:d5:5e:67:89:ab", src_ip, 3333).unwrap(), - Endpoint::parse("e0:d5:5e:67:89:ac", dst_ip, 4444).unwrap(), +/// Send a packet out that should be punted to the service port with a `Time +/// Exceeded` request, and verify that `counter` is incremented by one. +async fn one_time_exceeded_test( + switch: &Switch, + ingress: PhysPort, + packet: packet::Packet, + sc_egress: PhysPort, + icmp_type: u8, + icmp_code: u8, + counter: &str, +) -> anyhow::Result { + let mut expected = packet.clone(); + common::set_icmp_needed( + switch, + &mut expected, + ingress, + sc_egress, + icmp_type, + icmp_code, ); - let hdr = packet.hdrs.ipv4_hdr.as_mut().unwrap(); - hdr.ipv4_ttl = ttl; - packet::ipv4::Ipv4Hdr::update_checksum(&mut packet); - - assert!(one_drop_test(switch, ingress, packet, reason).await?); - - Ok(()) -} - -/// Sets up a route to 10.10.10.0/24 through port 10. We then send a packet -/// with a ttl of 0, and expect an ipv4_ttl_invalid drop. -#[tokio::test] -#[ignore] -async fn test_ipv4_ttl_invalid() -> TestResult { - test_ipv4_ttl_test(0, "ipv4_ttl_invalid").await + one_counted_test( + switch, + TestPacket { packet: Arc::new(packet), port: ingress }, + Some(TestPacket { packet: Arc::new(expected), port: SERVICE_PORT }), + counter, + None, + ) + .await } -#[tokio::test] -#[ignore] -async fn test_ipv4_ttl_invalid_nat_ingress() -> TestResult { +/// Send a TTL 0 packet out that reaches a NAT ingress entry, optionally +/// VLAN-tagged, and verify that the complete punted packet (with the tag +/// included) requests a `Time Exceeded` reply. +async fn nat_ingress_ttl_zero_impl(vlan: Option) -> TestResult { let switch = &*get_switch().await; let ingress = PhysPort(14); let nat_ip = "192.168.1.2"; @@ -267,11 +291,43 @@ async fn test_ipv4_ttl_invalid_nat_ingress() -> TestResult { packet.hdrs.ipv4_hdr.as_mut().unwrap().ipv4_ttl = 0; packet::ipv4::Ipv4Hdr::update_checksum(&mut packet); - assert!(one_drop_test(switch, ingress, packet, "ipv4_ttl_invalid").await?); + if let Some(vlan) = vlan { + packet.hdrs.eth_hdr.as_mut().unwrap().eth_8021q = + Some(packet::eth::EthQHdr { + eth_pcp: 0, + eth_dei: 0, + eth_vlan_tag: vlan, + }); + } + + assert!( + one_time_exceeded_test( + switch, + ingress, + packet, + NO_PORT, + packet::icmp::ICMP_TIME_EXCEEDED, + packet::icmp::ICMP_EXC_TTL, + "ipv4_ttl_exceeded", + ) + .await? + ); Ok(()) } +#[tokio::test] +#[ignore] +async fn test_ipv4_ttl_zero_exceeded_nat_ingress() -> TestResult { + nat_ingress_ttl_zero_impl(None).await +} + +#[tokio::test] +#[ignore] +async fn test_ipv4_ttl_zero_exceeded_nat_ingress_vlan() -> TestResult { + nat_ingress_ttl_zero_impl(Some(200)).await +} + /// Set a port to accept only inbound traffic with a corresponding NAT mapping. /// Send in a packet that has no corresponding NAT entry, and check the /// nat_ingress_miss counter. @@ -308,36 +364,6 @@ async fn test_nat_filtering() -> TestResult { Ok(()) } -async fn test_ipv6_ttl(ttl: u8, reason: &str) -> TestResult { - let switch = &*get_switch().await; - let ingress = PhysPort(10); - let subnet = "fd00:1122:3344:0100::0/64"; - let nexthop_ip = "fd00:1122:3344:0100::1"; - let src_ip = "fd00:1122:3344:0200:1111::0"; - let dst_ip = "fd00:1122:3344:0100:2222::0"; - - common::set_route_ipv6(switch, subnet, ingress, nexthop_ip).await?; - - let mut packet = common::gen_udp_packet( - Endpoint::parse("e0:d5:5e:67:89:ab", src_ip, 3333).unwrap(), - Endpoint::parse("e0:d5:5e:67:89:ac", dst_ip, 4444).unwrap(), - ); - - let hdr = packet.hdrs.ipv6_hdr.as_mut().unwrap(); - hdr.ipv6_hop_lim = ttl; - - assert!(one_drop_test(switch, ingress, packet, reason).await?); - - Ok(()) -} -/// Sets up a route to fd00:1122:3344:0100::0/64 through port 10. We then send -/// a packet with a hop_limit of 0, and expect an ipv6_ttl_invalid drop. -#[tokio::test] -#[ignore] -async fn test_ipv6_ttl_invalid() -> TestResult { - test_ipv6_ttl(0, "ipv6_ttl_invalid").await -} - /// Any ICMP_ECHO request with an icmp_code other than 0 should result in a /// BAD_PING drop. #[tokio::test] diff --git a/dpd-client/tests/integration_tests/route_ipv6.rs b/dpd-client/tests/integration_tests/route_ipv6.rs index d657f294..02e580b3 100644 --- a/dpd-client/tests/integration_tests/route_ipv6.rs +++ b/dpd-client/tests/integration_tests/route_ipv6.rs @@ -16,7 +16,9 @@ use packet::{Endpoint, ipv6, sidecar}; use crate::integration_tests::common; use crate::integration_tests::common::prelude::*; -use crate::integration_tests::counters::one_counted_test; +use crate::integration_tests::counters::{ + check_counter_unchanged, one_counted_test, +}; use packet::eth::EthQHdr; use dpd_client::types; @@ -516,9 +518,7 @@ async fn test_link_local_multicast_outbound() -> TestResult { switch.packet_test(vec![send], expected) } -#[tokio::test] -#[ignore] -async fn test_ipv6_link_local_multicast_hop_limit_one() -> TestResult { +async fn link_local_mcast_hop_limit_impl(hop_limit: u8) -> TestResult { let switch = &*get_switch().await; let ingress = PhysPort(10); @@ -527,13 +527,10 @@ async fn test_ipv6_link_local_multicast_hop_limit_one() -> TestResult { let dst = Endpoint::parse("33:33:00:00:00:01", "ff02::1", 4444).unwrap(); let mut send = common::gen_udp_packet(src, dst); - - // Set hop limit to 1 - this should be ALLOWED for link-local multicast - ipv6::Ipv6Hdr::adjust_hlim(&mut send, -254); // Set to 1 (255 - 254 = 1) + ipv6::Ipv6Hdr::adjust_hlim(&mut send, i16::from(hop_limit) - 255); let test_pkt = TestPacket { packet: Arc::new(send.clone()), port: ingress }; - // Link-local multicast packets should be forwarded to userspace with sidecar header let mut recv = send.clone(); common::add_sidecar_hdr( switch, @@ -547,61 +544,24 @@ async fn test_ipv6_link_local_multicast_hop_limit_one() -> TestResult { let expected = vec![TestPacket { packet: Arc::new(recv), port: SERVICE_PORT }]; - // Verify that the hop limit invalid counter does NOT increment - let ctr_baseline_hop_limit = - switch.get_counter("ipv6_ttl_invalid", None).await.unwrap(); - - let result = switch.packet_test(vec![test_pkt], expected); + let counter = "ipv6_ttl_invalid"; + let ctr_baseline = switch.get_counter(counter, None).await?; + switch.packet_test(vec![test_pkt], expected)?; + check_counter_unchanged(switch, counter, ctr_baseline, None).await?; - // Verify hop limit invalid counter did NOT increment (packet was not dropped) - let ctr_final_hop_limit = - switch.get_counter("ipv6_ttl_invalid", None).await.unwrap(); - - assert_eq!( - ctr_final_hop_limit, ctr_baseline_hop_limit, - "Hop limit invalid counter should not increment for link-local multicast with hop limit 1" - ); + Ok(()) +} - result +#[tokio::test] +#[ignore] +async fn test_ipv6_link_local_multicast_hop_limit_one() -> TestResult { + link_local_mcast_hop_limit_impl(1).await } #[tokio::test] #[ignore] async fn test_ipv6_link_local_mcast_hop_limit_zero() -> TestResult { - let switch = &*get_switch().await; - - let ingress = PhysPort(10); - - let src = Endpoint::parse("e0:d5:5e:67:89:ab", "fe80::1", 3333).unwrap(); - let dst = Endpoint::parse("33:33:00:00:00:01", "ff02::1", 4444).unwrap(); - - let mut send = common::gen_udp_packet(src, dst); - ipv6::Ipv6Hdr::adjust_hlim(&mut send, -255); - - let test_pkt = TestPacket { packet: Arc::new(send.clone()), port: ingress }; - - let mut recv = send.clone(); - common::add_sidecar_hdr( - switch, - &mut recv, - sidecar::SC_FWD_TO_USERSPACE, - ingress, - NO_PORT, - None, - ); - - let expected = - vec![TestPacket { packet: Arc::new(recv), port: SERVICE_PORT }]; - - let ctr_baseline = - switch.get_counter("ipv6_ttl_invalid", None).await.unwrap(); - - let result = switch.packet_test(vec![test_pkt], expected); - - let ctr_final = switch.get_counter("ipv6_ttl_invalid", None).await.unwrap(); - assert_eq!(ctr_final, ctr_baseline, "link-local mcast at hop limit 0"); - - result + link_local_mcast_hop_limit_impl(0).await } #[tokio::test] diff --git a/dpd-client/tests/integration_tests/service.rs b/dpd-client/tests/integration_tests/service.rs index fcbde936..34cc6205 100644 --- a/dpd-client/tests/integration_tests/service.rs +++ b/dpd-client/tests/integration_tests/service.rs @@ -4,18 +4,20 @@ // // Copyright 2026 Oxide Computer Company -use std::net::Ipv6Addr; +use std::net::{IpAddr, Ipv6Addr}; use std::sync::Arc; use ::common::network::MacAddr; use dpd_client::ClientInfo; use dpd_client::types::Ipv4Entry; use dpd_client::types::Ipv6Entry; -use packet::{Endpoint, eth, icmp, ipv4, ipv6, sidecar}; +use packet::{Endpoint, eth, icmp, ipv4, sidecar}; use crate::integration_tests::common; use crate::integration_tests::common::prelude::*; -use crate::integration_tests::counters::one_counted_test; +use crate::integration_tests::counters::{ + check_counter_unchanged, one_counted_test, +}; // Packets sent to an IP address assigned to a switch port should be forwarded // to the service port with an added sidecar header. @@ -60,28 +62,58 @@ async fn test_service_ipv4_unicast() -> TestResult { switch.packet_test(vec![send], vec![expected]) } -#[tokio::test] -#[ignore] -async fn test_service_ipv4_unicast_ttl_zero() -> TestResult { +/// Send a packet out with the provided TTL/hop limit to a switch address, +/// and verify it is delivered to the service port without counting as a +/// TTL drop. +async fn service_unicast_ttl_impl( + router_ip: IpAddr, + src_ip: &str, + ttl: u8, +) -> TestResult { let switch = &*get_switch().await; let ingress = PhysPort(12); - let router_ip = "10.10.10.1"; let router_mac = "02:78:39:45:b9:00"; let (port_id, link_id) = switch.link_id(ingress).unwrap(); - let entry = Ipv4Entry { - addr: router_ip.parse().unwrap(), - tag: switch.client.inner().tag.clone(), - }; - switch.client.link_ipv4_create(&port_id, &link_id, &entry).await.unwrap(); + let tag = switch.client.inner().tag.clone(); + match router_ip { + IpAddr::V4(addr) => { + let entry = Ipv4Entry { addr, tag }; + switch + .client + .link_ipv4_create(&port_id, &link_id, &entry) + .await + .unwrap(); + } + IpAddr::V6(addr) => { + let entry = Ipv6Entry { addr, tag }; + switch + .client + .link_ipv6_create(&port_id, &link_id, &entry) + .await + .unwrap(); + } + } let mut send_pkt = common::gen_udp_packet( - Endpoint::parse("e0:d5:5e:67:89:ab", "10.10.10.10", 3333).unwrap(), - Endpoint::parse(router_mac, router_ip, 4444).unwrap(), + Endpoint::parse("e0:d5:5e:67:89:ab", src_ip, 3333).unwrap(), + Endpoint::parse(router_mac, &router_ip.to_string(), 4444).unwrap(), ); - send_pkt.hdrs.ipv4_hdr.as_mut().unwrap().ipv4_ttl = 0; - ipv4::Ipv4Hdr::update_checksum(&mut send_pkt); + + let ip_family = if router_ip.is_ipv4() { + send_pkt.hdrs.ipv4_hdr.as_mut().unwrap().ipv4_ttl = ttl; + ipv4::Ipv4Hdr::update_checksum(&mut send_pkt); + "ipv4" + } else { + send_pkt.hdrs.ipv6_hdr.as_mut().unwrap().ipv6_hop_lim = ttl; + "ipv6" + }; + + let counters = [ + format!("{ip_family}_ttl_invalid"), + format!("{ip_family}_ttl_exceeded"), + ]; let mut recv_pkt = send_pkt.clone(); let send = TestPacket { packet: Arc::new(send_pkt), port: ingress }; @@ -94,66 +126,56 @@ async fn test_service_ipv4_unicast_ttl_zero() -> TestResult { NO_PORT, None, ); + let expected = TestPacket { packet: Arc::new(recv_pkt), port: SERVICE_PORT }; - let ctr_baseline = - switch.get_counter("ipv4_ttl_invalid", None).await.unwrap(); + let mut baselines = Vec::with_capacity(counters.len()); + for counter in &counters { + baselines.push(switch.get_counter(counter, None).await?); + } + switch.packet_test(vec![send], vec![expected])?; + for (counter, baseline) in counters.iter().zip(baselines) { + check_counter_unchanged(switch, counter, baseline, None).await?; + } - let result = switch.packet_test(vec![send], vec![expected]); + Ok(()) +} - let ctr_final = switch.get_counter("ipv4_ttl_invalid", None).await.unwrap(); - assert_eq!(ctr_final, ctr_baseline, "ttl 0 to a switch address"); +#[tokio::test] +#[ignore] +async fn test_service_ipv4_unicast_ttl_zero() -> TestResult { + service_unicast_ttl_impl("10.10.10.1".parse().unwrap(), "10.10.10.10", 0) + .await +} - result +#[tokio::test] +#[ignore] +async fn test_service_ipv4_unicast_ttl_one() -> TestResult { + service_unicast_ttl_impl("10.10.10.1".parse().unwrap(), "10.10.10.10", 1) + .await } #[tokio::test] #[ignore] async fn test_service_ipv6_unicast_hop_limit_zero() -> TestResult { - let switch = &*get_switch().await; - - let ingress = PhysPort(12); - let router_ip = "fd00:1122:3344:0100::1"; - let router_mac = "02:78:39:45:b9:00"; - - let (port_id, link_id) = switch.link_id(ingress).unwrap(); - let entry = Ipv6Entry { - addr: router_ip.parse().unwrap(), - tag: switch.client.inner().tag.clone(), - }; - switch.client.link_ipv6_create(&port_id, &link_id, &entry).await.unwrap(); - - let mut send_pkt = common::gen_udp_packet( - Endpoint::parse("e0:d5:5e:67:89:ab", "fd00:1122:3344:0200::1", 3333) - .unwrap(), - Endpoint::parse(router_mac, router_ip, 4444).unwrap(), - ); - ipv6::Ipv6Hdr::adjust_hlim(&mut send_pkt, -255); - let mut recv_pkt = send_pkt.clone(); - - let send = TestPacket { packet: Arc::new(send_pkt), port: ingress }; - - common::add_sidecar_hdr( - switch, - &mut recv_pkt, - sidecar::SC_FWD_TO_USERSPACE, - ingress, - NO_PORT, - None, - ); - let expected = - TestPacket { packet: Arc::new(recv_pkt), port: SERVICE_PORT }; - - let ctr_baseline = - switch.get_counter("ipv6_ttl_invalid", None).await.unwrap(); - - let result = switch.packet_test(vec![send], vec![expected]); - - let ctr_final = switch.get_counter("ipv6_ttl_invalid", None).await.unwrap(); - assert_eq!(ctr_final, ctr_baseline, "hop limit 0 to a switch address"); + service_unicast_ttl_impl( + "fd00:1122:3344:0100::1".parse().unwrap(), + "fd00:1122:3344:0200::1", + 0, + ) + .await +} - result +#[tokio::test] +#[ignore] +async fn test_service_ipv6_unicast_hop_limit_one() -> TestResult { + service_unicast_ttl_impl( + "fd00:1122:3344:0100::1".parse().unwrap(), + "fd00:1122:3344:0200::1", + 1, + ) + .await } // Packets sent to an IP address assigned to a switch port should be forwarded @@ -380,9 +402,9 @@ async fn test_ndp_needed() -> TestResult { Ok(()) } -#[tokio::test] -#[ignore] -async fn test_ttl_exceeded() -> TestResult { +/// Forward an IPv4 packet with the given TTL, and verify that it requests an +/// ICMP Time Exceeded and counts as ipv4_ttl_exceeded. +async fn ipv4_ttl_exceeded_impl(ttl: u8) -> TestResult { let switch = &*get_switch().await; let ingress = PhysPort(10); @@ -404,9 +426,9 @@ async fn test_ttl_exceeded() -> TestResult { common::set_route_ipv4(switch, "10.10.10.0/24", egress, "10.10.10.1") .await?; - // build the initial packet and set its ttl to expire in 1 hop let mut send_pkt = common::gen_udp_packet(orig_src, orig_dst); - ipv4::Ipv4Hdr::adjust_ttl(&mut send_pkt, -254); + send_pkt.hdrs.ipv4_hdr.as_mut().unwrap().ipv4_ttl = ttl; + ipv4::Ipv4Hdr::update_checksum(&mut send_pkt); let mut recv_pkt = send_pkt.clone(); @@ -439,7 +461,19 @@ async fn test_ttl_exceeded() -> TestResult { #[tokio::test] #[ignore] -async fn test_hop_limit_exceeded() -> TestResult { +async fn test_ttl_exceeded() -> TestResult { + ipv4_ttl_exceeded_impl(1).await +} + +#[tokio::test] +#[ignore] +async fn test_ttl_zero_exceeded() -> TestResult { + ipv4_ttl_exceeded_impl(0).await +} + +/// Forward an IPv6 packet with the given hop limit, and verify that it +/// requests an ICMPv6 Time Exceeded and counts as ipv6_ttl_exceeded. +async fn ipv6_hop_limit_exceeded_impl(hop_limit: u8) -> TestResult { let switch = &*get_switch().await; let ingress = PhysPort(10); @@ -463,9 +497,8 @@ async fn test_hop_limit_exceeded() -> TestResult { common::set_route_ipv6(switch, "::/0", egress, "fd00:1122:7788:0101::1") .await?; - // build the initial packet and set its ttl to expire in 1 hop let mut send_pkt = common::gen_udp_packet(orig_src, orig_dst); - ipv6::Ipv6Hdr::adjust_hlim(&mut send_pkt, -254); + send_pkt.hdrs.ipv6_hdr.as_mut().unwrap().ipv6_hop_lim = hop_limit; let mut recv_pkt = send_pkt.clone(); @@ -496,6 +529,18 @@ async fn test_hop_limit_exceeded() -> TestResult { Ok(()) } +#[tokio::test] +#[ignore] +async fn test_hop_limit_exceeded() -> TestResult { + ipv6_hop_limit_exceeded_impl(1).await +} + +#[tokio::test] +#[ignore] +async fn test_hop_limit_zero_exceeded() -> TestResult { + ipv6_hop_limit_exceeded_impl(0).await +} + // Arp packets sent to an IP address assigned to a switch port should be // forwarded to the service port with an added sidecar header, even if the port // is configured in uplink mode. diff --git a/dpd/p4/constants.p4 b/dpd/p4/constants.p4 index a50f9fc8..9913bdaf 100644 --- a/dpd/p4/constants.p4 +++ b/dpd/p4/constants.p4 @@ -79,9 +79,24 @@ const bit<8> DROP_NDP_NULL = 0x07; const bit<8> DROP_NDP_MISS = 0x08; const bit<8> DROP_MULTICAST_TO_LOCAL_INTERFACE = 0x09; const bit<8> DROP_IPV4_CHECKSUM_ERR = 0x0A; +// Native multicast packets with an IPv4 TTL of 0 are dropped in the parser +// without an ICMP error reply (see RFC 1812 §5.3.1) +// +// NOTE: unicast packets to a switch address are received at any TTL +// (see RFC 1812 §4.2.2.9) const bit<8> DROP_IPV4_TTL_INVALID = 0x0B; +// IPv4 unicast TTL of 0 or 1 on a forwarded path, which requests an ICMP Time +// Exceeded from the host, counted without a drop (see RFC 1812 §5.3.1) const bit<8> DROP_IPV4_TTL_EXCEEDED = 0x0C; +// Native non-link-local multicast packets with an IPv6 hop limit of 0 are +// dropped in the parser without an ICMPv6 error reply (see RFC 4443 §2.4(e.3)) +// and ff01::/16 is dropped earlier as DROP_MULTICAST_TO_LOCAL_INTERFACE. +// +// NOTE: unicast packets to a switch address are received at any hop limit +// (see RFC 8200 §3) const bit<8> DROP_IPV6_TTL_INVALID = 0x0D; +// IPv6 unicast hop limit of 0 or 1 on a forwarded path, which requests an +// ICMPv6 Time Exceeded from the host, counted without a drop (see RFC 4443 §3.3) const bit<8> DROP_IPV6_TTL_EXCEEDED = 0x0E; const bit<8> DROP_IPV4_UNROUTEABLE = 0x0F; const bit<8> DROP_IPV6_UNROUTEABLE = 0x10; @@ -96,8 +111,9 @@ const bit<8> DROP_GENEVE_OPTIONS_TOO_LONG = 0x18; const bit<8> DROP_GENEVE_OPTION_MALFORMED = 0x19; const bit<8> DROP_GENEVE_OPTION_UNKNOWN = 0x1A; const bit<8> DROP_SCTP = 0x1B; -// Multicast TTL expired, dropped without an ICMP(v6) Time Exceeded -// (see RFC 1812 §4.3.2.7, §5.3.1; RFC 4443 §2.4(e.3)) +// Multicast TTL expired, dropped without an ICMP(v6) Time Exceeded: +// TTL/hop limit of 1 in the parser, or inner TTL/hop limit of 0 or 1 +// on the decapsulation path (see RFC 1812 §4.3.2.7, §5.3.1; RFC 4443 §2.4(e.3)) const bit<8> DROP_MULTICAST_TTL_EXCEEDED = 0x1C; // MAX(DROP_xxx) + 1 const bit<32> DROP_REASON_MAX = 0x1D; diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index 3ce3d565..eb44ff13 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -17,9 +17,6 @@ const bit<9> USER_SPACE_SERVICE_PORT = 192; #include #include -// This is the port that we use to send packets to user space. -#define IS_SERVICE(p) ((p) == USER_SPACE_SERVICE_PORT) - // Includes the checksum for the original data, the geneve header, the // outer udp header, and the outer ipv6 pseudo-header. // NOTE: safe to include geneve oxg_ext_tag here (via nat_ingress_csum) @@ -500,8 +497,21 @@ control AttachedSubnetIngress ( control NatIngress ( inout sidecar_headers_t hdr, inout sidecar_ingress_meta_t meta, - in ingress_intrinsic_metadata_t ig_intr_md + in ingress_intrinsic_metadata_t ig_intr_md, + inout ingress_intrinsic_metadata_for_tm_t ig_tm_md ) { + action icmp_error(bit<8> type, bit<8> code) { + hdr.sidecar.sc_code = SC_ICMP_NEEDED; + hdr.sidecar.sc_ingress = (bit<16>)ig_intr_md.ingress_port; + hdr.sidecar.sc_egress = 0; + hdr.sidecar.sc_ether_type = hdr.ethernet.ether_type; + hdr.sidecar.sc_payload = (bit<128>)type << 8 | (bit<128>)code; + hdr.sidecar.setValid(); + hdr.ethernet.ether_type = ETHERTYPE_SIDECAR; + meta.service_routed = true; + ig_tm_md.ucast_egress_port = USER_SPACE_SERVICE_PORT; + } + DirectCounter>(CounterType_t.PACKETS_AND_BYTES) ipv4_ingress_ctr; DirectCounter>(CounterType_t.PACKETS_AND_BYTES) ipv6_ingress_ctr; DirectCounter>(CounterType_t.PACKETS_AND_BYTES) mcast_ipv4_ingress_ctr; @@ -725,24 +735,28 @@ control NatIngress ( } if (ingress_hit.apply().hit) { + // A TTL/hop limit of 0 would expire when being forwarded to the + // sled, leading us to send a request for an ICMP(v6) Time + // Exceeded from the host instead of performing encapsulation + // (RFC 1812 §5.3.1; RFC 4443 §3.3). if (hdr.ipv4.isValid()) { if (hdr.ipv4.ttl == 0) { - meta.drop_reason = DROP_IPV4_TTL_INVALID; - meta.dropped = true; + icmp_error(ICMP_TIME_EXCEEDED, ICMP_EXC_TTL); + meta.drop_reason = DROP_IPV4_TTL_EXCEEDED; } else { CalculateIPv4Len.apply(hdr, meta); encap_ipv4(); } } else if (hdr.ipv6.isValid()) { if (hdr.ipv6.hop_limit == 0) { - meta.drop_reason = DROP_IPV6_TTL_INVALID; - meta.dropped = true; + icmp_error(ICMP6_TIME_EXCEEDED, ICMP_EXC_TTL); + meta.drop_reason = DROP_IPV6_TTL_EXCEEDED; } else { encap_ipv6(); } } - if (hdr.vlan.isValid()) { + if (hdr.vlan.isValid() && !meta.service_routed) { // When setting up the inner headers above, we // copied the ether type from the outer to // the inner. If this is a vlan packet, we @@ -1257,12 +1271,6 @@ control Router4 ( } apply { - if (hdr.ipv4.ttl == 0 && !meta.nat_egress_hit) { - meta.dropped = true; - meta.drop_reason = DROP_IPV4_TTL_INVALID; - return; - } - route4_result_t fwd; fwd.is_v6 = false; fwd.nexthop6 = 0; @@ -1285,7 +1293,13 @@ control Router4 ( // Dont set meta.dropped because we want an error packet // to go out. meta.drop_reason = DROP_IPV4_UNROUTEABLE; - } else if (hdr.ipv4.ttl <= 8w1 && !IS_SERVICE(fwd.port)) { + } else if (hdr.ipv4.ttl <= 8w1) { + // A forwarded TTL of 0 or 1 expires here and requests a Time + // Exceeded (RFC 1812 §5.3.1). Traffic delivered locally to a switch + // address is service-routed and never reaches the L3Router control + // (RFC 1812 §4.2.2.9). + // Geneve traffic to a switch tunnel endpoint is decapped before the + // inner frame reaches L3Router. icmp_error(ICMP_TIME_EXCEEDED, ICMP_EXC_TTL); // Dont set meta.dropped because we want an error packet // to go out. @@ -1390,12 +1404,6 @@ control Router6 ( } apply { - if (hdr.ipv6.hop_limit == 0 && !meta.nat_egress_hit) { - meta.dropped = true; - meta.drop_reason = DROP_IPV6_TTL_INVALID; - return; - } - route6_result_t fwd; fwd.nexthop = 0; fwd.port = 0; @@ -1416,7 +1424,13 @@ control Router6 ( meta.drop_reason = DROP_IPV6_UNROUTEABLE; // Dont set meta.dropped because we want an error packet // to go out. - } else if (hdr.ipv6.hop_limit <= 8w1 && !IS_SERVICE(fwd.port)) { + } else if (hdr.ipv6.hop_limit <= 8w1) { + // A forwarded hop limit of 0 or 1 expires here and requests a + // Time Exceeded (RFC 4443 §3.3). Traffic delivered locally to a switch + // address is service-routed and never reaches the L3Router control + // (RFC 8200 §3). + // Geneve traffic to a switch tunnel endpoint is decapped before the + // inner frame reaches L3Router. icmp_error(ICMP6_TIME_EXCEEDED, ICMP_EXC_TTL); meta.drop_reason = DROP_IPV6_TTL_EXCEEDED; // Dont set meta.dropped because we want an error packet @@ -2172,7 +2186,7 @@ control Ingress( // that the NAT'd outer address is used for multicast // replication to inbound groups if (!meta.dropped && !hdr.geneve.isValid()) { - nat_ingress.apply(hdr, meta, ig_intr_md); + nat_ingress.apply(hdr, meta, ig_intr_md, ig_tm_md); } if (!meta.dropped) { diff --git a/packet/src/lib.rs b/packet/src/lib.rs index 689b8e6a..91a7096f 100644 --- a/packet/src/lib.rs +++ b/packet/src/lib.rs @@ -321,6 +321,8 @@ pub struct Headers { pub eth_hdr: Option, pub lldp_hdr: Option, pub sidecar_hdr: Option, + /// 802.1q tag following the sidecar header (see [`sidecar::SidecarVlanHdr`]). + pub sidecar_vlan_hdr: Option, pub ipv4_hdr: Option, pub ipv6_hdr: Option, pub tcp_hdr: Option, @@ -336,6 +338,7 @@ impl Headers { Headers { eth_hdr: None, sidecar_hdr: None, + sidecar_vlan_hdr: None, lldp_hdr: None, ipv4_hdr: None, ipv6_hdr: None, @@ -377,6 +380,11 @@ impl PartialEq for Packet { let mut is_eq = test("eth hdr", a.eth_hdr.as_ref(), b.eth_hdr.as_ref()); is_eq &= test("sidecar hdr", a.sidecar_hdr.as_ref(), b.sidecar_hdr.as_ref()); + is_eq &= test( + "sidecar vlan hdr", + a.sidecar_vlan_hdr.as_ref(), + b.sidecar_vlan_hdr.as_ref(), + ); is_eq &= test("lldp hdr", a.lldp_hdr.as_ref(), b.lldp_hdr.as_ref()); is_eq &= test("ipv4 hdr", a.ipv4_hdr.as_ref(), b.ipv4_hdr.as_ref()); is_eq &= test("ipv6 hdr", a.ipv6_hdr.as_ref(), b.ipv6_hdr.as_ref()); diff --git a/packet/src/sidecar.rs b/packet/src/sidecar.rs index fbd4cc62..9828c24d 100644 --- a/packet/src/sidecar.rs +++ b/packet/src/sidecar.rs @@ -21,6 +21,31 @@ pub const SC_ARP_NEEDED: u8 = 3; pub const SC_NEIGHBOR_NEEDED: u8 = 4; const SC_HDR_SZ: usize = 24; +const SC_VLAN_HDR_SZ: usize = 4; + +/// An 802.1q tag that follows the sidecar header. +/// +/// Our headers.p4 declares the sidecar header ahead of the VLAN header, laying +/// out a packet crossing the CPU port as `eth | sidecar | vlan | l3`, +/// with the `sc_ether_type` set to the VLAN EtherType: +/// +/// ```text +/// struct sidecar_headers_t { +/// ethernet_h ethernet; +/// sidecar_h sidecar; +/// dot1q_h vlan; +/// ... +/// } +/// ``` +/// +/// This is distinct from [`eth::EthHdr::eth_8021q`], which precedes the sidecar +/// header, and conveys the inner EtherType that the sidecar header no longer +/// possesses. +#[derive(Eq, PartialEq, Copy, Clone, Debug)] +pub struct SidecarVlanHdr { + pub tag: eth::EthQHdr, + pub ether_type: u16, +} #[derive(Eq, PartialEq, Copy, Clone)] pub struct SidecarHdr { @@ -48,7 +73,23 @@ impl Protocol for SidecarHdr { sc_payload: pb.get_bytes(16)?.try_into().unwrap(), }; - let mut hdrs = match hdr.sc_ether_type { + let vlan = if hdr.sc_ether_type == eth::ETHER_VLAN { + if pb.bytes_left() < SC_VLAN_HDR_SZ { + return Err(crate::parse_error( + pb, + "sidecar vlan header too short", + )); + } + Some(SidecarVlanHdr { + tag: eth::EthQHdr::from(pb.get_u16()?), + ether_type: pb.get_u16()?, + }) + } else { + None + }; + let ether_type = vlan.map_or(hdr.sc_ether_type, |v| v.ether_type); + + let mut hdrs = match ether_type { eth::ETHER_ARP => arp::ArpHdr::parse(pb), eth::ETHER_IPV4 => ipv4::Ipv4Hdr::parse(pb), eth::ETHER_IPV6 => ipv6::Ipv6Hdr::parse(pb), @@ -56,6 +97,9 @@ impl Protocol for SidecarHdr { eth::ETHER_SIDECAR => { Err(crate::parse_error(pb, "nested sidecar headers")) } + eth::ETHER_VLAN => { + Err(crate::parse_error(pb, "nested sidecar vlan headers")) + } _ => { // println!("unsupported ethertype: {:x}", hdr.sc_ether_type); Ok(Headers::new()) @@ -63,6 +107,7 @@ impl Protocol for SidecarHdr { }?; hdrs.sidecar_hdr = Some(hdr); + hdrs.sidecar_vlan_hdr = vlan; Ok(hdrs) } @@ -90,7 +135,8 @@ impl Protocol for SidecarHdr { fn deparse(pkt: &Packet, hdr_size: usize) -> PacketResult { let sidecar_hdr = pkt.hdrs.sidecar_hdr.as_ref().unwrap(); - let mut v = eth::EthHdr::deparse(pkt, hdr_size + SC_HDR_SZ)?; + let mut v = + eth::EthHdr::deparse(pkt, hdr_size + SidecarHdr::header_size(pkt))?; v.put_u8(sidecar_hdr.sc_code); v.put_u8(0); @@ -98,11 +144,18 @@ impl Protocol for SidecarHdr { v.put_u16(sidecar_hdr.sc_egress); v.put_u16(sidecar_hdr.sc_ether_type); v.put_slice(&sidecar_hdr.sc_payload); + if let Some(vlan) = pkt.hdrs.sidecar_vlan_hdr { + v.put_u16(vlan.tag.into()); + v.put_u16(vlan.ether_type); + } Ok(v) } fn header_size(packet: &Packet) -> usize { match packet.hdrs.sidecar_hdr { + Some(_) if packet.hdrs.sidecar_vlan_hdr.is_some() => { + SC_HDR_SZ + SC_VLAN_HDR_SZ + } Some(_) => SC_HDR_SZ, None => 0, } @@ -157,3 +210,103 @@ impl fmt::Debug for SidecarHdr { ) } } + +#[cfg(test)] +mod tests { + use super::*; + + const IPV4_UDP: [u8; 28] = [ + 0x45, 0x00, 0x00, 0x1c, 0x00, 0x01, 0x00, 0x00, 0x40, 0x11, 0x66, 0xde, + 0x0a, 0x00, 0x00, 0x01, 0x0a, 0x00, 0x00, 0x02, 0x0d, 0x05, 0x11, 0x5c, + 0x00, 0x08, 0x00, 0x00, + ]; + + fn sidecar_frame(vlan: Option<(u16, u16)>) -> Vec { + let mut raw = vec![ + 0x02, + 0x00, + 0x00, + 0x00, + 0x00, + 0x01, + 0x02, + 0x00, + 0x00, + 0x00, + 0x00, + 0x02, + 0x09, + 0x01, + SC_ICMP_NEEDED, + 0x00, + 0x00, + 0x11, + 0x00, + 0x00, + ]; + let sc_ether_type = match vlan { + Some(_) => eth::ETHER_VLAN, + None => eth::ETHER_IPV4, + }; + raw.extend_from_slice(&sc_ether_type.to_be_bytes()); + raw.extend_from_slice(&[0u8; 16]); + if let Some((tci, ether_type)) = vlan { + raw.extend_from_slice(&tci.to_be_bytes()); + raw.extend_from_slice(ðer_type.to_be_bytes()); + } + raw.extend_from_slice(&IPV4_UDP); + raw + } + + #[test] + fn tagged_sidecar_round_trip() { + let raw = sidecar_frame(Some((0x20c8, eth::ETHER_IPV4))); + let pkt = Packet::parse(&raw).unwrap(); + + let sc = pkt.hdrs.sidecar_hdr.unwrap(); + assert_eq!(sc.sc_ether_type, eth::ETHER_VLAN); + let vlan = pkt.hdrs.sidecar_vlan_hdr.unwrap(); + assert_eq!(vlan.tag.eth_pcp, 1); + assert_eq!(vlan.tag.eth_dei, 0); + assert_eq!(vlan.tag.eth_vlan_tag, 200); + assert_eq!(vlan.ether_type, eth::ETHER_IPV4); + assert!(pkt.hdrs.eth_hdr.unwrap().eth_8021q.is_none()); + assert!(pkt.hdrs.ipv4_hdr.is_some()); + assert!(pkt.hdrs.udp_hdr.is_some()); + + assert_eq!(SidecarHdr::header_size(&pkt), SC_HDR_SZ + SC_VLAN_HDR_SZ); + assert_eq!(&pkt.deparse().unwrap()[..], &raw[..]); + } + + #[test] + fn untagged_sidecar_round_trip() { + let raw = sidecar_frame(None); + let pkt = Packet::parse(&raw).unwrap(); + + assert!(pkt.hdrs.sidecar_vlan_hdr.is_none()); + assert!(pkt.hdrs.ipv4_hdr.is_some()); + assert_eq!(SidecarHdr::header_size(&pkt), SC_HDR_SZ); + assert_eq!(&pkt.deparse().unwrap()[..], &raw[..]); + } + + #[test] + fn eq_distinguishes_sidecar_vlan() { + let tagged = + Packet::parse(&sidecar_frame(Some((0x00c8, eth::ETHER_IPV4)))) + .unwrap(); + let untagged = Packet::parse(&sidecar_frame(None)).unwrap(); + assert!(tagged != untagged); + + let mut other_tag = tagged.clone(); + other_tag.hdrs.sidecar_vlan_hdr.as_mut().unwrap().tag.eth_vlan_tag = + 201; + assert!(tagged != other_tag); + + let mut other_type = tagged.clone(); + other_type.hdrs.sidecar_vlan_hdr.as_mut().unwrap().ether_type = + eth::ETHER_IPV6; + assert!(tagged != other_type); + + assert!(tagged == tagged.clone()); + } +} From 2c584fcfa6cc8fdd7de0d9fc4ada6eba49d6cfd0 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Fri, 30 Oct 2026 11:41:36 +0000 Subject: [PATCH 4/4] [review] updates --- .../tests/integration_tests/counters.rs | 29 +++++++++++++++---- dpd-client/tests/integration_tests/service.rs | 9 +++--- packet/src/lib.rs | 1 - 3 files changed, 27 insertions(+), 12 deletions(-) diff --git a/dpd-client/tests/integration_tests/counters.rs b/dpd-client/tests/integration_tests/counters.rs index 89f9b850..efe4278e 100644 --- a/dpd-client/tests/integration_tests/counters.rs +++ b/dpd-client/tests/integration_tests/counters.rs @@ -85,16 +85,33 @@ pub async fn check_counter_unchanged( counter_name: &str, baseline: u64, client_name: Option<&str>, +) -> anyhow::Result<()> { + check_counters_unchanged(switch, &[(counter_name, baseline)], client_name) + .await +} + +/// Assert that each `(counter, baseline)` pair holds for a single polling +/// cycle. +/// +/// Note: Every execution of this method must wait out the full cycle, about +/// 2s (`COUNTER_POLL_ATTEMPTS` × `COUNTER_POLL_INTERVAL`). Pass every counter a +/// test checks in a single call. +pub async fn check_counters_unchanged( + switch: &Switch, + counters: &[(&str, u64)], + client_name: Option<&str>, ) -> anyhow::Result<()> { let table = client_name.unwrap_or("ingress_drop_reason"); for poll in 1..=COUNTER_POLL_ATTEMPTS { tokio::time::sleep(COUNTER_POLL_INTERVAL).await; - let value = switch.get_counter(counter_name, client_name).await?; - if value != baseline { - bail!( - "counter '{counter_name}' in {table} expected to stay at \ - {baseline}, but reached {value} on poll {poll}" - ); + for &(counter_name, baseline) in counters { + let value = switch.get_counter(counter_name, client_name).await?; + if value != baseline { + bail!( + "counter '{counter_name}' in {table} expected to stay at \ + {baseline}, but reached {value} on poll {poll}" + ); + } } } Ok(()) diff --git a/dpd-client/tests/integration_tests/service.rs b/dpd-client/tests/integration_tests/service.rs index 34cc6205..c20c5af9 100644 --- a/dpd-client/tests/integration_tests/service.rs +++ b/dpd-client/tests/integration_tests/service.rs @@ -16,7 +16,7 @@ use packet::{Endpoint, eth, icmp, ipv4, sidecar}; use crate::integration_tests::common; use crate::integration_tests::common::prelude::*; use crate::integration_tests::counters::{ - check_counter_unchanged, one_counted_test, + check_counters_unchanged, one_counted_test, }; // Packets sent to an IP address assigned to a switch port should be forwarded @@ -132,12 +132,11 @@ async fn service_unicast_ttl_impl( let mut baselines = Vec::with_capacity(counters.len()); for counter in &counters { - baselines.push(switch.get_counter(counter, None).await?); + baselines + .push((counter.as_str(), switch.get_counter(counter, None).await?)); } switch.packet_test(vec![send], vec![expected])?; - for (counter, baseline) in counters.iter().zip(baselines) { - check_counter_unchanged(switch, counter, baseline, None).await?; - } + check_counters_unchanged(switch, &baselines, None).await?; Ok(()) } diff --git a/packet/src/lib.rs b/packet/src/lib.rs index 91a7096f..9fc7dd90 100644 --- a/packet/src/lib.rs +++ b/packet/src/lib.rs @@ -321,7 +321,6 @@ pub struct Headers { pub eth_hdr: Option, pub lldp_hdr: Option, pub sidecar_hdr: Option, - /// 802.1q tag following the sidecar header (see [`sidecar::SidecarVlanHdr`]). pub sidecar_vlan_hdr: Option, pub ipv4_hdr: Option, pub ipv6_hdr: Option,