-
Notifications
You must be signed in to change notification settings - Fork 732
[macsec]: Disable LAG member forwarding on MACsec session down #4744
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
base: master
Are you sure you want to change the base?
Changes from all commits
24fb817
e799b7d
3e304e9
5fd8093
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 |
|---|---|---|
|
|
@@ -6519,6 +6519,16 @@ void PortsOrch::doLagMemberTask(Consumer &consumer) | |
| /* Sync an enabled member */ | ||
| if (status == "enabled") | ||
| { | ||
| /* If the MACsec data plane on this member is down, suppress the | ||
| * teamsyncd-driven re-enable. MACsec controls collection and | ||
| * distribution directly until its SAs are re-established. */ | ||
| if (!port.m_macsec_sa_active) | ||
| { | ||
| SWSS_LOG_NOTICE("Skip enabling LAG member %s: MACsec SA inactive", | ||
| port.m_alias.c_str()); | ||
| it = consumer.m_toSync.erase(it); | ||
| continue; | ||
| } | ||
| /* enable collection first, distribution-only mode | ||
| * is not supported on Mellanox platform | ||
| */ | ||
|
|
@@ -11560,6 +11570,89 @@ void PortsOrch::setMACsecEnabledState(sai_object_id_t port_id, bool enabled) | |
| { | ||
| setPortMtu(p, p.m_mtu); | ||
| } | ||
|
|
||
| /* | ||
| * When MACsec is enabled on a port, the MACsec hardware will drop traffic | ||
| * until the SAs are established. Thus, the MACsec data plane is considered | ||
| * down (false). When MACsec is disabled on the port, the port returns to | ||
| * normal cleartext forwarding, so the MACsec data plane constraint is lifted (true). | ||
| */ | ||
| setLagMemberMacsecSaActive(p, !enabled); | ||
| } | ||
|
|
||
| void PortsOrch::setLagMemberMacsecSaActive(Port &port, bool enabled) | ||
| { | ||
| SWSS_LOG_ENTER(); | ||
|
|
||
| /* Nothing to do if the intent is unchanged. Both MACsec SCs going empty on | ||
| * a session timeout would otherwise drive a redundant disable (and a | ||
| * duplicate SAI write + log notice) per direction. */ | ||
| if (port.m_macsec_sa_active == enabled) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| /* Persist the MACsec data-plane intent so that a later teamsyncd refresh | ||
| * of APP_LAG_MEMBER_TABLE (handled in doLagMemberTask) does not silently | ||
| * re-enable the member while MACsec is down. Always update this, including | ||
| * for ports that are not yet (or no longer) LAG members. setMACsecEnabledState | ||
| * is shared for all MACsec ports; only hostif/SAI side effects below are | ||
| * LAG-member-specific. */ | ||
| port.m_macsec_sa_active = enabled; | ||
| auto it = m_portList.find(port.m_alias); | ||
| if (it != m_portList.end()) | ||
| { | ||
| it->second.m_macsec_sa_active = enabled; | ||
| } | ||
|
|
||
| /* Non-LAG ports: intent is recorded above; skip hostif flap and SAI LAG | ||
| * member attribute writes (flapping a standalone hostif would risk dropping | ||
| * routing adjacencies). */ | ||
| if (port.m_lag_member_id == SAI_NULL_OBJECT_ID) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (!enabled) | ||
| { | ||
| /* Flap the host interface oper status to force teamd to instantly drop | ||
| * the LAG member (bypassing the 90s LACP timeout) without permanently | ||
| * holding carrier down (which would block wpa_supplicant EAPOL). */ | ||
| if (port.m_oper_status == SAI_PORT_OPER_STATUS_UP) | ||
| { | ||
| SWSS_LOG_NOTICE("Flapping host interface %s to force teamd LACP reset due to MACsec down", | ||
| port.m_alias.c_str()); | ||
| setHostIntfsOperStatus(port, false); | ||
|
Contributor
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. setHostIntfsOperStatus has a single production caller today — updatePortOperStatus in the SAI port-oper-status notification path — where it mirrors a real hardware link event to the kernel. Using it here to synthesize a fake link flap has a few side effects:
|
||
| setHostIntfsOperStatus(port, true); | ||
| } | ||
|
|
||
| /* Disable collection/distribution directly via SAI rather than writing | ||
| * APP_LAG_MEMBER_TABLE, to avoid a write race with teamsyncd. */ | ||
| bool distribution_ok = setDistributionOnLagMember(port, false); | ||
| bool collection_ok = setCollectionOnLagMember(port, false); | ||
|
|
||
| if (!collection_ok || !distribution_ok) | ||
| { | ||
|
karthik-nexthop marked this conversation as resolved.
|
||
| SWSS_LOG_ERROR("Failed to disable collection/distribution on LAG member %s", | ||
|
Contributor
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. Should we reset port.m_macsec_sa_active for any SAI call failures?
Author
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 set m_macsec_sa_active = false before the SAI calls specifically so doLagMemberTask will suppress a later teamsyncd status=enabled refresh. If the SAI disable fails, the hardware may still be forwarding on that member — we log that as an error, and I agree that risk remains. But reverting the flag on failure would let orchagent re-enable collection/distribution from APP_DB while MACsec is still down, which makes the failure mode worse. Leaving the suppress flag set is the more conservative choice; a retry of the SAI disable would be a separate improvement if we want one. |
||
| port.m_alias.c_str()); | ||
| return; | ||
| } | ||
|
|
||
| SWSS_LOG_NOTICE("MACsec disabled LAG member %s", port.m_alias.c_str()); | ||
| } | ||
| else | ||
| { | ||
| /* MACsec data plane is up again. This path only clears m_macsec_sa_active | ||
| * above; it does not call setCollectionOnLagMember / | ||
| * setDistributionOnLagMember. Re-enable is driven by teamsyncd | ||
| * (TeamPortSync::onChange writes APP_LAG_MEMBER_TABLE status=enabled | ||
| * when teamd selects the member). doLagMemberTask then calls | ||
| * setCollectionOnLagMember(true) and setDistributionOnLagMember(true) | ||
| * once LACP has completed, avoiding hashing to a member before teamd | ||
| * selects it. */ | ||
| SWSS_LOG_NOTICE("MACsec SA active on %s; awaiting teamsyncd to re-enable LAG member", | ||
|
Contributor
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. Which module sets setDistributionOnLagMember and setCollectionOnLagMember to true? Will be good to update in this section.
Author
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.
Re-enable is driven by teamsyncd. Will update the comment. |
||
| port.m_alias.c_str()); | ||
| } | ||
| } | ||
|
|
||
| bool PortsOrch::isMACsecPort(sai_object_id_t port_id) const | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the behavior after orchagent restart and not specific to warmboot. This can impact security, please check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not specific to warmboot.
m_macsec_sa_activeis in-memory only and defaults to true, so any orchagent restart loses the prior intent. I’ll correct the comment.