[sub-mac] fix frame counter update and its sync with KeyManager (#7029)

This commit fixes two related aspects of updating of the frame counter
in `SubMac` and synchronizing the value with `KeyManager`.

It fixes an issue (when radio platform handles tx security and frame
counter management) where the counter value in `SubMac` and
`KeyManager` were set to old value used in the last transmission
(from `TxDone` callback). This would then lead to an already used
frame counter value to be included in MLE Link Frame Counter TLV.
Under `MULTI_RADIO` this could cause more harm since when appending
MLE Link Frame Counter TLV we move all counters for each radio link
up to max value among them and set the counter value on all radio
links(which would then cause the counter on radio platform to be set
to an older value).

This commit also updates the code to handle the case where the counter
value updates may be signaled out of order. This may happen due to a
new counter value being used for an enhanced-ack by radio platform
during tx of a frame. Note that the newer counter used for
enhanced-ack is processed from `SubMac::HandleReceiveDone()` which
can happen before processing of the older counter value from
`HandleTransmitDone()`.

src/core/mac/sub_mac.cpp

@@ -279,7 +279,7 @@ void SubMac::HandleReceiveDone(RxFrame *aFrame, Error aError)
 
     if (!ShouldHandleTransmitSecurity() && aFrame != nullptr && aFrame->mInfo.mRxInfo.mAckedWithSecEnhAck)
     {
-        UpdateFrameCounter(aFrame->mInfo.mRxInfo.mAckFrameCounter);
+        SignalFrameCounterUsed(aFrame->mInfo.mRxInfo.mAckFrameCounter);
     }
 
 #if OPENTHREAD_CONFIG_MAC_CSL_RECEIVER_ENABLE
@@ -362,7 +362,7 @@ void SubMac::ProcessTransmitSecurity(void)
         uint32_t frameCounter = GetFrameCounter();
 
         mTransmitFrame.SetFrameCounter(frameCounter);
-        UpdateFrameCounter(frameCounter + 1);
+        SignalFrameCounterUsed(frameCounter);
     }
 
     extAddress = &GetExtAddress();
@@ -536,7 +536,7 @@ void SubMac::HandleTransmitDone(TxFrame &aFrame, RxFrame *aAckFrame, Error aErro
         OT_UNREACHABLE_CODE(ExitNow());
     }
 
-    UpdateFrameCounterOnTxDone(aFrame);
+    SignalFrameCounterUsedOnTxDone(aFrame);
 
     // Determine whether a CSMA retry is required.
 
@@ -571,7 +571,7 @@ exit:
     return;
 }
 
-void SubMac::UpdateFrameCounterOnTxDone(const TxFrame &aFrame)
+void SubMac::SignalFrameCounterUsedOnTxDone(const TxFrame &aFrame)
 {
     uint8_t  keyIdMode;
     uint32_t frameCounter;
@@ -598,7 +598,7 @@ void SubMac::UpdateFrameCounterOnTxDone(const TxFrame &aFrame)
     VerifyOrExit(keyIdMode == Frame::kKeyIdMode1);
 
     VerifyOrExit(aFrame.GetFrameCounter(frameCounter) == kErrorNone, OT_ASSERT(allowError));
-    UpdateFrameCounter(frameCounter);
+    SignalFrameCounterUsed(frameCounter);
 
 exit:
     return;
@@ -876,11 +876,23 @@ exit:
     return;
 }
 
-void SubMac::UpdateFrameCounter(uint32_t aFrameCounter)
+void SubMac::SignalFrameCounterUsed(uint32_t aFrameCounter)
 {
-    mFrameCounter = aFrameCounter;
+    mCallbacks.FrameCounterUsed(aFrameCounter);
 
-    mCallbacks.FrameCounterUpdated(aFrameCounter);
+    // It not always guaranteed that this method is invoked in order
+    // for different counter values (i.e., we may get it for a
+    // smaller counter value after a lager one). This may happen due
+    // to a new counter value being used for an enhanced-ack during
+    // tx of a frame. Note that the newer counter used for enhanced-ack
+    // is processed from `HandleReceiveDone()` which can happen before
+    // processing of the older counter value from `HandleTransmitDone()`.
+
+    VerifyOrExit(mFrameCounter <= aFrameCounter);
+    mFrameCounter = aFrameCounter + 1;
+
+exit:
+    return;
 }
 
 void SubMac::SetFrameCounter(uint32_t aFrameCounter)

src/core/mac/sub_mac.hpp

