[macsec]: Disable LAG member forwarding on MACsec session down by karthik-nexthop · Pull Request #4744 · sonic-net/sonic-swss

karthik-nexthop · 2026-07-10T11:09:56Z

What I did

Added m_macsec_sa_active on Port (orchagent/port.h) to track MACsec data-plane intent in-memory.
Added PortsOrch::setLagMemberState() (orchagent/portsorch.cpp/.h) that:
- On MACsec down for a LAG member: flaps hostif oper-status (forces teamd to deselect the member without ~90s LACP timeout, without holding carrier down permanently so EAPOL/MKA can still flow), then disables ingress/egress collection+distribution directly via SAI.
- On MACsec up: clears the suppression flag only; lets teamsyncd's APP_LAG_MEMBER_TABLE status=enabled drive SAI re-enable after LACP completes.
- Skips hostif flap and SAI LAG writes on non-LAG ports (only persists the flag).
In doLagMemberTask: suppresses teamsyncd-driven re-enable when m_macsec_sa_active is false.
In setMACsecEnabledState: calls setLagMemberState(p, !enabled) when MACsec is enabled/disabled on a port.
In macsecorch: hooks createMACsecSA / deleteMACsecSA to call setLagMemberState only when both ingress and egress directions are up/down (bothDirectionsUp / bothDirectionsDown), so asymmetric rekey teardown does not drop the member prematurely.
Added unit tests in tests/mock_tests/portsorch_ut.cpp:
- MacsecDownDisablesLagMemberAndSuppressesTeamdReEnable
- MacsecDownDoesNotFlapNonLagPort

Why I did it

Fix issue #19254
When an MKA/MACsec session expires on a PortChannel member (e.g. EAPOL blocked, keepalive timeout), the MACsec data plane goes down but the LAG member can remain selected by teamd. Traffic may continue to hash to that member, causing black-holed or ICV-failed frames instead of failing over to healthy members.
After orchagent disables the member's collection/distribution, a routine teamsyncd refresh of APP_LAG_MEMBER_TABLE with status=enabled can silently re-enable forwarding while MACsec is still down.

How I verified it
Unit tests:

make -C tests/mock_tests portsorch_ut
./tests/mock_tests/portsorch_ut --gtest_filter='*MacsecDown*'

Related sonic-mgmt coverage:
tests/macsec/test_fault_handling.py::test_eapol_block_brings_lag_member_down.

Details if related
macsec_changes_explained.md

Signed-off-by: karthik-nexthop <karthik@nexthop.ai>

linux-foundation-easycla · 2026-07-10T11:10:02Z

The committers listed above are authorized under a signed CLA.

✅ login: karthik-nexthop / name: Karthik Siruvalam (34399b4)
✅ login: karthik-nexthop / name: karthik-nexthop (24fb817)

mssonicbld · 2026-07-10T11:10:04Z

/azp run

azure-pipelines · 2026-07-10T11:10:17Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2026-07-10T11:22:41Z

Hi, there are workflow run(s) waiting for approval, you may be first-time contributor. I will notify maintainers to help approve once PR is approved. Thanks!

_{---Powered by SONiC BuildBot}

Keep m_lag_forced_admin_down and m_macsec_sa_active Port members added for MACsec LAG member forwarding control. Signed-off-by: Karthik Siruvalam <karthik@nexthop.ai>

mssonicbld · 2026-07-15T05:49:36Z

/azp run

azure-pipelines · 2026-07-15T05:49:46Z

Azure Pipelines: Successfully started running 1 pipeline(s).

abdosi · 2026-07-22T17:15:45Z

@karthik-nexthop : we need to add one test case that can verify continuous flaps of macsec sessionn and it's impact on teamd to make sure their is no is no race condition for this for APP_DB and SAI programming.

Add MacsecContinuousFlapNoAppDbSaiRace to cover APP_DB/SAI races under repeated MACsec session flaps. Remove unused m_lag_forced_admin_down left over from a cherry-pick conflict (not present on upstream master). Signed-off-by: Karthik Siruvalam <karthik@nexthop.ai>

mssonicbld · 2026-07-27T06:47:46Z

/azp run

azure-pipelines · 2026-07-27T06:47:58Z

Azure Pipelines: Successfully started running 1 pipeline(s).

karthik-nexthop · 2026-07-27T13:32:10Z

/azp run

azure-pipelines · 2026-07-27T13:32:17Z

Commenter does not have sufficient privileges for PR 4744 in repo sonic-net/sonic-swss

saravanan-nexthop · 2026-07-27T13:50:24Z

/azp run

azure-pipelines · 2026-07-27T13:50:36Z

Azure Pipelines: Successfully started running 1 pipeline(s).

rlhui · 2026-08-05T17:08:11Z

@prsunny , would we please review/merge? thanks

kperumalbfn · 2026-08-05T21:05:04Z

