[vnetorch] [vxlanorch] fix a set of memory usage issues (#2352)

* [vnetorch] fix use-after-free in removeBfdSession()
* using a copy of monitor ip instead of a reference since the reference gets invalidated after the endpoint is erased

Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

Author: Yakiv-Huryk

orchagent/muxorch.cpp

@@ -351,8 +351,8 @@ static bool remove_nh_tunnel(sai_object_id_t nh_id, IpAddress& ipAddr)
     return true;
 }
 
-MuxCable::MuxCable(string name, IpPrefix& srv_ip4, IpPrefix& srv_ip6, IpAddress peer_ip)
-         :mux_name_(name), srv_ip4_(srv_ip4), srv_ip6_(srv_ip6), peer_ip4_(peer_ip)
+MuxCable::MuxCable(string name, IpPrefix& srv_ip4, IpPrefix& srv_ip6, IpAddress peer_ip, std::set<IpAddress> skip_neighbors)
+         :mux_name_(name), srv_ip4_(srv_ip4), srv_ip6_(srv_ip6), peer_ip4_(peer_ip), skip_neighbors_(skip_neighbors)
 {
     mux_orch_ = gDirectory.get<MuxOrch*>();
     mux_cb_orch_ = gDirectory.get<MuxCableOrch*>();
@@ -534,6 +534,11 @@ bool MuxCable::nbrHandler(bool enable, bool update_rt)
 void MuxCable::updateNeighbor(NextHopKey nh, bool add)
 {
     sai_object_id_t tnh = mux_orch_->getNextHopTunnelId(MUX_TUNNEL, peer_ip4_);
+    if (add && skip_neighbors_.count(nh.ip_address) != 0)
+    {
+        SWSS_LOG_NOTICE("Skip update neighbor %s on %s", nh.ip_address.to_string().c_str(), nh.alias.c_str());
+        return;
+    }
     nbr_handler_->update(nh, tnh, add, state_);
     if (add)
     {
@@ -1208,9 +1213,37 @@ bool MuxOrch::handleMuxCfg(const Request& request)
     auto srv_ip = request.getAttrIpPrefix("server_ipv4");
     auto srv_ip6 = request.getAttrIpPrefix("server_ipv6");
 
+    std::set<IpAddress> skip_neighbors;
+    std::string cable_type = "active-standby";
+
     const auto& port_name = request.getKeyString(0);
     auto op = request.getOperation();
 
+    for (const auto &name : request.getAttrFieldNames())
+    {
+        if (name == "soc_ipv4")
+        {
+            auto soc_ip = request.getAttrIpPrefix("soc_ipv4");
+            SWSS_LOG_NOTICE("%s: %s was added to ignored neighbor list", port_name.c_str(), soc_ip.getIp().to_string().c_str());
+            skip_neighbors.insert(soc_ip.getIp());
+        }
+        else if (name == "soc_ipv6")
+        {
+            auto soc_ip6 = request.getAttrIpPrefix("soc_ipv6");
+            SWSS_LOG_NOTICE("%s: %s was added to ignored neighbor list", port_name.c_str(), soc_ip6.getIp().to_string().c_str());
+            skip_neighbors.insert(soc_ip6.getIp());
+        }
+        else if (name == "cable_type")
+        {
+            cable_type = request.getAttrString("cable_type");
+        }
+    }
+
+    if (cable_type != "active-active")
+    {
+        skip_neighbors.clear();
+    }
+
     if (op == SET_COMMAND)
     {
         if(isMuxExists(port_name))
@@ -1226,7 +1259,7 @@ bool MuxOrch::handleMuxCfg(const Request& request)
         }
 
         mux_cable_tb_[port_name] = std::make_unique<MuxCable>
-                                   (MuxCable(port_name, srv_ip, srv_ip6, mux_peer_switch_));
+                                   (MuxCable(port_name, srv_ip, srv_ip6, mux_peer_switch_, skip_neighbors));
 
         SWSS_LOG_NOTICE("Mux entry for port '%s' was added", port_name.c_str());
     }

orchagent/muxorch.h

@@ -76,7 +76,7 @@ class MuxNbrHandler
 class MuxCable
 {
 public:
-    MuxCable(string name, IpPrefix& srv_ip4, IpPrefix& srv_ip6, IpAddress peer_ip);
+    MuxCable(string name, IpPrefix& srv_ip4, IpPrefix& srv_ip6, IpAddress peer_ip, std::set<IpAddress> skip_neighbors);
 
     bool isActive() const
     {
@@ -115,6 +115,8 @@ class MuxCable
     IpPrefix srv_ip4_, srv_ip6_;
     IpAddress peer_ip4_;
 
+    std::set<IpAddress> skip_neighbors_;
+
     MuxOrch *mux_orch_;
     MuxCableOrch *mux_cb_orch_;
     MuxStateOrch *mux_state_orch_;
@@ -132,6 +134,7 @@ const request_description_t mux_cfg_request_description = {
                 { "server_ipv6", REQ_T_IP_PREFIX },
                 { "address_ipv4", REQ_T_IP },
                 { "soc_ipv4", REQ_T_IP_PREFIX },
+                { "soc_ipv6", REQ_T_IP_PREFIX },
                 { "cable_type", REQ_T_STRING },
             },
             { }

orchagent/vnetorch.cpp

@@ -1602,7 +1602,8 @@ void VNetRouteOrch::delEndpointMonitor(const string& vnet, NextHopGroupKey& next
         if (nexthop_info_[vnet].find(ip) != nexthop_info_[vnet].end()) {
             if (--nexthop_info_[vnet][ip].ref_count == 0)
             {
-                removeBfdSession(vnet, nhk, nexthop_info_[vnet][ip].monitor_addr);
+                IpAddress monitor_addr = nexthop_info_[vnet][ip].monitor_addr;
+                removeBfdSession(vnet, nhk, monitor_addr);
             }
         }
     }

orchagent/vxlanorch.cpp

@@ -1110,13 +1110,14 @@ void VxlanTunnel::updateRemoteEndPointIpRef(const std::string remote_vtep, bool
             it->second.ip_refcnt++;
         }
         SWSS_LOG_DEBUG("Incrementing remote end point %s reference to %d", remote_vtep.c_str(),
-                       it->second.ip_refcnt);
+                       tnl_users_[remote_vtep].ip_refcnt);
     }
     else
     {
         if (it == tnl_users_.end())
         {
             SWSS_LOG_ERROR("Cannot decrement ref. End point not referenced %s", remote_vtep.c_str());
+            return;
         }
         it->second.ip_refcnt--;
 

tests/test_mux.py

@@ -27,6 +27,7 @@ class TestMuxTunnelBase(object):
 
     SERV1_IPV4                  = "192.168.0.100"
     SERV1_IPV6                  = "fc02:1000::100"
+    SERV1_SOC_IPV4              = "192.168.0.102"
     SERV2_IPV4                  = "192.168.0.101"
     SERV2_IPV6                  = "fc02:1000::101"
     IPV4_MASK                   = "/32"
@@ -74,7 +75,12 @@ def create_vlan_interface(self, confdb, asicdb, dvs):
 
     def create_mux_cable(self, confdb):
 
-        fvs = { "server_ipv4":self.SERV1_IPV4+self.IPV4_MASK, "server_ipv6":self.SERV1_IPV6+self.IPV6_MASK }
+        fvs = {
+            "server_ipv4":self.SERV1_IPV4 + self.IPV4_MASK,
+            "server_ipv6":self.SERV1_IPV6 + self.IPV6_MASK,
+            "soc_ipv4": self.SERV1_SOC_IPV4 + self.IPV4_MASK,
+            "cable_type": "active-active"
+        }
         confdb.create_entry(self.CONFIG_MUX_CABLE, "Ethernet0", fvs)
 
         fvs = { "server_ipv4":self.SERV2_IPV4+self.IPV4_MASK, "server_ipv6":self.SERV2_IPV6+self.IPV6_MASK }
@@ -201,6 +207,9 @@ def create_and_test_neighbor(self, confdb, appdb, asicdb, dvs, dvs_route):
         self.add_neighbor(dvs, self.SERV1_IPV6, "00:00:00:00:00:01", True)
         srv1_v6 = self.check_neigh_in_asic_db(asicdb, self.SERV1_IPV6, 3)
 
+        self.add_neighbor(dvs, self.SERV1_SOC_IPV4, "00:00:00:00:00:01")
+        srv1_soc_v4 = self.check_neigh_in_asic_db(asicdb, self.SERV1_SOC_IPV4, 4)
+
         existing_keys = asicdb.get_keys(self.ASIC_NEIGH_TABLE)
 
         self.add_neighbor(dvs, self.SERV2_IPV4, "00:00:00:00:00:02")
@@ -212,21 +221,23 @@ def create_and_test_neighbor(self, confdb, appdb, asicdb, dvs, dvs_route):
         dvs_route.check_asicdb_route_entries([self.SERV2_IPV4+self.IPV4_MASK, self.SERV2_IPV6+self.IPV6_MASK])
 
         # The first standby route also creates as tunnel Nexthop
-        self.check_tnl_nexthop_in_asic_db(asicdb, 3)
+        self.check_tnl_nexthop_in_asic_db(asicdb, 4)
 
         # Change state to Standby. This will delete Neigh and add Route
         self.set_mux_state(appdb, "Ethernet0", "standby")
 
         asicdb.wait_for_deleted_entry(self.ASIC_NEIGH_TABLE, srv1_v4)
         asicdb.wait_for_deleted_entry(self.ASIC_NEIGH_TABLE, srv1_v6)
         dvs_route.check_asicdb_route_entries([self.SERV1_IPV4+self.IPV4_MASK, self.SERV1_IPV6+self.IPV6_MASK])
+        self.check_neigh_in_asic_db(asicdb, self.SERV1_SOC_IPV4, 2)
+        dvs_route.check_asicdb_deleted_route_entries([self.SERV1_SOC_IPV4+self.IPV4_MASK])
 
         # Change state to Active. This will add Neigh and delete Route
         self.set_mux_state(appdb, "Ethernet4", "active")
 
         dvs_route.check_asicdb_deleted_route_entries([self.SERV2_IPV4+self.IPV4_MASK, self.SERV2_IPV6+self.IPV6_MASK])
-        self.check_neigh_in_asic_db(asicdb, self.SERV2_IPV4, 3)
-        self.check_neigh_in_asic_db(asicdb, self.SERV2_IPV6, 3)
+        self.check_neigh_in_asic_db(asicdb, self.SERV2_IPV4, 4)
+        self.check_neigh_in_asic_db(asicdb, self.SERV2_IPV6, 4)
 
 
     def create_and_test_fdb(self, appdb, asicdb, dvs, dvs_route):
@@ -244,7 +255,7 @@ def create_and_test_fdb(self, appdb, asicdb, dvs, dvs_route):
         self.add_neighbor(dvs, ip_2, "00:00:00:00:00:12", True)
 
         # ip_1 is on Active Mux, hence added to Host table
-        self.check_neigh_in_asic_db(asicdb, ip_1, 4)
+        self.check_neigh_in_asic_db(asicdb, ip_1, 5)
 
         # ip_2 is on Standby Mux, hence added to Route table
         dvs_route.check_asicdb_route_entries([ip_2+self.IPV6_MASK])
@@ -260,7 +271,7 @@ def create_and_test_fdb(self, appdb, asicdb, dvs, dvs_route):
 
         # ip_2 moved to active Mux, hence remove from Route table
         dvs_route.check_asicdb_deleted_route_entries([ip_2+self.IPV6_MASK])
-        self.check_neigh_in_asic_db(asicdb, ip_2, 4)
+        self.check_neigh_in_asic_db(asicdb, ip_2, 5)
 
         # Simulate FDB aging out test case
         ip_3 = "192.168.0.200"
@@ -783,7 +794,7 @@ def test_Peer(self, dvs, testlog, setup):
 
         self.create_and_test_peer(db, asicdb, "peer",  "1.1.1.1", "10.1.0.32", encap_tc_to_dscp_map_id, encap_tc_to_queue_map_id)
 
-    def test_Neighbor(self, dvs, dvs_route, testlog):
+    def test_Neighbor_aaa(self, dvs, dvs_route, testlog):
         """ test Neighbor entries and mux state change """
 
         confdb = dvs.get_config_db()