-
Notifications
You must be signed in to change notification settings - Fork 303
[vslib]Add MACsec forward and filters to HostInterfaceInfo #719
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
Changes from 1 commit
8b4cb33
fe58aaa
8b7f3f8
38be8ab
f98468c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,13 +15,19 @@ | |
using namespace saivs; | ||
|
||
#define ETH_FRAME_BUFFER_SIZE (0x4000) | ||
#define CONTROL_MESSAGE_BUFFER_SIZE (0x1000) | ||
#define IEEE_8021Q_ETHER_TYPE (0x8100) | ||
#define MAC_ADDRESS_SIZE (6) | ||
#define VLAN_TAG_SIZE (4) | ||
|
||
MACsecForwarder::MACsecForwarder( | ||
_In_ const std::string &macsecInterfaceName, | ||
_In_ int tapfd): | ||
_In_ int tapfd, | ||
_In_ std::shared_ptr<HostInterfaceInfo> info): | ||
m_tapfd(tapfd), | ||
m_macsecInterfaceName(macsecInterfaceName), | ||
m_runThread(true) | ||
m_runThread(true), | ||
m_info(info) | ||
{ | ||
SWSS_LOG_ENTER(); | ||
|
||
|
@@ -112,6 +118,25 @@ void MACsecForwarder::forward() | |
|
||
while (m_runThread) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why you need separate thread in macsec ? there is already thread thats deals with packet processing, why not tap into that ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we already have 2 threads per port that transfer the data, and you are having some another thread for the path that is already processing data, this seems a waist of resources There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I remember you asked this question before, sorry that I did not explain it clear. Meanwhile, all encrypted data on the thread 2 will be dropped by the macsec ingress filter. Because linux kernel will help us to forward the encrypted data. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so mu understanding here is now like this: we have regular traffic, and encrypted traffic coming to the same interface then based on something (dont know what it is) you can distinguish which one is which, and for encrypted traffic you are sending this traffic to separate created socket that will decrypt this traffic inside kernel and then you can read that socket (or other one?) to get planetext traffic, process it, block or forward, maybe generate fdb notification, and then forward back encrypted traffic (re-encrypted if some filters are applied that modify pakcket) to the existing tap device? if this is the case, both encrypted and plain text traffic come to the single tap interface, and right now this traffic is processing 1 by 1 packet in order that they arrived, with your async solution there is possibility that arrived packets will be reordered, and this is probably whats not happening in real hardware, but thats my guess. Anyway in this case i think you should block for processing in the pipeline. if my understanding here is wrong, please setup a call meeting with me, we can talk and share on teams. I'm in CET timezone and available from 11am to 10pm everyday There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. basically this MACsecForwarder::forward() is exactly the same as HostInterfaceInfo::veth2tap_fun, but not having this filter option, and just uses different FD, but other processes are the same, i think this could be somehow unified, but maybe not this PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree. I add a new super class |
||
{ | ||
struct msghdr msg; | ||
memset(&msg, 0, sizeof(struct msghdr)); | ||
|
||
struct sockaddr_storage srcAddr; | ||
|
||
struct iovec iov[1]; | ||
|
||
iov[0].iov_base = buffer; // buffer for message | ||
iov[0].iov_len = sizeof(buffer); | ||
|
||
char control[CONTROL_MESSAGE_BUFFER_SIZE]; // buffer for control messages | ||
|
||
msg.msg_name = &srcAddr; | ||
msg.msg_namelen = sizeof(srcAddr); | ||
msg.msg_iov = iov; | ||
msg.msg_iovlen = 1; | ||
msg.msg_control = control; | ||
msg.msg_controllen = sizeof(control); | ||
|
||
swss::Selectable *sel = NULL; | ||
int result = s.select(&sel); | ||
|
||
|
@@ -128,7 +153,7 @@ void MACsecForwarder::forward() | |
if (sel == &m_exitEvent) // thread end event | ||
break; | ||
|
||
ssize_t size = read(m_macsecfd, buffer, sizeof(buffer)); | ||
ssize_t size = recvmsg(m_macsecfd, &msg, 0); | ||
Pterosaur marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (size < 0) | ||
{ | ||
|
@@ -151,6 +176,55 @@ void MACsecForwarder::forward() | |
|
||
continue; | ||
} | ||
else if (size < (ssize_t)sizeof(ethhdr)) | ||
{ | ||
SWSS_LOG_ERROR("invalid ethernet frame length: %zu", msg.msg_controllen); | ||
|
||
continue; | ||
} | ||
|
||
struct cmsghdr *cmsg; | ||
|
||
for (cmsg = CMSG_FIRSTHDR(&msg); cmsg != NULL; cmsg = CMSG_NXTHDR(&msg, cmsg)) | ||
Pterosaur marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
if (cmsg->cmsg_level != SOL_PACKET || cmsg->cmsg_type != PACKET_AUXDATA) | ||
continue; | ||
|
||
struct tpacket_auxdata* aux = (struct tpacket_auxdata*)CMSG_DATA(cmsg); | ||
|
||
if ((aux->tp_status & TP_STATUS_VLAN_VALID) && | ||
(aux->tp_status & TP_STATUS_VLAN_TPID_VALID)) | ||
{ | ||
SWSS_LOG_DEBUG("got vlan tci: 0x%x, vlanid: %d", aux->tp_vlan_tci, aux->tp_vlan_tci & 0xFFF); | ||
|
||
// inject vlan tag into frame | ||
|
||
// for overlapping buffers | ||
memmove(buffer + 2 * MAC_ADDRESS_SIZE + VLAN_TAG_SIZE, | ||
buffer + 2 * MAC_ADDRESS_SIZE, | ||
size - (2 * MAC_ADDRESS_SIZE)); | ||
|
||
uint16_t tci = htons(aux->tp_vlan_tci); | ||
uint16_t tpid = htons(IEEE_8021Q_ETHER_TYPE); | ||
|
||
uint8_t* pvlan = (uint8_t *)(buffer + 2 * MAC_ADDRESS_SIZE); | ||
memcpy(pvlan, &tpid, sizeof(uint16_t)); | ||
memcpy(pvlan + sizeof(uint16_t), &tci, sizeof(uint16_t)); | ||
|
||
size += VLAN_TAG_SIZE; | ||
|
||
break; | ||
} | ||
} | ||
|
||
if (m_info == nullptr) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think this should be checked on the constructor and throw if empty There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
{ | ||
SWSS_LOG_ERROR("The HostInterfaceInfo on the MACsec port %s is empty", m_macsecInterfaceName.c_str()); | ||
|
||
break; | ||
} | ||
|
||
m_info->async_process_packet_for_fdb_event(buffer, size); | ||
Pterosaur marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (write(m_tapfd, buffer, static_cast<int>(size)) < 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this also seems like a HostInterfaceInfo::veth2tap_fun end of the function, so this is also could be moved to a common function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -579,7 +579,7 @@ bool MACsecManager::add_macsec_forwarder( | |
|
||
auto &manager = itr->second; | ||
|
||
manager.m_forwarder = std::make_shared<MACsecForwarder>(macsecInterface, manager.m_info->m_tapfd); | ||
manager.m_forwarder = std::make_shared<MACsecForwarder>(macsecInterface, manager.m_info->m_tapfd, manager.m_info); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. manager.m_info->m_tapfd in not necessary parameter, remove; you are already passing manager.m_info inside, so you can internally use that m_tapfd, and also m_tapfd should be private, did you change visibility of that item ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
return true; | ||
} | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.