recover from stale-SAK after dirty macsec docker restart

[senthil-nexthop]

Why I did it

After a macsec docker restart that doesn't give macsecmgrd enough time to call disableMACsec on every port (e.g. systemd TimeoutStopSec elapsed mid-loop, SIGKILL, or simply 16 ports exceeding the stop window), orchagent's MACsecOrch in-memory state survives with OIDs pointing at SAs whose SAK was distributed in the prior MKA cycle. The post-restart wpa_supplicant negotiates a fresh SAK and writes it to APPL_DB, but three code paths fail to propagate it to SAI. The result is asymmetric encryption: the ASIC keeps using the stale SAK while userspace believes the re-key happened, so ICV fails on every received frame and LACPDUs / LLDPDUs get silently dropped.

How I did it

Three matching re-key paths in orchagent/macsecorch.cpp, all detected by the presence of a sak field in the incoming SET:

1. taskUpdateIngressSA: wpa_supplicant's macsec_sonic driver installs a new ingress SA in two stages: stage-1 writes active=false + full key material; stage-2 writes active=true only. After a dirty restart the SA pre-exists, so the legacy path deletes on active=false and the subsequent active=true createMACsecSA fails with no SAK to consume. New behaviour: when active=false arrives on an existing SA AND the SET carries a SAK, do deleteMACsecSA + createMACsecSA inline using THIS sa_attr's key material — atomically. When no SAK is in the SET, original delete-only path is kept (true deactivate).

2. createMACsecSA: the function's "SA already exists -> return task_success" early-exit silently discarded any SAK in the SET. SAI_MACSEC_SA_ATTR_SAK is create-only in SAI, so a re-key requires delete+create. New behaviour: when the SA exists and the SET carries a SAK, treat as re-key: deleteMACsecSA + recurse. When no SAK is present, keep the existing fast-path. Defends both directions and both code paths that call createMACsecSA.

3. taskUpdateEgressSA: the egress "SA already exists" branch only updated next_pn; the new SAK in
   the SET was silently dropped. Mirror the ingress re-key pattern: detect SAK presence, deleteMACsecSA +
   createMACsecSA with the new key material.

How to verify it

Pre-requisite: at least one DUT with multiple MACsec-protected ports in a LACP portchannel, peer also macsec-enabled.

1. Bring MACsec up clean, confirm show interfaces portchannel shows members Selected and IN_PKTS_OK advancing.
2. Trigger a dirty restart that bypasses macsecmgrd's per-port disable: sudo docker kill -s 9 macsec.
3. Wait ~30–45s for macsec to respawn and MKA to re-converge.
4. Confirm userspace SAK == ASIC SAK in both directions:

   show macsec <port> | grep -E '^\s+sak '
   for oid in $(redis-cli -n 2 HGETALL COUNTERS_MACSEC_NAME_MAP \
                | paste - - | awk '/<port>:/{print $NF}'); do
     redis-cli -n 1 HGET "ASIC_STATE:SAI_OBJECT_TYPE_MACSEC_SA:$oid" \
                          SAI_MACSEC_SA_ATTR_SAK
   done
   

5. PortChannel members stay Selected; IN_PKTS_OK continues to climb on both ends; IN_PKTS_NOT_VALID and IN_PKTS_NOT_USING_SA stay at zero; show lldp table shows the peer entry.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[saravanan-nexthop]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[abdosi]

@rawal01 Can you please test this issue on 202405 release ?
@bmridul : can you also please help check ?

@senthil-nexthop Please have test-gap in sonic-mgmt to cover this.

[arlakshm]

@senthil-nexthop, can you please add UT for this change

[senthil-nexthop]

@senthil-nexthop, can you please add UT for this change

Done, added macsecorch_ut to cover all 3 scenarios.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[abdosi]

Automated Review (SONiC Review Agent)

Files reviewed: 2 (orchagent/macsecorch.cpp +69/-2, tests/mock_tests/macsecorch_ut.cpp +424 new)
Findings: 0 critical, 0 warnings, 4 info

