From 8e416bcf954fd4a0d0d854e5dac69eba51d29ad1 Mon Sep 17 00:00:00 2001
From: Kefu Chai <kchai@redhat.com>
Date: Wed, 21 Apr 2021 19:50:48 +0800
Subject: [PATCH] common/pick_address: prefer non-loopback addresses

instead of filtering out loopback ifaces, check for loopback addresses,
and prefer non-loopback addresses over loopback addresses.

before this change, iface named "lo" is filtered out by default,
and "lo" is allowed if `ms_bind_exclude_lo_iface` is false.

after this change, iface with address out of 127/8 is prefered.
the iface marked down is not considered.

the option of "ms_bind_exclude_lo_iface" is removed. the tests are
updated accordingly.

Fixes: https://tracker.ceph.com/issues/50456
Signed-off-by: Kefu Chai <kchai@redhat.com>
(cherry picked from commit a9b9bcd53215a07608a28ac2c8e4a8c8b8e80e66)

Conflicts:
	src/common/options/global.yaml.in
	src/common/pick_address.cc: trivial resolution
---
 src/common/options.cc      |  5 ---
 src/common/pick_address.cc | 90 ++++++++++++++++++++++++--------------
 src/common/pick_address.h  |  1 -
 src/test/test_ipaddr.cc    | 57 ++++++++++--------------
 4 files changed, 79 insertions(+), 74 deletions(-)

