allow configuring the default untagged VLAN for ports

Using unbridged, untagged ports at the same time as VID 1 as default
PVID on a bridge may cause unexpected leakage and forwarding of traffic.

Moving to the reserved VID 4095 would avoid this, but we cannot
unconditionally use it until we solved #414, as otherwise we would leak
packets with VID 4095.

So for now allow changing the VID used internally for ports, but keep
the default at 1.

Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de>

pkg/systemd/sysconfig.template

@@ -17,6 +17,9 @@
 #
 # Clear switch configuration on connect
 # FLAGS_clear_switch_configuration=true
+#
+# Vlan ID used for untagged traffic on unbridged ports (1-4095):
+# FLAGS_port_untagged_vid=1
 
 ### glog
 #

src/baseboxd.cc

@@ -23,6 +23,8 @@ DEFINE_bool(use_knet, true, "Use KNET interfaces");
 DEFINE_bool(mark_fwd_offload, true, "Mark switched packets as offloaded");
 DEFINE_bool(clear_switch_configuration, true,
             "Clear switch configuration on connect");
+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;
@@ -31,6 +33,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;
@@ -50,9 +59,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/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_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)) {

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);