Skip to content

Commit 6ff9100

Browse files
PterosaurPterosaur
authored
[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 <[email protected]> * Add volatile qualifer for state Signed-off-by: Ze Gan <[email protected]> * Polish code format Signed-off-by: Ze Gan <[email protected]> * Fix the macsec device was shutdown by ouutside Signed-off-by: Ze Gan <[email protected]> * fix typo Signed-off-by: Ze Gan <[email protected]> * Remove overkill qualifier Signed-off-by: Ze Gan <[email protected]> * fix typo Signed-off-by: Ze Gan <[email protected]> * Fix spell warning Signed-off-by: Ze Gan <[email protected]> * Fix bug Signed-off-by: Ze Gan <[email protected]> * Format code Signed-off-by: Ze Gan <[email protected]> * Fix unittest Signed-off-by: Ze Gan <[email protected]> * Add unittest for State guard Signed-off-by: Ze Gan <[email protected]> * Update Makefile.am for testing MACsec Filter State Guard Signed-off-by: Ze Gan <[email protected]>
1 parent b91f75f commit 6ff9100

12 files changed

+161
-26
lines changed

‎unittest/vslib/Makefile.am‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ tests_SOURCES = main.cpp \
2222
TestMACsecEgressFilter.cpp \
2323
TestMACsecForwarder.cpp \
2424
TestMACsecIngressFilter.cpp \
25+
TestMACsecFilterStateGuard.cpp \
2526
TestNetMsgRegistrar.cpp \
2627
TestRealObjectIdManager.cpp \
2728
TestResourceLimiter.cpp \

‎unittest/vslib/TestMACsecEgressFilter.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,5 +47,5 @@ TEST(MACsecEgressFilter, forward)
4747

4848
filter.set_macsec_fd(70); // bad fd
4949

50-
EXPECT_EQ(filter.execute(packet, len), TrafficFilter::ERROR);
50+
EXPECT_EQ(filter.execute(packet, len), TrafficFilter::TERMINATE);
5151
}

‎unittest/vslib/TestMACsecFilterStateGuard.cpp‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
#include "MACsecFilterStateGuard.h"
2+
3+
#include <gtest/gtest.h>
4+
5+
using namespace saivs;
6+
7+
TEST(MACsecFilterStateGuard, ctr)
8+
{
9+
MACsecFilter::MACsecFilterState state = MACsecFilter::MACsecFilterState::MACSEC_FILTER_STATE_IDLE;
10+
11+
{
12+
MACsecFilterStateGuard guard(state, MACsecFilter::MACsecFilterState::MACSEC_FILTER_STATE_BUSY);
13+
14+
EXPECT_EQ(state, MACsecFilter::MACsecFilterState::MACSEC_FILTER_STATE_BUSY);
15+
}
16+
17+
EXPECT_EQ(state, MACsecFilter::MACsecFilterState::MACSEC_FILTER_STATE_IDLE);
18+
}

