[vslib]: fix create MACsec SA error (sonic-net#986)

* fix get_cipher_name from wrong attr variable Signed-off-by: Ze Gan <[email protected]> * Add unittest Signed-off-by: Ze Gan <[email protected]> * Re-create failed MACsec SAs Signed-off-by: Ze Gan <[email protected]>
abdosi · Jan 11, 2022 · d5866a3 · d5866a3
1 parent f36f7ce
commit d5866a3
Show file tree

Hide file tree

Showing 8 changed files with 270 additions and 8 deletions.
diff --git 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
@@ -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
@@ -2,6 +2,8 @@
 
 #include <gtest/gtest.h>
 
+#include <unordered_set>
+
 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<MACsecAttr, MACsecAttr::Hash> 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
@@ -0,0 +1,110 @@
+#include "SwitchStateBase.h"
+#include "MACsecAttr.h"
+
+#include <gtest/gtest.h>
+
+#include <vector>
+
+using namespace saivs;
+
+TEST(SwitchStateBase, loadMACsecAttrFromMACsecSA)
+{
+    auto sc = std::make_shared<SwitchConfig>(0, "");
+    auto scc = std::make_shared<SwitchConfigContainer>();
+
+    SwitchStateBase ss(
+            0x2100000000,
+            std::make_shared<RealObjectIdManager>(0, scc),
+            sc);
+
+    sai_attribute_t attr;
+    std::vector<sai_attribute_t> 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<uint32_t>(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<SwitchConfig>(0, "");
+    auto scc = std::make_shared<SwitchConfigContainer>();
+
+    SwitchStateBase ss(
+            0x2100000000,
+            std::make_shared<RealObjectIdManager>(0, scc),
+            sc);
+
+    MACsecAttr macsecAttr;
+
+    ss.m_uncreatedIngressMACsecSAs.insert(macsecAttr);
+
+    ss.retryCreateIngressMaCsecSAs();
+}
+
+TEST(SwitchStateBase, removeMACsecPort)
+{
+    auto sc = std::make_shared<SwitchConfig>(0, "");
+    auto scc = std::make_shared<SwitchConfigContainer>();
+
+    SwitchStateBase ss(
+            0x2100000000,
+            std::make_shared<RealObjectIdManager>(0, scc),
+            sc);
+
+    auto eq = std::make_shared<EventQueue>(std::make_shared<Signal>());
+
+    int s = socket(AF_INET, SOCK_DGRAM, 0);
+    int fd = socket(AF_INET, SOCK_DGRAM, 0);
+    auto hii = std::make_shared<HostInterfaceInfo>(0, s, fd, "tap", 0, eq);
+    ss.m_hostif_info_map["tap"] = hii;
+
+    sai_attribute_t attr;
+    std::vector<sai_attribute_t> 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<uint32_t>(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
@@ -3,6 +3,8 @@
 #include "saimacsec.h"
 #include "swss/logger.h"
 
+#include <functional>
+
 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<std::string>()(attr.m_macsecName) ^ std::hash<std::string>()(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
@@ -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
@@ -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<sai_object_id_t, sai_object_id_t> m_macsecFlowPortMap;
 
+            std::unordered_set<MACsecAttr, MACsecAttr::Hash> m_uncreatedIngressMACsecSAs;
+
         protected:
 
             constexpr static const int maxDebugCounters = 32;

diff --git a/vslib/SwitchStateBaseMACsec.cpp b/vslib/SwitchStateBaseMACsec.cpp
@@ -93,6 +93,10 @@ sai_status_t SwitchStateBase::setAclEntryMACsecFlowActive(
                             static_cast<std::uint32_t>(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<std::uint32_t>(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<uint32_t>(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<std::uint32_t>(itr->m_an),
+                itr->m_macsecName.c_str());
+
+            itr = m_uncreatedIngressMACsecSAs.erase(itr);
+        }
+        else
+        {
+            itr ++;
+        }
+    }
+}