The fix is well-reasoned, addresses a real and well-documented bug, and the three re-key paths are mirror-symmetric. Recursion termination in createMACsecSA is safe because each MACsecOrchContext is constructed fresh per call (the cached m_macsec_sa in the outer context is never re-dereferenced; deleteMACsecSA erases the m_sa_ids entry; the recursive call rebuilds the context, sees nullptr, and falls through to the create path which re-enables the MACsec ACL flow if needed). Exception safety is fine — get_value() (line 122-146) wraps lexical_convert in catch(std::exception&), so a malformed SAK cleanly falls back to the legacy path rather than throwing. DCO sign-off is present.

---

🔵 INFO: Inherited partial-failure semantics in deleteMACsecSA now exercised on more code paths

File: orchagent/macsecorch.cpp:2299 (and 1130, 1212)

deleteMACsecSA(string, direction) unconditionally erases m_sa_ids[an] and may call setMACsecFlowActive(false) even when the SAI removal call failed (see line 2479-2495: SAI failure sets result = task_failed but m_sa_ids.erase(an) still runs, and the flow may be deactivated). On a re-key path that fails the delete step, the new code returns del_status immediately, leaving:

• orchagent state: SA gone from m_sa_ids
• ASIC state: old (stale) SA still present — leaked
• ACL: possibly demoted to packet_action (encryption bypassed) if it was the only SA

A subsequent retry of the SET will create a fresh SA, but the old hardware SA is permanently leaked, and on the egress side the leaked SA may continue to be referenced by the SC for encoding until garbage-collected at SC teardown.

Impact: Worse on the new re-key path than before because re-key is the normal case after a dirty restart, multiplying the chances of hitting this. Pre-existing — not introduced by this PR.

Note: Consider a follow-up that either (a) attempts to re-create even when deleteMACsecSA returns task_failed (since m_sa_ids was erased and the create path will re-install state), or (b) propagates task_need_retry so the Orch framework re-drives the SET. Either is better than returning task_failed and leaving the port without an active SA.

---

🔵 INFO: taskUpdateIngressSA else-branch (no active field) still silently drops SAK

File: orchagent/macsecorch.cpp:1225-1242

When an ingress SET arrives for an existing SA without an active field but with a sak, the function falls into the else branch and only applies lowest_acceptable_pn — the SAK is silently dropped. The defense-in-depth probe added to createMACsecSA only fires when callers go through the active branch (line 1181) or directly through createMACsecSA. A future wpa_supplicant/macsec_sonic driver change that posts SAK-only updates (no active field) would re-introduce the original bug on this branch.

Note: Consider mirroring the probe here for symmetry:

} else {
    MACsecSAK probe_sak = {{0}, false};
    if (get_value(sa_attr, "sak", probe_sak)) {
        auto del_status = deleteMACsecSA(port_sci_an, SAI_MACSEC_DIRECTION_INGRESS);
        if (del_status != task_success) return del_status;
        return createMACsecSA(port_sci_an, sa_attr, SAI_MACSEC_DIRECTION_INGRESS);
    }
    // existing lowest_acceptable_pn update path...
}

---

🔵 INFO: Fragile test fixture — PortsOrch constructed via malloc+memset+placement-new of a single field

File: tests/mock_tests/macsecorch_ut.cpp:236-248

The fixture allocates raw memory for a PortsOrch, zero-fills it, and placement-new's only m_portList. The remaining members (m_gearboxInterfaceMap, m_gearboxPhyMap, saiOidToAlias, sibling maps, the vtable for the Orch/Subject base subobjects, etc.) are technically uninitialized — formally undefined behavior. It works on libstdc++ because zero-byte std::maps happen to look like empty maps, and the methods reached from the tested paths (getPort(string,Port&), getGearboxPhy()) are non-virtual and only read those maps. Any future tested code path that calls a virtual PortsOrch method or accesses a non-trivially-zero-initializable member (e.g., a std::mutex, std::unique_ptr to something with non-zero default) will segfault or trigger UBSan.

