[MACsec]: Fix Bug: MACsec device will be terminated exceptionally if …

…the MACsec port was disabled in runtime (sonic-net#875) * Add MACsec filter state guard Signed-off-by: Ze Gan <ganze718@gmail.com> * Add volatile qualifer for state Signed-off-by: Ze Gan <ganze718@gmail.com> * Polish code format Signed-off-by: Ze Gan <ganze718@gmail.com> * Fix the macsec device was shutdown by ouutside Signed-off-by: Ze Gan <ganze718@gmail.com> * fix typo Signed-off-by: Ze Gan <ganze718@gmail.com> * Remove overkill qualifier Signed-off-by: Ze Gan <ganze718@gmail.com> * fix typo Signed-off-by: Ze Gan <ganze718@gmail.com> * Fix spell warning Signed-off-by: Ze Gan <ganze718@gmail.com> * Fix bug Signed-off-by: Ze Gan <ganze718@gmail.com> * Format code Signed-off-by: Ze Gan <ganze718@gmail.com> * Fix unittest Signed-off-by: Ze Gan <ganze718@gmail.com> * Add unittest for State guard Signed-off-by: Ze Gan <ganze718@gmail.com> * Update Makefile.am for testing MACsec Filter State Guard Signed-off-by: Ze Gan <ganze718@gmail.com>
Metaswitch · Aug 25, 2021 · 6ff9100 · 6ff9100
1 parent b91f75f
commit 6ff9100
Show file tree

Hide file tree

Showing 12 changed files with 161 additions and 26 deletions.
diff --git a/unittest/vslib/Makefile.am b/unittest/vslib/Makefile.am
@@ -22,6 +22,7 @@ tests_SOURCES = main.cpp \
 				TestMACsecEgressFilter.cpp \
 				TestMACsecForwarder.cpp \
 				TestMACsecIngressFilter.cpp \
+				TestMACsecFilterStateGuard.cpp \
 				TestNetMsgRegistrar.cpp \
 				TestRealObjectIdManager.cpp \
 				TestResourceLimiter.cpp \

diff --git a/unittest/vslib/TestMACsecEgressFilter.cpp b/unittest/vslib/TestMACsecEgressFilter.cpp
@@ -47,5 +47,5 @@ TEST(MACsecEgressFilter, forward)
 
     filter.set_macsec_fd(70); // bad fd
 
-    EXPECT_EQ(filter.execute(packet, len), TrafficFilter::ERROR);
+    EXPECT_EQ(filter.execute(packet, len), TrafficFilter::TERMINATE);
 }
