Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[vslib]Add MACsec Filters #713

Merged
merged 4 commits into from
Nov 20, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions vslib/inc/MACsecEgressFilter.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#pragma once

#include "MACsecFilter.h"

namespace saivs
{
class MACsecEgressFilter : public MACsecFilter
{
public:
MACsecEgressFilter(_In_ const std::string &macsecInterfaceName);

virtual ~MACsecEgressFilter() = default;

protected:
virtual FilterStatus forward(
_In_ const void *buffer,
_In_ ssize_t length) override;
};
}
36 changes: 36 additions & 0 deletions vslib/inc/MACsecFilter.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#pragma once

#include "TrafficFilter.h"

#include <string>

namespace saivs
{
class MACsecFilter : public TrafficFilter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

base class to new line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, please review it.

{
public:
MACsecFilter(
_In_ const std::string &macsec_interface_name);

virtual ~MACsecFilter() = default;

virtual FilterStatus execute(
_Inout_ void *buffer,
_Inout_ ssize_t &length) override;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why signed size ? and not size_t ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function will be call after recvmsg, So I directly use the type in this code
https://github.com/Azure/sonic-sairedis/blob/425ffbcbf57fdb66160c1ba84bbec2c0f1f5e983/vslib/src/HostInterfaceInfo.cpp#L145
Should I cast it to unsigned size?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, this is fine, i though this size is buffer length, since then it make no sense for it to allow negative numbers
so i looked at your code, so you actually passing &length, but forward function is without (&), and you are not modifying length parameter in any way, so it should also be without (&), then recvmsg returns ssize, and its negative when error, but you done return that ether, you have your own FilterStatus error code, so i think this ssize_t& should be change to size_t in all places and if needed cast to int in recvmgs method since thats the definition

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree your point. size_t should make more sense.
And the function execute, I think, is for processing the data, maybe it will modify the data. So the paramete of execute are with (&).
But the function forward, I think, is for forwarding data. The forwarding action will never change the data. So the parameters are without (&).


void enable_macsec_device(bool enable);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

each param needs prefix In and 1 per line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, please review it.


void set_macsec_fd(int macsecfd);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here 1 param per line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, please review it.


protected:
virtual FilterStatus forward(
_In_ const void *buffer,
_In_ ssize_t length) = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why ssize_t ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reason as previous comment.


bool m_macsec_device_enable;

int m_macsecfd;

const std::string m_macsec_interface_name;
};
}
19 changes: 19 additions & 0 deletions vslib/inc/MACsecIngressFilter.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#pragma once

#include "MACsecFilter.h"

namespace saivs
{
class MACsecIngressFilter : public MACsecFilter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

base class to new line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, please review it.

{
public:
MACsecIngressFilter(_In_ const std::string &macsec_interface_name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 param per line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, please review it.


virtual ~MACsecIngressFilter() = default;

protected:
virtual FilterStatus forward(
_In_ const void *buffer,
_In_ ssize_t length) override;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why ssize

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reason as previous comment.

};
}
32 changes: 32 additions & 0 deletions vslib/inc/TrafficFilter.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#pragma once

#include "swss/sal.h"

#include <sys/types.h>

namespace saivs
{
enum FilterPriority
{
MACSEC_FILTER,
};

class TrafficFilter
{
public:
enum FilterStatus
{
CONTINUE,
TERMINATE,
ERROR,
};

TrafficFilter() = default;

virtual ~TrafficFilter() = default;

virtual FilterStatus execute(
_Inout_ void *buffer,
_Inout_ ssize_t &length) = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why ssize

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reason as previous comment.

};
}
35 changes: 35 additions & 0 deletions vslib/inc/TrafficFilterPipes.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#pragma once

#include "TrafficFilter.h"

#include <memory>
#include <map>
#include <mutex>

namespace saivs
{
class TrafficFilterPipes
{
public:
TrafficFilterPipes() = default;

virtual ~TrafficFilterPipes() = default;

bool installFilter(
_In_ int priority,
_In_ std::shared_ptr<TrafficFilter> filter);

bool uninstallFilter(
_In_ std::shared_ptr<TrafficFilter> filter);

TrafficFilter::FilterStatus execute(
_Inout_ void *buffer,
_Inout_ ssize_t &length);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why ssize

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reason as previous comment.


private:
typedef std::map<int, std::shared_ptr<TrafficFilter> > FilterPriorityQueue;

std::mutex m_mutex;
FilterPriorityQueue m_filters;
};
}
48 changes: 48 additions & 0 deletions vslib/src/MACsecEgressFilter.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#include "MACsecEgressFilter.h"

#include <swss/logger.h>

#include <unistd.h>
#include <string.h>

using namespace saivs;

MACsecEgressFilter::MACsecEgressFilter(_In_ const std::string &macsecInterfaceName):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 param per line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, please review it.

MACsecFilter(macsecInterfaceName)
{
SWSS_LOG_ENTER();

// empty intentionally
}

TrafficFilter::FilterStatus MACsecEgressFilter::forward(
_In_ const void *buffer,
_In_ ssize_t length)
{
SWSS_LOG_ENTER();

if (write(m_macsecfd, buffer, length) < 0)
{
if (errno != ENETDOWN && errno != EIO)
{
SWSS_LOG_ERROR(
"failed to write to macsec device %s fd %d, errno(%d): %s",
m_macsec_interface_name.c_str(),
m_macsecfd,
errno,
strerror(errno));
}

if (errno == EBADF)
{
SWSS_LOG_ERROR(
"ending thread for macsec device %s fd %d",
m_macsec_interface_name.c_str(),
m_macsecfd);

return TrafficFilter::ERROR;
}
}

return TrafficFilter::TERMINATE;
}
58 changes: 58 additions & 0 deletions vslib/src/MACsecFilter.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
#include "MACsecFilter.h"

#include "swss/logger.h"
#include "swss/select.h"

#include <sys/socket.h>
#include <linux/if_packet.h>
#include <linux/if_ether.h>
#include <arpa/inet.h>
#include <net/if.h>

using namespace saivs;

#define EAPOL_ETHER_TYPE (0x888e)

MACsecFilter::MACsecFilter(
_In_ const std::string &macsec_interface_name):
m_macsec_device_enable(false),
m_macsecfd(0),
m_macsec_interface_name(macsec_interface_name)
{
SWSS_LOG_ENTER();

// empty intentionally
}

void MACsecFilter::enable_macsec_device(bool enable)
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing SWSS_LOG_ENTER()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, please review it.

m_macsec_device_enable = enable;
}

