Automated g4 rollback of changelist 373010349. *** Reason for rollback *** This change no longer downs the interface when taking down the qbone tunnel. Instead, we close the fd we have for interacting with the TUN -- this has the effect of removing the interface when nopersist is set on the device. *** Original change description *** Automated g4 rollback of changelist 372620094. *** Reason for rollback *** Breaking Endor because we bring down the TUN Device when tearing down the bonnet in the init container. *** Original change description *** Defer TUN device creation until after the quic connection has been established. IOS XR's XR container takes a few seconds for networking state to be sync'd from the host -- during which time any manipulations to the linux contaier's host networking stack breaks the synchronizatio... *** PiperOrigin-RevId: 373210829
diff --git a/quic/qbone/bonnet/mock_tun_device.h b/quic/qbone/bonnet/mock_tun_device.h index d593913..056346c 100644 --- a/quic/qbone/bonnet/mock_tun_device.h +++ b/quic/qbone/bonnet/mock_tun_device.h
@@ -18,6 +18,8 @@ MOCK_METHOD(bool, Down, (), (override)); + MOCK_METHOD(void, CloseDevice, (), (override)); + MOCK_METHOD(int, GetFileDescriptor, (), (const, override)); };
diff --git a/quic/qbone/bonnet/tun_device.cc b/quic/qbone/bonnet/tun_device.cc index 3ca52e8..86e68d7 100644 --- a/quic/qbone/bonnet/tun_device.cc +++ b/quic/qbone/bonnet/tun_device.cc
@@ -39,7 +39,7 @@ if (!persist_) { Down(); } - CleanUpFileDescriptor(); + CloseDevice(); } bool TunDevice::Init() { @@ -63,39 +63,33 @@ // TODO(pengg): might be better to use netlink socket, once we have a library to // use bool TunDevice::Up() { - if (setup_tun_ && !is_interface_up_) { - struct ifreq if_request; - memset(&if_request, 0, sizeof(if_request)); - // copy does not zero-terminate the result string, but we've memset the - // entire struct. - interface_name_.copy(if_request.ifr_name, IFNAMSIZ); - if_request.ifr_flags = IFF_UP; - - is_interface_up_ = - NetdeviceIoctl(SIOCSIFFLAGS, reinterpret_cast<void*>(&if_request)); - return is_interface_up_; - } else { + if (!setup_tun_) { return true; } + struct ifreq if_request; + memset(&if_request, 0, sizeof(if_request)); + // copy does not zero-terminate the result string, but we've memset the + // entire struct. + interface_name_.copy(if_request.ifr_name, IFNAMSIZ); + if_request.ifr_flags = IFF_UP; + + return NetdeviceIoctl(SIOCSIFFLAGS, reinterpret_cast<void*>(&if_request)); } // TODO(pengg): might be better to use netlink socket, once we have a library to // use bool TunDevice::Down() { - if (setup_tun_ && is_interface_up_) { - struct ifreq if_request; - memset(&if_request, 0, sizeof(if_request)); - // copy does not zero-terminate the result string, but we've memset the - // entire struct. - interface_name_.copy(if_request.ifr_name, IFNAMSIZ); - if_request.ifr_flags = 0; - - is_interface_up_ = - !NetdeviceIoctl(SIOCSIFFLAGS, reinterpret_cast<void*>(&if_request)); - return !is_interface_up_; - } else { + if (!setup_tun_) { return true; } + struct ifreq if_request; + memset(&if_request, 0, sizeof(if_request)); + // copy does not zero-terminate the result string, but we've memset the + // entire struct. + interface_name_.copy(if_request.ifr_name, IFNAMSIZ); + if_request.ifr_flags = 0; + + return NetdeviceIoctl(SIOCSIFFLAGS, reinterpret_cast<void*>(&if_request)); } int TunDevice::GetFileDescriptor() const { @@ -103,6 +97,10 @@ } bool TunDevice::OpenDevice() { + if (file_descriptor_ != kInvalidFd) { + CloseDevice(); + } + struct ifreq if_request; memset(&if_request, 0, sizeof(if_request)); // copy does not zero-terminate the result string, but we've memset the entire @@ -117,7 +115,7 @@ if_request.ifr_flags = IFF_TUN | IFF_MULTI_QUEUE | IFF_NO_PI; // TODO(pengg): port MakeCleanup to quic/platform? This makes the call to - // CleanUpFileDescriptor nicer and less error-prone. + // CloseDevice nicer and less error-prone. // When the device is running with IFF_MULTI_QUEUE set, each call to open will // create a queue which can be used to read/write packets from/to the device. const std::string tun_device_path = @@ -125,18 +123,18 @@ int fd = kernel_.open(tun_device_path.c_str(), O_RDWR); if (fd < 0) { QUIC_PLOG(WARNING) << "Failed to open " << tun_device_path; - CleanUpFileDescriptor(); + CloseDevice(); return false; } file_descriptor_ = fd; if (!CheckFeatures(fd)) { - CleanUpFileDescriptor(); + CloseDevice(); return false; } if (kernel_.ioctl(fd, TUNSETIFF, reinterpret_cast<void*>(&if_request)) != 0) { QUIC_PLOG(WARNING) << "Failed to TUNSETIFF on fd(" << fd << ")"; - CleanUpFileDescriptor(); + CloseDevice(); return false; } @@ -144,7 +142,7 @@ fd, TUNSETPERSIST, persist_ ? reinterpret_cast<void*>(&if_request) : nullptr) != 0) { QUIC_PLOG(WARNING) << "Failed to TUNSETPERSIST on fd(" << fd << ")"; - CleanUpFileDescriptor(); + CloseDevice(); return false; } @@ -166,7 +164,7 @@ if_request.ifr_mtu = mtu_; if (!NetdeviceIoctl(SIOCSIFMTU, reinterpret_cast<void*>(&if_request))) { - CleanUpFileDescriptor(); + CloseDevice(); return false; } @@ -206,7 +204,7 @@ return true; } -void TunDevice::CleanUpFileDescriptor() { +void TunDevice::CloseDevice() { if (file_descriptor_ != kInvalidFd) { kernel_.close(file_descriptor_); file_descriptor_ = kInvalidFd;
diff --git a/quic/qbone/bonnet/tun_device.h b/quic/qbone/bonnet/tun_device.h index bff85f5..129e1d1 100644 --- a/quic/qbone/bonnet/tun_device.h +++ b/quic/qbone/bonnet/tun_device.h
@@ -49,6 +49,11 @@ // Marks the interface down to stop receiving packets. bool Down() override; + // Closes the open file descriptor for the TUN device (if one exists). + // It is safe to reinitialize and reuse this TunDevice after calling + // CloseDevice. + void CloseDevice() override; + // Gets the file descriptor that can be used to send/receive packets. // This returns -1 when the TUN device is in an invalid state. int GetFileDescriptor() const override; @@ -63,10 +68,6 @@ // Checks if the required kernel features exists. bool CheckFeatures(int tun_device_fd); - // Closes the opened file descriptor and makes sure the file descriptor - // is no longer available from GetFileDescriptor; - void CleanUpFileDescriptor(); - // Opens a socket and makes netdevice ioctl call bool NetdeviceIoctl(int request, void* argp); @@ -76,7 +77,6 @@ const bool setup_tun_; int file_descriptor_; KernelInterface& kernel_; - bool is_interface_up_ = false; }; } // namespace quic
diff --git a/quic/qbone/bonnet/tun_device_interface.h b/quic/qbone/bonnet/tun_device_interface.h index e99c547..3c6f962 100644 --- a/quic/qbone/bonnet/tun_device_interface.h +++ b/quic/qbone/bonnet/tun_device_interface.h
@@ -12,7 +12,7 @@ // An interface with methods for interacting with a TUN device. class TunDeviceInterface { public: - virtual ~TunDeviceInterface() {} + virtual ~TunDeviceInterface() = default; // Actually creates/reopens and configures the device. virtual bool Init() = 0; @@ -23,6 +23,11 @@ // Marks the interface down to stop receiving packets. virtual bool Down() = 0; + // Closes the open file descriptor for the TUN device (if one exists). + // It is safe to reinitialize and reuse this TunDevice after calling + // CloseDevice. + virtual void CloseDevice() = 0; + // Gets the file descriptor that can be used to send/receive packets. // This returns -1 when the TUN device is in an invalid state. virtual int GetFileDescriptor() const = 0;
diff --git a/quic/qbone/bonnet/tun_device_packet_exchanger.cc b/quic/qbone/bonnet/tun_device_packet_exchanger.cc index 2119671..4783e9c 100644 --- a/quic/qbone/bonnet/tun_device_packet_exchanger.cc +++ b/quic/qbone/bonnet/tun_device_packet_exchanger.cc
@@ -11,14 +11,12 @@ namespace quic { TunDevicePacketExchanger::TunDevicePacketExchanger( - int fd, size_t mtu, KernelInterface* kernel, QbonePacketExchanger::Visitor* visitor, size_t max_pending_packets, StatsInterface* stats) : QbonePacketExchanger(visitor, max_pending_packets), - fd_(fd), mtu_(mtu), kernel_(kernel), stats_(stats) {} @@ -78,8 +76,8 @@ return std::make_unique<QuicData>(read_buffer.release(), result, true); } -int TunDevicePacketExchanger::file_descriptor() const { - return fd_; +void TunDevicePacketExchanger::set_file_descriptor(int fd) { + fd_ = fd; } const TunDevicePacketExchanger::StatsInterface*
diff --git a/quic/qbone/bonnet/tun_device_packet_exchanger.h b/quic/qbone/bonnet/tun_device_packet_exchanger.h index 1d28fee..115f5b5 100644 --- a/quic/qbone/bonnet/tun_device_packet_exchanger.h +++ b/quic/qbone/bonnet/tun_device_packet_exchanger.h
@@ -35,8 +35,6 @@ ABSL_MUST_USE_RESULT virtual int64_t PacketsWritten() const = 0; }; - // |fd| is a open file descriptor on a TUN device that's opened for both read - // and write. // |mtu| is the mtu of the TUN device. // |kernel| is not owned but should out live objects of this class. // |visitor| is not owned but should out live objects of this class. @@ -44,14 +42,13 @@ // the TUN device become blocked. // |stats| is notified about packet read/write statistics. It is not owned, // but should outlive objects of this class. - TunDevicePacketExchanger(int fd, - size_t mtu, + TunDevicePacketExchanger(size_t mtu, KernelInterface* kernel, QbonePacketExchanger::Visitor* visitor, size_t max_pending_packets, StatsInterface* stats); - ABSL_MUST_USE_RESULT int file_descriptor() const; + void set_file_descriptor(int fd); ABSL_MUST_USE_RESULT const StatsInterface* stats_interface() const;
diff --git a/quic/qbone/bonnet/tun_device_packet_exchanger_test.cc b/quic/qbone/bonnet/tun_device_packet_exchanger_test.cc index 4a00c60..c8f3ff0 100644 --- a/quic/qbone/bonnet/tun_device_packet_exchanger_test.cc +++ b/quic/qbone/bonnet/tun_device_packet_exchanger_test.cc
@@ -30,14 +30,15 @@ class TunDevicePacketExchangerTest : public QuicTest { protected: TunDevicePacketExchangerTest() - : exchanger_(kFd, - kMtu, + : exchanger_(kMtu, &mock_kernel_, &mock_visitor_, kMaxPendingPackets, - &mock_stats_) {} + &mock_stats_) { + exchanger_.set_file_descriptor(kFd); + } - ~TunDevicePacketExchangerTest() override {} + ~TunDevicePacketExchangerTest() override = default; MockKernel mock_kernel_; StrictMock<MockVisitor> mock_visitor_;
diff --git a/quic/qbone/bonnet/tun_device_test.cc b/quic/qbone/bonnet/tun_device_test.cc index 9c216af..6a02f07 100644 --- a/quic/qbone/bonnet/tun_device_test.cc +++ b/quic/qbone/bonnet/tun_device_test.cc
@@ -139,6 +139,7 @@ TunDevice tun_device(kDeviceName, 1500, false, true, &mock_kernel_); EXPECT_FALSE(tun_device.Init()); EXPECT_EQ(tun_device.GetFileDescriptor(), -1); + ExpectDown(false); } TEST_F(TunDeviceTest, FailToCheckFeature) { @@ -147,6 +148,7 @@ TunDevice tun_device(kDeviceName, 1500, false, true, &mock_kernel_); EXPECT_FALSE(tun_device.Init()); EXPECT_EQ(tun_device.GetFileDescriptor(), -1); + ExpectDown(false); } TEST_F(TunDeviceTest, TooFewFeature) { @@ -160,6 +162,7 @@ TunDevice tun_device(kDeviceName, 1500, false, true, &mock_kernel_); EXPECT_FALSE(tun_device.Init()); EXPECT_EQ(tun_device.GetFileDescriptor(), -1); + ExpectDown(false); } TEST_F(TunDeviceTest, FailToSetFlag) {