+    setLagMemberState(p, !enabled);
+}
+
+void PortsOrch::setLagMemberState(Port &port, bool enabled)


Though this method is invoked in MacSec path, this name is very generic. Could you rename it?

will update it.

kperumalbfn · 2026-08-05T21:08:19Z

+        it->second.m_macsec_sa_active = enabled;
+    }
+
+    const bool is_lag_member = (port.m_lag_member_id != SAI_NULL_OBJECT_ID);


We should move this check to the start of the method and bypass if this port is not a member of a lag.

I'll move the LAG-member check up and return early for non-members so we skip hostif/SAI work immediately.

kperumalbfn · 2026-08-05T21:23:09Z

+        /* MACsec data plane is up again. Clear the suppression flag and let
+         * teamsyncd's status=enabled refresh drive SAI re-enable once LACP
+         * completes, avoiding hashing to a member before teamd selects it. */
+        SWSS_LOG_NOTICE("MACsec SA active on %s; awaiting teamsyncd to re-enable LAG member",


Which module sets setDistributionOnLagMember and setCollectionOnLagMember to true? Will be good to update in this section.

setLagMemberState(true) itself does not re-enable SAI collection/distribution — it only clears m_macsec_sa_active.

Re-enable is driven by teamsyncd. Will update the comment.

kperumalbfn · 2026-08-05T21:26:26Z

+            ASSERT_FALSE(ingressDisable) << "cycle " << i;
+        }
+
+        // Rapid down/up without an intervening teamsyncd enable: final intent wins.


Could you also check with LAG+min_links of LAG configuration?