void MACsecFilter::set_macsec_fd(int macsecfd)
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing swss_log_enter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, please review it.

m_macsecfd = macsecfd;
}

TrafficFilter::FilterStatus MACsecFilter::execute(
_Inout_ void *buffer,
_Inout_ ssize_t &length)
{
SWSS_LOG_ENTER();

auto mac_hdr = static_cast<const ethhdr *>(buffer);

if (ntohs(mac_hdr->h_proto) == EAPOL_ETHER_TYPE)
{
// EAPOL traffic will never be delivered to MACsec device
return TrafficFilter::CONTINUE;
}

if (m_macsec_device_enable)
{
return forward(buffer, length);
}

// Drop all non-EAPOL packets if macsec device haven't been enable.
return TrafficFilter::TERMINATE;
}
30 changes: 30 additions & 0 deletions vslib/src/MACsecIngressFilter.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
#include "MACsecIngressFilter.h"

#include "swss/logger.h"

#include <unistd.h>
#include <string.h>

using namespace saivs;

MACsecIngressFilter::MACsecIngressFilter(
_In_ const std::string &macsec_interface_name) :
MACsecFilter(macsec_interface_name)
{
SWSS_LOG_ENTER();

// empty intentionally
}

TrafficFilter::FilterStatus MACsecIngressFilter::forward(
_In_ const void *buffer,
_In_ ssize_t length)
{
SWSS_LOG_ENTER();

// MACsec interface will automatically forward ingress MACsec traffic
// by Linux Kernel.
// So this filter just need to drop all ingress MACsec traffic directly

return TrafficFilter::TERMINATE;
}
76 changes: 76 additions & 0 deletions vslib/src/TrafficFilterPipes.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
#include "TrafficFilterPipes.h"

#include "swss/logger.h"

using namespace saivs;

bool TrafficFilterPipes::installFilter(
_In_ int priority,
_In_ std::shared_ptr<TrafficFilter> filter)
{
SWSS_LOG_ENTER();

std::unique_lock<std::mutex> guard(m_mutex);

return m_filters.emplace(priority, filter).second;
}

bool TrafficFilterPipes::uninstallFilter(
_In_ std::shared_ptr<TrafficFilter> filter)
{
SWSS_LOG_ENTER();

std::unique_lock<std::mutex> guard(m_mutex);

for (auto itr = m_filters.begin();
itr != m_filters.end();
itr ++)
{
if (itr->second == filter)
{
m_filters.erase(itr);

return true;
}
}

return false;
}

TrafficFilter::FilterStatus TrafficFilterPipes::execute(
_Inout_ void *buffer,
_Inout_ ssize_t &length)
{
SWSS_LOG_ENTER();

std::unique_lock<std::mutex> guard(m_mutex);
TrafficFilter::FilterStatus ret = TrafficFilter::CONTINUE;

for (auto itr = m_filters.begin(); itr != m_filters.end();)
{
auto filter = itr->second;

if (filter)
{
ret = filter->execute(
buffer,
length);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

join 3 lines to one line ? this is small enough

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, please review it.


if (ret == TrafficFilter::CONTINUE)
{
itr ++;
}
else
{
break;
}
}
else
{
itr = m_filters.erase(itr);
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, please review it.

}

return ret;
}