patchpanel: move Datapath setup to Datapath
This patch moves all the initial setup for configuring the routing layer
from Manager to Datapath, making it all internal to Datapath. This
allows to refactor Datapath internals without impacting Manager and
allows to hide implementation details from the upper layer.
Incidentally, new unit tests are added for exercising the initial setup
and final tear down of the routing layer.
There is no functional change in this patch.
BUG=b:161507671
TEST=Unit tests.
Change-Id: Ia16648d70f15d741ae95d69becc6e41294e76abc
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2397097
Tested-by: Hugo Benichi <hugobenichi@google.com>
Commit-Queue: Hugo Benichi <hugobenichi@google.com>
Reviewed-by: Garrick Evans <garrick@chromium.org>
diff --git a/patchpanel/datapath.cc b/patchpanel/datapath.cc
index 6155749..eb2e4fa 100644
--- a/patchpanel/datapath.cc
+++ b/patchpanel/datapath.cc
@@ -38,6 +38,12 @@
constexpr char kLocalhostAddr[] = "127.0.0.1";
constexpr uint16_t kAdbServerPort = 5555;
+// Constants used for dropping locally originated traffic bound to an incorrect
+// source IPv4 address.
+constexpr char kGuestIPv4Subnet[] = "100.115.92.0/23";
+constexpr std::array<const char*, 6> kPhysicalIfnamePrefixes{
+ {"eth+", "wlan+", "mlan+", "usb+", "wwan+", "rmnet+"}};
+
std::string PrefixIfname(const std::string& prefix, const std::string& ifname) {
std::string n = prefix + ifname;
if (n.length() < IFNAMSIZ)
@@ -86,6 +92,71 @@
return *process_runner_;
}
+void Datapath::Start() {
+ // Enable IPv4 packet forwarding
+ if (process_runner_->sysctl_w("net.ipv4.ip_forward", "1") != 0)
+ LOG(ERROR) << "Failed to update net.ipv4.ip_forward."
+ << " Guest connectivity will not work correctly.";
+
+ // Limit local port range: Android owns 47104-61000.
+ // TODO(garrick): The original history behind this tweak is gone. Some
+ // investigation is needed to see if it is still applicable.
+ if (process_runner_->sysctl_w("net.ipv4.ip_local_port_range",
+ "32768 47103") != 0)
+ LOG(ERROR) << "Failed to limit local port range. Some Android features or"
+ << " apps may not work correctly.";
+
+ // Enable IPv6 packet forwarding
+ if (process_runner_->sysctl_w("net.ipv6.conf.all.forwarding", "1") != 0)
+ LOG(ERROR) << "Failed to update net.ipv6.conf.all.forwarding."
+ << " IPv6 functionality may be broken.";
+
+ if (!AddSNATMarkRules())
+ LOG(ERROR) << "Failed to install SNAT mark rules."
+ << " Guest connectivity may be broken.";
+
+ if (!AddForwardEstablishedRule())
+ LOG(ERROR) << "Failed to install forwarding rule for established"
+ << " connections.";
+
+ // chromium:898210: Drop any locally originated traffic that would exit a
+ // physical interface with a source IPv4 address from the subnet of IPs used
+ // for VMs, containers, and connected namespaces This is needed to prevent
+ // packets leaking with an incorrect src IP when a local process binds to the
+ // wrong interface.
+ for (const auto& oif : kPhysicalIfnamePrefixes) {
+ if (!AddSourceIPv4DropRule(oif, kGuestIPv4Subnet))
+ LOG(WARNING) << "Failed to set up IPv4 drop rule for src ip "
+ << kGuestIPv4Subnet << " exiting " << oif;
+ }
+
+ if (!AddOutboundIPv4SNATMark("vmtap+"))
+ LOG(ERROR) << "Failed to set up NAT for TAP devices."
+ << " Guest connectivity may be broken.";
+}
+
+void Datapath::Stop() {
+ RemoveOutboundIPv4SNATMark("vmtap+");
+ RemoveForwardEstablishedRule();
+ RemoveSNATMarkRules();
+ for (const auto& oif : kPhysicalIfnamePrefixes)
+ RemoveSourceIPv4DropRule(oif, kGuestIPv4Subnet);
+
+ // Restore original local port range.
+ // TODO(garrick): The original history behind this tweak is gone. Some
+ // investigation is needed to see if it is still applicable.
+ if (process_runner_->sysctl_w("net.ipv4.ip_local_port_range",
+ "32768 61000") != 0)
+ LOG(ERROR) << "Failed to restore local port range";
+
+ // Disable packet forwarding
+ if (process_runner_->sysctl_w("net.ipv6.conf.all.forwarding", "0") != 0)
+ LOG(ERROR) << "Failed to restore net.ipv6.conf.all.forwarding.";
+
+ if (process_runner_->sysctl_w("net.ipv4.ip_forward", "0") != 0)
+ LOG(ERROR) << "Failed to restore net.ipv4.ip_forward.";
+}
+
bool Datapath::NetnsAttachName(const std::string& netns_name, pid_t netns_pid) {
// Try first to delete any netns with name |netns_name| in case patchpanel
// did not exit cleanly.
@@ -585,8 +656,8 @@
const std::string& op,
const std::string& oif) {
if (oif.empty()) {
- LOG(ERROR) << "Cannot change POSTROUTING CONNMARK set-mark with no "
- "interface specified";
+ LOG(ERROR) << "Cannot change POSTROUTING CONNMARK set-mark with no"
+ " interface specified";
return false;
}
diff --git a/patchpanel/datapath.h b/patchpanel/datapath.h
index ea7ae28..5ec53a5 100644
--- a/patchpanel/datapath.h
+++ b/patchpanel/datapath.h
@@ -53,6 +53,12 @@
ioctl_t ioctl_hook);
virtual ~Datapath() = default;
+ // Start and stop the Datapath, creating or destroying the initial iptables
+ // setup needed for forwarding traffic from VMs and containers and for
+ // fwmark based routing.
+ virtual void Start();
+ virtual void Stop();
+
// Attaches the name |netns_name| to a network namespace identified by
// |netns_pid|. If |netns_name| had already been created, it will be deleted
// first.
diff --git a/patchpanel/datapath_fuzzer.cc b/patchpanel/datapath_fuzzer.cc
index cdbf9f2..bc2773c 100644
--- a/patchpanel/datapath_fuzzer.cc
+++ b/patchpanel/datapath_fuzzer.cc
@@ -75,12 +75,12 @@
std::vector<uint8_t> bytes = provider.ConsumeBytes<uint8_t>(mac.size());
std::copy(std::begin(bytes), std::begin(bytes), std::begin(mac));
+ datapath.Start();
+ datapath.Stop();
datapath.AddBridge(ifname, addr, prefix_len);
datapath.RemoveBridge(ifname);
- datapath.StartRoutingDevice(ifname, ifname2, addr,
- TrafficSource::UNKNOWN);
- datapath.StopRoutingDevice(ifname, ifname2, addr,
- TrafficSource::UNKNOWN);
+ datapath.StartRoutingDevice(ifname, ifname2, addr, TrafficSource::UNKNOWN);
+ datapath.StopRoutingDevice(ifname, ifname2, addr, TrafficSource::UNKNOWN);
datapath.AddVirtualInterfacePair(netns_name, ifname, bridge);
datapath.ToggleInterface(ifname, provider.ConsumeBool());
datapath.ConfigureInterface(ifname, mac, addr, prefix_len,
diff --git a/patchpanel/datapath_test.cc b/patchpanel/datapath_test.cc
index f69e7d6..2c17804 100644
--- a/patchpanel/datapath_test.cc
+++ b/patchpanel/datapath_test.cc
@@ -114,6 +114,154 @@
EXPECT_NE(IpFamily::IPv4, IpFamily::IPv6);
}
+TEST(DatapathTest, Start) {
+ MockProcessRunner runner;
+ MockFirewall firewall;
+ // Asserts for sysctl modifications
+ EXPECT_CALL(runner, sysctl_w(StrEq("net.ipv4.ip_forward"), StrEq("1"), true));
+ EXPECT_CALL(runner, sysctl_w(StrEq("net.ipv4.ip_local_port_range"),
+ StrEq("32768 47103"), true));
+ EXPECT_CALL(runner, sysctl_w(StrEq("net.ipv6.conf.all.forwarding"),
+ StrEq("1"), true));
+ // Asserts for AddSNATMarkRules
+ EXPECT_CALL(
+ runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-A", "FORWARD", "-m", "mark", "--mark", "1/1", "-m",
+ "state", "--state", "INVALID", "-j", "DROP", "-w"),
+ true, nullptr));
+ EXPECT_CALL(runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-A", "FORWARD", "-m", "mark", "--mark",
+ "1/1", "-j", "ACCEPT", "-w"),
+ true, nullptr));
+ EXPECT_CALL(runner,
+ iptables(StrEq("nat"),
+ ElementsAre("-A", "POSTROUTING", "-m", "mark", "--mark",
+ "1/1", "-j", "MASQUERADE", "-w"),
+ true, nullptr));
+ // Asserts for AddForwardEstablishedRule
+ EXPECT_CALL(runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-A", "FORWARD", "-m", "state", "--state",
+ "ESTABLISHED,RELATED", "-j", "ACCEPT", "-w"),
+ true, nullptr));
+ // Asserts for AddSourceIPv4DropRule() calls.
+ EXPECT_CALL(runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-I", "OUTPUT", "-o", "eth+", "-s",
+ "100.115.92.0/23", "-j", "DROP", "-w"),
+ true, nullptr));
+ EXPECT_CALL(runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-I", "OUTPUT", "-o", "wlan+", "-s",
+ "100.115.92.0/23", "-j", "DROP", "-w"),
+ true, nullptr));
+ EXPECT_CALL(runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-I", "OUTPUT", "-o", "mlan+", "-s",
+ "100.115.92.0/23", "-j", "DROP", "-w"),
+ true, nullptr));
+ EXPECT_CALL(runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-I", "OUTPUT", "-o", "usb+", "-s",
+ "100.115.92.0/23", "-j", "DROP", "-w"),
+ true, nullptr));
+ EXPECT_CALL(runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-I", "OUTPUT", "-o", "wwan+", "-s",
+ "100.115.92.0/23", "-j", "DROP", "-w"),
+ true, nullptr));
+ EXPECT_CALL(runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-I", "OUTPUT", "-o", "rmnet+", "-s",
+ "100.115.92.0/23", "-j", "DROP", "-w"),
+ true, nullptr));
+ // Asserts for AddOutboundIPv4SNATMark("vmtap+")
+ EXPECT_CALL(runner,
+ iptables(StrEq("mangle"),
+ ElementsAre("-A", "PREROUTING", "-i", "vmtap+", "-j",
+ "MARK", "--set-mark", "1/1", "-w"),
+ true, nullptr));
+
+ Datapath datapath(&runner, &firewall);
+ datapath.Start();
+}
+
+TEST(DatapathTest, Stop) {
+ MockProcessRunner runner;
+ MockFirewall firewall;
+ // Asserts for sysctl modifications
+ EXPECT_CALL(runner, sysctl_w(StrEq("net.ipv4.ip_local_port_range"),
+ StrEq("32768 61000"), true));
+ EXPECT_CALL(runner, sysctl_w(StrEq("net.ipv6.conf.all.forwarding"),
+ StrEq("0"), true));
+ EXPECT_CALL(runner, sysctl_w(StrEq("net.ipv4.ip_forward"), StrEq("0"), true));
+ // Asserts for RemoveOutboundIPv4SNATMark("vmtap+")
+ EXPECT_CALL(runner,
+ iptables(StrEq("mangle"),
+ ElementsAre("-D", "PREROUTING", "-i", "vmtap+", "-j",
+ "MARK", "--set-mark", "1/1", "-w"),
+ true, nullptr));
+ // Asserts for RemoveForwardEstablishedRule
+ EXPECT_CALL(runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-D", "FORWARD", "-m", "state", "--state",
+ "ESTABLISHED,RELATED", "-j", "ACCEPT", "-w"),
+ true, nullptr));
+ // Asserts for RemoveSNATMarkRules
+ EXPECT_CALL(
+ runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-D", "FORWARD", "-m", "mark", "--mark", "1/1", "-m",
+ "state", "--state", "INVALID", "-j", "DROP", "-w"),
+ true, nullptr));
+ EXPECT_CALL(runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-D", "FORWARD", "-m", "mark", "--mark",
+ "1/1", "-j", "ACCEPT", "-w"),
+ true, nullptr));
+ EXPECT_CALL(runner,
+ iptables(StrEq("nat"),
+ ElementsAre("-D", "POSTROUTING", "-m", "mark", "--mark",
+ "1/1", "-j", "MASQUERADE", "-w"),
+ true, nullptr));
+ // Asserts for RemoveSourceIPv4DropRule() calls.
+ EXPECT_CALL(runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-D", "OUTPUT", "-o", "eth+", "-s",
+ "100.115.92.0/23", "-j", "DROP", "-w"),
+ true, nullptr));
+ EXPECT_CALL(runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-D", "OUTPUT", "-o", "wlan+", "-s",
+ "100.115.92.0/23", "-j", "DROP", "-w"),
+ true, nullptr));
+ EXPECT_CALL(runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-D", "OUTPUT", "-o", "mlan+", "-s",
+ "100.115.92.0/23", "-j", "DROP", "-w"),
+ true, nullptr));
+ EXPECT_CALL(runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-D", "OUTPUT", "-o", "usb+", "-s",
+ "100.115.92.0/23", "-j", "DROP", "-w"),
+ true, nullptr));
+ EXPECT_CALL(runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-D", "OUTPUT", "-o", "wwan+", "-s",
+ "100.115.92.0/23", "-j", "DROP", "-w"),
+ true, nullptr));
+ EXPECT_CALL(runner,
+ iptables(StrEq("filter"),
+ ElementsAre("-D", "OUTPUT", "-o", "rmnet+", "-s",
+ "100.115.92.0/23", "-j", "DROP", "-w"),
+ true, nullptr));
+
+ Datapath datapath(&runner, &firewall);
+ datapath.Stop();
+}
+
TEST(DatapathTest, AddTAP) {
MockProcessRunner runner;
MockFirewall firewall;
diff --git a/patchpanel/manager.cc b/patchpanel/manager.cc
index 841526c..2f14964 100644
--- a/patchpanel/manager.cc
+++ b/patchpanel/manager.cc
@@ -36,12 +36,6 @@
namespace {
constexpr int kSubprocessRestartDelayMs = 900;
-// Constants used for dropping locally originated traffic bound to an incorrect
-// source IPv4 address.
-constexpr char kGuestIPv4Subnet[] = "100.115.92.0/23";
-constexpr std::array<const char*, 6> kPhysicalIfnamePrefixes{
- {"eth+", "wlan+", "mlan+", "usb+", "wwan+", "rmnet+"}};
-
// Time interval between epoll checks on file descriptors committed by callers
// of ConnectNamespace DBus API.
constexpr const base::TimeDelta kConnectNamespaceCheckInterval =
@@ -222,7 +216,14 @@
routing_svc_ = std::make_unique<RoutingService>();
- StartDatapath();
+ // b/162966185: Allow Jetstream to disable:
+ // - the IP forwarding setup used for hosting VMs and containers,
+ // - the iptables rules for fwmark based routing.
+ if (!USE_JETSTREAM_ROUTING) {
+ datapath_->Start();
+ shill_client_->RegisterDevicesChangedHandler(base::BindRepeating(
+ &Manager::OnDevicesChanged, weak_factory_.GetWeakPtr()));
+ }
nd_proxy_->RegisterNDProxyMessageHandler(
base::Bind(&Manager::OnNDProxyMessage, weak_factory_.GetWeakPtr()));
@@ -259,7 +260,8 @@
connected_namespaces_fdkeys.push_back(kv.first);
for (const int fdkey : connected_namespaces_fdkeys)
DisconnectNamespace(fdkey);
- StopDatapath();
+ if (!USE_JETSTREAM_ROUTING)
+ datapath_->Stop();
}
void Manager::OnSubprocessExited(pid_t pid, const siginfo_t&) {
@@ -297,90 +299,6 @@
}
}
-void Manager::StartDatapath() {
- // b/162966185: Allow Jetstream to disable:
- // - the IP forwarding setup used for hosting VMs and containers,
- // - the iptables rules for fwmark based routing.
- if (USE_JETSTREAM_ROUTING) {
- LOG(INFO) << "Skipping Datapath routing setup";
- return;
- }
-
- auto& runner = datapath_->runner();
-
- // Enable IPv4 packet forwarding
- if (runner.sysctl_w("net.ipv4.ip_forward", "1") != 0) {
- LOG(ERROR) << "Failed to update net.ipv4.ip_forward."
- << " Guest connectivity will not work correctly.";
- }
- // Limit local port range: Android owns 47104-61000.
- // TODO(garrick): The original history behind this tweak is gone. Some
- // investigation is needed to see if it is still applicable.
- if (runner.sysctl_w("net.ipv4.ip_local_port_range", "32768 47103") != 0) {
- LOG(ERROR) << "Failed to limit local port range. Some Android features or"
- << " apps may not work correctly.";
- }
- // Enable IPv6 packet forwarding
- if (runner.sysctl_w("net.ipv6.conf.all.forwarding", "1") != 0) {
- LOG(ERROR) << "Failed to update net.ipv6.conf.all.forwarding."
- << " IPv6 functionality may be broken.";
- }
-
- if (!datapath_->AddSNATMarkRules()) {
- LOG(ERROR) << "Failed to install SNAT mark rules."
- << " Guest connectivity may be broken.";
- }
- if (!datapath_->AddForwardEstablishedRule()) {
- LOG(ERROR) << "Failed to install forwarding rule for established"
- << " connections.";
- }
-
- // chromium:898210: Drop any locally originated traffic that would exit a
- // physical interface with a source IPv4 address from the subnet of IPs used
- // for VMs, containers, and connected namespaces This is needed to prevent
- // packets leaking with an incorrect src IP when a local process binds to the
- // wrong interface.
- for (const auto& oif : kPhysicalIfnamePrefixes) {
- if (!datapath_->AddSourceIPv4DropRule(oif, kGuestIPv4Subnet))
- LOG(WARNING) << "Failed to set up IPv4 drop rule for src ip "
- << kGuestIPv4Subnet << " exiting " << oif;
- }
-
- if (!datapath_->AddOutboundIPv4SNATMark("vmtap+")) {
- LOG(ERROR) << "Failed to set up NAT for TAP devices."
- << " Guest connectivity may be broken.";
- }
-
- shill_client_->RegisterDevicesChangedHandler(base::BindRepeating(
- &Manager::OnDevicesChanged, weak_factory_.GetWeakPtr()));
-}
-
-void Manager::StopDatapath() {
- if (USE_JETSTREAM_ROUTING)
- return;
-
- datapath_->RemoveOutboundIPv4SNATMark("vmtap+");
- datapath_->RemoveForwardEstablishedRule();
- datapath_->RemoveSNATMarkRules();
- for (const auto& oif : kPhysicalIfnamePrefixes)
- datapath_->RemoveSourceIPv4DropRule(oif, kGuestIPv4Subnet);
-
- auto& runner = datapath_->runner();
- // Restore original local port range.
- // TODO(garrick): The original history behind this tweak is gone. Some
- // investigation is needed to see if it is still applicable.
- if (runner.sysctl_w("net.ipv4.ip_local_port_range", "32768 61000") != 0) {
- LOG(ERROR) << "Failed to restore local port range";
- }
- // Disable packet forwarding
- if (runner.sysctl_w("net.ipv6.conf.all.forwarding", "0") != 0) {
- LOG(ERROR) << "Failed to restore net.ipv6.conf.all.forwarding.";
- }
- if (runner.sysctl_w("net.ipv4.ip_forward", "0") != 0) {
- LOG(ERROR) << "Failed to restore net.ipv4.ip_forward.";
- }
-}
-
void Manager::OnDevicesChanged(const std::set<std::string>& added,
const std::set<std::string>& removed) {
for (const std::string& ifname : removed)
diff --git a/patchpanel/manager.h b/patchpanel/manager.h
index 3f9d5e7..2901327 100644
--- a/patchpanel/manager.h
+++ b/patchpanel/manager.h
@@ -89,13 +89,6 @@
const std::set<std::string>& removed);
void InitialSetup();
- // Start and stop the Datapath, creating or destroying the initial iptables
- // setup needed for forwarding traffic from VMs and containers and for
- // fwmark based routing. These functions are NOPs if USE_JETSTREAM_ROUTING is
- // set.
- void StartDatapath();
- void StopDatapath();
-
bool StartArc(pid_t pid);
void StopArc();
bool StartArcVm(uint32_t cid);
diff --git a/patchpanel/mock_datapath.h b/patchpanel/mock_datapath.h
index f867bdc..4a76c3d 100644
--- a/patchpanel/mock_datapath.h
+++ b/patchpanel/mock_datapath.h
@@ -22,6 +22,8 @@
: Datapath(runner, firewall) {}
~MockDatapath() = default;
+ MOCK_METHOD0(Start, void());
+ MOCK_METHOD0(Stop, void());
MOCK_METHOD2(NetnsAttachName,
bool(const std::string& netns_name, pid_t netns_pid));
MOCK_METHOD1(NetnsDeleteName, bool(const std::string& netns_name));
