0

AddressTrackerLinux: add tests for nl_pid uniqueness

AddressTrackerLinux uses netlink (`man 7 netlink`), a sockets-based
interface, to communicate with the kernel about the status of network
devices. The man page states, "If the application sets nl_pid before
calling bind(2), then it is up to the application to make sure that
nl_pid is unique." Alternatively, the field can be set to zero, which
lets the kernel pick a value.

It seems that netlink wants non-zero `nl_pid` values to be *globally*
unique, even across namespaces. I'm inferring this based on testing.

Currently, `AddressTrackerLinux::Init` sets `sockaddr_nl::nl_pid` to the
value of `getpid()`. Based on the downstream bug report, this behavior
seems to be problematic in PID namespaces, which provide isolated
process ID number spaces, thus increasing the chances of `nl_pid`
collision.

This CL adds two tests. The first test initializes two
AddressTrackerLinux objects in the same process and checks that they
both were able to establish a netlink socket. When the test is enabled,
it fails because the second `nl_pid` is the same as the first.

The second, higher fidelity test spins up two child processes inside
Linux PID namespaces (see `man 7 pid_namespaces` and `man 2 clone`).
Each child process initializes an AddressTrackerLinux object. The test
fails if `bind()` or any other netlink-related initialization steps
fail, e.g. because the address is already in use. The child processes
synchronize with a pipe to ensure that the two trackers' lifetimes
overlap. As a result, this test will fail deterministically when
`nl_pid` values are reused! (However, it cannot deterministically detect
a poor strategy that usually, but not always, produces unique values,
such as `rand() % 10`.)

Both new tests are marked disabled until we land the fix.

Bug: 1224428
Change-Id: I91998e60dd10a6fd3e5bf7c981bdd4067cfcfb72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3002106
Commit-Queue: Dan McArdle <dmcardle@chromium.org>
Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#902678}
This commit is contained in:
Dan McArdle
2021-07-16 21:45:47 +00:00
committed by Chromium LUCI CQ
parent aa0b6e33bb
commit 3a2da5af55
3 changed files with 185 additions and 0 deletions