diff --git a/unittest/vslib/TestMACsecFilterStateGuard.cpp b/unittest/vslib/TestMACsecFilterStateGuard.cpp
@@ -0,0 +1,18 @@
+#include "MACsecFilterStateGuard.h"
+
+#include <gtest/gtest.h>
+
+using namespace saivs;
+
+TEST(MACsecFilterStateGuard, ctr)
+{
+    MACsecFilter::MACsecFilterState state = MACsecFilter::MACsecFilterState::MACSEC_FILTER_STATE_IDLE;
+
+    {
+        MACsecFilterStateGuard guard(state, MACsecFilter::MACsecFilterState::MACSEC_FILTER_STATE_BUSY);
+
+        EXPECT_EQ(state, MACsecFilter::MACsecFilterState::MACSEC_FILTER_STATE_BUSY);
+    }
+
+    EXPECT_EQ(state, MACsecFilter::MACsecFilterState::MACSEC_FILTER_STATE_IDLE);
+}
diff --git a/vslib/MACsecEgressFilter.cpp b/vslib/MACsecEgressFilter.cpp
@@ -26,7 +26,7 @@ TrafficFilter::FilterStatus MACsecEgressFilter::forward(
     {
         if (errno != ENETDOWN && errno != EIO)
         {
-            SWSS_LOG_ERROR(
+            SWSS_LOG_WARN(
                     "failed to write to macsec device %s fd %d, errno(%d): %s",
                     m_macsecInterfaceName.c_str(),
                     m_macsecfd,
@@ -36,12 +36,13 @@ TrafficFilter::FilterStatus MACsecEgressFilter::forward(
 
         if (errno == EBADF)
         {
-            SWSS_LOG_ERROR(
+            // If the MACsec device was deleted by outside,
+            // this action should not terminate the Main tap thread.
+            // So just report a warning.
+            SWSS_LOG_WARN(
                     "ending thread for macsec device %s fd %d",
                     m_macsecInterfaceName.c_str(),
                     m_macsecfd);
-
-            return TrafficFilter::ERROR;
         }
     }
 

diff --git a/vslib/MACsecFilter.cpp b/vslib/MACsecFilter.cpp
@@ -1,4 +1,5 @@
 #include "MACsecFilter.h"
+#include "MACsecFilterStateGuard.h"
 
 #include "swss/logger.h"
 #include "swss/select.h"
@@ -17,7 +18,8 @@ MACsecFilter::MACsecFilter(
         _In_ const std::string &macsecInterfaceName):
     m_macsecDeviceEnable(false),
     m_macsecfd(0),
-    m_macsecInterfaceName(macsecInterfaceName)
+    m_macsecInterfaceName(macsecInterfaceName),
+    m_state(MACsecFilter::MACsecFilterState::MACSEC_FILTER_STATE_IDLE)
 {
     SWSS_LOG_ENTER();
 
@@ -30,6 +32,17 @@ void MACsecFilter::enable_macsec_device(
     SWSS_LOG_ENTER();
 
     m_macsecDeviceEnable = enable;
+
+    // The function, execute(), may be running in another thread,
+    // the macsec device enable state cannot be changed in the busy state.
+    // Because if the macsec device was deleted in the busy state,
+    // the error value of function, forward, will be returned
+    // to the caller of execute()
+    // and the caller thread will exit due to the error return value.
+    while(get_state() == MACsecFilter::MACsecFilterState::MACSEC_FILTER_STATE_BUSY)
+    {
+        // Waiting the MACsec filter exit from the BUSY state
+    }
 }
 
 void MACsecFilter::set_macsec_fd(
@@ -40,12 +53,20 @@ void MACsecFilter::set_macsec_fd(
     m_macsecfd = macsecfd;
 }
 
+MACsecFilter::MACsecFilterState MACsecFilter::get_state() const
+{
+    SWSS_LOG_ENTER();
+
+    return m_state;
+}
+
 TrafficFilter::FilterStatus MACsecFilter::execute(
         _Inout_ void *buffer,
         _Inout_ size_t &length)
 {
     SWSS_LOG_ENTER();
 
+    MACsecFilterStateGuard state_guard(m_state, MACsecFilter::MACsecFilterState::MACSEC_FILTER_STATE_BUSY);
     auto mac_hdr = static_cast<const ethhdr *>(buffer);
 
     if (ntohs(mac_hdr->h_proto) == EAPOL_ETHER_TYPE)

diff --git a/vslib/MACsecFilter.h b/vslib/MACsecFilter.h
@@ -11,6 +11,14 @@ namespace saivs
     {
         public:
 
+            typedef enum _MACsecFilterState
+            {
+                MACSEC_FILTER_STATE_IDLE,
+
+                MACSEC_FILTER_STATE_BUSY,
+
+            } MACsecFilterState;
+
             MACsecFilter(
                     _In_ const std::string &macsecInterfaceName);
 
@@ -26,16 +34,20 @@ namespace saivs
             void set_macsec_fd(
                     _In_ int macsecfd);
 
+            MACsecFilterState get_state() const;
+
         protected:
 
             virtual FilterStatus forward(
                     _In_ const void *buffer,
                     _In_ size_t length) = 0;
 
-            bool m_macsecDeviceEnable;
+            volatile bool m_macsecDeviceEnable;
 
             int m_macsecfd;
 
             const std::string m_macsecInterfaceName;
+
+            MACsecFilterState m_state;
     };
 }
diff --git a/vslib/MACsecFilterStateGuard.cpp b/vslib/MACsecFilterStateGuard.cpp
@@ -0,0 +1,23 @@
+#include "MACsecFilterStateGuard.h"
+
+#include <swss/logger.h>
+
+using namespace saivs;
+
+MACsecFilterStateGuard::MACsecFilterStateGuard(
+    _Inout_ MACsecFilter::MACsecFilterState &guarded_state,
+    _In_ MACsecFilter::MACsecFilterState target_state):
+    m_guarded_state(guarded_state)
+{
+    SWSS_LOG_ENTER();
+
+    m_old_state = m_guarded_state;
+    m_guarded_state = target_state;
+}
+
+MACsecFilterStateGuard::~MACsecFilterStateGuard()
+{
+    SWSS_LOG_ENTER();
+
+    m_guarded_state = m_old_state;
+}
diff --git a/vslib/MACsecFilterStateGuard.h b/vslib/MACsecFilterStateGuard.h
@@ -0,0 +1,22 @@
+#pragma once
+
+#include "MACsecFilter.h"
+
+namespace saivs
+{
+    class MACsecFilterStateGuard
+    {
+        public:
+
+            MACsecFilterStateGuard(
+                    _Inout_ MACsecFilter::MACsecFilterState &guarded_state,
+                    _In_ MACsecFilter::MACsecFilterState target_state);
+
+            ~MACsecFilterStateGuard();
+
+        private:
+
+            MACsecFilter::MACsecFilterState m_old_state;
+            MACsecFilter::MACsecFilterState &m_guarded_state;
+    };
+}
diff --git a/vslib/MACsecManager.cpp b/vslib/MACsecManager.cpp
@@ -253,11 +253,6 @@ bool MACsecManager::delete_macsec_sa(
         }
     }
 
-    // The SA is the last one in this MACsec SC, delete the MACsec SC
-    if (get_macsec_sa_count(attr.m_macsecName, attr.m_direction, attr.m_sci) == 0)
-    {
-        return delete_macsec_sc(attr);
-    }
 
     return true;
 }

diff --git a/vslib/Makefile.am b/vslib/Makefile.am
@@ -23,6 +23,7 @@ libSaiVS_a_SOURCES = \
 					  LaneMap.cpp \
 					  LaneMapFileParser.cpp \
 					  MACsecAttr.cpp \
+					  MACsecFilterStateGuard.cpp \
 					  MACsecEgressFilter.cpp \
 					  MACsecFilter.cpp \
 					  MACsecForwarder.cpp \

diff --git a/vslib/SwitchStateBase.h b/vslib/SwitchStateBase.h
@@ -584,6 +584,8 @@ namespace saivs
 
             MACsecManager m_macsecManager;
 
+            std::unordered_map<sai_object_id_t, sai_object_id_t> m_macsecFlowPortMap;
+
         protected:
 
             constexpr static const int maxDebugCounters = 32;

diff --git a/vslib/SwitchStateBaseMACsec.cpp b/vslib/SwitchStateBaseMACsec.cpp
@@ -65,15 +65,34 @@ sai_status_t SwitchStateBase::setAclEntryMACsecFlowActive(
 
         if (loadMACsecAttrsFromACLEntry(entryId, attr, SAI_OBJECT_TYPE_MACSEC_SA, macsecAttrs) == SAI_STATUS_SUCCESS)
         {
+            // In Linux MACsec model, Egress SA need to be created before ingress SA
             for (auto &macsecAttr : macsecAttrs)
             {
-                if (m_macsecManager.create_macsec_sa(macsecAttr))
+                if (macsecAttr.m_direction == SAI_MACSEC_DIRECTION_EGRESS)
                 {
-                    SWSS_LOG_NOTICE(
+                    if (m_macsecManager.create_macsec_sa(macsecAttr))
+                    {
+                        SWSS_LOG_NOTICE(
                             "Enable MACsec SA %s:%u at the device %s",
                             macsecAttr.m_sci.c_str(),
                             static_cast<std::uint32_t>(macsecAttr.m_an),
                             macsecAttr.m_macsecName.c_str());
+                    }
+                }
+            }
+
+            for (auto &macsecAttr : macsecAttrs)
+            {
+                if (macsecAttr.m_direction == SAI_MACSEC_DIRECTION_INGRESS)
+                {
+                    if (m_macsecManager.create_macsec_sa(macsecAttr))
+                    {
+                        SWSS_LOG_NOTICE(
+                            "Enable MACsec SA %s:%u at the device %s",
+                            macsecAttr.m_sci.c_str(),
+                            static_cast<std::uint32_t>(macsecAttr.m_an),
+                            macsecAttr.m_macsecName.c_str());
+                    }
                 }
             }
         }
@@ -91,20 +110,15 @@ sai_status_t SwitchStateBase::setAclEntryMACsecFlowActive(
 
         if (loadMACsecAttrsFromACLEntry(entryId, attr, SAI_OBJECT_TYPE_MACSEC_SC, macsecAttrs) == SAI_STATUS_SUCCESS)
         {
-            if (!macsecAttrs.empty())
+            for (auto &macsecAttr : macsecAttrs)
             {
-                if (macsecAttrs.size() > 1)
-                {
-                    SWSS_LOG_ERROR("Duplicated MACsec sc for the ACL entry %s", sai_serialize_object_id(entryId).c_str());
-
-                    CHECK_STATUS(set_internal(SAI_OBJECT_TYPE_ACL_ENTRY, sai_serialize_object_id(entryId), attr));
-
-                    return SAI_STATUS_FAILURE;
-                }
-
-                if (m_macsecManager.delete_macsec_sc(macsecAttrs.back()))
+                if (m_macsecManager.delete_macsec_sa(macsecAttr))
                 {
-                    SWSS_LOG_NOTICE("The MACsec port %s is created.", macsecAttrs.back().m_macsecName.c_str());
+                    SWSS_LOG_NOTICE(
+                        "Delete MACsec SA %s:%u at the device %s",
+                        macsecAttr.m_sci.c_str(),
+                        static_cast<std::uint32_t>(macsecAttr.m_an),
+                        macsecAttr.m_macsecName.c_str());
                 }
             }
         }
@@ -207,6 +221,20 @@ sai_status_t SwitchStateBase::removeMACsecPort(
         }
     }
 
+    auto itr = m_macsecFlowPortMap.begin();
+
+    while (itr != m_macsecFlowPortMap.end())
+    {
+        if (itr->second == macsecPortId)
+        {
+            itr = m_macsecFlowPortMap.erase(itr);
+        }
+        else
+        {
+            itr ++;
+        }
+    }
+
     auto sid = sai_serialize_object_id(macsecPortId);
     return remove_internal(SAI_OBJECT_TYPE_MACSEC_PORT, sid);
 }
@@ -279,6 +307,15 @@ sai_status_t SwitchStateBase::findPortByMACsecFlow(
 {
     SWSS_LOG_ENTER();
 
+    auto itr = m_macsecFlowPortMap.find(macsecFlowId);
+
+    if (itr != m_macsecFlowPortMap.end())
+    {
+        portId = itr->second;
+
+        return SAI_STATUS_SUCCESS;
+    }
+
     sai_attribute_t attr;
 
     // MACsec flow => ACL entry => ACL table
@@ -359,6 +396,8 @@ sai_status_t SwitchStateBase::findPortByMACsecFlow(
 
     portId = port_ids.front();
 
+    m_macsecFlowPortMap[macsecFlowId] = portId;
+
     return SAI_STATUS_SUCCESS;
 }