[vslib]Add MACsec forward and filters to HostInterfaceInfo #719

Pterosaur · 2020-11-23T06:04:42Z

Add MACsec filter before VLAN Tag and FDB event processing.

Refer: MACsec in VLAN-aware Bridges

Signed-off-by: Ze Gan ganze718@gmail.com

Signed-off-by: Ze Gan <ganze718@gmail.com>

kcudnik · 2020-11-23T17:51:05Z

vslib/src/MACsecForwarder.cpp

@@ -112,6 +118,25 @@ void MACsecForwarder::forward()

    while (m_runThread)


why you need separate thread in macsec ? there is already thread thats deals with packet processing, why not tap into that ?

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

I remember you asked this question before, sorry that I did not explain it clear.
I think we need three threads in current design. The previous two are for forwarding data between tap and veth, and the new one is for forwarding the plaintext data from macsec to tap.

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.
All plaintext data, except EAPOL traffic, from tap interface will be captured by the MACsec egress filter that forwards to the macsec device for encryption.
Who will help us to forward the plaintext data from macsec device? I believe it should be thread 3.

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

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

Agree. I add a new super class TrafficForwarder. Maybe we can leverage it to refactor this part in the another PR.

vslib/src/MACsecForwarder.cpp

kcudnik · 2020-11-24T10:08:13Z

vslib/src/MACsecForwarder.cpp

@@ -112,6 +118,25 @@ void MACsecForwarder::forward()

    while (m_runThread)


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

vslib/src/HostInterfaceInfo.cpp

vslib/src/MACsecForwarder.cpp

kcudnik · 2020-11-25T15:52:02Z

vslib/src/MACsecForwarder.cpp

+            break;
+        }
+
+        m_info->async_process_packet_for_fdb_event(buffer, size);

        if (write(m_tapfd, buffer, static_cast<int>(size)) < 0)


this also seems like a HostInterfaceInfo::veth2tap_fun end of the function, so this is also could be moved to a common function?

kcudnik · 2020-11-25T15:53:25Z

vslib/src/MACsecManager.cpp

@@ -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);


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 ?

kcudnik · 2020-11-25T16:00:37Z

vslib/src/MACsecForwarder.cpp

+            }
+        }
+
+        if (m_info == nullptr)


i think this should be checked on the constructor and throw if empty

Signed-off-by: Ze Gan <ganze718@gmail.com>

vslib/src/TrafficForwarder.cpp

vslib/inc/TrafficForwarder.h

vslib/src/TrafficForwarder.cpp

Signed-off-by: Ze Gan <ganze718@gmail.com>

vslib/inc/TrafficForwarder.h

Signed-off-by: Ze Gan <ganze718@gmail.com>

kcudnik · 2020-11-30T19:21:07Z

retest vs please

…#719) Add MACsec filter before VLAN Tag and FDB event processing. Refer: MACsec in VLAN-aware Bridges

Pterosaur force-pushed the add_macsec_forward_and_filters_to_hostinterfaceinfo branch from 301e633 to e974c85 Compare November 23, 2020 06:07

Pterosaur changed the title ~~Add MACsec forward and filters to HostInterfaceInfo~~ [vslib]Add MACsec forward and filters to HostInterfaceInfo Nov 23, 2020

Pterosaur mentioned this pull request Nov 23, 2020

Macsec High level design sonic-net/SONiC#652

Merged

Pterosaur force-pushed the add_macsec_forward_and_filters_to_hostinterfaceinfo branch from e974c85 to 7df69f3 Compare November 23, 2020 06:58

Add MACsec forward and filters to HostInterfaceInfo

8b4cb33

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur force-pushed the add_macsec_forward_and_filters_to_hostinterfaceinfo branch from 7df69f3 to 8b4cb33 Compare November 23, 2020 07:13

Pterosaur marked this pull request as ready for review November 23, 2020 08:34

kcudnik requested changes Nov 23, 2020

View reviewed changes

kcudnik requested changes Nov 24, 2020

View reviewed changes

Pterosaur commented Nov 25, 2020

View reviewed changes

vslib/src/HostInterfaceInfo.cpp Show resolved Hide resolved

Pterosaur commented Nov 25, 2020

View reviewed changes

vslib/src/HostInterfaceInfo.cpp Show resolved Hide resolved

Pterosaur commented Nov 25, 2020

View reviewed changes

vslib/src/MACsecForwarder.cpp Outdated Show resolved Hide resolved

kcudnik reviewed Nov 25, 2020

View reviewed changes

kcudnik requested changes Nov 25, 2020

View reviewed changes

kcudnik reviewed Nov 25, 2020

View reviewed changes

Polish code

fe58aaa

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur requested a review from kcudnik November 26, 2020 08:00