Merge pull request #435 from bisdn/jogo_2.0.x_fixes

2.0.x: backport fixes from main

pkg/systemd/sysconfig.template

@@ -14,6 +14,9 @@
 #
 # Mark switched packets as offloaded:
 # FLAGS_mark_fwd_offload=true
+#
+# Vlan ID used for untagged traffic on unbridged ports (1-4095):
+# FLAGS_port_untagged_vid=1
 
 ### glog
 #

src/baseboxd.cc

@@ -21,6 +21,8 @@ DEFINE_int32(port, 6653, "Listening port");
 DEFINE_int32(ofdpa_grpc_port, 50051, "Listening port of ofdpa gRPC server");
 DEFINE_bool(use_knet, true, "Use KNET interfaces");
 DEFINE_bool(mark_fwd_offload, true, "Mark switched packets as offloaded");
+DEFINE_int32(port_untagged_vid, 1,
+             "VLAN ID used for untagged traffic on unbridged ports");
 
 static bool validate_port(const char *flagname, gflags::int32 value) {
   VLOG(3) << __FUNCTION__ << ": flagname=" << flagname << ", value=" << value;
@@ -29,6 +31,13 @@ static bool validate_port(const char *flagname, gflags::int32 value) {
   return false;
 }
 
+static bool validate_vid(const char *flagname, gflags::int32 value) {
+  VLOG(3) << __FUNCTION__ << ": flagname=" << flagname << ", value=" << value;
+  if (value > 0 && value <= 4095) // value is ok
+    return true;
+  return false;
+}
+
 int main(int argc, char **argv) {
   using basebox::cnetlink;
   using basebox::controller;
@@ -48,9 +57,14 @@ int main(int argc, char **argv) {
     exit(1);
   }
 
+  if (!gflags::RegisterFlagValidator(&FLAGS_port_untagged_vid, &validate_vid)) {
+    std::cerr << "Failed to register vid validator" << std::endl;
+    exit(1);
+  }
+
   // all variables can be set from env
-  FLAGS_tryfromenv =
-      std::string("multicast,port,ofdpa_grpc_port,use_knet,mark_fwd_offload");
+  FLAGS_tryfromenv = std::string("multicast,port,ofdpa_grpc_port,use_knet,mark_"
+                                 "fwd_offload,port_untagged_vid");
   gflags::SetUsageMessage("");
   gflags::SetVersionString(PROJECT_VERSION);
 

src/netlink/cnetlink.cc

@@ -1292,7 +1292,6 @@ void cnetlink::link_created(rtnl_link *link) noexcept {
   case LT_BRIDGE_SLAVE: // a new bridge slave was created
     try {
       auto master = rtnl_link_get_master(link);
-      uint8_t vlan_filtering = 0;
 
       // check for new bridge slaves
       if (master == 0) {
@@ -1321,22 +1320,23 @@ void cnetlink::link_created(rtnl_link *link) noexcept {
       auto br_link = get_link_by_ifindex(master);
       LOG(INFO) << __FUNCTION__ << ": using bridge " << br_link.get();
 
-      // we only support vlan aware bridges
-      if (rtnl_link_bridge_get_vlan_filtering(br_link.get(), &vlan_filtering) <
-              0 ||
-          vlan_filtering != 1) {
-        LOG(WARNING)
-            << __FUNCTION__
-            << ": unsupported bridge: configured with vlan_filtering 0";
-        ignored_bridges.insert(master);
-        return;
-      }
-
       bool new_bridge = false;
       // slave interface
       // use only the first bridge an interface is attached to
       // XXX TODO more bridges!
       if (bridge == nullptr) {
+        uint8_t vlan_filtering = 0;
+
+        // we only support vlan aware bridges
+        if (rtnl_link_bridge_get_vlan_filtering(br_link.get(),
+                                                &vlan_filtering) < 0 ||
+            vlan_filtering != 1) {
+          LOG(WARNING)
+              << __FUNCTION__
+              << ": unsupported bridge: configured with vlan_filtering 0";
+          ignored_bridges.insert(master);
+          return;
+        }
         bridge = new nl_bridge(this->swi, port_man, this, vlan, vxlan);
         bridge->set_bridge_interface(br_link.get());
         vxlan->register_bridge(bridge);

src/netlink/nl_bond.cc

@@ -5,6 +5,7 @@
 #include "nl_bond.h"
 
 #include <cassert>
+#include <gflags/gflags.h>
 #include <glog/logging.h>
 #include <linux/if_bridge.h>
 #include <netlink/route/link.h>
@@ -14,6 +15,8 @@
 #include "nl_output.h"
 #include "sai.h"
 
+DECLARE_int32(port_untagged_vid);
+
 namespace basebox {
 
 nl_bond::nl_bond(cnetlink *nl) : swi(nullptr), nl(nl) {}
@@ -242,8 +245,8 @@ int nl_bond::add_lag_member(rtnl_link *bond, rtnl_link *link) {
     std::deque<uint16_t> vlans;
 
     if (nl->has_l3_addresses(bond)) {
-      swi->ingress_port_vlan_add(port_id, 1, true);
-      swi->egress_port_vlan_add(port_id, 1, true);
+      swi->ingress_port_vlan_add(port_id, FLAGS_port_untagged_vid, true);
+      swi->egress_port_vlan_add(port_id, FLAGS_port_untagged_vid, true);
     }
 
     nl->get_vlans(rtnl_link_get_ifindex(bond), &vlans);
@@ -322,8 +325,8 @@ int nl_bond::remove_lag_member(rtnl_link *bond, rtnl_link *link) {
     }
 
     if (nl->has_l3_addresses(bond)) {
-      swi->ingress_port_vlan_remove(port_id, 1, true);
-      swi->egress_port_vlan_remove(port_id, 1);
+      swi->ingress_port_vlan_remove(port_id, FLAGS_port_untagged_vid, true);
+      swi->egress_port_vlan_remove(port_id, FLAGS_port_untagged_vid);
     }
   }
 #endif

src/netlink/nl_bridge.cc

@@ -837,7 +837,7 @@ int nl_bridge::fdb_timeout(rtnl_link *br_link, uint16_t vid,
                       rtnl_link_get_ifindex(br_link)) {
     // * remove l2 entry from kernel
     nl_msg *msg = nullptr;
-    rtnl_neigh_build_delete_request(n.get(), NLM_F_REQUEST, &msg);
+    rtnl_neigh_build_delete_request(n_lookup.get(), NLM_F_REQUEST, &msg);
     assert(msg);
 
     // send the message and create new fdb entry

src/netlink/nl_l3.cc

@@ -9,6 +9,7 @@
 #include <unordered_set>
 #include <utility>
 
+#include <gflags/gflags.h>
 #include <glog/logging.h>
 #include <netlink/route/addr.h>
 #include <netlink/route/link.h>
@@ -28,6 +29,8 @@
 #include "sai.h"
 #include "utils/rofl-utils.h"
 
+DECLARE_int32(port_untagged_vid);
+
 namespace std {
 
 template <> struct hash<rofl::caddress_ll> {
@@ -476,7 +479,7 @@ int nl_l3::del_l3_addr(struct rtnl_addr *a) {
 
   std::deque<rtnl_addr *> addresses;
   get_l3_addrs(link, &addresses, family);
-  if (vid == 1 && addresses.empty()) {
+  if (vid == FLAGS_port_untagged_vid && addresses.empty()) {
     struct rtnl_link *other;
 
     if (rtnl_link_is_vlan(link)) {
@@ -1998,9 +2001,8 @@ int nl_l3::del_l3_unicast_route(rtnl_route *r, bool keep_route) {
   for (auto i : unresolved_nh) {
     auto it = std::find_if(nh_callbacks.begin(), nh_callbacks.end(),
                            [&](std::pair<nh_reachable *, nh_params> &cb) {
-                             return cb.first == this &&
-                                    cb.second.nh.ifindex == i.ifindex &&
-                                    !nl_addr_cmp_prefix(cb.second.np.addr, dst);
+                             return cb.first == this && cb.second.nh == i &&
+                                    !nl_addr_cmp(cb.second.np.addr, dst);
                            });
 
     if (it != nh_callbacks.end())
@@ -2056,6 +2058,20 @@ int nl_l3::del_l3_unicast_route(rtnl_route *r, bool keep_route) {
 
   // remove egress references
   for (auto n : neighs) {
+    int ifindex = rtnl_neigh_get_ifindex(n);
+    struct nl_addr *addr = rtnl_neigh_get_dst(n);
+
+    auto it =
+        std::find_if(nh_unreach_callbacks.begin(), nh_unreach_callbacks.end(),
+                     [&](std::pair<nh_unreachable *, nh_params> &cb) {
+                       return cb.first == this &&
+                              cb.second.nh.ifindex == ifindex &&
+                              !nl_addr_cmp(cb.second.nh.nh, addr) &&
+                              !nl_addr_cmp(cb.second.np.addr, dst);
+                     });
+
+    if (it != nh_unreach_callbacks.end())
+      nh_unreach_callbacks.erase(it);
     rv = del_l3_neigh_egress(n);
 
     if (rv < 0 and rv != -EEXIST) {

src/netlink/nl_l3_interfaces.h

@@ -52,12 +52,23 @@ struct nh_stub {
   }
 
   bool operator<(const nh_stub &other) const {
-    if (nl_addr_cmp(nh, other.nh) < 0)
+    int cmp = nl_addr_cmp(nh, other.nh);
+
+    if (cmp < 0)
       return true;
+    if (cmp > 0)
+      return false;
 
     return ifindex < other.ifindex;
   }
 
+  bool operator==(const nh_stub &other) const {
+    if (nl_addr_cmp(nh, other.nh) != 0)
+      return false;
+
+    return ifindex == other.ifindex;
+  }
+
   nl_addr *nh;
   int ifindex;
 };

src/netlink/nl_vlan.cc

@@ -4,6 +4,7 @@
 
 #include <cassert>
 
+#include <gflags/gflags.h>
 #include <glog/logging.h>
 #include <netlink/route/link.h>
 #include <netlink/route/link/vlan.h>
@@ -15,6 +16,8 @@
 #include "nl_vlan.h"
 #include "sai.h"
 
+DECLARE_int32(port_untagged_vid);
+
 namespace basebox {
 
 nl_vlan::nl_vlan(cnetlink *nl) : swi(nullptr), nl(nl) {}
@@ -310,7 +313,8 @@ int nl_vlan::enable_vlans(rtnl_link *link) {
     uint16_t vrf_id = get_vrf_id(vid, link);
 
     // assume the default vlan is untagged
-    (void)enable_vlan(port_id, it.first.second, vid != default_vid, vrf_id);
+    (void)enable_vlan(port_id, it.first.second, vid != FLAGS_port_untagged_vid,
+                      vrf_id);
   }
 
   return 0;
@@ -334,7 +338,8 @@ int nl_vlan::disable_vlans(rtnl_link *link) {
     uint16_t vrf_id = get_vrf_id(vid, link);
 
     // assume the default vlan is untagged
-    (void)disable_vlan(port_id, it.first.second, vid != default_vid, vrf_id);
+    (void)disable_vlan(port_id, it.first.second, vid != FLAGS_port_untagged_vid,
+                       vrf_id);
   }
 
   return 0;
@@ -459,12 +464,13 @@ uint16_t nl_vlan::get_vid(rtnl_link *link) {
 
   switch (lt) {
   case LT_BRIDGE:
-    VLOG(2) << __FUNCTION__ << ": bridge default vid " << default_vid;
-    vid = default_vid;
+    VLOG(2) << __FUNCTION__ << ": bridge default vid "
+            << FLAGS_port_untagged_vid;
+    vid = FLAGS_port_untagged_vid;
     break;
   case LT_TUN:
   case LT_BOND:
-    vid = default_vid;
+    vid = FLAGS_port_untagged_vid;
     break;
   case LT_VLAN:
   case LT_VRF_SLAVE:
@@ -473,7 +479,7 @@ uint16_t nl_vlan::get_vid(rtnl_link *link) {
   default:
     // port or bond interface
     if (nl->get_port_id(link) > 0)
-      vid = default_vid;
+      vid = FLAGS_port_untagged_vid;
     else if (rtnl_link_get_ifindex(link) != 1)
       LOG(ERROR) << __FUNCTION__ << ": unsupported link type " << lt
                  << " of link " << link;

src/netlink/nl_vlan.h

@@ -58,7 +58,6 @@ class nl_vlan {
 private:
   static const uint16_t vid_low = 1;
   static const uint16_t vid_high = 0xfff;
-  static const uint16_t default_vid = vid_low;
 
   int enable_vlan(uint32_t port_id, uint16_t vid, bool tagged,
                   uint16_t vrf_id = 0);

src/netlink/nl_vxlan.cc

@@ -11,6 +11,8 @@
 #include <unordered_map>
 #include <utility>
 
+#include <gflags/gflags.h>
+
 #include <netlink/cache.h>
 #include <netlink/route/link.h>
 #include <netlink/route/neighbour.h>
@@ -26,6 +28,8 @@
 #include "nl_route_query.h"
 #include "nl_vxlan.h"
 
+DECLARE_int32(port_untagged_vid);
+
 namespace basebox {
 
 struct pport_vlan {
@@ -1051,7 +1055,8 @@ int nl_vxlan::create_next_hop(rtnl_neigh *neigh, uint32_t *next_hop_id) {
   }
 
   uint64_t dst_mac = nlall2uint64(addr);
-  uint16_t vlan_id = 1; // XXX TODO currently hardcoded to vid 1
+  uint16_t vlan_id =
+      FLAGS_port_untagged_vid; // XXX TODO currently hardcoded to untagged vid
   auto tnh = tunnel_nh(src_mac, dst_mac, physical_port, vlan_id);
   auto tnh_it = tunnel_next_hop_id.equal_range(tnh);
 
@@ -1127,7 +1132,8 @@ int nl_vxlan::delete_next_hop(rtnl_neigh *neigh) {
   }
 
   uint64_t dst_mac = nlall2uint64(addr);
-  uint16_t vlan_id = 1; // XXX TODO currently hardcoded to vid 1
+  uint16_t vlan_id =
+      FLAGS_port_untagged_vid; // XXX TODO currently hardcoded to untagged vid
   auto tnh = tunnel_nh(src_mac, dst_mac, physical_port, vlan_id);
 
   return delete_next_hop(tnh);

src/netlink/tap_manager.cc

@@ -390,31 +390,33 @@ int tap_manager::set_port_speed(const std::string name, uint32_t speed,
   // mode bitmaps.
 
   struct {
-    struct ethtool_link_settings req;
     __u32 link_mode_data[3 * ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32];
-  } link_settings;
+    struct ethtool_link_settings req;
+  } link_settings_request;
+  auto *link_settings =
+      reinterpret_cast<struct ethtool_link_settings *>(&link_settings_request);
 
-  memset(&link_settings, 0, sizeof(link_settings));
-  link_settings.req.cmd = ETHTOOL_GLINKSETTINGS;
+  memset(&link_settings_request, 0, sizeof(link_settings_request));
+  link_settings->cmd = ETHTOOL_GLINKSETTINGS;
 
-  ifr.ifr_data = reinterpret_cast<char *>(&link_settings);
+  ifr.ifr_data = reinterpret_cast<char *>(link_settings);
   int error = ioctl(sockFd, SIOCETHTOOL, static_cast<void *>(&ifr));
   if (error < 0) {
     LOG(ERROR) << __FUNCTION__ << " handshake failed error=" << error;
     close(sockFd);
     return error;
   }
 
-  if (link_settings.req.link_mode_masks_nwords >= 0 ||
-      link_settings.req.cmd != ETHTOOL_GLINKSETTINGS) {
+  if (link_settings->link_mode_masks_nwords >= 0 ||
+      link_settings->cmd != ETHTOOL_GLINKSETTINGS) {
     close(sockFd);
     return -EOPNOTSUPP;
   }
 
-  link_settings.req.link_mode_masks_nwords =
-      -link_settings.req.link_mode_masks_nwords;
+  link_settings->link_mode_masks_nwords =
+      -link_settings->link_mode_masks_nwords;
 
-  ifr.ifr_data = reinterpret_cast<char *>(&link_settings);
+  ifr.ifr_data = reinterpret_cast<char *>(link_settings);
   error = ioctl(sockFd, SIOCETHTOOL, static_cast<void *>(&ifr));
   if (error < 0) {
     LOG(ERROR) << __FUNCTION__ << " failed to get port= " << name
@@ -423,11 +425,11 @@ int tap_manager::set_port_speed(const std::string name, uint32_t speed,
     return error;
   }
 
-  link_settings.req.duplex = duplex;
-  link_settings.req.speed = ETHTOOL_SPEED(speed);
-  link_settings.req.cmd = ETHTOOL_SLINKSETTINGS;
+  link_settings->duplex = duplex;
+  link_settings->speed = ETHTOOL_SPEED(speed);
+  link_settings->cmd = ETHTOOL_SLINKSETTINGS;
 
-  ifr.ifr_data = reinterpret_cast<char *>(&link_settings);
+  ifr.ifr_data = reinterpret_cast<char *>(link_settings);
   error = ioctl(sockFd, SIOCETHTOOL, static_cast<void *>(&ifr));
   if (error < 0) {
     LOG(ERROR) << __FUNCTION__ << " failed to set port= " << name