patchpanel: simplify traffic account jump rules
This patch simplifies the creation of traffic accounting jump rules by
adding a single check point for returning early if traffic account rules
for a given interface already exist.
BUG=b:161060333
BUG=b:160112868
TEST=Unit tests. Checked GetTrafficCounters still work with:
$ dbus-send --system --dest=org.chromium.PatchPanel --print-reply \
/org/chromium/PatchPanel org.chromium.PatchPanel.GetTrafficCounters
Checked that accounting chains and jump rules for eth0 are created only
once when plugging in and out an eth adapter multiple times.
Change-Id: I60afecf58466177686b815726b7b5f4ff4d7ce93
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2562980
Reviewed-by: Jie Jiang <jiejiang@chromium.org>
Commit-Queue: Hugo Benichi <hugobenichi@google.com>
Tested-by: Hugo Benichi <hugobenichi@google.com>
diff --git a/patchpanel/counters_service.cc b/patchpanel/counters_service.cc
index 8e30f86..7c631bb 100644
--- a/patchpanel/counters_service.cc
+++ b/patchpanel/counters_service.cc
@@ -170,22 +170,8 @@
}
void CountersService::IptablesNewRule(std::vector<std::string> params) {
- DCHECK_GT(params.size(), 0);
- const std::string action = params[0];
- DCHECK(action == "-I" || action == "-A");
- params.emplace_back("-w");
-
- params[0] = "-C";
- if (runner_->iptables(kMangleTable, params, false /*log_failures*/) != 0) {
- params[0] = action;
- runner_->iptables(kMangleTable, params);
- }
-
- params[0] = "-C";
- if (runner_->ip6tables(kMangleTable, params, false /*log_failures*/) != 0) {
- params[0] = action;
- runner_->ip6tables(kMangleTable, params);
- }
+ runner_->iptables(kMangleTable, params);
+ runner_->ip6tables(kMangleTable, params);
}
void CountersService::SetupChainsAndRules(const std::string& ifname) {
@@ -197,24 +183,35 @@
// Note that the length of a chain name must less than 29 chars and IFNAMSIZ
// is 16 so we can only use at most 12 chars for the prefix.
- // Ingress traffic chain.
const std::string ingress_chain = "rx_" + ifname;
- MakeAccountingChain(ingress_chain);
- IptablesNewRule({"-A", "FORWARD", "-i", ifname, "-j", ingress_chain});
- IptablesNewRule({"-A", "INPUT", "-i", ifname, "-j", ingress_chain});
- SetupAccountingRules(ingress_chain);
-
- // Egress traffic chain.
const std::string egress_chain = "tx_" + ifname;
- MakeAccountingChain(egress_chain);
- IptablesNewRule({"-A", "POSTROUTING", "-o", ifname, "-j", egress_chain});
+
+ // Creates egress and ingress traffic chains, or stops if they already exist.
+ if (!MakeAccountingChain(ingress_chain) ||
+ !MakeAccountingChain(egress_chain)) {
+ LOG(INFO) << "Traffic accounting chains already exist for " << ifname;
+ return;
+ }
+
+ // Add jump rules
+ datapath_->ModifyIptables(
+ IpFamily::Dual, kMangleTable,
+ {"-A", "FORWARD", "-i", ifname, "-j", ingress_chain, "-w"});
+ datapath_->ModifyIptables(
+ IpFamily::Dual, kMangleTable,
+ {"-A", "INPUT", "-i", ifname, "-j", ingress_chain, "-w"});
+ datapath_->ModifyIptables(
+ IpFamily::Dual, kMangleTable,
+ {"-A", "POSTROUTING", "-o", ifname, "-j", egress_chain, "-w"});
+
+ SetupAccountingRules(ingress_chain);
SetupAccountingRules(egress_chain);
}
void CountersService::SetupAccountingRules(const std::string& chain_name) {
// TODO(jiejiang): This function will be extended to matching on fwmark for
// different sources.
- IptablesNewRule({"-A", chain_name});
+ IptablesNewRule({"-A", chain_name, "-w"});
}
} // namespace patchpanel