@ -11,6 +11,7 @@
#include <utility>
#include "base/callback_helpers.h"
#include "base/check.h"
#include "base/files/scoped_file.h"
#include "base/logging.h"
#include "base/posix/eintr_wrapper.h"
@ -276,6 +277,11 @@ void AddressTrackerLinux::Init() {
}
}
bool AddressTrackerLinux::DidTrackingInitSucceedForTesting() const {
CHECK(tracking_);
return watcher_ != nullptr;
}
void AddressTrackerLinux::AbortAndForceOnline() {
watcher_.reset();
netlink_fd_.reset();

@ -21,6 +21,7 @@
#include "base/compiler_specific.h"
#include "base/files/file_descriptor_watcher_posix.h"
#include "base/files/scoped_file.h"
#include "base/gtest_prod_util.h"
#include "base/synchronization/condition_variable.h"
#include "base/synchronization/lock.h"
#include "base/threading/thread_checker.h"
@ -88,6 +89,11 @@ class NET_EXPORT_PRIVATE AddressTrackerLinux {
private:
friend class AddressTrackerLinuxTest;
FRIEND_TEST_ALL_PREFIXES(AddressTrackerLinuxNetlinkTest,
TestInitializeTwoTrackers);
FRIEND_TEST_ALL_PREFIXES(AddressTrackerLinuxNetlinkTest,
TestInitializeTwoTrackersInPidNamespaces);
friend int ChildProcessInitializeTrackerForTesting();
// In tracking mode, holds |lock| while alive. In non-tracking mode,
// enforces single-threaded access.
@ -146,6 +152,10 @@ class NET_EXPORT_PRIVATE AddressTrackerLinux {
// for |connection_type_initialized_cv_|.
int GetThreadsWaitingForConnectionTypeInitForTesting();
// Used by AddressTrackerLinuxNetlinkTest, returns true iff `Init` succeeded.
// Undefined for non-tracking mode.
bool DidTrackingInitSucceedForTesting() const;
// Gets the name of an interface given the interface index |interface_index|.
// May return empty string if it fails but should not return NULL. This is
// overridden by tests.

@ -5,6 +5,7 @@
#include "net/base/address_tracker_linux.h"
#include <linux/if.h>
#include <sched.h>
#include <memory>
#include <unordered_set>
@ -12,13 +13,19 @@
#include "base/bind.h"
#include "base/callback_helpers.h"
#include "base/command_line.h"
#include "base/files/file_util.h"
#include "base/strings/string_number_conversions.h"
#include "base/synchronization/waitable_event.h"
#include "base/test/multiprocess_test.h"
#include "base/test/spin_wait.h"
#include "base/test/task_environment.h"
#include "base/test/test_simple_task_runner.h"
#include "base/threading/simple_thread.h"
#include "build/build_config.h"
#include "net/base/ip_address.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "testing/multiprocess_func_list.h"
#if defined(OS_ANDROID)
#include "base/android/build_info.h"
@ -764,5 +771,167 @@ TEST_F(AddressTrackerLinuxTest, TunnelInterfaceName) {
} // namespace
// This is a regression test for https://crbug.com/1224428.
//
// This test initializes two instances of `AddressTrackerLinux` in the same
// process. The test will fail if the implementation reuses the value of
// `sockaddr_nl::nl_pid`.
//
// Note: consumers generally should not need to create two tracking instances of
// `AddressTrackerLinux` in the same process.
TEST(AddressTrackerLinuxNetlinkTest, DISABLED_TestInitializeTwoTrackers) {
base::test::TaskEnvironment task_env(
base::test::TaskEnvironment::MainThreadType::IO);
AddressTrackerLinux tracker1(base::DoNothing(), base::DoNothing(),
base::DoNothing(), {});
AddressTrackerLinux tracker2(base::DoNothing(), base::DoNothing(),
base::DoNothing(), {});
tracker1.Init();
tracker2.Init();
EXPECT_TRUE(tracker1.DidTrackingInitSucceedForTesting());
EXPECT_TRUE(tracker2.DidTrackingInitSucceedForTesting());
}
// These tests use `base::LaunchOptions::clone_flags` for fine-grained control
// over the clone syscall, but the field is only defined on Linux and ChromeOS.
// Unfortunately, this means these tests do not have coverage on Android.
#if defined(OS_LINUX) || defined(OS_CHROMEOS)
// These tests require specific flag values defined in <sched.h>.
#if defined(CLONE_NEWUSER) && defined(CLONE_NEWPID)
namespace {
const char* const kSwitchParentWriteFd = "addresstrackerlinux_parent_write_fd";
const char* const kSwitchReadFd = "addresstrackerlinux_read_fd";
enum IPCMessage {
// Sent from child to parent once the child has initialized its tracker.
kChildInitializedAndWaiting,
// Sent from child to parent when it was unable to initialize its tracker.
kChildFailed,
// Sent from parent to child when all children are permitted to exit.
kChildMayExit,
};
base::File GetSwitchValueFile(const base::CommandLine* command_line,
base::StringPiece name) {
std::string value = command_line->GetSwitchValueASCII(name);
int fd;
CHECK(base::StringToInt(value, &fd));
return base::File(fd);
}
} // namespace
// This is a regression test for https://crbug.com/1224428.
//
// This test creates multiple concurrent `AddressTrackerLinux` instances in
// separate processes, each in their own PID namespaces.
TEST(AddressTrackerLinuxNetlinkTest,
DISABLED_TestInitializeTwoTrackersInPidNamespaces) {
// This test initializes `kNumChildren` instances of `AddressTrackerLinux` in
// tracking mode, each in their own child process running in a PID namespace.
// The test will fail if the implementation reuses the value of
// `sockaddr_nl::nl_pid`.
//
// The child processes use pipes to synchronize. Each child initializes a
// tracker, sends a message to the parent, and waits for the parent to
// respond, indicating that all children are done setting up. This ensures
// that the tracker objects have overlapping lifetimes, and thus that the
// underlying netlink sockets have overlapping lifetimes. This coexistence is
// necessary, but not sufficient, for a `sockaddr_nl::nl_pid` value collision.
constexpr size_t kNumChildren = 2;
base::ScopedFD parent_read_fd, parent_write_fd;
ASSERT_TRUE(base::CreatePipe(&parent_read_fd, &parent_write_fd));
struct Child {
base::ScopedFD read_fd;
base::ScopedFD write_fd;
base::Process process;
} children[kNumChildren];
for (Child& child : children) {
ASSERT_TRUE(base::CreatePipe(&child.read_fd, &child.write_fd));
// Since the child process will wipe its address space by calling execvp, we
// must share the file descriptors via its command line.
base::CommandLine command_line(
base::GetMultiProcessTestChildBaseCommandLine());
command_line.AppendSwitchASCII(kSwitchParentWriteFd,
base::NumberToString(parent_write_fd.get()));
command_line.AppendSwitchASCII(kSwitchReadFd,
base::NumberToString(child.read_fd.get()));
base::LaunchOptions options;
// Indicate that the child process requires these file descriptors.
// Otherwise, they will be closed. See `base::CloseSuperfluousFds`.
options.fds_to_remap = {{child.read_fd.get(), child.read_fd.get()},
{parent_write_fd.get(), parent_write_fd.get()}};
// Clone into a new PID namespace. Making it a new user namespace as well to
// skirt the CAP_SYS_ADMIN requirement.
options.clone_flags = CLONE_NEWPID | CLONE_NEWUSER;
child.process = base::SpawnMultiProcessTestChild(
"ChildProcessInitializeTrackerForTesting", command_line, options);
}
// Wait for all children to finish initializing their tracking
// AddressTrackerLinuxes.
base::File parent_reader(std::move(parent_read_fd));
for (const Child& child : children) {
ASSERT_TRUE(child.process.IsValid());
uint8_t message[] = {0};
ASSERT_TRUE(parent_reader.ReadAtCurrentPosAndCheck(message));
ASSERT_EQ(message[0], kChildInitializedAndWaiting);
}
// Tell children to exit and wait for them to exit.
for (Child& child : children) {
base::File child_writer(std::move(child.write_fd));
const uint8_t kMessage[] = {kChildMayExit};
ASSERT_TRUE(child_writer.WriteAtCurrentPosAndCheck(kMessage));
int exit_code = 0;
ASSERT_TRUE(child.process.WaitForExit(&exit_code));
ASSERT_EQ(exit_code, 0);
}
}
MULTIPROCESS_TEST_MAIN(ChildProcessInitializeTrackerForTesting) {
base::test::TaskEnvironment task_env(
base::test::TaskEnvironment::MainThreadType::IO);
const base::CommandLine* command_line =
base::CommandLine::ForCurrentProcess();
base::File reader = GetSwitchValueFile(command_line, kSwitchReadFd);
base::File parent_writer =
GetSwitchValueFile(command_line, kSwitchParentWriteFd);
// Initialize an `AddressTrackerLinux` in tracking mode and ensure that it
// created a netlink socket.
AddressTrackerLinux tracker(base::DoNothing(), base::DoNothing(),
base::DoNothing(), {});
tracker.Init();
if (!tracker.DidTrackingInitSucceedForTesting()) {
const uint8_t kMessage[] = {kChildFailed};
parent_writer.WriteAtCurrentPosAndCheck(kMessage);
return 1;
}
// Signal to the parent that we have initialized the tracker.
const uint8_t kMessage[] = {kChildInitializedAndWaiting};
if (!parent_writer.WriteAtCurrentPosAndCheck(kMessage))
return 1;
// Block until the parent says all children have initialized their trackers.
uint8_t message[] = {0};
if (!reader.ReadAtCurrentPosAndCheck(message) || message[0] != kChildMayExit)
return 1;
return 0;
}
#endif // defined(OS_LINUX) || defined(OS_CHROMEOS)
#endif // defined(CLONE_NEWUSER) && defined(CLONE_NEWPID)
} // namespace internal
} // namespace net