Skip to content

Commit

Permalink
[vnetorch] [vxlanorch] fix a set of memory usage issues (sonic-net#2352)
Browse files Browse the repository at this point in the history 
* [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 <[email protected]>
Signed-off-by: Longxiang Lyu <[email protected]>
  • Loading branch information
@Yakiv-Huryk @lolyu
Yakiv-Huryk authored and lolyu committed Jul 9, 2022
1 parent 1aaccd6 commit ca28572
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 13 deletions.
39 changes: 36 additions & 3 deletions orchagent/muxorch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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*>();
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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))
Expand All @@ -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());
}
Expand Down
5 changes: 4 additions & 1 deletion orchagent/muxorch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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_;
Expand All @@ -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 },
},
{ }
Expand Down
3 changes: 2 additions & 1 deletion orchagent/vnetorch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion orchagent/vxlanorch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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--;

Expand Down
25 changes: 18 additions & 7 deletions tests/test_mux.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 }
Expand Down Expand Up @@ -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")
Expand All @@ -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):
Expand All @@ -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])
Expand All @@ -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"
Expand Down Expand Up @@ -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()
Expand Down

0 comments on commit ca28572

Please sign in to comment.