diff --git a/src/common/options.cc b/src/common/options.cc
index a5dbd0b65cb7f..85f4203be3686 100644
--- a/src/common/options.cc
+++ b/src/common/options.cc
@@ -997,11 +997,6 @@ std::vector<Option> get_global_options() {
     .set_default(100_M)
     .set_description("Limit messages that are read off the network but still being processed"),
 
-    Option("ms_bind_exclude_lo_iface", Option::TYPE_BOOL, Option::LEVEL_ADVANCED)
-    .set_default(true)
-    .set_flag(Option::FLAG_STARTUP)
-    .set_description("Allow servers to bind loopback network interfaces (lo)"),
-
     Option("ms_bind_ipv4", Option::TYPE_BOOL, Option::LEVEL_ADVANCED)
     .set_default(true)
     .set_description("Bind servers to IPv4 address(es)")
diff --git a/src/common/pick_address.cc b/src/common/pick_address.cc
index 11a67e0d700b7..be4e549f3e23e 100644
--- a/src/common/pick_address.cc
+++ b/src/common/pick_address.cc
@@ -33,6 +33,9 @@
 #include "common/errno.h"
 #include "common/numa.h"
 
+#include <net/if.h>
+#include <netinet/in.h>
+
 #define dout_subsys ceph_subsys_
 
 namespace {
@@ -42,15 +45,42 @@ bool matches_with_name(const ifaddrs& ifa, const std::string& if_name)
   return if_name.compare(ifa.ifa_name) == 0;
 }
 
-bool is_addr_allowed(const ifaddrs& ifa, bool exclude_lo_iface)
+static int is_loopback_addr(sockaddr* addr)
 {
-  if (exclude_lo_iface && strcmp(ifa.ifa_name, "lo") == 0) {
-    return false;
+  if (addr->sa_family == AF_INET) {
+    const sockaddr_in* sin = (struct sockaddr_in *)(addr);
+    const in_addr_t net = ntohl(sin->sin_addr.s_addr) >> IN_CLASSA_NSHIFT;
+    return net == IN_LOOPBACKNET ? 1 : 0;
+  } else if (addr->sa_family == AF_INET6) {
+    sockaddr_in6* sin6 = (struct sockaddr_in6 *)(addr);
+    return IN6_IS_ADDR_LOOPBACK(&sin6->sin6_addr) ? 1 : 0;
+  } else {
+    return -1;
   }
-  if (boost::starts_with(ifa.ifa_name, "lo:")) {
-    return false;
+}
+
+static int grade_addr(const ifaddrs& ifa)
+{
+  if (ifa.ifa_addr == nullptr) {
+    return -1;
   }
-  return true;
+  int score = 0;
+  if (ifa.ifa_flags & IFF_UP) {
+    score += 4;
+  }
+  switch (is_loopback_addr(ifa.ifa_addr)) {
+  case 0:
+    // prefer non-loopback addresses
+    score += 2;
+    break;
+  case 1:
+    score += 0;
+    break;
+  default:
+    score = -1;
+    break;
+  }
+  return score;
 }
 
 bool matches_with_net(const ifaddrs& ifa,
@@ -104,13 +134,13 @@ bool matches_with_numa_node(const ifaddrs& ifa, int numa_node)
 #endif
 }
 }
+
 const struct sockaddr *find_ip_in_subnet_list(
   CephContext *cct,
   const struct ifaddrs *ifa,
   unsigned ipv,
   const std::string &networks,
   const std::string &interfaces,
-  bool exclude_lo_iface,
   int numa_node)
 {
   const auto ifs = get_str_list(interfaces);
@@ -120,25 +150,32 @@ const struct sockaddr *find_ip_in_subnet_list(
       exit(1);
   }
 
+  int best_score = -1;
+  const sockaddr* best_addr = nullptr;
   for (const auto* addr = ifa; addr != nullptr; addr = addr->ifa_next) {
-    if (!is_addr_allowed(*addr, exclude_lo_iface)) {
-      continue;
-    }
-    if ((ifs.empty() ||
-         std::any_of(std::begin(ifs), std::end(ifs),
+    if (!ifs.empty() &&
+	std::none_of(std::begin(ifs), std::end(ifs),
                      [&](const auto& if_name) {
                        return matches_with_name(*addr, if_name);
-                     })) &&
-        (nets.empty() ||
-         std::any_of(std::begin(nets), std::end(nets),
+                     })) {
+      continue;
+    }
+    if (!nets.empty() &&
+	std::none_of(std::begin(nets), std::end(nets),
                      [&](const auto& net) {
                        return matches_with_net(cct, *addr, net, ipv);
-                     })) &&
-        matches_with_numa_node(*addr, numa_node)) {
-      return addr->ifa_addr;
+                     })) {
+      continue;
+    }
+    if (!matches_with_numa_node(*addr, numa_node)) {
+      continue;
+    }
+    if (int score = grade_addr(*addr); score > best_score) {
+      best_score = score;
+      best_addr = addr->ifa_addr;
     }
   }
-  return nullptr;
+  return best_addr;
 }
 
 #ifndef WITH_SEASTAR
@@ -163,7 +200,6 @@ static void fill_in_one_address(CephContext *cct,
 				const struct ifaddrs *ifa,
 				const string &networks,
 				const string &interfaces,
-				bool exclude_lo_iface,
 				const char *conf_var,
 				int numa_node = -1)
 {
@@ -173,7 +209,6 @@ static void fill_in_one_address(CephContext *cct,
     CEPH_PICK_ADDRESS_IPV4|CEPH_PICK_ADDRESS_IPV6,
     networks,
     interfaces,
-    exclude_lo_iface,
     numa_node);
   if (!found) {
     lderr(cct) << "unable to find any IP address in networks '" << networks
@@ -226,27 +261,21 @@ void pick_addresses(CephContext *cct, int needs)
     exit(1);
   }
   auto free_ifa = make_scope_guard([ifa] { freeifaddrs(ifa); });
-
-  const bool exclude_lo_iface =
-    cct->_conf.get_val<bool>("ms_bind_exclude_lo_iface");
   if ((needs & CEPH_PICK_ADDRESS_PUBLIC) &&
     public_addr.is_blank_ip() && !public_network.empty()) {
     fill_in_one_address(cct, ifa, public_network, public_network_interface,
-			exclude_lo_iface,
 			"public_addr");
   }
 
   if ((needs & CEPH_PICK_ADDRESS_CLUSTER) && cluster_addr.is_blank_ip()) {
     if (!cluster_network.empty()) {
       fill_in_one_address(cct, ifa, cluster_network, cluster_network_interface,
-			  exclude_lo_iface,
 			  "cluster_addr");
     } else {
       if (!public_network.empty()) {
         lderr(cct) << "Public network was set, but cluster network was not set " << dendl;
         lderr(cct) << "    Using public network also for cluster network" << dendl;
         fill_in_one_address(cct, ifa, public_network, public_network_interface,
-			    exclude_lo_iface,
 			    "cluster_addr");
       }
     }
@@ -260,14 +289,12 @@ static int fill_in_one_address(
   unsigned ipv,
   const string &networks,
   const string &interfaces,
-  bool exclude_lo_iface,
   entity_addrvec_t *addrs,
   int numa_node = -1)
 {
   const struct sockaddr *found = find_ip_in_subnet_list(cct, ifa, ipv,
 							networks,
 							interfaces,
-							exclude_lo_iface,
 							numa_node);
   if (!found) {
     std::string ip_type = "";
@@ -382,8 +409,6 @@ int pick_addresses(
       !networks.empty()) {
     int ipv4_r = !(ipv & CEPH_PICK_ADDRESS_IPV4) ? 0 : -1;
     int ipv6_r = !(ipv & CEPH_PICK_ADDRESS_IPV6) ? 0 : -1;
-    const bool exclude_lo_iface =
-      cct->_conf.get_val<bool>("ms_bind_exclude_lo_iface");
     // first try on preferred numa node (if >= 0), then anywhere.
     while (true) {
       // note: pass in ipv to filter the matching addresses
@@ -391,14 +416,12 @@ int pick_addresses(
 	  (flags & CEPH_PICK_ADDRESS_PREFER_IPV4)) {
 	ipv4_r = fill_in_one_address(cct, ifa, CEPH_PICK_ADDRESS_IPV4,
                                      networks, interfaces,
-				     exclude_lo_iface,
 				     addrs,
                                      preferred_numa_node);
       }
       if (ipv & CEPH_PICK_ADDRESS_IPV6) {
 	ipv6_r = fill_in_one_address(cct, ifa, CEPH_PICK_ADDRESS_IPV6,
                                      networks, interfaces,
-				     exclude_lo_iface,
 				     addrs,
                                      preferred_numa_node);
       }
@@ -406,7 +429,6 @@ int pick_addresses(
 	  !(flags & CEPH_PICK_ADDRESS_PREFER_IPV4)) {
 	ipv4_r = fill_in_one_address(cct, ifa, CEPH_PICK_ADDRESS_IPV4,
                                      networks, interfaces,
-				     exclude_lo_iface,
 				     addrs,
                                      preferred_numa_node);
       }
diff --git a/src/common/pick_address.h b/src/common/pick_address.h
index 2a41f5a8d03c9..1faf60dbd75bc 100644
--- a/src/common/pick_address.h
+++ b/src/common/pick_address.h
@@ -88,7 +88,6 @@ const struct sockaddr *find_ip_in_subnet_list(
   unsigned ipv,
   const std::string &networks,
   const std::string &interfaces,
-  bool exclude_lo_iface,
   int numa_node=-1);
 
 int get_iface_numa_node(
diff --git a/src/test/test_ipaddr.cc b/src/test/test_ipaddr.cc
index 1ba220f8964d5..f4c2d3a62b2c1 100644
--- a/src/test/test_ipaddr.cc
+++ b/src/test/test_ipaddr.cc
@@ -12,6 +12,7 @@
 #endif
 #include <arpa/inet.h>
 #include <ifaddrs.h>
+#include <net/if.h>
 
 static void ipv4(struct sockaddr_in *addr, const char *s) {
   int err;
@@ -189,10 +190,12 @@ TEST(CommonIPAddr, TestV4_SkipLoopback)
   one.ifa_name = lo;
 
   two.ifa_next = &three;
+  two.ifa_flags = IFF_UP;
   two.ifa_addr = (struct sockaddr*)&a_two;
   two.ifa_name = lo0;
 
   three.ifa_next = NULL;
+  three.ifa_flags = IFF_UP;
   three.ifa_addr = (struct sockaddr*)&a_three;
   three.ifa_name = eth0;
 
@@ -201,21 +204,18 @@ TEST(CommonIPAddr, TestV4_SkipLoopback)
   ipv4(&a_three, "10.1.2.3");
 
   const struct sockaddr *result = nullptr;
+  // we prefer the non-loopback address despite the loopback addresses
   result =
     find_ip_in_subnet_list(nullptr, (struct ifaddrs*)&one,
                            CEPH_PICK_ADDRESS_IPV4 | CEPH_PICK_ADDRESS_IPV6,
-                           "10.0.0.0/8", "", true);
+                           "", "");
   ASSERT_EQ((struct sockaddr*)&a_three, result);
+  // the subnet criteria leaves us no choice but the UP loopback address
   result =
     find_ip_in_subnet_list(nullptr, (struct ifaddrs*)&one,
                            CEPH_PICK_ADDRESS_IPV4 | CEPH_PICK_ADDRESS_IPV6,
-                           "127.0.0.0/8", "", true);
-  ASSERT_EQ(nullptr, result);
-  result =
-    find_ip_in_subnet_list(nullptr, (struct ifaddrs*)&one,
-                           CEPH_PICK_ADDRESS_IPV4 | CEPH_PICK_ADDRESS_IPV6,
-                           "127.0.0.0/8", "", false);
-  ASSERT_EQ((struct sockaddr*)&a_one, result);
+                           "127.0.0.0/8", "");
+  ASSERT_EQ((struct sockaddr*)&a_two, result);
 }
 
 TEST(CommonIPAddr, TestV6_Simple)
@@ -315,43 +315,37 @@ TEST(CommonIPAddr, TestV6_SkipLoopback)
   struct sockaddr_in6 a_one;
   struct sockaddr_in6 a_two;
   struct sockaddr_in6 a_three;
-  struct sockaddr_in6 net;
-
-  memset(&net, 0, sizeof(net));
 
   one.ifa_next = &two;
+  ipv6(&a_one, "::1");
   one.ifa_addr = (struct sockaddr*)&a_one;
   one.ifa_name = lo;
 
   two.ifa_next = &three;
+  two.ifa_flags = IFF_UP;
+  ipv6(&a_two, "::1");
   two.ifa_addr = (struct sockaddr*)&a_two;
   two.ifa_name = lo0;
 
   three.ifa_next = NULL;
+  three.ifa_flags = IFF_UP;
+  ipv6(&a_three, "2001:1234:5678:90ab::beef");
   three.ifa_addr = (struct sockaddr*)&a_three;
   three.ifa_name = eth0;
 
-  ipv6(&a_one, "::1");
-  ipv6(&a_two, "::1");
-  ipv6(&a_three, "2001:1234:5678:90ab::beef");
-  ipv6(&net, "ff00::1");
-
   const struct sockaddr *result = nullptr;
+  // we prefer the non-loopback address despite the loopback addresses
   result =
     find_ip_in_subnet_list(nullptr, (struct ifaddrs*)&one,
                            CEPH_PICK_ADDRESS_IPV4 | CEPH_PICK_ADDRESS_IPV6,
-                           "2001:1234:5678:90ab::0/64", "", true);
+                           "", "");
   ASSERT_EQ((struct sockaddr*)&a_three, result);
+  // the subnet criteria leaves us no choice but the UP loopback address
   result =
     find_ip_in_subnet_list(nullptr, (struct ifaddrs*)&one,
                            CEPH_PICK_ADDRESS_IPV4 | CEPH_PICK_ADDRESS_IPV6,
-                           "::1/128", "", true);
-  ASSERT_EQ(nullptr, result);
-  result =
-    find_ip_in_subnet_list(nullptr, (struct ifaddrs*)&one,
-                           CEPH_PICK_ADDRESS_IPV4 | CEPH_PICK_ADDRESS_IPV6,
-                           "::1/128", "", false);
-  ASSERT_EQ((struct sockaddr*)&a_one, result);
+                           "::1/128", "");
+  ASSERT_EQ((struct sockaddr*)&a_two, result);
 }
 
 TEST(CommonIPAddr, ParseNetwork_Empty)
@@ -706,8 +700,7 @@ TEST(pick_address, find_ip_in_subnet_list)
     &one,
     CEPH_PICK_ADDRESS_IPV4,
     "10.1.0.0/16",
-    "eth0",
-    true);
+    "eth0");
   ASSERT_EQ((struct sockaddr*)&a_one, result);
 
   result = find_ip_in_subnet_list(
@@ -715,8 +708,7 @@ TEST(pick_address, find_ip_in_subnet_list)
     &one,
     CEPH_PICK_ADDRESS_IPV4,
     "10.2.0.0/16",
-    "eth1",
-    true);
+    "eth1");
   ASSERT_EQ((struct sockaddr*)&a_two, result);
 
   // match by eth name
@@ -725,8 +717,7 @@ TEST(pick_address, find_ip_in_subnet_list)
     &one,
     CEPH_PICK_ADDRESS_IPV4,
     "10.0.0.0/8",
-    "eth0",
-    true);
+    "eth0");
   ASSERT_EQ((struct sockaddr*)&a_one, result);
 
   result = find_ip_in_subnet_list(
@@ -734,8 +725,7 @@ TEST(pick_address, find_ip_in_subnet_list)
     &one,
     CEPH_PICK_ADDRESS_IPV4,
     "10.0.0.0/8",
-    "eth1",
-    true);
+    "eth1");
   ASSERT_EQ((struct sockaddr*)&a_two, result);
 
   result = find_ip_in_subnet_list(
@@ -743,8 +733,7 @@ TEST(pick_address, find_ip_in_subnet_list)
     &one,
     CEPH_PICK_ADDRESS_IPV6,
     "2001::/16",
-    "eth1",
-    true);
+    "eth1");
   ASSERT_EQ((struct sockaddr*)&a_three, result);
 }
 
-- 
2.39.5