@@ -195,12 +195,15 @@ public:
         void EnergyScanDone(int8_t aMaxRssi);
 
         /**
-         * This method notifies user of `SubMac` that MAC frame counter is updated.
+         * This method notifies user of `SubMac` that a specific MAC frame counter is used for transmission.
          *
-         * @param[in]  aFrameCounter  The MAC frame counter value.
+         * It is possible that this callback is invoked out of order in terms of counter values (i.e., called for a
+         * smaller counter value after a call for a larger counter value).
+         *
+         * @param[in]  aFrameCounter  The MAC frame counter value which was used.
          *
          */
-        void FrameCounterUpdated(uint32_t aFrameCounter);
+        void FrameCounterUsed(uint32_t aFrameCounter);
     };
 
     /**
@@ -622,7 +625,7 @@ private:
     bool ShouldHandleTransmitTargetTime(void) const;
 
     void ProcessTransmitSecurity(void);
-    void UpdateFrameCounter(uint32_t aFrameCounter);
+    void SignalFrameCounterUsed(uint32_t aFrameCounter);
     void StartCsmaBackoff(void);
     void BeginTransmit(void);
     void SampleRssi(void);
@@ -630,7 +633,7 @@ private:
     void HandleReceiveDone(RxFrame *aFrame, Error aError);
     void HandleTransmitStarted(TxFrame &aFrame);
     void HandleTransmitDone(TxFrame &aFrame, RxFrame *aAckFrame, Error aError);
-    void UpdateFrameCounterOnTxDone(const TxFrame &aFrame);
+    void SignalFrameCounterUsedOnTxDone(const TxFrame &aFrame);
     void HandleEnergyScanDone(int8_t aMaxRssi);
 
     static void HandleTimer(Timer &aTimer);

src/core/mac/sub_mac_callbacks.cpp

@@ -107,9 +107,9 @@ void SubMac::Callbacks::EnergyScanDone(int8_t aMaxRssi)
     }
 }
 
-void SubMac::Callbacks::FrameCounterUpdated(uint32_t aFrameCounter)
+void SubMac::Callbacks::FrameCounterUsed(uint32_t aFrameCounter)
 {
-    Get<KeyManager>().MacFrameCounterUpdated(aFrameCounter);
+    Get<KeyManager>().MacFrameCounterUsed(aFrameCounter);
 }
 
 #elif OPENTHREAD_RADIO
@@ -142,7 +142,7 @@ void SubMac::Callbacks::EnergyScanDone(int8_t aMaxRssi)
     Get<LinkRaw>().InvokeEnergyScanDone(aMaxRssi);
 }
 
-void SubMac::Callbacks::FrameCounterUpdated(uint32_t aFrameCounter)
+void SubMac::Callbacks::FrameCounterUsed(uint32_t aFrameCounter)
 {
     OT_UNUSED_VARIABLE(aFrameCounter);
 }

src/core/thread/key_manager.cpp

@@ -429,17 +429,26 @@ void KeyManager::SetAllMacFrameCounters(uint32_t aMacFrameCounter)
 }
 
 #if OPENTHREAD_CONFIG_RADIO_LINK_IEEE_802_15_4_ENABLE
-void KeyManager::MacFrameCounterUpdated(uint32_t aMacFrameCounter)
+void KeyManager::MacFrameCounterUsed(uint32_t aMacFrameCounter)
 {
-    mMacFrameCounters.Set154(aMacFrameCounter);
+    // This is callback from `SubMac` to indicate that a frame
+    // counter value is used for tx. We ensure to handle it
+    // even if it is called out of order.
+
+    VerifyOrExit(mMacFrameCounters.Get154() <= aMacFrameCounter);
+
+    mMacFrameCounters.Set154(aMacFrameCounter + 1);
 
     if (mMacFrameCounters.Get154() >= mStoredMacFrameCounter)
     {
         IgnoreError(Get<Mle::MleRouter>().Store());
     }
+
+exit:
+    return;
 }
 #else
-void KeyManager::MacFrameCounterUpdated(uint32_t)
+void KeyManager::MacFrameCounterUsed(uint32_t)
 {
 }
 #endif

src/core/thread/key_manager.hpp

@@ -541,12 +541,14 @@ public:
     void UpdateKeyMaterial(void);
 
     /**
-     * This method handles MAC frame counter change (callback from `SubMac` for 15.4 security frame change)
+     * This method handles MAC frame counter changes (callback from `SubMac` for 15.4 security frame change).
+     *
+     * This is called to indicate the @p aMacFrameCounter value is now used.
      *
      * @param[in]  aMacFrameCounter  The 15.4 link MAC frame counter value.
      *
      */
-    void MacFrameCounterUpdated(uint32_t aMacFrameCounter);
+    void MacFrameCounterUsed(uint32_t aMacFrameCounter);
 
 private:
     static constexpr uint32_t kDefaultKeySwitchGuardTime = 624;