‎vslib/MACsecEgressFilter.cpp‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ TrafficFilter::FilterStatus MACsecEgressFilter::forward(
2626
{
2727
if (errno != ENETDOWN && errno != EIO)
2828
{
29-
SWSS_LOG_ERROR(
29+
SWSS_LOG_WARN(
3030
"failed to write to macsec device %s fd %d, errno(%d): %s",
3131
m_macsecInterfaceName.c_str(),
3232
m_macsecfd,
@@ -36,12 +36,13 @@ TrafficFilter::FilterStatus MACsecEgressFilter::forward(
3636

3737
if (errno == EBADF)
3838
{
39-
SWSS_LOG_ERROR(
39+
// If the MACsec device was deleted by outside,
40+
// this action should not terminate the Main tap thread.
41+
// So just report a warning.
42+
SWSS_LOG_WARN(
4043
"ending thread for macsec device %s fd %d",
4144
m_macsecInterfaceName.c_str(),
4245
m_macsecfd);
43-
44-
return TrafficFilter::ERROR;
4546
}
4647
}
4748

‎vslib/MACsecFilter.cpp‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#include "MACsecFilter.h"
2+
#include "MACsecFilterStateGuard.h"
23

34
#include "swss/logger.h"
45
#include "swss/select.h"
@@ -17,7 +18,8 @@ MACsecFilter::MACsecFilter(
1718
_In_ const std::string &macsecInterfaceName):
1819
m_macsecDeviceEnable(false),
1920
m_macsecfd(0),
20-
m_macsecInterfaceName(macsecInterfaceName)
21+
m_macsecInterfaceName(macsecInterfaceName),
22+
m_state(MACsecFilter::MACsecFilterState::MACSEC_FILTER_STATE_IDLE)
2123
{
2224
SWSS_LOG_ENTER();
2325

@@ -30,6 +32,17 @@ void MACsecFilter::enable_macsec_device(
3032
SWSS_LOG_ENTER();
3133

3234
m_macsecDeviceEnable = enable;
35+
36+
// The function, execute(), may be running in another thread,
37+
// the macsec device enable state cannot be changed in the busy state.
38+
// Because if the macsec device was deleted in the busy state,
39+
// the error value of function, forward, will be returned
40+
// to the caller of execute()
41+
// and the caller thread will exit due to the error return value.
42+
while(get_state() == MACsecFilter::MACsecFilterState::MACSEC_FILTER_STATE_BUSY)
43+
{
44+
// Waiting the MACsec filter exit from the BUSY state
45+
}
3346
}
3447

3548
void MACsecFilter::set_macsec_fd(
@@ -40,12 +53,20 @@ void MACsecFilter::set_macsec_fd(
4053
m_macsecfd = macsecfd;
4154
}
4255

56+
MACsecFilter::MACsecFilterState MACsecFilter::get_state() const
57+
{
58+
SWSS_LOG_ENTER();
59+
60+
return m_state;
61+
}
62+
4363
TrafficFilter::FilterStatus MACsecFilter::execute(
4464
_Inout_ void *buffer,
4565
_Inout_ size_t &length)
4666
{
4767
SWSS_LOG_ENTER();
4868

69+
MACsecFilterStateGuard state_guard(m_state, MACsecFilter::MACsecFilterState::MACSEC_FILTER_STATE_BUSY);
4970
auto mac_hdr = static_cast<const ethhdr *>(buffer);
5071

5172
if (ntohs(mac_hdr->h_proto) == EAPOL_ETHER_TYPE)

‎vslib/MACsecFilter.h‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,14 @@ namespace saivs
1111
{
1212
public:
1313

14+
typedef enum _MACsecFilterState
15+
{
16+
MACSEC_FILTER_STATE_IDLE,
17+
18+
MACSEC_FILTER_STATE_BUSY,
19+
20+
} MACsecFilterState;
21+
1422
MACsecFilter(
1523
_In_ const std::string &macsecInterfaceName);
1624

@@ -26,16 +34,20 @@ namespace saivs
2634
void set_macsec_fd(
2735
_In_ int macsecfd);
2836

37+
MACsecFilterState get_state() const;
38+
2939
protected:
3040

3141
virtual FilterStatus forward(
3242
_In_ const void *buffer,
3343
_In_ size_t length) = 0;
3444

35-
bool m_macsecDeviceEnable;
45+
volatile bool m_macsecDeviceEnable;
3646

3747
int m_macsecfd;
3848

3949
const std::string m_macsecInterfaceName;
50+
51+
MACsecFilterState m_state;
4052
};
4153
}

‎vslib/MACsecFilterStateGuard.cpp‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
#include "MACsecFilterStateGuard.h"
2+
3+
#include <swss/logger.h>
4+
5+
using namespace saivs;
6+
7+
MACsecFilterStateGuard::MACsecFilterStateGuard(
8+
_Inout_ MACsecFilter::MACsecFilterState &guarded_state,
9+
_In_ MACsecFilter::MACsecFilterState target_state):
10+
m_guarded_state(guarded_state)
11+
{
12+
SWSS_LOG_ENTER();
13+
14+
m_old_state = m_guarded_state;
15+
m_guarded_state = target_state;
16+
}
17+
18+
MACsecFilterStateGuard::~MACsecFilterStateGuard()
19+
{
20+
SWSS_LOG_ENTER();
21+
22+
m_guarded_state = m_old_state;
23+
}

‎vslib/MACsecFilterStateGuard.h‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
#pragma once
2+
3+
#include "MACsecFilter.h"
4+
5+
namespace saivs
6+
{
7+
class MACsecFilterStateGuard
8+
{
9+
public:
10+
11+
MACsecFilterStateGuard(
12+
_Inout_ MACsecFilter::MACsecFilterState &guarded_state,
13+
_In_ MACsecFilter::MACsecFilterState target_state);
14+
15+
~MACsecFilterStateGuard();
16+
17+
private:
18+
19+
MACsecFilter::MACsecFilterState m_old_state;
20+
MACsecFilter::MACsecFilterState &m_guarded_state;
21+
};
22+
}

‎vslib/MACsecManager.cpp‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -253,11 +253,6 @@ bool MACsecManager::delete_macsec_sa(
253253
}
254254
}
255255

256-
// The SA is the last one in this MACsec SC, delete the MACsec SC
257-
if (get_macsec_sa_count(attr.m_macsecName, attr.m_direction, attr.m_sci) == 0)
258-
{
259-
return delete_macsec_sc(attr);
260-
}
261256

262257
return true;
263258
}

‎vslib/Makefile.am‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ libSaiVS_a_SOURCES = \
2323
LaneMap.cpp \
2424
LaneMapFileParser.cpp \
2525
MACsecAttr.cpp \
26+
MACsecFilterStateGuard.cpp \
2627
MACsecEgressFilter.cpp \
2728
MACsecFilter.cpp \
2829
MACsecForwarder.cpp \

0 commit comments

Comments
 (0)