From 6ff9100856304084ea33d9e314c2d85bd50dfafe Mon Sep 17 00:00:00 2001 From: Ze Gan Date: Wed, 25 Aug 2021 23:45:07 +0800 Subject: [PATCH] [MACsec]: Fix Bug: MACsec device will be terminated exceptionally if the MACsec port was disabled in runtime (#875) * Add MACsec filter state guard Signed-off-by: Ze Gan * Add volatile qualifer for state Signed-off-by: Ze Gan * Polish code format Signed-off-by: Ze Gan * Fix the macsec device was shutdown by ouutside Signed-off-by: Ze Gan * fix typo Signed-off-by: Ze Gan * Remove overkill qualifier Signed-off-by: Ze Gan * fix typo Signed-off-by: Ze Gan * Fix spell warning Signed-off-by: Ze Gan * Fix bug Signed-off-by: Ze Gan * Format code Signed-off-by: Ze Gan * Fix unittest Signed-off-by: Ze Gan * Add unittest for State guard Signed-off-by: Ze Gan * Update Makefile.am for testing MACsec Filter State Guard Signed-off-by: Ze Gan --- unittest/vslib/Makefile.am | 1 + unittest/vslib/TestMACsecEgressFilter.cpp | 2 +- unittest/vslib/TestMACsecFilterStateGuard.cpp | 18 +++++ vslib/MACsecEgressFilter.cpp | 9 +-- vslib/MACsecFilter.cpp | 23 ++++++- vslib/MACsecFilter.h | 14 +++- vslib/MACsecFilterStateGuard.cpp | 23 +++++++ vslib/MACsecFilterStateGuard.h | 22 ++++++ vslib/MACsecManager.cpp | 5 -- vslib/Makefile.am | 1 + vslib/SwitchStateBase.h | 2 + vslib/SwitchStateBaseMACsec.cpp | 67 +++++++++++++++---- 12 files changed, 161 insertions(+), 26 deletions(-) create mode 100644 unittest/vslib/TestMACsecFilterStateGuard.cpp create mode 100644 vslib/MACsecFilterStateGuard.cpp create mode 100644 vslib/MACsecFilterStateGuard.h diff --git a/unittest/vslib/Makefile.am b/unittest/vslib/Makefile.am index 9ac3b1fc2a7e..7139edab63e7 100644 --- 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 index 377b02538f33..e93656d7f245 100644 --- 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 new file mode 100644 index 000000000000..4bb9bfcd391f --- /dev/null +++ b/unittest/vslib/TestMACsecFilterStateGuard.cpp @@ -0,0 +1,18 @@ +#include "MACsecFilterStateGuard.h" + +#include + +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 index fefa1c4ad73c..4e4284d3c21a 100644 --- 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 index 77ed84df5d02..6c7e6e316fcc 100644 --- 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(buffer); if (ntohs(mac_hdr->h_proto) == EAPOL_ETHER_TYPE) diff --git a/vslib/MACsecFilter.h b/vslib/MACsecFilter.h index 165028e00893..cd75684c81dd 100644 --- 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 new file mode 100644 index 000000000000..f7d4b09420b0 --- /dev/null +++ b/vslib/MACsecFilterStateGuard.cpp @@ -0,0 +1,23 @@ +#include "MACsecFilterStateGuard.h" + +#include + +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 new file mode 100644 index 000000000000..c6e1007f0d7b --- /dev/null +++ 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 index fc8621abf743..8b8930d9cfcb 100644 --- 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 index 73fbf2d1e3ed..7b1451cf8a65 100644 --- 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 index ce531d6f22a6..d870deaabd84 100644 --- a/vslib/SwitchStateBase.h +++ b/vslib/SwitchStateBase.h @@ -584,6 +584,8 @@ namespace saivs MACsecManager m_macsecManager; + std::unordered_map m_macsecFlowPortMap; + protected: constexpr static const int maxDebugCounters = 32; diff --git a/vslib/SwitchStateBaseMACsec.cpp b/vslib/SwitchStateBaseMACsec.cpp index a73b51160026..af8d64436a60 100644 --- 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(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(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(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; }