That part is covered in the companion sonic-mgmt PR (#26062) — test_macsec_down_disables_lag_member reads CONFIG_DB min_links and gates the PortChannel/BGP checks on it. This mock UT doesn’t run teamd, so I didn’t try to cover min_links here

kperumalbfn · 2026-08-05T21:27:06Z

+
+        // Rapid down/up without an intervening teamsyncd enable: final intent wins.
+        Port rapid;
+        ASSERT_TRUE(gPortsOrch->getPort(memberAlias, rapid));


Check if LAG goes down(with min-links as total members) when one member's MAC sec fails

Yes — that’s covered in sonic-mgmt.

kperumalbfn · 2026-08-05T21:33:56Z

+     * orchagent/swss restarts (e.g. warm reboot) while MACsec is down, the
+     * member comes back enabled before its SAs are re-established. It is not
+     * reconciled from STATE_DB MACsec SA presence on init. */
+    bool                m_macsec_sa_active = true;


This changes the behavior after orchagent restart and not specific to warmboot. This can impact security, please check.

this is not specific to warmboot. m_macsec_sa_active is in-memory only and defaults to true, so any orchagent restart loses the prior intent. I’ll correct the comment.

kperumalbfn · 2026-08-05T21:36:43Z

+
+        if (!collection_ok || !distribution_ok)
+        {
+            SWSS_LOG_ERROR("Failed to disable collection/distribution on LAG member %s",


Should we reset port.m_macsec_sa_active for any SAI call failures?

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.

Rename setLagMemberState to setLagMemberMacsecSaActive, early-return for non-LAG members after persisting m_macsec_sa_active, and document the teamsyncd/doLagMemberTask re-enable path plus any-orchagent-restart limitation for the in-memory flag. Signed-off-by: Karthik Siruvalam <karthik@nexthop.ai>

mssonicbld · 2026-08-06T08:46:56Z

/azp run

mssonicbld · 2026-08-06T08:46:57Z

/azp run

azure-pipelines · 2026-08-06T08:47:08Z

Azure Pipelines: Successfully started running 1 pipeline(s).

azure-pipelines · 2026-08-06T08:47:08Z

Azure Pipelines: Successfully started running 1 pipeline(s).

prabhataravind · 2026-08-06T15:20:21Z

+        {
+            SWSS_LOG_NOTICE("Flapping host interface %s to force teamd LACP reset due to MACsec down",
+                            port.m_alias.c_str());
+            setHostIntfsOperStatus(port, false);


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:

Spurious telemetry: each call publishes an if-state event via event_publish, so monitoring systems will see two false link-flap events.

State divergence: updatePortOperStatus also updates m_oper_status, DB oper status, flap counters, nexthop reachability, gearbox state, and VoQ sync. Calling setHostIntfsOperStatus directly skips all of that, so orchagent's internal state won't reflect the kernel-visible flap.

Timing: back-to-back down/up with no delay risks the kernel coalescing the two netlink carrier events, in which case teamd never sees the down and never deselects the member — breaking the recovery path.
It would be probably cleaner to drive teamd deselection through a teamd-native mechanism (e.g. teamdctl runner port disable, or writing to the teamd control socket) rather than faking a link event through the SAI hostif layer?

prabhataravind

This PR introduces a second control plane for LAG member selection — macsecorch directly manipulates SAI LAG member state and mutes teamd via a suppression guard — which conflicts with SONiC's established architecture where teamd/LACP exclusively owns member selection through APP_LAG_MEMBER_TABLE.

The setHostIntfsOperStatus down/up flap here compounds the issue: this function's sole production caller is updatePortOperStatus (line 10064), where it mirrors real hardware link events. Using it synthetically to trick teamd into deselecting the member (a) publishes two false if-state telemetry events, (b) skips flap-count/nexthop/gearbox/VoQ state updates that updatePortOperStatus performs, and (c) depends on the kernel not coalescing the back-to-back netlink carrier events — an undocumented timing assumption.

Consider having macsecorch signal teamd directly (e.g. teamdctl port config update or a D-Bus/control-socket message) to deselect the member. Teamd would then write status=disabled to APP_LAG_MEMBER_TABLE, doLagMemberTask would disable collection/distribution through the normal path, and the suppression guard, hostif flap, and m_macsec_sa_active flag all become unnecessary.

Disable LAG member forwarding on MACsec session down

24fb817

Signed-off-by: karthik-nexthop <karthik@nexthop.ai>

karthik-nexthop requested review from Pterosaur and prsunny as code owners July 10, 2026 11:09

saravanan-nexthop requested review from judyjoseph and tjchadaga July 10, 2026 14:43

Resolve cherry-pick conflict in orchagent/port.h

e799b7d

Keep m_lag_forced_admin_down and m_macsec_sa_active Port members added for MACsec LAG member forwarding control. Signed-off-by: Karthik Siruvalam <karthik@nexthop.ai>

karthik-nexthop force-pushed the disable-lag-member-forwarding-on-macsec branch from 34399b4 to e799b7d Compare July 15, 2026 05:49

abdosi requested review from mlok-nokia and saksarav-nokia July 15, 2026 17:07

abdosi added this to SONiC Chassis Jul 15, 2026

abdosi added the P0 label Jul 15, 2026

saksarav-nokia reviewed Jul 15, 2026

View reviewed changes

Comment thread orchagent/portsorch.cpp

Comment thread orchagent/port.h Outdated

karthik-nexthop requested a review from saksarav-nokia July 28, 2026 05:18

karthik-nexthop mentioned this pull request Jul 28, 2026

[macsec] Add fault-handling tests for MKA timeout on LAG member sonic-net/sonic-mgmt#26062

Open

12 tasks

rlhui requested a review from abdosi July 29, 2026 17:08

saksarav-nokia previously approved these changes Jul 29, 2026

View reviewed changes

mlok-nokia previously approved these changes Jul 29, 2026

View reviewed changes

judyjoseph reviewed Aug 1, 2026

View reviewed changes

Comment thread orchagent/portsorch.cpp Outdated

karthik-nexthop requested a review from judyjoseph August 3, 2026 17:50

judyjoseph previously approved these changes Aug 5, 2026

View reviewed changes

prabhataravind requested a review from kperumalbfn August 5, 2026 20:51

kperumalbfn reviewed Aug 5, 2026

View reviewed changes

karthik-nexthop dismissed stale reviews from judyjoseph, mlok-nokia, and saksarav-nokia via 5fd8093 August 6, 2026 08:46

karthik-nexthop requested review from judyjoseph, kperumalbfn, mlok-nokia and saksarav-nokia August 6, 2026 08:47

prabhataravind reviewed Aug 6, 2026

View reviewed changes

prabhataravind requested changes Aug 6, 2026

View reviewed changes

rlhui assigned karthik-nexthop Aug 10, 2026

Uh oh!

Conversation

karthik-nexthop commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mssonicbld commented Jul 10, 2026

Uh oh!

azure-pipelines Bot commented Jul 10, 2026

Uh oh!

mssonicbld commented Jul 10, 2026

Uh oh!

mssonicbld commented Jul 15, 2026

Uh oh!

azure-pipelines Bot commented Jul 15, 2026

Uh oh!

Uh oh!

Uh oh!

abdosi commented Jul 22, 2026

Uh oh!

mssonicbld commented Jul 27, 2026

Uh oh!

azure-pipelines Bot commented Jul 27, 2026

Uh oh!

karthik-nexthop commented Jul 27, 2026

Uh oh!

azure-pipelines Bot commented Jul 27, 2026

Uh oh!

saravanan-nexthop commented Jul 27, 2026

Uh oh!

azure-pipelines Bot commented Jul 27, 2026

Uh oh!

Uh oh!

rlhui commented Aug 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Aug 6, 2026

Uh oh!

mssonicbld commented Aug 6, 2026

Uh oh!

azure-pipelines Bot commented Aug 6, 2026

Uh oh!

azure-pipelines Bot commented Aug 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prabhataravind left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

karthik-nexthop commented Jul 10, 2026 •

edited

Loading

linux-foundation-easycla Bot commented Jul 10, 2026 •

edited

Loading