Note: Lower-risk alternative: create a thin MockPortsOrch derived class or use the existing ut_helper::initPortsOrch() pattern that other mock_tests/* files use. Not blocking for this PR, but worth following up before adding more tests on this fixture.

---

🔵 INFO: Tests don't cover the deleteMACsecSA failure path during re-key

File: tests/mock_tests/macsecorch_ut.cpp (whole file)

The four tests exercise the happy path and the no-SAK fast-path, but there's no negative test for fake_remove_macsec_sa returning SAI_STATUS_FAILURE to verify that the re-key delete-failure branch logs and returns correctly without dereferencing freed state. Given the partial-failure semantics flagged above, an explicit test would be valuable.

Note: Optional. Could be added by parameterizing fake_remove_macsec_sa to return a configurable status.

---

✅ Verdict: Production logic is correct and the bug fix is sound. The four INFO items are all non-blocking — three relate to pre-existing concerns surfaced (but not caused) by this PR, and one is a test-quality nit.

_{Posted by an automated SONiC review agent. Findings are advisory; maintainers' judgment supersedes.}

[senthil-nexthop]

Added sonic-mgmt tests to verify the fix: sonic-net/sonic-mgmt#24903

[senthil-nexthop]

🔵 INFO: Inherited partial-failure semantics in deleteMACsecSA now exercised on more code paths

File: orchagent/macsecorch.cpp:2299 (and 1130, 1212)

The partial-failure case is present in existing code, we can review all of them and fix if necessary in a subsequent PR.

🔵 INFO: taskUpdateIngressSA else-branch (no active field) still silently drops SAK

File: orchagent/macsecorch.cpp:1225-1242

active is always observed to be False when the AN does not change but the SAK does.

🔵 INFO: Fragile test fixture — PortsOrch constructed via malloc+memset+placement-new of a single field

File: tests/mock_tests/macsecorch_ut.cpp:236-248
Note: Lower-risk alternative: create a thin MockPortsOrch derived class or use the existing ut_helper::initPortsOrch() pattern that other mock_tests/* files use. Not blocking for this PR, but worth following up before adding more tests on this fixture.

Can be followed up separately.

🔵 INFO: Tests don't cover the deleteMACsecSA failure path during re-key

File: tests/mock_tests/macsecorch_ut.cpp (whole file)

Not a blocker.

✅ Verdict: Production logic is correct and the bug fix is sound. The four INFO items are all non-blocking — three relate to pre-existing concerns surfaced (but not caused) by this PR, and one is a test-quality nit.

Posted by an automated SONiC review agent. Findings are advisory; maintainers' judgment supersedes.

[mssonicbld]

/azp run

[abdosi]

@prsunny : can you help merge this

[abdosi]

@prsunny : can you help merge this.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[azure-pipelines]

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

[mssonicbld]

This PR has backport request label(s) for branch(es): 202511,202605, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202511: <test result>) in the Test result section as well in your PR description.

_{---Powered by SONiC BuildBot}

[rookie-who]

Hi — this PR has a cherry-pick conflict for the 202605 branch (Cherry Pick Conflict_202605). Could you please rebase or resolve the conflict so we can proceed with the cherry-pick?

If you need help identifying the conflicting changes, let us know. Thanks!

Comment by @vaibhavhd via automated tooling

[senthil-nexthop]

Hi — this PR has a cherry-pick conflict for the 202605 branch (Cherry Pick Conflict_202605). Could you please rebase or resolve the conflict so we can proceed with the cherry-pick?

If you need help identifying the conflicting changes, let us know. Thanks!

Comment by @vaibhavhd via automated tooling

Backport PRs:
202605: #4796
202511: #4795

[saravanan-nexthop]

@rlhui @prsunny please help with this merge
Thank you

[rlhui]

@rlhui @prsunny please help with this merge Thank you

@prabhataravind , @prsunny , please help on this PR . Thanks.

[mssonicbld]

Cherry-pick PR to 202511:

[mssonicbld]

The cherry pick conflict has been handled manually. Removing cherry pick conflict label...

_{---Powered by SONiC BuildBot}

[mssonicbld]

The change is not in msft-202405 yet. @senthil-nexthop, please manually create the cherry pick PR for branch msft-202405.
You can ping the release branch owner(github account: bingwang-ms) to approve your cherry pick PR.
If this change is already in msft-202405, please comment "already in msft-202405". Thanks!

_{---Powered by SONiC BuildBot}

[mssonicbld]

The change is not in 202605 yet. @senthil-nexthop, please manually create the cherry pick PR for branch 202605.
You can ping the release branch owner(github account: vaibhavhd) to approve your cherry pick PR.
If this change is already in 202605, please comment "already in 202605". Thanks!

_{---Powered by SONiC BuildBot}