From 1733d3e6be3953e3a391bfcf2ea259f0a547e089 Mon Sep 17 00:00:00 2001 From: Ryan Zuklie Date: Tue, 12 Sep 2023 10:28:21 -0700 Subject: [PATCH 1/2] Make BundleKey a distinct struct from PacketTrace. This change refactors BundleKey to be a separate struct. This allows us to do the conversion (e.g. of sport/dport, or icmp fields in the future) in the constructor and change types (use optionals) where it is convenient to simplify the code. This change also fixes a bunch of desigated initializer order warnings in the test (fields should be declared in same order as defined). Test: atest libnetworkstats_test Change-Id: I6373d2512280e93c54395fde38dcff43705b449e --- .../libnetworkstats/NetworkTraceHandler.cpp | 44 ++++++++++--------- .../NetworkTraceHandlerTest.cpp | 44 +++++++++---------- .../include/netdbpf/NetworkTraceHandler.h | 30 ++++++++++--- 3 files changed, 69 insertions(+), 49 deletions(-) diff --git a/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp b/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp index ec63e41694..3520254acb 100644 --- a/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp +++ b/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp @@ -75,9 +75,20 @@ struct BundleDetails { uint32_t bytes = 0; }; -#define AGG_FIELDS(x) \ - (x).ifindex, (x).uid, (x).tag, (x).sport, (x).dport, (x).egress, \ - (x).ipProto, (x).tcpFlags +BundleKey::BundleKey(const PacketTrace& pkt) + : ifindex(pkt.ifindex), + uid(pkt.uid), + tag(pkt.tag), + egress(pkt.egress), + ipProto(pkt.ipProto), + ipVersion(pkt.ipVersion), + tcpFlags(pkt.tcpFlags), + localPort(ntohs(pkt.egress ? pkt.sport : pkt.dport)), + remotePort(ntohs(pkt.egress ? pkt.dport : pkt.sport)) {} + +#define AGG_FIELDS(x) \ + (x).ifindex, (x).uid, (x).tag, (x).egress, (x).ipProto, (x).ipVersion, \ + (x).tcpFlags, (x).localPort, (x).remotePort std::size_t BundleHash::operator()(const BundleKey& a) const { std::size_t seed = 0; @@ -179,7 +190,7 @@ void NetworkTraceHandler::Write(const std::vector& packets, dst->set_timestamp(pkt.timestampNs); auto* event = dst->set_network_packet(); event->set_length(pkt.length); - Fill(pkt, event); + Fill(BundleKey(pkt), event); } return; } @@ -187,14 +198,13 @@ void NetworkTraceHandler::Write(const std::vector& packets, uint64_t minTs = std::numeric_limits::max(); std::unordered_map bundles; for (const PacketTrace& pkt : packets) { - BundleKey key = pkt; + BundleKey key(pkt); // Dropping fields should remove them from the output and remove them from - // the aggregation key. In order to do the latter without changing the hash - // function, set the dropped fields to zero. - if (mDropTcpFlags) key.tcpFlags = 0; - if (mDropLocalPort) (key.egress ? key.sport : key.dport) = 0; - if (mDropRemotePort) (key.egress ? key.dport : key.sport) = 0; + // the aggregation key. Reset the optionals to indicate omission. + if (mDropTcpFlags) key.tcpFlags.reset(); + if (mDropLocalPort) key.localPort.reset(); + if (mDropRemotePort) key.remotePort.reset(); minTs = std::min(minTs, pkt.timestampNs); @@ -245,22 +255,16 @@ void NetworkTraceHandler::Write(const std::vector& packets, } } -void NetworkTraceHandler::Fill(const PacketTrace& src, +void NetworkTraceHandler::Fill(const BundleKey& src, NetworkPacketEvent* event) { event->set_direction(src.egress ? TrafficDirection::DIR_EGRESS : TrafficDirection::DIR_INGRESS); event->set_uid(src.uid); event->set_tag(src.tag); - if (!mDropLocalPort) { - event->set_local_port(ntohs(src.egress ? src.sport : src.dport)); - } - if (!mDropRemotePort) { - event->set_remote_port(ntohs(src.egress ? src.dport : src.sport)); - } - if (!mDropTcpFlags) { - event->set_tcp_flags(src.tcpFlags); - } + if (src.tcpFlags.has_value()) event->set_tcp_flags(*src.tcpFlags); + if (src.localPort.has_value()) event->set_local_port(*src.localPort); + if (src.remotePort.has_value()) event->set_remote_port(*src.remotePort); event->set_ip_proto(src.ipProto); diff --git a/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp b/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp index f2c1a867bf..2eb2e4f313 100644 --- a/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp +++ b/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp @@ -168,12 +168,12 @@ TEST_F(NetworkTraceHandlerTest, BasicBundling) { config.set_aggregation_threshold(10); std::vector input = { - PacketTrace{.uid = 123, .timestampNs = 2, .length = 200}, - PacketTrace{.uid = 123, .timestampNs = 1, .length = 100}, - PacketTrace{.uid = 123, .timestampNs = 4, .length = 300}, + PacketTrace{.timestampNs = 2, .length = 200, .uid = 123}, + PacketTrace{.timestampNs = 1, .length = 100, .uid = 123}, + PacketTrace{.timestampNs = 4, .length = 300, .uid = 123}, - PacketTrace{.uid = 456, .timestampNs = 2, .length = 400}, - PacketTrace{.uid = 456, .timestampNs = 4, .length = 100}, + PacketTrace{.timestampNs = 2, .length = 400, .uid = 456}, + PacketTrace{.timestampNs = 4, .length = 100, .uid = 456}, }; std::vector events; @@ -203,12 +203,12 @@ TEST_F(NetworkTraceHandlerTest, AggregationThreshold) { config.set_aggregation_threshold(3); std::vector input = { - PacketTrace{.uid = 123, .timestampNs = 2, .length = 200}, - PacketTrace{.uid = 123, .timestampNs = 1, .length = 100}, - PacketTrace{.uid = 123, .timestampNs = 4, .length = 300}, + PacketTrace{.timestampNs = 2, .length = 200, .uid = 123}, + PacketTrace{.timestampNs = 1, .length = 100, .uid = 123}, + PacketTrace{.timestampNs = 4, .length = 300, .uid = 123}, - PacketTrace{.uid = 456, .timestampNs = 2, .length = 400}, - PacketTrace{.uid = 456, .timestampNs = 4, .length = 100}, + PacketTrace{.timestampNs = 2, .length = 400, .uid = 456}, + PacketTrace{.timestampNs = 4, .length = 100, .uid = 456}, }; std::vector events; @@ -239,10 +239,10 @@ TEST_F(NetworkTraceHandlerTest, DropLocalPort) { __be16 b = htons(10001); std::vector input = { // Recall that local is `src` for egress and `dst` for ingress. - PacketTrace{.timestampNs = 1, .length = 2, .egress = true, .sport = a}, - PacketTrace{.timestampNs = 2, .length = 4, .egress = false, .dport = a}, - PacketTrace{.timestampNs = 3, .length = 6, .egress = true, .sport = b}, - PacketTrace{.timestampNs = 4, .length = 8, .egress = false, .dport = b}, + PacketTrace{.timestampNs = 1, .length = 2, .sport = a, .egress = true}, + PacketTrace{.timestampNs = 2, .length = 4, .dport = a, .egress = false}, + PacketTrace{.timestampNs = 3, .length = 6, .sport = b, .egress = true}, + PacketTrace{.timestampNs = 4, .length = 8, .dport = b, .egress = false}, }; std::vector events; @@ -274,10 +274,10 @@ TEST_F(NetworkTraceHandlerTest, DropRemotePort) { __be16 b = htons(80); std::vector input = { // Recall that remote is `dst` for egress and `src` for ingress. - PacketTrace{.timestampNs = 1, .length = 2, .egress = true, .dport = a}, - PacketTrace{.timestampNs = 2, .length = 4, .egress = false, .sport = a}, - PacketTrace{.timestampNs = 3, .length = 6, .egress = true, .dport = b}, - PacketTrace{.timestampNs = 4, .length = 8, .egress = false, .sport = b}, + PacketTrace{.timestampNs = 1, .length = 2, .dport = a, .egress = true}, + PacketTrace{.timestampNs = 2, .length = 4, .sport = a, .egress = false}, + PacketTrace{.timestampNs = 3, .length = 6, .dport = b, .egress = true}, + PacketTrace{.timestampNs = 4, .length = 8, .sport = b, .egress = false}, }; std::vector events; @@ -306,10 +306,10 @@ TEST_F(NetworkTraceHandlerTest, DropTcpFlags) { config.set_aggregation_threshold(10); std::vector input = { - PacketTrace{.timestampNs = 1, .uid = 123, .length = 1, .tcpFlags = 1}, - PacketTrace{.timestampNs = 2, .uid = 123, .length = 2, .tcpFlags = 2}, - PacketTrace{.timestampNs = 3, .uid = 456, .length = 3, .tcpFlags = 1}, - PacketTrace{.timestampNs = 4, .uid = 456, .length = 4, .tcpFlags = 2}, + PacketTrace{.timestampNs = 1, .length = 1, .uid = 123, .tcpFlags = 1}, + PacketTrace{.timestampNs = 2, .length = 2, .uid = 123, .tcpFlags = 2}, + PacketTrace{.timestampNs = 3, .length = 3, .uid = 456, .tcpFlags = 1}, + PacketTrace{.timestampNs = 4, .length = 4, .uid = 456, .tcpFlags = 2}, }; std::vector events; diff --git a/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h b/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h index bc10e68cce..853e26e09e 100644 --- a/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h +++ b/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h @@ -30,15 +30,31 @@ namespace android { namespace bpf { -// BundleKeys are PacketTraces where timestamp and length are ignored. -using BundleKey = PacketTrace; +// BundleKey encodes a PacketTrace minus timestamp and length. The key should +// match many packets over time for interning. For convenience, sport/dport +// are parsed here as either local/remote port. +struct BundleKey { + explicit BundleKey(const PacketTrace& pkt); -// BundleKeys are hashed using all fields except timestamp/length. + uint32_t ifindex; + uint32_t uid; + uint32_t tag; + + bool egress; + uint8_t ipProto; + uint8_t ipVersion; + + std::optional tcpFlags; + std::optional localPort; + std::optional remotePort; +}; + +// BundleKeys are hashed using a simple hash combine. struct BundleHash { std::size_t operator()(const BundleKey& a) const; }; -// BundleKeys are equal if all fields except timestamp/length are equal. +// BundleKeys are equal if all fields are equal. struct BundleEq { bool operator()(const BundleKey& a, const BundleKey& b) const; }; @@ -84,13 +100,13 @@ class NetworkTraceHandler NetworkTraceHandler::TraceContext& ctx); private: - // Convert a PacketTrace into a Perfetto trace packet. - void Fill(const PacketTrace& src, + // Fills in contextual information from a bundle without interning. + void Fill(const BundleKey& src, ::perfetto::protos::pbzero::NetworkPacketEvent* event); // Fills in contextual information either inline or via interning. ::perfetto::protos::pbzero::NetworkPacketBundle* FillWithInterning( - NetworkTraceState* state, const BundleKey& key, + NetworkTraceState* state, const BundleKey& src, ::perfetto::protos::pbzero::TracePacket* dst); static internal::NetworkTracePoller sPoller; From e72b48416cd3de021ffed18058a8b09af1a54108 Mon Sep 17 00:00:00 2001 From: Ryan Zuklie Date: Tue, 12 Sep 2023 13:59:20 -0700 Subject: [PATCH 2/2] Add ICMP type/code and udplite/sctp in Nettrace. This adds support for additional protocols and also provides more clear semantics of protocol specific fields, such as only including tcpFlags when the protocol is IPPROTO_TCP. Test: atest libnetworkstats_test & flash and trace Change-Id: Ic69fc75e85ebf8734027c942a253af4972ad14d8 --- .../libnetworkstats/NetworkTraceHandler.cpp | 28 ++++++++-- .../NetworkTraceHandlerTest.cpp | 55 ++++++++++++++++++- .../include/netdbpf/NetworkTraceHandler.h | 4 +- 3 files changed, 80 insertions(+), 7 deletions(-) diff --git a/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp b/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp index 3520254acb..9b1b72dac6 100644 --- a/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp +++ b/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp @@ -18,6 +18,7 @@ #include "netdbpf/NetworkTraceHandler.h" +#include #include #include #include @@ -81,14 +82,29 @@ BundleKey::BundleKey(const PacketTrace& pkt) tag(pkt.tag), egress(pkt.egress), ipProto(pkt.ipProto), - ipVersion(pkt.ipVersion), - tcpFlags(pkt.tcpFlags), - localPort(ntohs(pkt.egress ? pkt.sport : pkt.dport)), - remotePort(ntohs(pkt.egress ? pkt.dport : pkt.sport)) {} + ipVersion(pkt.ipVersion) { + switch (ipProto) { + case IPPROTO_TCP: + tcpFlags = pkt.tcpFlags; + FALLTHROUGH_INTENDED; + case IPPROTO_DCCP: + case IPPROTO_UDP: + case IPPROTO_UDPLITE: + case IPPROTO_SCTP: + localPort = ntohs(pkt.egress ? pkt.sport : pkt.dport); + remotePort = ntohs(pkt.egress ? pkt.dport : pkt.sport); + break; + case IPPROTO_ICMP: + case IPPROTO_ICMPV6: + icmpType = ntohs(pkt.sport); + icmpCode = ntohs(pkt.dport); + break; + } +} #define AGG_FIELDS(x) \ (x).ifindex, (x).uid, (x).tag, (x).egress, (x).ipProto, (x).ipVersion, \ - (x).tcpFlags, (x).localPort, (x).remotePort + (x).tcpFlags, (x).localPort, (x).remotePort, (x).icmpType, (x).icmpCode std::size_t BundleHash::operator()(const BundleKey& a) const { std::size_t seed = 0; @@ -265,6 +281,8 @@ void NetworkTraceHandler::Fill(const BundleKey& src, if (src.tcpFlags.has_value()) event->set_tcp_flags(*src.tcpFlags); if (src.localPort.has_value()) event->set_local_port(*src.localPort); if (src.remotePort.has_value()) event->set_remote_port(*src.remotePort); + if (src.icmpType.has_value()) event->set_icmp_type(*src.icmpType); + if (src.icmpCode.has_value()) event->set_icmp_code(*src.icmpCode); event->set_ip_proto(src.ipProto); diff --git a/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp b/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp index 2eb2e4f313..0c4f049ce1 100644 --- a/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp +++ b/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp @@ -113,7 +113,7 @@ TEST_F(NetworkTraceHandlerTest, WriteBasicFields) { .length = 100, .uid = 10, .tag = 123, - .ipProto = 6, + .ipProto = IPPROTO_TCP, .tcpFlags = 1, }, }; @@ -138,12 +138,14 @@ TEST_F(NetworkTraceHandlerTest, WriteDirectionAndPorts) { .sport = htons(8080), .dport = htons(443), .egress = true, + .ipProto = IPPROTO_TCP, }, PacketTrace{ .timestampNs = 2, .sport = htons(443), .dport = htons(8080), .egress = false, + .ipProto = IPPROTO_TCP, }, }; @@ -161,6 +163,42 @@ TEST_F(NetworkTraceHandlerTest, WriteDirectionAndPorts) { TrafficDirection::DIR_INGRESS); } +TEST_F(NetworkTraceHandlerTest, WriteIcmpTypeAndCode) { + std::vector input = { + PacketTrace{ + .timestampNs = 1, + .sport = htons(11), // type + .dport = htons(22), // code + .egress = true, + .ipProto = IPPROTO_ICMP, + }, + PacketTrace{ + .timestampNs = 2, + .sport = htons(33), // type + .dport = htons(44), // code + .egress = false, + .ipProto = IPPROTO_ICMPV6, + }, + }; + + std::vector events; + ASSERT_TRUE(TraceAndSortPackets(input, &events)); + + ASSERT_EQ(events.size(), 2); + EXPECT_FALSE(events[0].network_packet().has_local_port()); + EXPECT_FALSE(events[0].network_packet().has_remote_port()); + EXPECT_THAT(events[0].network_packet().icmp_type(), 11); + EXPECT_THAT(events[0].network_packet().icmp_code(), 22); + EXPECT_THAT(events[0].network_packet().direction(), + TrafficDirection::DIR_EGRESS); + EXPECT_FALSE(events[1].network_packet().local_port()); + EXPECT_FALSE(events[1].network_packet().remote_port()); + EXPECT_THAT(events[1].network_packet().icmp_type(), 33); + EXPECT_THAT(events[1].network_packet().icmp_code(), 44); + EXPECT_THAT(events[1].network_packet().direction(), + TrafficDirection::DIR_INGRESS); +} + TEST_F(NetworkTraceHandlerTest, BasicBundling) { // TODO: remove this once bundling becomes default. Until then, set arbitrary // aggregation threshold to enable bundling. @@ -245,6 +283,11 @@ TEST_F(NetworkTraceHandlerTest, DropLocalPort) { PacketTrace{.timestampNs = 4, .length = 8, .dport = b, .egress = false}, }; + // Set common fields. + for (PacketTrace& pkt : input) { + pkt.ipProto = IPPROTO_TCP; + } + std::vector events; ASSERT_TRUE(TraceAndSortPackets(input, &events, config)); ASSERT_EQ(events.size(), 2); @@ -280,6 +323,11 @@ TEST_F(NetworkTraceHandlerTest, DropRemotePort) { PacketTrace{.timestampNs = 4, .length = 8, .sport = b, .egress = false}, }; + // Set common fields. + for (PacketTrace& pkt : input) { + pkt.ipProto = IPPROTO_TCP; + } + std::vector events; ASSERT_TRUE(TraceAndSortPackets(input, &events, config)); ASSERT_EQ(events.size(), 2); @@ -312,6 +360,11 @@ TEST_F(NetworkTraceHandlerTest, DropTcpFlags) { PacketTrace{.timestampNs = 4, .length = 4, .uid = 456, .tcpFlags = 2}, }; + // Set common fields. + for (PacketTrace& pkt : input) { + pkt.ipProto = IPPROTO_TCP; + } + std::vector events; ASSERT_TRUE(TraceAndSortPackets(input, &events, config)); diff --git a/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h b/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h index 853e26e09e..6bf186aa63 100644 --- a/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h +++ b/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h @@ -32,7 +32,7 @@ namespace bpf { // BundleKey encodes a PacketTrace minus timestamp and length. The key should // match many packets over time for interning. For convenience, sport/dport -// are parsed here as either local/remote port. +// are parsed here as either local/remote port or icmp type/code. struct BundleKey { explicit BundleKey(const PacketTrace& pkt); @@ -47,6 +47,8 @@ struct BundleKey { std::optional tcpFlags; std::optional localPort; std::optional remotePort; + std::optional icmpType; + std::optional icmpCode; }; // BundleKeys are hashed using a simple hash combine.