From c4068517dd8dd9cbc28133d3005f2c1d6cd753b2 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Fri, 16 Oct 2026 11:08:51 +0100 Subject: [PATCH 1/4] Count ICMP-needed drops from L3Router Packets which are dropped in L3Router and its nested controls all need to be handled by the host, so we cannot actually drop these packets in the Tofino. However the counters were only ever modified when the packet is truly dropped, so these counters were non-functional. This PR separates out the counter increment to occur for both classes of drop. I did look into centralising everything onto `ig_dprsr_md.drop_ctl` and ridding ourselves of `meta.dropped`, but this ended up both uglier and took up more stages under multicast. Simple seems best, here! Fixes #404. --- .../tests/integration_tests/counters.rs | 44 ++++++++++++++----- .../tests/integration_tests/route_ipv4.rs | 29 ++++++++++-- .../tests/integration_tests/route_ipv6.rs | 14 +++++- dpd/p4/sidecar.p4 | 12 +++-- 4 files changed, 78 insertions(+), 21 deletions(-) diff --git a/dpd-client/tests/integration_tests/counters.rs b/dpd-client/tests/integration_tests/counters.rs index f793f2a6..239fbf87 100644 --- a/dpd-client/tests/integration_tests/counters.rs +++ b/dpd-client/tests/integration_tests/counters.rs @@ -22,21 +22,20 @@ use dpd_client::ClientInfo; use dpd_client::types::Ipv4Entry; use dpd_client::types::Ipv6Entry; -// 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 -// runs to completion we return the counter evaluation as a boolean rather than -// an Error. This allows us to trigger the error in the test routine, which -// gives us a more specific test failure message. -async fn one_drop_test( +/// Process a single packet, asserting that a given counter is incremented by +/// one and a packet is appropriately transformed (or dropped). +/// +/// If the test runs to completion we return the counter evaluation as a +/// boolean rather than an Error. This allows us to trigger the error in +/// the test routine, which gives us a more specific test failure message +pub async fn one_counted_test( switch: &Switch, - port: PhysPort, - packet: packet::Packet, + packet: TestPacket, + packet_after: Option, counter: &str, ) -> anyhow::Result { - let send = TestPacket { packet: Arc::new(packet), port }; - let old = switch.get_counter(counter, None).await?; - switch.packet_test(vec![send], Vec::new())?; + switch.packet_test(vec![packet], packet_after.into_iter().collect())?; let mut new = 0; for _i in 0..20 { @@ -45,7 +44,7 @@ async fn one_drop_test( // simulator and when that updated value is available to the SDE. To // avoid a long pointless delay here, we try multiple times with a short // sleep rather than once with a long sleep. - std::thread::sleep(std::time::Duration::from_millis(100)); + tokio::time::sleep(std::time::Duration::from_millis(100)).await; new = switch.get_counter(counter, None).await?; if old + 1 == new { break; @@ -55,6 +54,27 @@ async fn one_drop_test( Ok(old + 1 == new) } +/// 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 runs to completion we return the counter evaluation as a +/// boolean rather than an Error. This allows us to trigger the error in +/// the test routine, which gives us a more specific test failure message. +async fn one_drop_test( + switch: &Switch, + port: PhysPort, + packet: packet::Packet, + counter: &str, +) -> anyhow::Result { + one_counted_test( + switch, + TestPacket { packet: Arc::new(packet), port }, + None, + counter, + ) + .await +} + // Add an IPv4 address to a port on the switch async fn add_switch_ipv4(switch: &Switch, port: PhysPort, addr: &str) { let (port_id, link_id) = switch.link_id(port).unwrap(); diff --git a/dpd-client/tests/integration_tests/route_ipv4.rs b/dpd-client/tests/integration_tests/route_ipv4.rs index 75f7eceb..7e2d2aac 100644 --- a/dpd-client/tests/integration_tests/route_ipv4.rs +++ b/dpd-client/tests/integration_tests/route_ipv4.rs @@ -14,6 +14,7 @@ use reqwest::StatusCode; use crate::integration_tests::common; use crate::integration_tests::common::prelude::*; +use crate::integration_tests::counters::one_counted_test; use packet::Endpoint; use packet::eth::EthQHdr; @@ -159,7 +160,12 @@ async fn test_deleted_route_ipv4_impl(switch: &Switch) -> TestResult { common::set_icmp_unreachable(switch, &mut to_recv, ingress); let expected = TestPacket { packet: Arc::new(to_recv), port: egress }; - switch.packet_test(vec![send], vec![expected]) + assert!( + one_counted_test(switch, send, Some(expected), "ipv4_unrouteable") + .await? + ); + + Ok(()) } #[tokio::test] @@ -262,7 +268,12 @@ async fn test_route_ipv4_unrouteable() -> TestResult { common::set_icmp_unreachable(switch, &mut to_recv, ingress); let expected = TestPacket { packet: Arc::new(to_recv), port: egress }; - switch.packet_test(vec![send], vec![expected]) + assert!( + one_counted_test(switch, send, Some(expected), "ipv4_unrouteable") + .await? + ); + + Ok(()) } #[tokio::test] @@ -934,6 +945,7 @@ async fn test_v4_over_v6() -> TestResult { .expect_err("route should be gone after deleting its only target"); // Verify that packets to the deleted route produce ICMP unreachable. + // This must *also* increment the relevant drop counter. let to_send = common::gen_udp_packet( Endpoint::parse("e0:d5:5e:67:89:ab", "10.10.10.10", 3333).unwrap(), Endpoint::parse("e0:d5:5e:67:89:ac", "10.10.10.11", 4444).unwrap(), @@ -943,7 +955,11 @@ async fn test_v4_over_v6() -> TestResult { let send = TestPacket { packet: Arc::new(to_send), port: ingress }; let expected = TestPacket { packet: Arc::new(to_recv), port: SERVICE_PORT }; - switch.packet_test(vec![send], vec![expected])?; + + assert!( + one_counted_test(switch, send, Some(expected), "ipv4_unrouteable") + .await? + ); // Re-add the route and verify forwarding works again. common::set_route_ipv4_over_ipv6( @@ -979,7 +995,12 @@ async fn test_v4_over_v6() -> TestResult { let send = TestPacket { packet: Arc::new(to_send), port: ingress }; let expected = TestPacket { packet: Arc::new(to_recv), port: SERVICE_PORT }; - switch.packet_test(vec![send], vec![expected]) + assert!( + one_counted_test(switch, send, Some(expected), "ipv4_unrouteable") + .await? + ); + + Ok(()) } /// Test that individual v4 and v6 targets can be deleted from a mixed diff --git a/dpd-client/tests/integration_tests/route_ipv6.rs b/dpd-client/tests/integration_tests/route_ipv6.rs index 81932cac..1ae6398f 100644 --- a/dpd-client/tests/integration_tests/route_ipv6.rs +++ b/dpd-client/tests/integration_tests/route_ipv6.rs @@ -16,6 +16,7 @@ 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 packet::eth::EthQHdr; use dpd_client::types; @@ -201,7 +202,12 @@ async fn test_deleted_unicast_impl(switch: &Switch) -> TestResult { common::set_icmp6_unreachable(switch, &mut to_recv, ingress); let expected = TestPacket { packet: Arc::new(to_recv), port: egress }; - switch.packet_test(vec![send], vec![expected]) + assert!( + one_counted_test(switch, send, Some(expected), "ipv6_unrouteable") + .await? + ); + + Ok(()) } #[tokio::test] @@ -279,8 +285,12 @@ async fn test_unrouteable() -> TestResult { common::set_icmp6_unreachable(switch, &mut to_recv, ingress); let expected = TestPacket { packet: Arc::new(to_recv), port: egress }; + assert!( + one_counted_test(switch, send, Some(expected), "ipv6_unrouteable") + .await? + ); - switch.packet_test(vec![send], vec![expected]) + Ok(()) } // Define two routes, one with a 56-bit prefix and one with a 64-bit prefix. diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index b06a842a..108dba15 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -2187,11 +2187,17 @@ control Ingress( } } - if (meta.dropped) { - // Handle dropped packets - ig_dprsr_md.drop_ctl = 1; + // Drops recorded in the L3 router need to be counted, but + // we do not convert them to _actual_ packet drops in the + // deparser. We forward such packets to the host to generate an + // ICMP response in our stead. + if (meta.drop_reason != 0) { drop_port_ctr.count(ig_intr_md.ingress_port); drop_reason_ctr.count(meta.drop_reason); + } + + if (meta.dropped) { + ig_dprsr_md.drop_ctl = 1; } else if (!meta.is_mcast) { egress_ctr.count(ig_tm_md.ucast_egress_port); if (ig_tm_md.ucast_egress_port != USER_SPACE_SERVICE_PORT) { From 241e705bcaba8410923a654bf6e0eb48a58b98ec Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Mon, 26 Oct 2026 17:43:05 +0000 Subject: [PATCH 2/4] Review feedback: breakout `check_counter_increment`. --- .../tests/integration_tests/counters.rs | 68 ++++++++++++------- dpd-client/tests/integration_tests/mcast.rs | 32 +-------- 2 files changed, 44 insertions(+), 56 deletions(-) diff --git a/dpd-client/tests/integration_tests/counters.rs b/dpd-client/tests/integration_tests/counters.rs index 239fbf87..1f50d7ff 100644 --- a/dpd-client/tests/integration_tests/counters.rs +++ b/dpd-client/tests/integration_tests/counters.rs @@ -22,12 +22,47 @@ use dpd_client::ClientInfo; use dpd_client::types::Ipv4Entry; use dpd_client::types::Ipv6Entry; -/// Process a single packet, asserting that a given counter is incremented by -/// one and a packet is appropriately transformed (or dropped). +/// Awaits an increase in a given counter's value from `baseline` to +/// `baseline + increment`. /// -/// If the test runs to completion we return the counter evaluation as a -/// boolean rather than an Error. This allows us to trigger the error in -/// the test routine, which gives us a more specific test failure message +/// Returns `Ok(new_value)` if the expected increment is seen. +pub async fn check_counter_incremented( + switch: &Switch, + counter_name: &str, + baseline: u64, + expected_increment: u64, + client_name: Option<&str>, +) -> anyhow::Result { + let mut new_value = 0; + + // Briefly delay before reading the counter each time, as there is + // occasionally some lag between when we see the counter set in the + // simulator and when that updated value is available to the SDE. To + // avoid a long pointless delay here, we try multiple times with a short + // sleep rather than once with a long sleep. + for _ in 0..20 { + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + new_value = + switch.get_counter(counter_name, client_name).await.unwrap(); + + if new_value == baseline + expected_increment { + return Ok(new_value); + } + } + + // Counter didn't increment as expected. + Err(anyhow::anyhow!( + "Counter '{}' expected to increase by {} (from {} to {}), but only reached {}", + counter_name, + expected_increment, + baseline, + baseline + expected_increment, + new_value + )) +} + +/// 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( switch: &Switch, packet: TestPacket, @@ -36,30 +71,13 @@ pub async fn one_counted_test( ) -> anyhow::Result { let old = switch.get_counter(counter, None).await?; switch.packet_test(vec![packet], packet_after.into_iter().collect())?; - - let mut new = 0; - for _i in 0..20 { - // Briefly delay before reading the counter for a second time, as there is - // occasionally some lag between when we see the counter set in the - // simulator and when that updated value is available to the SDE. To - // avoid a long pointless delay here, we try multiple times with a short - // sleep rather than once with a long sleep. - tokio::time::sleep(std::time::Duration::from_millis(100)).await; - new = switch.get_counter(counter, None).await?; - if old + 1 == new { - break; - } - } - - Ok(old + 1 == new) + check_counter_incremented(switch, counter, old, 1, None) + .await + .map(|new| old + 1 == new) } /// 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 runs to completion we return the counter evaluation as a -/// boolean rather than an Error. This allows us to trigger the error in -/// the test routine, which gives us a more specific test failure message. async fn one_drop_test( switch: &Switch, port: PhysPort, diff --git a/dpd-client/tests/integration_tests/mcast.rs b/dpd-client/tests/integration_tests/mcast.rs index 6c5ac347..4bf0992f 100644 --- a/dpd-client/tests/integration_tests/mcast.rs +++ b/dpd-client/tests/integration_tests/mcast.rs @@ -12,6 +12,7 @@ use std::{ use crate::integration_tests::common; use crate::integration_tests::common::prelude::*; +use crate::integration_tests::counters::check_counter_incremented; use ::common::network::MacAddr; use anyhow::anyhow; use dpd_client::{Error, types}; @@ -71,37 +72,6 @@ fn has_vlan_action_for_ip( }) } -async fn check_counter_incremented( - switch: &Switch, - counter_name: &str, - baseline: u64, - expected_increment: u64, - client_name: Option<&str>, -) -> anyhow::Result { - let mut new_value = 0; - - // Poll for the counter value (with timeout) - for _i in 0..20 { - std::thread::sleep(std::time::Duration::from_millis(100)); - new_value = - switch.get_counter(counter_name, client_name).await.unwrap(); - - if new_value == baseline + expected_increment { - return Ok(new_value); - } - } - - // Counter didn't increment as expected - Err(anyhow!( - "Counter '{}' expected to increase by {} (from {} to {}), but only reached {}", - counter_name, - expected_increment, - baseline, - baseline + expected_increment, - new_value - )) -} - async fn create_test_multicast_group( switch: &Switch, group_ip: IpAddr, From 8e0b3420204f270809c20f73e68316833f90a794 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Mon, 26 Oct 2026 18:32:24 +0000 Subject: [PATCH 3/4] Review: add counter checks to existing service tests --- dpd-client/tests/integration_tests/service.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/dpd-client/tests/integration_tests/service.rs b/dpd-client/tests/integration_tests/service.rs index 2aeff4a5..7441dad2 100644 --- a/dpd-client/tests/integration_tests/service.rs +++ b/dpd-client/tests/integration_tests/service.rs @@ -13,6 +13,7 @@ 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; // Packets sent to an IP address assigned to a switch port should be forwarded // to the service port with an added sidecar header. @@ -221,7 +222,9 @@ async fn test_arp_needed() -> TestResult { let expected = TestPacket { packet: Arc::new(recv_pkt), port: SERVICE_PORT }; - switch.packet_test(vec![send], vec![expected]) + assert!(one_counted_test(switch, send, Some(expected), "arp_miss").await?); + + Ok(()) } #[tokio::test] @@ -266,7 +269,13 @@ async fn test_ttl_exceeded() -> TestResult { ); let expected = TestPacket { packet: Arc::new(recv_pkt), port: SERVICE_PORT }; - switch.packet_test(vec![send], vec![expected]) + + assert!( + one_counted_test(switch, send, Some(expected), "ipv4_ttl_exceeded") + .await? + ); + + Ok(()) } // Arp packets sent to an IP address assigned to a switch port should be From a4058fcbd767ab4f12154142383d1cd69bf45746 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Tue, 27 Oct 2026 11:35:17 +0000 Subject: [PATCH 4/4] Service tests for NDP, IPv6 TTL expiry --- dpd-client/tests/integration_tests/service.rs | 107 +++++++++++++++++- packet/src/icmp.rs | 4 +- 2 files changed, 107 insertions(+), 4 deletions(-) diff --git a/dpd-client/tests/integration_tests/service.rs b/dpd-client/tests/integration_tests/service.rs index 7441dad2..78e70bee 100644 --- a/dpd-client/tests/integration_tests/service.rs +++ b/dpd-client/tests/integration_tests/service.rs @@ -4,12 +4,13 @@ // // Copyright 2026 Oxide Computer Company +use std::net::Ipv6Addr; 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 packet::{Endpoint, eth, icmp, ipv4, ipv6, sidecar}; use crate::integration_tests::common; use crate::integration_tests::common::prelude::*; @@ -227,6 +228,55 @@ async fn test_arp_needed() -> TestResult { Ok(()) } +#[tokio::test] +#[ignore] +async fn test_ndp_needed() -> TestResult { + let switch = &*get_switch().await; + + let ingress_port = PhysPort(10); + let egress_port = PhysPort(15); + let router_ip = "fd00:1122:7788:0101::1"; + let src_mac = "e0:d5:5e:67:89:ab"; + let router_mac = switch.get_port_mac(ingress_port).unwrap(); + + // As in test_arp_needed, set a route without installing a neighbor. + common::set_route_ipv6(switch, "::/0", egress_port, router_ip).await?; + + let (send_pkt, mut recv_pkt) = common::gen_udp_routed_pair( + switch, + egress_port, + router_mac, + Endpoint::parse(src_mac, "fd00:1122:7788:0202::4", 3333).unwrap(), + Endpoint::parse("e0:d5:5e:67:89:ac", "fd00:1122:7788:0101::5", 4444) + .unwrap(), + ); + + let send = TestPacket { packet: Arc::new(send_pkt), port: ingress_port }; + + let payload = router_ip.parse::().unwrap().octets(); + common::add_sidecar_hdr( + switch, + &mut recv_pkt, + sidecar::SC_NEIGHBOR_NEEDED, + ingress_port, + egress_port, + Some(&payload[..]), + ); + + // Because the packet wasn't successfully routed, the src/dst mac addresses + // should be unchanged. + let src_eth = send.packet.hdrs.eth_hdr.as_ref().unwrap(); + eth::EthHdr::rewrite_smac(&mut recv_pkt, src_eth.eth_smac); + eth::EthHdr::rewrite_dmac(&mut recv_pkt, src_eth.eth_dmac); + + let expected = + TestPacket { packet: Arc::new(recv_pkt), port: SERVICE_PORT }; + + assert!(one_counted_test(switch, send, Some(expected), "ndp_miss").await?); + + Ok(()) +} + #[tokio::test] #[ignore] async fn test_ttl_exceeded() -> TestResult { @@ -265,7 +315,7 @@ async fn test_ttl_exceeded() -> TestResult { ingress, SERVICE_PORT, icmp::ICMP_TIME_EXCEEDED, - 0, + icmp::ICMP_EXC_TTL, ); let expected = TestPacket { packet: Arc::new(recv_pkt), port: SERVICE_PORT }; @@ -278,6 +328,59 @@ async fn test_ttl_exceeded() -> TestResult { Ok(()) } +#[tokio::test] +#[ignore] +async fn test_hop_limit_exceeded() -> TestResult { + let switch = &*get_switch().await; + + let ingress = PhysPort(10); + let egress = PhysPort(14); + let port_mac = "a8:40:25:00:00:0c".parse::()?; + + let orig_src = + Endpoint::parse("e0:d5:5e:67:89:ab", "fd00:1122:7788:0202::4", 3333) + .unwrap(); + let orig_dst = + Endpoint::parse("e0:d5:5e:67:89:ac", "fd00:1122:3344:0101::5", 4444) + .unwrap(); + + let (port_id, link_id) = switch.link_id(ingress).unwrap(); + switch + .client + .link_mac_set(&port_id, &link_id, &port_mac.into()) + .await + .unwrap(); + + 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); + + let mut recv_pkt = send_pkt.clone(); + + let send = TestPacket { packet: Arc::new(send_pkt), port: ingress }; + + common::set_icmp_needed( + switch, + &mut recv_pkt, + ingress, + SERVICE_PORT, + icmp::ICMP6_TIME_EXCEEDED, + icmp::ICMP6_TIME_EXCEED_TRANSIT, + ); + let expected = + TestPacket { packet: Arc::new(recv_pkt), port: SERVICE_PORT }; + + assert!( + one_counted_test(switch, send, Some(expected), "ipv6_ttl_exceeded") + .await? + ); + + Ok(()) +} + // 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/packet/src/icmp.rs b/packet/src/icmp.rs index bd81c1fc..f2e46048 100644 --- a/packet/src/icmp.rs +++ b/packet/src/icmp.rs @@ -69,8 +69,8 @@ pub const ICMP6_DST_UNREACH_BEYONDSCOPE: u8 = 2; pub const ICMP6_DST_UNREACH_ADDR: u8 = 3; pub const ICMP6_DST_UNREACH_NOPORT: u8 = 4; -pub const ICMP6_TIME_EXCEED_TRANSIT: u8 = 1; -pub const ICMP6_TIME_EXCEED_REASSEMBLY: u8 = 2; +pub const ICMP6_TIME_EXCEED_TRANSIT: u8 = 0; +pub const ICMP6_TIME_EXCEED_REASSEMBLY: u8 = 1; pub const ICMP6_PARAMPROB_HEADER: u8 = 0; pub const ICMP6_PARAMPROB_NEXTHEADER: u8 = 1;