patchpanel: add catch-all traffic accounting rule
This patch adds a catch-all accounting rule on all accounting chains
tx_<iface> and rx_<iface> for counting any traffic that has not been
correctly tagged. Such traffic is counted with source UNKNOWN.
This patch also updates the general description of CountersService in
counters_service.h.
BUG=b:171764279
TEST=Flashed rammus. Manually removed accounting rules for cronos and
other sources, checked that counters for UNKNOWN goes up, checked output
$ dbus-send --system --dest=org.chromium.PatchPanel --print-reply \
/org/chromium/PatchPanel org.chromium.PatchPanel.GetTrafficCounters
Change-Id: I745d4d479440ca96fb4bf81e33d809e6344ca509
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2581164
Tested-by: Hugo Benichi <hugobenichi@google.com>
Commit-Queue: Hugo Benichi <hugobenichi@google.com>
Reviewed-by: Jie Jiang <jiejiang@chromium.org>
diff --git a/patchpanel/counters_service.cc b/patchpanel/counters_service.cc
index 2b78b0a..6b7a131 100644
--- a/patchpanel/counters_service.cc
+++ b/patchpanel/counters_service.cc
@@ -34,11 +34,34 @@
// This regex extracts "tx" (direction), "eth0" (ifname) from this example.
constexpr LazyRE2 kChainLine = {R"(Chain (rx|tx)_(\w+).*)"};
-// The counter line looks like (some spaces are deleted to make it fit in one
-// line):
-// " 5374 876172 all -- any any anywhere anywhere mark match 0x2000/0x3f00"
-// The first two counters are captured for pkts and bytes.
+// The counter line for a defined source looks like (some spaces are deleted to
+// make it fit in one line):
+// " 5374 6172 RETURN all -- any any anywhere anywhere mark match 0x2000/0x3f00"
+// The final counter line for catching untagged traffic looks like:
+// " 5374 6172 all -- any any anywhere anywhere"
+// The first two counters are captured for pkts and bytes. For lines with a mark
+// matcher, the source is also captured.
constexpr LazyRE2 kCounterLine = {R"( *(\d+) +(\d+).*mark match (.*)/0x3f00)"};
+constexpr LazyRE2 kFinalCounterLine = {R"( *(\d+) +(\d+).*anywhere\s*)"};
+
+bool MatchCounterLine(const std::string& line,
+ uint64_t* pkts,
+ uint64_t* bytes,
+ TrafficSource* source) {
+ Fwmark mark;
+ if (RE2::FullMatch(line, *kCounterLine, pkts, bytes,
+ RE2::Hex(&mark.fwmark))) {
+ *source = mark.Source();
+ return true;
+ }
+
+ if (RE2::FullMatch(line, *kFinalCounterLine, pkts, bytes)) {
+ *source = TrafficSource::UNKNOWN;
+ return true;
+ }
+
+ return false;
+}
// Parses the output of `iptables -L -x -v` (or `ip6tables`) and adds the parsed
// values into the corresponding counters in |counters|. An example of |output|
@@ -76,32 +99,40 @@
// Skips the chain name line and the header line.
if (lines.cend() - it <= 2) {
- LOG(ERROR) << "Invalid iptables output";
+ LOG(ERROR) << "Invalid iptables output for " << direction << "_"
+ << ifname;
return false;
}
it += 2;
+ // Checks that there are some counter rules defined.
+ if (it == lines.cend() || it->empty()) {
+ LOG(ERROR) << "No counter rule defined for " << direction << "_"
+ << ifname;
+ return false;
+ }
+
// The next block of lines are the counters lines for individual sources.
for (; it != lines.cend() && !it->empty(); it++) {
uint64_t pkts, bytes;
- Fwmark mark;
- if (!RE2::FullMatch(*it, *kCounterLine, &pkts, &bytes,
- RE2::Hex(&mark.fwmark))) {
- LOG(ERROR) << "Cannot parse counter line \"" << *it << "\"";
+ TrafficSource source;
+ if (!MatchCounterLine(*it, &pkts, &bytes, &source)) {
+ LOG(ERROR) << "Cannot parse counter line \"" << *it << "\" for "
+ << direction << "_" << ifname;
return false;
}
if (pkts == 0 && bytes == 0)
continue;
- TrafficCounter::Source source = TrafficSourceToProto(mark.Source());
- auto& counter = (*counters)[std::make_pair(source, ifname)];
+ auto& counter =
+ (*counters)[std::make_pair(TrafficSourceToProto(source), ifname)];
if (direction == "rx") {
- counter.rx_packets += pkts;
counter.rx_bytes += bytes;
+ counter.rx_packets += pkts;
} else {
- counter.tx_packets += pkts;
counter.tx_bytes += bytes;
+ counter.tx_packets += pkts;
}
}
}
@@ -221,8 +252,11 @@
AddAccountingRule(ingress_chain, source);
AddAccountingRule(egress_chain, source);
}
- // TODO(b/160112868): add default rules for counting any traffic left as
- // UNKNOWN.
+ // Add catch-all accounting rule for any remaining and untagged traffic.
+ datapath_->ModifyIptables(IpFamily::Dual, kMangleTable,
+ {"-A", ingress_chain, "-w"});
+ datapath_->ModifyIptables(IpFamily::Dual, kMangleTable,
+ {"-A", egress_chain, "-w"});
return true;
}
diff --git a/patchpanel/counters_service.h b/patchpanel/counters_service.h
index 3e3460d..cd66647 100644
--- a/patchpanel/counters_service.h
+++ b/patchpanel/counters_service.h
@@ -33,20 +33,17 @@
// and will not change the fate of a packet. When a new interface comes up, we
// will create the following new rules/chains (using both iptables and
// ip6tables):
-// - Four accounting chains:
-// - For rx packets, `ingress_input_{ifname}` and `ingress_forward_{ifname}`
-// for INPUT and FORWARD chain, respectively;
-// - For tx packets, `egress_postrouting_{ifname}` and
-// `egress_forward_{ifname}` for POSTROUTING and FORWARD chain,
-// respectively. Note that we use `--socket-exists` in POSTROUTING chain to
-// avoid packets from FORWARD being matched again here.
-// - One accounting rule in each accounting chain, which provides the actual
-// counter for accounting. We will extend this to several rules when source
-// marking is ready.
-// - One jumping rule for each accounting chain in the corresponding prebuilt
+// - Two accounting chains:
+// - For rx packets, `rx_{ifname}` for INPUT and FORWARD chains;
+// - For tx packets, `tx_{ifname}` for POSTROUTING chain.
+// - One accounting rule in each accounting chain for every source defined in
+// RoutingService plus one final accounting rule for untagged traffic.
+// - Jumping rules for each accounting chain in the corresponding prebuilt
// chain, which matches packets with this new interface.
-// The above rules and chains will never be removed once created, so we will
-// check if one rule exists before creating it.
+// The above accounting rules and chains will never be removed once created, so
+// we will check if one rule exists before creating it. Jumping rules are added
+// and removed dynamically based on physical device and vpn device creation and
+// removal events.
//
// Query: Two commands (iptables and ip6tables) will be executed in the mangle
// table to get all the chains and rules. And then we perform a text parsing on
diff --git a/patchpanel/counters_service_test.cc b/patchpanel/counters_service_test.cc
index 313620a..8ece81e 100644
--- a/patchpanel/counters_service_test.cc
+++ b/patchpanel/counters_service_test.cc
@@ -92,6 +92,7 @@
0 0 RETURN all -- any any anywhere anywhere mark match 0x2200/0x3f00
0 0 RETURN all -- any any anywhere anywhere mark match 0x2300/0x3f00
0 0 RETURN all -- any any anywhere anywhere mark match 0x2400/0x3f00
+ 4 123 all -- any any anywhere anywhere
Chain tx_wlan0 (1 references)
pkts bytes target prot opt in out source destination
@@ -105,6 +106,7 @@
0 0 RETURN all -- any any anywhere anywhere mark match 0x2200/0x3f00
0 0 RETURN all -- any any anywhere anywhere mark match 0x2300/0x3f00
0 0 RETURN all -- any any anywhere anywhere mark match 0x2400/0x3f00
+ 0 0 all -- any any anywhere anywhere
Chain rx_eth0 (2 references)
pkts bytes target prot opt in out source destination
@@ -118,6 +120,7 @@
0 0 RETURN all -- any any anywhere anywhere mark match 0x2200/0x3f00
0 0 RETURN all -- any any anywhere anywhere mark match 0x2300/0x3f00
0 0 RETURN all -- any any anywhere anywhere mark match 0x2400/0x3f00
+ 6 345 all -- any any anywhere anywhere
Chain rx_wlan0 (2 references)
pkts bytes target prot opt in out source destination
@@ -131,6 +134,7 @@
0 0 RETURN all -- any any anywhere anywhere mark match 0x2200/0x3f00
0 0 RETURN all -- any any anywhere anywhere mark match 0x2300/0x3f00
0 0 RETURN all -- any any anywhere anywhere mark match 0x2400/0x3f00
+ 0 0 all -- any any anywhere anywhere
)";
class MockProcessRunner : public MinijailedProcessRunner {
@@ -162,27 +166,14 @@
std::make_unique<CountersService>(datapath_.get(), &runner_);
}
- // Makes `iptables` returning a bad |output|. Expects an empty map from
- // GetCounters().
- void TestBadIptablesOutput(const std::string& output) {
+ // Makes `iptables` and `ip6tables` returning |ipv4_output| and
+ // |ipv6_output|, respectively. Expects an empty map from GetCounters().
+ void TestBadIptablesOutput(const std::string& ipv4_output,
+ const std::string& ipv6_output) {
EXPECT_CALL(runner_, iptables(_, _, _, _))
- .WillRepeatedly(DoAll(SetArgPointee<3>(output), Return(0)));
+ .WillRepeatedly(DoAll(SetArgPointee<3>(ipv4_output), Return(0)));
EXPECT_CALL(runner_, ip6tables(_, _, _, _))
- .WillRepeatedly(DoAll(SetArgPointee<3>(kIptablesOutput), Return(0)));
-
- auto actual = counters_svc_->GetCounters({});
- std::map<SourceDevice, Counter> expected;
-
- EXPECT_THAT(actual, ContainerEq(expected));
- }
-
- // Makes `ip6tables` returning a bad |output|. Expects an empty map from
- // GetCounters().
- void TestBadIp6tablesOutput(const std::string& output) {
- EXPECT_CALL(runner_, iptables(_, _, _, _))
- .WillRepeatedly(DoAll(SetArgPointee<3>(kIptablesOutput), Return(0)));
- EXPECT_CALL(runner_, ip6tables(_, _, _, _))
- .WillRepeatedly(DoAll(SetArgPointee<3>(output), Return(0)));
+ .WillRepeatedly(DoAll(SetArgPointee<3>(ipv6_output), Return(0)));
auto actual = counters_svc_->GetCounters({});
std::map<SourceDevice, Counter> expected;
@@ -241,6 +232,8 @@
"RETURN", "-w"},
{"-A", "rx_vpn", "-m", "mark", "--mark", "0x00002400/0x00003f00", "-j",
"RETURN", "-w"},
+ {"-A", "tx_vpn", "-w"},
+ {"-A", "rx_vpn", "-w"},
};
for (const auto& rule : expected_calls) {
@@ -296,6 +289,8 @@
"RETURN", "-w"},
{"-A", "rx_vpn", "-m", "mark", "--mark", "0x00002400/0x00003f00", "-j",
"RETURN", "-w"},
+ {"-A", "tx_vpn", "-w"},
+ {"-A", "rx_vpn", "-w"},
{"-N", "rx_wlan0", "-w"},
{"-N", "tx_wlan0", "-w"},
{"-A", "INPUT", "-i", "wlan0", "-j", "rx_wlan0", "-w"},
@@ -341,6 +336,8 @@
"RETURN", "-w"},
{"-A", "rx_wlan0", "-m", "mark", "--mark", "0x00002400/0x00003f00", "-j",
"RETURN", "-w"},
+ {"-A", "tx_wlan0", "-w"},
+ {"-A", "rx_wlan0", "-w"},
};
for (const auto& rule : expected_calls) {
@@ -399,6 +396,8 @@
"RETURN", "-w"},
{"-A", "rx_eth0", "-m", "mark", "--mark", "0x00002400/0x00003f00", "-j",
"RETURN", "-w"},
+ {"-A", "tx_eth0", "-w"},
+ {"-A", "rx_eth0", "-w"},
};
for (const auto& rule : expected_calls) {
@@ -454,6 +453,8 @@
"RETURN", "-w"},
{"-A", "rx_vpn", "-m", "mark", "--mark", "0x00002400/0x00003f00", "-j",
"RETURN", "-w"},
+ {"-A", "tx_vpn", "-w"},
+ {"-A", "rx_vpn", "-w"},
{"-A", "FORWARD", "-i", "tun0", "-j", "rx_vpn", "-w"},
{"-A", "INPUT", "-i", "tun0", "-j", "rx_vpn", "-w"},
{"-A", "POSTROUTING", "-o", "tun0", "-j", "tx_vpn", "-w"},
@@ -528,12 +529,16 @@
{{TrafficCounter::CROSVM, "eth0"},
{0 /*rx_bytes*/, 0 /*rx_packets*/, 5380 /*tx_bytes*/,
78 /*tx_packets*/}},
+ {{TrafficCounter::UNKNOWN, "eth0"},
+ {690 /*rx_bytes*/, 12 /*rx_packets*/, 246 /*tx_bytes*/,
+ 8 /*tx_packets*/}},
{{TrafficCounter::CHROME, "wlan0"},
{56196 /*rx_bytes*/, 306 /*rx_packets*/, 114008 /*tx_bytes*/,
620 /*tx_packets*/}},
{{TrafficCounter::SYSTEM, "wlan0"},
{1680 /*rx_bytes*/, 12 /*rx_packets*/, 5602 /*tx_bytes*/,
- 48 /*tx_packets*/}}};
+ 48 /*tx_packets*/}},
+ };
EXPECT_THAT(actual, ContainerEq(expected));
}
@@ -566,19 +571,43 @@
{{TrafficCounter::CROSVM, "eth0"},
{0 /*rx_bytes*/, 0 /*rx_packets*/, 5380 /*tx_bytes*/,
78 /*tx_packets*/}},
+ {{TrafficCounter::UNKNOWN, "eth0"},
+ {690 /*rx_bytes*/, 12 /*rx_packets*/, 246 /*tx_bytes*/,
+ 8 /*tx_packets*/}},
+ };
+
+ EXPECT_THAT(actual, ContainerEq(expected));
+}
+
+TEST_F(CountersServiceTest, QueryTraffic_UnknownTrafficOnly) {
+ const std::string unkwown_traffic_only = R"(
+Chain tx_eth0 (1 references)
+ pkts bytes target prot opt in out source destination
+ 6511 68041668 all -- any any anywhere anywhere
+)";
+
+ EXPECT_CALL(runner_, iptables(_, _, _, _))
+ .WillRepeatedly(DoAll(SetArgPointee<3>(unkwown_traffic_only), Return(0)));
+ EXPECT_CALL(runner_, ip6tables(_, _, _, _))
+ .WillRepeatedly(DoAll(SetArgPointee<3>(unkwown_traffic_only), Return(0)));
+
+ auto actual = counters_svc_->GetCounters({});
+
+ std::map<SourceDevice, Counter> expected{
+ {{TrafficCounter::UNKNOWN, "eth0"},
+ {0 /*rx_bytes*/, 0 /*rx_packets*/, 136083336 /*tx_bytes*/,
+ 13022 /*tx_packets*/}},
};
EXPECT_THAT(actual, ContainerEq(expected));
}
TEST_F(CountersServiceTest, QueryTrafficCountersWithEmptyIPv4Output) {
- const std::string kEmptyOutput = "";
- TestBadIptablesOutput(kEmptyOutput);
+ TestBadIptablesOutput("", kIptablesOutput);
}
TEST_F(CountersServiceTest, QueryTrafficCountersWithEmptyIPv6Output) {
- const std::string kEmptyOutput = "";
- TestBadIp6tablesOutput(kEmptyOutput);
+ TestBadIptablesOutput(kIptablesOutput, "");
}
TEST_F(CountersServiceTest, QueryTrafficCountersWithOnlyChainName) {
@@ -589,7 +618,7 @@
Chain tx_wlan0 (1 references)
)";
- TestBadIptablesOutput(kBadOutput);
+ TestBadIptablesOutput(kBadOutput, kIptablesOutput);
}
TEST_F(CountersServiceTest, QueryTrafficCountersWithOnlyChainNameAndHeader) {
@@ -598,10 +627,10 @@
pkts bytes target prot opt in out source destination
6511 68041668 RETURN all -- any any anywhere anywhere
-Chain tx_fwd_wlan0 (1 references)
+Chain tx_wlan0 (1 references)
pkts bytes target prot opt in out source destination
)";
- TestBadIptablesOutput(kBadOutput);
+ TestBadIptablesOutput(kBadOutput, kIptablesOutput);
}
TEST_F(CountersServiceTest, QueryTrafficCountersWithNotFinishedCountersLine) {
@@ -613,7 +642,7 @@
Chain tx_wlan0 (1 references)
pkts bytes target prot opt in out source destination pkts bytes target prot opt in out source destination
0 )";
- TestBadIptablesOutput(kBadOutput);
+ TestBadIptablesOutput(kBadOutput, kIptablesOutput);
}
} // namespace