From d5866a3dccfb3bc50853d740d54203b5cae61eed Mon Sep 17 00:00:00 2001 From: Ze Gan Date: Tue, 11 Jan 2022 17:17:53 +0800 Subject: [PATCH] [vslib]: fix create MACsec SA error (#986) * fix get_cipher_name from wrong attr variable Signed-off-by: Ze Gan * Add unittest Signed-off-by: Ze Gan * Re-create failed MACsec SAs Signed-off-by: Ze Gan --- tests/aspell.en.pws | 1 + unittest/vslib/Makefile.am | 3 +- unittest/vslib/TestMACsecAttr.cpp | 46 ++++++++ unittest/vslib/TestSwitchStateBaseMACsec.cpp | 110 +++++++++++++++++++ vslib/MACsecAttr.cpp | 21 +++- vslib/MACsecAttr.h | 9 ++ vslib/SwitchStateBase.h | 4 + vslib/SwitchStateBaseMACsec.cpp | 84 +++++++++++++- 8 files changed, 270 insertions(+), 8 deletions(-) create mode 100644 unittest/vslib/TestSwitchStateBaseMACsec.cpp diff --git a/tests/aspell.en.pws b/tests/aspell.en.pws index 93c1fef792fa..f0720a002056 100644 --- a/tests/aspell.en.pws +++ b/tests/aspell.en.pws @@ -449,3 +449,4 @@ zeromq zmq ZMQ ZMQ +uncreated diff --git a/unittest/vslib/Makefile.am b/unittest/vslib/Makefile.am index 87538dd4558b..42e4039a1ba0 100644 --- a/unittest/vslib/Makefile.am +++ b/unittest/vslib/Makefile.am @@ -40,7 +40,8 @@ tests_SOURCES = main.cpp \ TestSwitch.cpp \ TestSwitchMLNX2700.cpp \ TestSwitchBCM56850.cpp \ - TestSwitchBCM81724.cpp + TestSwitchBCM81724.cpp \ + TestSwitchStateBaseMACsec.cpp tests_CXXFLAGS = $(DBGFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS_COMMON) -fno-access-control tests_LDADD = $(LDADD_GTEST) $(top_srcdir)/vslib/libSaiVS.a -lhiredis -lswsscommon -lnl-genl-3 -lnl-nf-3 -lnl-route-3 -lnl-3 \ diff --git a/unittest/vslib/TestMACsecAttr.cpp b/unittest/vslib/TestMACsecAttr.cpp index 707e8fb42008..a83005a683a7 100644 --- a/unittest/vslib/TestMACsecAttr.cpp +++ b/unittest/vslib/TestMACsecAttr.cpp @@ -2,6 +2,8 @@ #include +#include + using namespace saivs; TEST(MACsecAttr, ctr) @@ -46,3 +48,47 @@ TEST(MACsecAttr, is_xpn) EXPECT_TRUE(attr.is_xpn()); } + +TEST(MACsecAttr, unordered_set) +{ + MACsecAttr attr1, attr2, attr3; + std::unordered_set attrSet; + + attr1.m_macsecName = "abc"; + attr2.m_macsecName = "abc"; + attr3.m_macsecName = "123"; + attrSet.insert(attr1); + + EXPECT_NE(attrSet.find(attr2), attrSet.end()); + EXPECT_EQ(attrSet.find(attr3), attrSet.end()); + + + attrSet.clear(); + attr1.m_sci = "0"; + attrSet.insert(attr1); + + EXPECT_EQ(attrSet.find(attr2), attrSet.end()); + + attr2.m_sci = "0"; + + EXPECT_NE(attrSet.find(attr2), attrSet.end()); + + + attrSet.clear(); + attr1.m_sci = "0"; + attr1.m_an = 0; + attrSet.insert(attr1); + + EXPECT_EQ(attrSet.find(attr2), attrSet.end()); + + attr2.m_an = 0; + + EXPECT_NE(attrSet.find(attr2), attrSet.end()); + + + attrSet.clear(); + attr1.m_authKey = "abc"; + attrSet.insert(attr1); + + EXPECT_NE(attrSet.find(attr2), attrSet.end()); +} diff --git a/unittest/vslib/TestSwitchStateBaseMACsec.cpp b/unittest/vslib/TestSwitchStateBaseMACsec.cpp new file mode 100644 index 000000000000..597a950f18ee --- /dev/null +++ b/unittest/vslib/TestSwitchStateBaseMACsec.cpp @@ -0,0 +1,110 @@ +#include "SwitchStateBase.h" +#include "MACsecAttr.h" + +#include + +#include + +using namespace saivs; + +TEST(SwitchStateBase, loadMACsecAttrFromMACsecSA) +{ + auto sc = std::make_shared(0, ""); + auto scc = std::make_shared(); + + SwitchStateBase ss( + 0x2100000000, + std::make_shared(0, scc), + sc); + + sai_attribute_t attr; + std::vector attrs; + MACsecAttr macsecAttr; + + attr.id = SAI_MACSEC_SC_ATTR_FLOW_ID; + attrs.push_back(attr); + attr.id = SAI_MACSEC_SC_ATTR_MACSEC_SCI; + attrs.push_back(attr); + attr.id = SAI_MACSEC_SC_ATTR_ENCRYPTION_ENABLE; + attrs.push_back(attr); + attr.id = SAI_MACSEC_SC_ATTR_MACSEC_CIPHER_SUITE; + attr.value.s32 = sai_macsec_cipher_suite_t::SAI_MACSEC_CIPHER_SUITE_GCM_AES_128; + attrs.push_back(attr); + EXPECT_EQ( + SAI_STATUS_SUCCESS, + ss.create_internal( + SAI_OBJECT_TYPE_MACSEC_SC, + "oid:0x0", + 0, + static_cast(attrs.size()), + attrs.data())); + + attr.id = SAI_MACSEC_SA_ATTR_SC_ID; + attr.value.oid = 0; + ss.loadMACsecAttrFromMACsecSA(0, 1 , &attr, macsecAttr); + + EXPECT_EQ(macsecAttr.m_cipher, MACsecAttr::CIPHER_NAME_GCM_AES_128); +} + +TEST(SwitchStateBase, retryCreateIngressMaCsecSAs) +{ + // Due to this function highly depends on system environment which cannot be tested directly, + // Just create this Test block for passing coverage + auto sc = std::make_shared(0, ""); + auto scc = std::make_shared(); + + SwitchStateBase ss( + 0x2100000000, + std::make_shared(0, scc), + sc); + + MACsecAttr macsecAttr; + + ss.m_uncreatedIngressMACsecSAs.insert(macsecAttr); + + ss.retryCreateIngressMaCsecSAs(); +} + +TEST(SwitchStateBase, removeMACsecPort) +{ + auto sc = std::make_shared(0, ""); + auto scc = std::make_shared(); + + SwitchStateBase ss( + 0x2100000000, + std::make_shared(0, scc), + sc); + + auto eq = std::make_shared(std::make_shared()); + + int s = socket(AF_INET, SOCK_DGRAM, 0); + int fd = socket(AF_INET, SOCK_DGRAM, 0); + auto hii = std::make_shared(0, s, fd, "tap", 0, eq); + ss.m_hostif_info_map["tap"] = hii; + + sai_attribute_t attr; + std::vector attrs; + attr.id = SAI_MACSEC_PORT_ATTR_MACSEC_DIRECTION; + attrs.push_back(attr); + attr.id = SAI_MACSEC_PORT_ATTR_PORT_ID; + attr.value.oid = 0; + attrs.push_back(attr); + ss.create_internal( + SAI_OBJECT_TYPE_MACSEC_PORT, + "oid:0x0", + 0, + static_cast(attrs.size()), + attrs.data()); + + ss.m_macsecFlowPortMap[0] = 0; + ss.m_macsecFlowPortMap[1] = 1; + + MACsecAttr macsecAttr; + ss.m_uncreatedIngressMACsecSAs.insert(macsecAttr); + macsecAttr.m_macsecName = "macsec_vtap"; + ss.m_uncreatedIngressMACsecSAs.insert(macsecAttr); + + EXPECT_EQ(SAI_STATUS_SUCCESS, ss.removeMACsecPort(0)); + EXPECT_EQ(1, ss.m_macsecFlowPortMap.size()); + EXPECT_EQ(1, ss.m_uncreatedIngressMACsecSAs.size()); +} diff --git a/vslib/MACsecAttr.cpp b/vslib/MACsecAttr.cpp index d1718b330d5e..0d4d9a23f337 100644 --- a/vslib/MACsecAttr.cpp +++ b/vslib/MACsecAttr.cpp @@ -3,6 +3,8 @@ #include "saimacsec.h" #include "swss/logger.h" +#include + using namespace saivs; const std::string MACsecAttr::CIPHER_NAME_INVALID = ""; @@ -17,7 +19,17 @@ const std::string MACsecAttr::CIPHER_NAME_GCM_AES_XPN_256 = "GCM-AES-XPN-256"; const std::string MACsecAttr::DEFAULT_CIPHER_NAME = MACsecAttr::CIPHER_NAME_GCM_AES_128; -MACsecAttr::MACsecAttr() +const macsec_an_t MACsecAttr::AN_INVALID = -1; + +size_t MACsecAttr::Hash::operator()(const MACsecAttr &attr) const +{ + SWSS_LOG_ENTER(); + + return std::hash()(attr.m_macsecName) ^ std::hash()(attr.m_sci) ^ attr.m_an; +} + +MACsecAttr::MACsecAttr() : m_an(AN_INVALID) + { SWSS_LOG_ENTER(); @@ -62,3 +74,10 @@ bool MACsecAttr::is_xpn() const return m_cipher == CIPHER_NAME_GCM_AES_XPN_128 || m_cipher == CIPHER_NAME_GCM_AES_XPN_256; } + +bool MACsecAttr::operator==(const MACsecAttr &attr) const +{ + SWSS_LOG_ENTER(); + + return m_macsecName == attr.m_macsecName && m_sci == attr.m_sci && m_an == attr.m_an; +} diff --git a/vslib/MACsecAttr.h b/vslib/MACsecAttr.h index 8b2cce5dba1a..37f6493c9b74 100644 --- a/vslib/MACsecAttr.h +++ b/vslib/MACsecAttr.h @@ -27,6 +27,13 @@ namespace saivs static const std::string DEFAULT_CIPHER_NAME; + static const macsec_an_t AN_INVALID; + + struct Hash + { + size_t operator()(const MACsecAttr &attr) const; + }; + // Explicitly declare constructor and destructor as non-inline functions // to avoid 'call is unlikely and code size would grow [-Werror=inline]' MACsecAttr(); @@ -37,6 +44,8 @@ namespace saivs bool is_xpn() const; + bool operator==(const MACsecAttr &attr) const; + std::string m_cipher; std::string m_vethName; std::string m_macsecName; diff --git a/vslib/SwitchStateBase.h b/vslib/SwitchStateBase.h index 4ca0c234523f..a258e2204707 100644 --- a/vslib/SwitchStateBase.h +++ b/vslib/SwitchStateBase.h @@ -591,10 +591,14 @@ namespace saivs _In_ sai_object_id_t macsec_sa_id, _Out_ sai_attribute_t &attr); + void retryCreateIngressMaCsecSAs(); + MACsecManager m_macsecManager; std::unordered_map m_macsecFlowPortMap; + std::unordered_set m_uncreatedIngressMACsecSAs; + protected: constexpr static const int maxDebugCounters = 32; diff --git a/vslib/SwitchStateBaseMACsec.cpp b/vslib/SwitchStateBaseMACsec.cpp index bb3913a35ee4..1f248996dddf 100644 --- a/vslib/SwitchStateBaseMACsec.cpp +++ b/vslib/SwitchStateBaseMACsec.cpp @@ -93,6 +93,10 @@ sai_status_t SwitchStateBase::setAclEntryMACsecFlowActive( static_cast(macsecAttr.m_an), macsecAttr.m_macsecName.c_str()); } + else + { + m_uncreatedIngressMACsecSAs.insert(macsecAttr); + } } } } @@ -199,6 +203,21 @@ sai_status_t SwitchStateBase::createMACsecSA( macsecAttr.m_sci.c_str(), static_cast(macsecAttr.m_an), macsecAttr.m_macsecName.c_str()); + + // Maybe there are some uncreated ingress SAs that were added into m_uncreatedIngressMACsecSAs + // because the corresponding egress SA has not been created. + // So retry to create them. + if (macsecAttr.m_direction == SAI_MACSEC_DIRECTION_EGRESS) + { + retryCreateIngressMaCsecSAs(); + } + } + else + { + // In Linux MACsec model, Egress SA need to be created before ingress SA. + // So, if try to create the ingress SA firstly, it will failed. + // But to create the egress SA should be always successful. + m_uncreatedIngressMACsecSAs.insert(macsecAttr); } } @@ -221,17 +240,31 @@ sai_status_t SwitchStateBase::removeMACsecPort( } } - auto itr = m_macsecFlowPortMap.begin(); + auto flowItr = m_macsecFlowPortMap.begin(); - while (itr != m_macsecFlowPortMap.end()) + while (flowItr != m_macsecFlowPortMap.end()) { - if (itr->second == macsecPortId) + if (flowItr->second == macsecPortId) { - itr = m_macsecFlowPortMap.erase(itr); + flowItr = m_macsecFlowPortMap.erase(flowItr); } else { - itr ++; + flowItr ++; + } + } + + auto saItr = m_uncreatedIngressMACsecSAs.begin(); + + while (saItr != m_uncreatedIngressMACsecSAs.end()) + { + if (saItr->m_macsecName == macsecAttr.m_macsecName) + { + saItr = m_uncreatedIngressMACsecSAs.erase(saItr); + } + else + { + saItr ++; } } @@ -257,6 +290,20 @@ sai_status_t SwitchStateBase::removeMACsecSC( } } + auto saItr = m_uncreatedIngressMACsecSAs.begin(); + + while (saItr != m_uncreatedIngressMACsecSAs.end()) + { + if (saItr->m_macsecName == macsecAttr.m_macsecName && saItr->m_sci == macsecAttr.m_sci) + { + saItr = m_uncreatedIngressMACsecSAs.erase(saItr); + } + else + { + saItr ++; + } + } + auto sid = sai_serialize_object_id(macsecScId); return remove_internal(SAI_OBJECT_TYPE_MACSEC_SC, sid); } @@ -550,7 +597,7 @@ sai_status_t SwitchStateBase::loadMACsecAttrFromMACsecSA( CHECK_STATUS(get(SAI_OBJECT_TYPE_MACSEC_SC, attr->value.oid, static_cast(attrs.size()), attrs.data())); - macsecAttr.m_cipher = MACsecAttr::get_cipher_name(attr->value.s32); + macsecAttr.m_cipher = MACsecAttr::get_cipher_name(attrs[3].value.s32); if (macsecAttr.m_cipher == MACsecAttr::CIPHER_NAME_INVALID) { @@ -841,3 +888,28 @@ sai_status_t SwitchStateBase::getMACsecSAPacketNumber( return SAI_STATUS_FAILURE; } + +void SwitchStateBase::retryCreateIngressMaCsecSAs() +{ + SWSS_LOG_ENTER(); + + auto itr = m_uncreatedIngressMACsecSAs.begin(); + + while (itr != m_uncreatedIngressMACsecSAs.end()) + { + if (m_macsecManager.create_macsec_sa(*itr)) + { + SWSS_LOG_NOTICE( + "Enable MACsec SA %s:%u at the device %s", + itr->m_sci.c_str(), + static_cast(itr->m_an), + itr->m_macsecName.c_str()); + + itr = m_uncreatedIngressMACsecSAs.erase(itr); + } + else + { + itr ++; + } + } +}