From a2d1e6bc1dd2b7d685d4ec86d013f8d28bb1a6d6 Mon Sep 17 00:00:00 2001 From: Jonas Gorski Date: Thu, 2 May 2024 17:16:43 +0200 Subject: [PATCH] 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 --- pkg/systemd/sysconfig.template | 3 +++ src/baseboxd.cc | 18 ++++++++++++++++-- src/netlink/nl_bond.cc | 11 +++++++---- src/netlink/nl_l3.cc | 5 ++++- src/netlink/nl_vlan.cc | 18 ++++++++++++------ src/netlink/nl_vlan.h | 1 - src/netlink/nl_vxlan.cc | 10 ++++++++-- 7 files changed, 50 insertions(+), 16 deletions(-) diff --git a/pkg/systemd/sysconfig.template b/pkg/systemd/sysconfig.template index 6c4e5bb9..851da861 100644 --- a/pkg/systemd/sysconfig.template +++ b/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 # diff --git a/src/baseboxd.cc b/src/baseboxd.cc index ad361f7b..584dd4d1 100644 --- a/src/baseboxd.cc +++ b/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); diff --git a/src/netlink/nl_bond.cc b/src/netlink/nl_bond.cc index deab6f6c..22d1096e 100644 --- a/src/netlink/nl_bond.cc +++ b/src/netlink/nl_bond.cc @@ -5,6 +5,7 @@ #include "nl_bond.h" #include +#include #include #include #include @@ -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 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 diff --git a/src/netlink/nl_l3.cc b/src/netlink/nl_l3.cc index caeaaa05..27a42233 100644 --- a/src/netlink/nl_l3.cc +++ b/src/netlink/nl_l3.cc @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -28,6 +29,8 @@ #include "sai.h" #include "utils/rofl-utils.h" +DECLARE_int32(port_untagged_vid); + namespace std { template <> struct hash { @@ -476,7 +479,7 @@ int nl_l3::del_l3_addr(struct rtnl_addr *a) { std::deque 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)) { diff --git a/src/netlink/nl_vlan.cc b/src/netlink/nl_vlan.cc index 359aa2e6..ef1bbb86 100644 --- a/src/netlink/nl_vlan.cc +++ b/src/netlink/nl_vlan.cc @@ -4,6 +4,7 @@ #include +#include #include #include #include @@ -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; diff --git a/src/netlink/nl_vlan.h b/src/netlink/nl_vlan.h index 24b7afb0..5a0b4160 100644 --- a/src/netlink/nl_vlan.h +++ b/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); diff --git a/src/netlink/nl_vxlan.cc b/src/netlink/nl_vxlan.cc index c3438485..4d6139ea 100644 --- a/src/netlink/nl_vxlan.cc +++ b/src/netlink/nl_vxlan.cc @@ -11,6 +11,8 @@ #include #include +#include + #include #include #include @@ -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);