patchpanel: create veth pairs directly across 2 netns
This patch changes how veth pairs are created by patchpanel for ARC++
and for patchpanel ConnectNamespace DBus API. Instead of creating the
veth pair in the target network namespace and briging back one half to
the host network namespace, the pair is directly created across the host
network namespace and target network namespace. This requires naming the
network namespace with $ ip netns attach NAME PID. Therefore ArcService
will now systematically attach the name "arc_netns" to the ARC++
network namespace at ARC++ startup.
The advantages of creating the veth pair directly across 2 namespaces
are:
- does not require to call RestoreDefaultNamespace (equivalent to
the command $ nsenter -i <pid> -n -- ip link set arc_ns<id> netns 1)
which was found to be racy in crbug/1095170).
- does not require to enter the ARC network namespace for ARC interface
creation. Eventually when ARC will obtain the full interface
configuration from patchpanel, pending migrating net.mojom from Chrome
to patchpanel, it will not be necessary to enter the ARC container at
all from patchpanel.
BUG=b:160736881
BUG=crbug:1095170
Test: Unit tests pass. Manual tests are WIP
Change-Id: Ic31c7dfbb1eb88b55314e44733ff55ba821b41a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2289719
Tested-by: Hugo Benichi <hugobenichi@google.com>
Tested-by: Andreea-Elena Costinas <acostinas@google.com>
Reviewed-by: Andreea-Elena Costinas <acostinas@google.com>
Reviewed-by: Garrick Evans <garrick@chromium.org>
Commit-Queue: Hugo Benichi <hugobenichi@google.com>
diff --git a/patchpanel/arc_service.cc b/patchpanel/arc_service.cc
index 06dd369..52d0482 100644
--- a/patchpanel/arc_service.cc
+++ b/patchpanel/arc_service.cc
@@ -34,6 +34,7 @@
namespace {
constexpr pid_t kInvalidPID = 0;
constexpr uint32_t kInvalidCID = 0;
+constexpr char kArcNetnsName[] = "arc_netns";
constexpr char kArcIfname[] = "arc0";
constexpr char kArcBridge[] = "arcbr0";
constexpr std::array<const char*, 2> kEthernetInterfacePrefixes{{"eth", "usb"}};
@@ -326,6 +327,11 @@
impl_ = std::make_unique<VmImpl>(datapath_, forwarder_, configs);
} else {
impl_ = std::make_unique<ContainerImpl>(datapath_, forwarder_);
+ if (!datapath_->NetnsAttachName(kArcNetnsName, id)) {
+ LOG(ERROR) << "Failed to attach name " << kArcNetnsName << " to pid "
+ << id;
+ return false;
+ }
}
if (!impl_->Start(id)) {
impl_.reset();
@@ -356,11 +362,11 @@
// The tap device associated with arc_device is created by VmImpl::Start().
} else {
arc_device_ifname = ArcVethHostName(arc_device_->guest_ifname());
- if (!datapath_->ConnectVethPair(impl_->id(), arc_device_ifname,
- arc_device_->guest_ifname(),
- arc_device_->config().mac_addr(),
- arc_device_->config().guest_ipv4_addr(), 30,
- arc_device_->options().fwd_multicast)) {
+ if (!datapath_->ConnectVethPair(
+ impl_->id(), kArcNetnsName, arc_device_ifname,
+ arc_device_->guest_ifname(), arc_device_->config().mac_addr(),
+ arc_device_->config().guest_ipv4_addr(), 30,
+ arc_device_->options().fwd_multicast)) {
LOG(ERROR) << "Cannot create virtual link for device "
<< arc_device_->phys_ifname();
return false;
@@ -372,7 +378,7 @@
<< kArcBridge;
return false;
}
- LOG(INFO) << "Started ARC management device " << arc_device_.get();
+ LOG(INFO) << "Started ARC management device " << *arc_device_.get();
// Start already known Shill <-> ARC mapped devices.
for (const auto& d : devices_)
@@ -394,15 +400,18 @@
LOG(ERROR) << "Failed to bring down arc bridge "
<< "- it may not restart correctly";
- if (guest_ == GuestMessage::ARC)
+ if (guest_ == GuestMessage::ARC) {
datapath_->RemoveInterface(ArcVethHostName(arc_device_->phys_ifname()));
+ if (!datapath_->NetnsDeleteName(kArcNetnsName))
+ LOG(WARNING) << "Failed to delete netns name " << kArcNetnsName;
+ }
if (impl_) {
impl_->Stop(id);
impl_.reset();
}
- LOG(INFO) << "Stopped ARC management device " << arc_device_.get();
+ LOG(INFO) << "Stopped ARC management device " << *arc_device_.get();
arc_device_.reset();
}
@@ -558,9 +567,10 @@
// Set up the virtual pair inside the container namespace.
const std::string veth_ifname = ArcVethHostName(device->guest_ifname());
const auto& config = device->config();
- if (!datapath_->ConnectVethPair(pid_, veth_ifname, device->guest_ifname(),
- config.mac_addr(), config.guest_ipv4_addr(),
- 30, device->options().fwd_multicast)) {
+ if (!datapath_->ConnectVethPair(pid_, kArcNetnsName, veth_ifname,
+ device->guest_ifname(), config.mac_addr(),
+ config.guest_ipv4_addr(), 30,
+ device->options().fwd_multicast)) {
LOG(ERROR) << "Cannot create virtual link for device "
<< device->phys_ifname();
return false;
diff --git a/patchpanel/arc_service_test.cc b/patchpanel/arc_service_test.cc
index d064eb7..01aad32 100644
--- a/patchpanel/arc_service_test.cc
+++ b/patchpanel/arc_service_test.cc
@@ -113,6 +113,8 @@
}
TEST_F(ArcServiceTest, VerifyAddrConfigs) {
+ EXPECT_CALL(*datapath_, NetnsAttachName(StrEq("arc_netns"), kTestPID))
+ .WillOnce(Return(true));
EXPECT_CALL(*datapath_, AddBridge(StrEq("arcbr0"), kArcHostIP, 30))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_, AddBridge(StrEq("arc_eth0"), kFirstEthHostIP, 30))
@@ -138,6 +140,8 @@
}
TEST_F(ArcServiceTest, VerifyAddrOrder) {
+ EXPECT_CALL(*datapath_, NetnsAttachName(StrEq("arc_netns"), kTestPID))
+ .WillOnce(Return(true));
EXPECT_CALL(*datapath_, AddBridge(StrEq("arcbr0"), kArcHostIP, 30))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_, AddBridge(StrEq("arc_eth0"), kFirstEthHostIP, 30))
@@ -177,11 +181,14 @@
// ContainerImpl
TEST_F(ArcServiceTest, ContainerImpl_Start) {
+ EXPECT_CALL(*datapath_, NetnsAttachName(StrEq("arc_netns"), kTestPID))
+ .WillOnce(Return(true));
// Expectations for arc0 setup.
EXPECT_CALL(*datapath_, AddBridge(StrEq("arcbr0"), kArcHostIP, 30))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_,
- AddVirtualInterfacePair(StrEq("vetharc0"), StrEq("arc0")))
+ AddVirtualInterfacePair(StrEq("arc_netns"), StrEq("vetharc0"),
+ StrEq("arc0")))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_,
ConfigureInterface(StrEq("arc0"), _, kArcGuestIP, 30, true, _))
@@ -197,10 +204,13 @@
}
TEST_F(ArcServiceTest, ContainerImpl_FailsToCreateInterface) {
+ EXPECT_CALL(*datapath_, NetnsAttachName(StrEq("arc_netns"), kTestPID))
+ .WillOnce(Return(true));
EXPECT_CALL(*datapath_, AddBridge(StrEq("arcbr0"), kArcHostIP, 30))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_,
- AddVirtualInterfacePair(StrEq("vetharc0"), StrEq("arc0")))
+ AddVirtualInterfacePair(StrEq("arc_netns"), StrEq("vetharc0"),
+ StrEq("arc0")))
.WillOnce(Return(false));
EXPECT_CALL(*datapath_, ConfigureInterface(_, _, _, _, _, _)).Times(0);
EXPECT_CALL(*datapath_, RemoveBridge(_)).Times(0);
@@ -210,10 +220,13 @@
}
TEST_F(ArcServiceTest, ContainerImpl_FailsToConfigureInterface) {
+ EXPECT_CALL(*datapath_, NetnsAttachName(StrEq("arc_netns"), kTestPID))
+ .WillOnce(Return(true));
EXPECT_CALL(*datapath_, AddBridge(StrEq("arcbr0"), kArcHostIP, 30))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_,
- AddVirtualInterfacePair(StrEq("vetharc0"), StrEq("arc0")))
+ AddVirtualInterfacePair(StrEq("arc_netns"), StrEq("vetharc0"),
+ StrEq("arc0")))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_,
ConfigureInterface(StrEq("arc0"), _, kArcGuestIP, 30, true, _))
@@ -228,10 +241,13 @@
}
TEST_F(ArcServiceTest, ContainerImpl_FailsToAddInterfaceToBridge) {
+ EXPECT_CALL(*datapath_, NetnsAttachName(StrEq("arc_netns"), kTestPID))
+ .WillOnce(Return(true));
EXPECT_CALL(*datapath_, AddBridge(StrEq("arcbr0"), kArcHostIP, 30))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_,
- AddVirtualInterfacePair(StrEq("vetharc0"), StrEq("arc0")))
+ AddVirtualInterfacePair(StrEq("arc_netns"), StrEq("vetharc0"),
+ StrEq("arc0")))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_,
ConfigureInterface(StrEq("arc0"), _, kArcGuestIP, 30, true, _))
@@ -249,11 +265,14 @@
}
TEST_F(ArcServiceTest, ContainerImpl_OnStartDevice) {
+ EXPECT_CALL(*datapath_, NetnsAttachName(StrEq("arc_netns"), kTestPID))
+ .WillOnce(Return(true));
// Expectations for arc0 setup.
EXPECT_CALL(*datapath_, AddBridge(StrEq("arcbr0"), kArcHostIP, 30))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_,
- AddVirtualInterfacePair(StrEq("vetharc0"), StrEq("arc0")))
+ AddVirtualInterfacePair(StrEq("arc_netns"), StrEq("vetharc0"),
+ StrEq("arc0")))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_,
ConfigureInterface(StrEq("arc0"), _, kArcGuestIP, 30, true, _))
@@ -266,7 +285,8 @@
EXPECT_CALL(*datapath_, AddBridge(StrEq("arc_eth0"), kFirstEthHostIP, 30))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_,
- AddVirtualInterfacePair(StrEq("vetheth0"), StrEq("eth0")))
+ AddVirtualInterfacePair(StrEq("arc_netns"), StrEq("vetheth0"),
+ StrEq("eth0")))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_, ConfigureInterface(StrEq("eth0"), _, kFirstEthGuestIP,
30, true, _))
@@ -290,11 +310,16 @@
}
TEST_F(ArcServiceTest, ContainerImpl_Stop) {
+ EXPECT_CALL(*datapath_, NetnsAttachName(StrEq("arc_netns"), kTestPID))
+ .WillOnce(Return(true));
+ EXPECT_CALL(*datapath_, NetnsDeleteName(StrEq("arc_netns")))
+ .WillOnce(Return(true));
// Expectations for arc0 setup.
EXPECT_CALL(*datapath_, AddBridge(StrEq("arcbr0"), kArcHostIP, 30))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_,
- AddVirtualInterfacePair(StrEq("vetharc0"), StrEq("arc0")))
+ AddVirtualInterfacePair(StrEq("arc_netns"), StrEq("vetharc0"),
+ StrEq("arc0")))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_,
ConfigureInterface(StrEq("arc0"), _, kArcGuestIP, 30, true, _))
@@ -316,11 +341,14 @@
}
TEST_F(ArcServiceTest, ContainerImpl_OnStopDevice) {
+ EXPECT_CALL(*datapath_, NetnsAttachName(StrEq("arc_netns"), kTestPID))
+ .WillOnce(Return(true));
// Expectations for arc0 setup.
EXPECT_CALL(*datapath_, AddBridge(StrEq("arcbr0"), kArcHostIP, 30))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_,
- AddVirtualInterfacePair(StrEq("vetharc0"), StrEq("arc0")))
+ AddVirtualInterfacePair(StrEq("arc_netns"), StrEq("vetharc0"),
+ StrEq("arc0")))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_,
ConfigureInterface(StrEq("arc0"), _, kArcGuestIP, 30, true, _))
@@ -333,7 +361,8 @@
EXPECT_CALL(*datapath_, AddBridge(StrEq("arc_eth0"), kFirstEthHostIP, 30))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_,
- AddVirtualInterfacePair(StrEq("vetheth0"), StrEq("eth0")))
+ AddVirtualInterfacePair(StrEq("arc_netns"), StrEq("vetheth0"),
+ StrEq("eth0")))
.WillOnce(Return(true));
EXPECT_CALL(*datapath_, ConfigureInterface(StrEq("eth0"), _, kFirstEthGuestIP,
30, true, _))
diff --git a/patchpanel/datapath.cc b/patchpanel/datapath.cc
index c2b02ed..d8e1580 100644
--- a/patchpanel/datapath.cc
+++ b/patchpanel/datapath.cc
@@ -67,6 +67,18 @@
return *process_runner_;
}
+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.
+ if (process_runner_->ip_netns_delete(netns_name, false /*log_failures*/) == 0)
+ LOG(INFO) << "Deleted left over network namespace name " << netns_name;
+ return process_runner_->ip_netns_attach(netns_name, netns_pid) == 0;
+}
+
+bool Datapath::NetnsDeleteName(const std::string& netns_name) {
+ return process_runner_->ip_netns_delete(netns_name) == 0;
+}
+
bool Datapath::AddBridge(const std::string& ifname,
uint32_t ipv4_addr,
uint32_t ipv4_prefix_len) {
@@ -218,28 +230,30 @@
process_runner_->ip("tuntap", "del", {ifname, "mode", "tap"});
}
-bool Datapath::ConnectVethPair(pid_t pid,
+bool Datapath::ConnectVethPair(pid_t netns_pid,
+ const std::string& netns_name,
const std::string& veth_ifname,
const std::string& peer_ifname,
const MacAddress& remote_mac_addr,
uint32_t remote_ipv4_addr,
uint32_t remote_ipv4_prefix_len,
bool remote_multicast_flag) {
- // Set up the virtual pair inside the remote namespace.
+ // Set up the virtual pair across the current namespace and |netns_name|.
+ if (!AddVirtualInterfacePair(netns_name, veth_ifname, peer_ifname)) {
+ LOG(ERROR) << "Failed to create veth pair " << veth_ifname << ","
+ << peer_ifname;
+ return false;
+ }
+
+ // Configure the remote veth in namespace |netns_name|.
{
- ScopedNS ns(pid);
- if (!ns.IsValid() && pid != kTestPID) {
+ ScopedNS ns(netns_pid);
+ if (!ns.IsValid() && netns_pid != kTestPID) {
LOG(ERROR)
<< "Cannot create virtual link -- invalid container namespace?";
return false;
}
- if (!AddVirtualInterfacePair(veth_ifname, peer_ifname)) {
- LOG(ERROR) << "Failed to create veth pair " << veth_ifname << ","
- << peer_ifname;
- return false;
- }
-
if (!ConfigureInterface(peer_ifname, remote_mac_addr, remote_ipv4_addr,
remote_ipv4_prefix_len, true /* link up */,
remote_multicast_flag)) {
@@ -249,32 +263,21 @@
}
}
- // Now pull the local end out into the local namespace.
- if (runner().RestoreDefaultNamespace(veth_ifname, pid) != 0) {
- LOG(ERROR) << "Failed to prepare interface " << veth_ifname;
- {
- ScopedNS ns(pid);
- if (ns.IsValid()) {
- RemoveInterface(peer_ifname);
- } else {
- LOG(ERROR) << "Failed to re-enter container namespace pid " << pid;
- }
- }
- return false;
- }
if (!ToggleInterface(veth_ifname, true /*up*/)) {
LOG(ERROR) << "Failed to bring up interface " << veth_ifname;
RemoveInterface(veth_ifname);
return false;
}
+
return true;
}
-bool Datapath::AddVirtualInterfacePair(const std::string& veth_ifname,
+bool Datapath::AddVirtualInterfacePair(const std::string& netns_name,
+ const std::string& veth_ifname,
const std::string& peer_ifname) {
- return process_runner_->ip(
- "link", "add",
- {veth_ifname, "type", "veth", "peer", "name", peer_ifname}) == 0;
+ return process_runner_->ip("link", "add",
+ {veth_ifname, "type", "veth", "peer", "name",
+ peer_ifname, "netns", netns_name}) == 0;
}
bool Datapath::ToggleInterface(const std::string& ifname, bool up) {
diff --git a/patchpanel/datapath.h b/patchpanel/datapath.h
index dcdcf04..304d1ec 100644
--- a/patchpanel/datapath.h
+++ b/patchpanel/datapath.h
@@ -6,6 +6,7 @@
#define PATCHPANEL_DATAPATH_H_
#include <net/route.h>
+#include <sys/types.h>
#include <string>
@@ -40,6 +41,14 @@
Datapath(MinijailedProcessRunner* process_runner, ioctl_t ioctl_hook);
virtual ~Datapath() = default;
+ // 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.
+ virtual bool NetnsAttachName(const std::string& netns_name, pid_t netns_pid);
+
+ // Deletes the name |netns_name| of a network namespace.
+ virtual bool NetnsDeleteName(const std::string& netns_name);
+
virtual bool AddBridge(const std::string& ifname,
uint32_t ipv4_addr,
uint32_t ipv4_prefix_len);
@@ -72,6 +81,7 @@
// namespace corresponding to |pid|, and set up the remote interface
// |peer_ifname| according // to the given parameters.
virtual bool ConnectVethPair(pid_t pid,
+ const std::string& netns_name,
const std::string& veth_ifname,
const std::string& peer_ifname,
const MacAddress& remote_mac_addr,
@@ -80,7 +90,8 @@
bool remote_multicast_flag);
// Creates a virtual interface pair.
- virtual bool AddVirtualInterfacePair(const std::string& veth_ifname,
+ virtual bool AddVirtualInterfacePair(const std::string& netns_ifname,
+ const std::string& veth_ifname,
const std::string& peer_ifname);
// Sets the link status.
diff --git a/patchpanel/datapath_test.cc b/patchpanel/datapath_test.cc
index b512011..c80950a 100644
--- a/patchpanel/datapath_test.cc
+++ b/patchpanel/datapath_test.cc
@@ -60,8 +60,6 @@
MockProcessRunner() = default;
~MockProcessRunner() = default;
- MOCK_METHOD2(RestoreDefaultNamespace,
- int(const std::string& ifname, pid_t pid));
MOCK_METHOD1(WriteSentinelToContainer, int(pid_t pid));
MOCK_METHOD3(brctl,
int(const std::string& cmd,
@@ -98,6 +96,12 @@
int(const std::string& key,
const std::string& value,
bool log_failures));
+ MOCK_METHOD3(ip_netns_attach,
+ int(const std::string& netns_name,
+ pid_t netns_pid,
+ bool log_failures));
+ MOCK_METHOD2(ip_netns_delete,
+ int(const std::string& netns_name, bool log_failures));
};
TEST(DatapathTest, AddTAP) {
@@ -149,6 +153,21 @@
datapath.RemoveTAP("foo0");
}
+TEST(DatapathTest, NetnsAttachName) {
+ MockProcessRunner runner;
+ EXPECT_CALL(runner, ip_netns_delete(StrEq("netns_foo"), false));
+ EXPECT_CALL(runner, ip_netns_attach(StrEq("netns_foo"), 1234, true));
+ Datapath datapath(&runner);
+ EXPECT_TRUE(datapath.NetnsAttachName("netns_foo", 1234));
+}
+
+TEST(DatapathTest, NetnsDeleteName) {
+ MockProcessRunner runner;
+ EXPECT_CALL(runner, ip_netns_delete(StrEq("netns_foo"), true));
+ Datapath datapath(&runner);
+ EXPECT_TRUE(datapath.NetnsDeleteName("netns_foo"));
+}
+
TEST(DatapathTest, AddBridge) {
MockProcessRunner runner;
Datapath datapath(&runner);
@@ -170,7 +189,7 @@
MockProcessRunner runner;
EXPECT_CALL(runner, ip(StrEq("link"), StrEq("add"),
ElementsAre("veth_foo", "type", "veth", "peer", "name",
- "peer_foo"),
+ "peer_foo", "netns", "netns_foo"),
true));
EXPECT_CALL(runner, ip(StrEq("addr"), StrEq("add"),
ElementsAre("100.115.92.169/30", "brd",
@@ -182,12 +201,11 @@
"01:02:03:04:05:06", "multicast", "on"),
true))
.WillOnce(Return(0));
- EXPECT_CALL(runner, RestoreDefaultNamespace(StrEq("veth_foo"), kTestPID));
EXPECT_CALL(runner, ip(StrEq("link"), StrEq("set"),
ElementsAre("veth_foo", "up"), true));
Datapath datapath(&runner);
- EXPECT_TRUE(datapath.ConnectVethPair(kTestPID, "veth_foo", "peer_foo",
- {1, 2, 3, 4, 5, 6},
+ EXPECT_TRUE(datapath.ConnectVethPair(kTestPID, "netns_foo", "veth_foo",
+ "peer_foo", {1, 2, 3, 4, 5, 6},
Ipv4Addr(100, 115, 92, 169), 30, true));
}
@@ -195,10 +213,11 @@
MockProcessRunner runner;
EXPECT_CALL(runner, ip(StrEq("link"), StrEq("add"),
ElementsAre("veth_foo", "type", "veth", "peer", "name",
- "peer_foo"),
+ "peer_foo", "netns", "netns_foo"),
true));
Datapath datapath(&runner);
- EXPECT_TRUE(datapath.AddVirtualInterfacePair("veth_foo", "peer_foo"));
+ EXPECT_TRUE(
+ datapath.AddVirtualInterfacePair("netns_foo", "veth_foo", "peer_foo"));
}
TEST(DatapathTest, ToggleInterface) {
diff --git a/patchpanel/fake_process_runner.h b/patchpanel/fake_process_runner.h
index 27ee171..5767812 100644
--- a/patchpanel/fake_process_runner.h
+++ b/patchpanel/fake_process_runner.h
@@ -31,10 +31,6 @@
return 0;
}
- int RestoreDefaultNamespace(const std::string& ifname, pid_t pid) override {
- return 0;
- }
-
void Capture(bool on, std::vector<std::string>* runs = nullptr) {
capture_ = on;
if (runs)
diff --git a/patchpanel/manager.cc b/patchpanel/manager.cc
index c279c3e..abc295b 100644
--- a/patchpanel/manager.cc
+++ b/patchpanel/manager.cc
@@ -784,25 +784,32 @@
}
const std::string ifname_id = std::to_string(connected_namespaces_next_id_);
+ const std::string netns_name = "connected_netns_" + ifname_id;
const std::string host_ifname = "arc_ns" + ifname_id;
const std::string client_ifname = "veth" + ifname_id;
const uint32_t host_ipv4_addr = subnet->AddressAtOffset(0);
const uint32_t client_ipv4_addr = subnet->AddressAtOffset(1);
// Veth interface configuration and client routing configuration:
- // - create veth pair inside client namespace.
+ // - attach a name to the client namespace.
+ // - create veth pair across the current namespace and the client namespace.
// - configure IPv4 address on remote veth inside client namespace.
// - configure IPv4 address on local veth inside host namespace.
// - add a default IPv4 /0 route sending traffic to that remote veth.
- // - bring back one veth to the host namespace, and set it up.
pid_t pid = request.pid();
- if (!datapath_->ConnectVethPair(pid, host_ifname, client_ifname,
+ if (!datapath_->NetnsAttachName(netns_name, pid)) {
+ LOG(ERROR) << "ConnectNamespaceRequest: failed to attach name "
+ << netns_name << " to namespace pid " << pid;
+ return;
+ }
+ if (!datapath_->ConnectVethPair(pid, netns_name, host_ifname, client_ifname,
addr_mgr_.GenerateMacAddress(),
client_ipv4_addr, subnet->PrefixLength(),
false /* enable_multicast */)) {
LOG(ERROR) << "ConnectNamespaceRequest: failed to create veth pair for "
"namespace pid "
<< pid;
+ datapath_->NetnsDeleteName(netns_name);
return;
}
if (!datapath_->ConfigureInterface(
@@ -812,22 +819,22 @@
LOG(ERROR) << "ConnectNamespaceRequest: cannot configure host interface "
<< host_ifname;
datapath_->RemoveInterface(host_ifname);
+ datapath_->NetnsDeleteName(netns_name);
return;
}
+ bool peer_route_setup_success;
{
ScopedNS ns(pid);
- if (!ns.IsValid()) {
- LOG(ERROR) << "ConnectNamespaceRequest: cannot enter client pid " << pid;
- datapath_->RemoveInterface(host_ifname);
- return;
- }
- if (!datapath_->AddIPv4Route(host_ipv4_addr, INADDR_ANY, INADDR_ANY)) {
- LOG(ERROR)
- << "ConnectNamespaceRequest: failed to add default /0 route to "
- << host_ifname << " inside namespace pid " << pid;
- datapath_->RemoveInterface(host_ifname);
- return;
- }
+ peer_route_setup_success =
+ ns.IsValid() &&
+ datapath_->AddIPv4Route(host_ipv4_addr, INADDR_ANY, INADDR_ANY);
+ }
+ if (!peer_route_setup_success) {
+ LOG(ERROR) << "ConnectNamespaceRequest: failed to add default /0 route to "
+ << host_ifname << " inside namespace pid " << pid;
+ datapath_->RemoveInterface(host_ifname);
+ datapath_->NetnsDeleteName(netns_name);
+ return;
}
// Host namespace routing configuration
@@ -846,6 +853,7 @@
LOG(ERROR)
<< "ConnectNamespaceRequest: failed to set route to client namespace";
datapath_->RemoveInterface(host_ifname);
+ datapath_->NetnsDeleteName(netns_name);
return;
}
if (!datapath_->AddOutboundIPv4(host_ifname)) {
@@ -855,6 +863,7 @@
datapath_->RemoveInterface(host_ifname);
datapath_->DeleteIPv4Route(host_ipv4_addr, subnet->BaseAddress(),
subnet->Netmask());
+ datapath_->NetnsDeleteName(netns_name);
return;
}
if (!datapath_->AddOutboundIPv4SNATMark(host_ifname)) {
@@ -865,6 +874,7 @@
datapath_->DeleteIPv4Route(host_ipv4_addr, subnet->BaseAddress(),
subnet->Netmask());
datapath_->RemoveOutboundIPv4(host_ifname);
+ datapath_->NetnsDeleteName(netns_name);
return;
}
@@ -878,6 +888,7 @@
subnet->Netmask());
datapath_->RemoveOutboundIPv4(host_ifname);
datapath_->RemoveOutboundIPv4SNATMark(host_ifname);
+ datapath_->NetnsDeleteName(netns_name);
return;
}
@@ -895,6 +906,7 @@
subnet->Netmask());
datapath_->RemoveOutboundIPv4(host_ifname);
datapath_->RemoveOutboundIPv4SNATMark(host_ifname);
+ datapath_->NetnsDeleteName(netns_name);
return;
}
@@ -913,6 +925,7 @@
connected_namespaces_[fdkey] = {};
ConnectNamespaceInfo& ns_info = connected_namespaces_[fdkey];
ns_info.pid = request.pid();
+ ns_info.netns_name = std::move(netns_name);
ns_info.outbound_ifname = request.outbound_physical_device();
ns_info.host_ifname = std::move(host_ifname);
ns_info.client_ifname = std::move(client_ifname);
@@ -944,6 +957,7 @@
// - destroy veth pair.
// - remove forwarding rules on host namespace.
// - remove SNAT marking rule on host namespace.
+ // Delete the network namespace attached to the client namespace.
// Note that the default route set inside the client namespace by patchpanel
// is not destroyed: it is assumed the client will also teardown its
// namespace if it triggered DisconnectNamespace.
@@ -953,6 +967,7 @@
datapath_->DeleteIPv4Route(it->second.client_subnet->AddressAtOffset(0),
it->second.client_subnet->BaseAddress(),
it->second.client_subnet->Netmask());
+ datapath_->NetnsDeleteName(it->second.netns_name);
LOG(INFO) << "Disconnected network namespace " << it->second;
diff --git a/patchpanel/manager.h b/patchpanel/manager.h
index fed2adf..abf6a2b 100644
--- a/patchpanel/manager.h
+++ b/patchpanel/manager.h
@@ -38,6 +38,8 @@
struct ConnectNamespaceInfo {
// The pid of the client network namespace.
pid_t pid;
+ // The name attached to the client network namespace.
+ std::string netns_name;
// Name of the shill device for routing outbound traffic from the client
// namespace. Empty if outbound traffic should be forwarded to the highest
// priority network (physical or virtual).
diff --git a/patchpanel/minijailed_process_runner.cc b/patchpanel/minijailed_process_runner.cc
index b050498..a433621 100644
--- a/patchpanel/minijailed_process_runner.cc
+++ b/patchpanel/minijailed_process_runner.cc
@@ -35,7 +35,6 @@
constexpr char kIptablesPath[] = "/sbin/iptables";
constexpr char kIp6tablesPath[] = "/sbin/ip6tables";
constexpr char kModprobePath[] = "/sbin/modprobe";
-constexpr char kNsEnterPath[] = "/usr/bin/nsenter";
constexpr char kSysctlPath[] = "/usr/sbin/sysctl";
// An empty string will be returned if read fails.
@@ -139,13 +138,6 @@
return RunSyncDestroy(argv, mj_, jail, log_failures, nullptr);
}
-int MinijailedProcessRunner::RestoreDefaultNamespace(const std::string& ifname,
- pid_t pid) {
- return RunSync({kNsEnterPath, "-t", base::NumberToString(pid), "-n", "--",
- kIpPath, "link", "set", ifname, "netns", "1"},
- mj_, true, nullptr);
-}
-
int MinijailedProcessRunner::brctl(const std::string& cmd,
const std::vector<std::string>& argv,
bool log_failures) {
@@ -236,4 +228,18 @@
return RunSync(args, mj_, log_failures, nullptr);
}
+int MinijailedProcessRunner::ip_netns_attach(const std::string& netns_name,
+ pid_t netns_pid,
+ bool log_failures) {
+ std::vector<std::string> args = {kIpPath, "netns", "attach", netns_name,
+ std::to_string(netns_pid)};
+ return RunSync(args, mj_, log_failures, nullptr);
+}
+
+int MinijailedProcessRunner::ip_netns_delete(const std::string& netns_name,
+ bool log_failures) {
+ std::vector<std::string> args = {kIpPath, "netns", "delete", netns_name};
+ return RunSync(args, mj_, log_failures, nullptr);
+}
+
} // namespace patchpanel
diff --git a/patchpanel/minijailed_process_runner.h b/patchpanel/minijailed_process_runner.h
index 1fc11b5..1fe21d8 100644
--- a/patchpanel/minijailed_process_runner.h
+++ b/patchpanel/minijailed_process_runner.h
@@ -6,6 +6,8 @@
#define PATCHPANEL_MINIJAILED_PROCESS_RUNNER_H_
#include <memory>
+#include <sys/types.h>
+
#include <string>
#include <vector>
@@ -36,10 +38,6 @@
std::unique_ptr<SyscallImpl> syscall);
virtual ~MinijailedProcessRunner() = default;
- // Moves interface |ifname| back into the default namespace
- // |pid| identifies the pid of the current namespace.
- virtual int RestoreDefaultNamespace(const std::string& ifname, pid_t pid);
-
// Runs brctl.
virtual int brctl(const std::string& cmd,
const std::vector<std::string>& argv,
@@ -82,6 +80,16 @@
const std::string& value,
bool log_failures = true);
+ // Attaches a name to the network namespace of the given pid
+ // TODO(hugobenichi) How can patchpanel create a |netns_name| file in
+ // /run/netns without running ip as root ?
+ virtual int ip_netns_attach(const std::string& netns_name,
+ pid_t netns_pid,
+ bool log_failures = true);
+
+ virtual int ip_netns_delete(const std::string& netns_name,
+ bool log_failures = true);
+
protected:
// Runs a process (argv[0]) with optional arguments (argv[1]...)
// in a minijail as an unprivileged user with CAP_NET_ADMIN and
diff --git a/patchpanel/minijailed_process_runner_test.cc b/patchpanel/minijailed_process_runner_test.cc
index 6b9cacd..112d54b 100644
--- a/patchpanel/minijailed_process_runner_test.cc
+++ b/patchpanel/minijailed_process_runner_test.cc
@@ -60,17 +60,6 @@
return arg[index] == nullptr;
}
-TEST_F(MinijailProcessRunnerTest, RestoreDefaultNamespace) {
- const std::vector<std::string> args = {
- "-t", "12345", "-n", "--", "/bin/ip", "link", "set", "foo", "netns", "1",
- };
- EXPECT_CALL(mj_, New());
- EXPECT_CALL(mj_, DropRoot(_, _, _)).Times(0);
- EXPECT_CALL(mj_, RunPipesAndDestroy(
- _, IsProcessArgs("/usr/bin/nsenter", args), _, _, _, _));
- runner_.RestoreDefaultNamespace("foo", 12345);
-}
-
TEST_F(MinijailProcessRunnerTest, modprobe_all) {
uint64_t caps = CAP_TO_MASK(CAP_SYS_MODULE);
diff --git a/patchpanel/mock_datapath.h b/patchpanel/mock_datapath.h
index c835344..a889116 100644
--- a/patchpanel/mock_datapath.h
+++ b/patchpanel/mock_datapath.h
@@ -20,6 +20,10 @@
explicit MockDatapath(MinijailedProcessRunner* runner) : Datapath(runner) {}
~MockDatapath() = default;
+ MOCK_METHOD2(NetnsAttachName,
+ bool(const std::string& netns_name, pid_t netns_pid));
+ MOCK_METHOD1(NetnsDeleteName, bool(const std::string& netns_name));
+
MOCK_METHOD3(AddBridge,
bool(const std::string& ifname,
uint32_t ipv4_addr,
@@ -27,13 +31,15 @@
MOCK_METHOD1(RemoveBridge, void(const std::string& ifname));
MOCK_METHOD2(AddToBridge,
bool(const std::string& br_ifname, const std::string& ifname));
+
MOCK_METHOD4(AddTAP,
std::string(const std::string& name,
const MacAddress* mac_addr,
const SubnetAddress* ipv4_addr,
const std::string& user));
- MOCK_METHOD2(AddVirtualInterfacePair,
- bool(const std::string& veth_ifname,
+ MOCK_METHOD3(AddVirtualInterfacePair,
+ bool(const std::string& netns_name,
+ const std::string& veth_ifname,
const std::string& peer_ifname));
MOCK_METHOD2(ToggleInterface, bool(const std::string& ifname, bool up));
MOCK_METHOD6(ConfigureInterface,
