From 2089716959eb75e62b2765db70dab2d4c09386cf Mon Sep 17 00:00:00 2001 From: Jonathan Hui Date: Fri, 15 Nov 2019 13:27:31 -0800 Subject: [PATCH] [mle] clean up MleRouter::HandleAdvertisement (#4338) - Ensure there are no side effects if MLE frame counters have not yet been syncrhonzied. - Do not remove stale neighbor until end of processing Advertisement. This allows for complete processing of Advertisement if frame counters have already been established. --- src/core/thread/mle.cpp | 2 +- src/core/thread/mle_router.cpp | 74 +++++++++++++++++------------- src/core/thread/mle_router_ftd.hpp | 2 +- src/core/thread/mle_router_mtd.hpp | 2 +- 4 files changed, 46 insertions(+), 34 deletions(-) diff --git a/src/core/thread/mle.cpp b/src/core/thread/mle.cpp index 02f72cc0c..0c8c2ab08 100644 --- a/src/core/thread/mle.cpp +++ b/src/core/thread/mle.cpp @@ -2834,7 +2834,7 @@ otError Mle::HandleAdvertisement(const Message &aMessage, const Ip6::MessageInfo { if (IsFullThreadDevice()) { - SuccessOrExit(error = Get().HandleAdvertisement(aMessage, aMessageInfo)); + SuccessOrExit(error = Get().HandleAdvertisement(aMessage, aMessageInfo, aNeighbor)); } else if ((aNeighbor == &mParent) && (mParent.GetRloc16() != sourceAddress.GetRloc16())) { diff --git a/src/core/thread/mle_router.cpp b/src/core/thread/mle_router.cpp index 80401880f..4e42d6352 100644 --- a/src/core/thread/mle_router.cpp +++ b/src/core/thread/mle_router.cpp @@ -1138,7 +1138,9 @@ exit: return rval; } -otError MleRouter::HandleAdvertisement(const Message &aMessage, const Ip6::MessageInfo &aMessageInfo) +otError MleRouter::HandleAdvertisement(const Message & aMessage, + const Ip6::MessageInfo &aMessageInfo, + Neighbor * aNeighbor) { otError error = OT_ERROR_NONE; const otThreadLinkInfo *linkInfo = static_cast(aMessageInfo.GetLinkInfo()); @@ -1149,7 +1151,6 @@ otError MleRouter::HandleAdvertisement(const Message &aMessage, const Ip6::Messa RouteTlv route; uint32_t partitionId; Router * router; - Neighbor * neighbor; uint8_t routerId; uint8_t routerCount; @@ -1159,12 +1160,6 @@ otError MleRouter::HandleAdvertisement(const Message &aMessage, const Ip6::Messa SuccessOrExit(error = Tlv::GetTlv(aMessage, Tlv::kSourceAddress, sizeof(sourceAddress), sourceAddress)); VerifyOrExit(sourceAddress.IsValid(), error = OT_ERROR_PARSE); - // Remove stale neighbors - if ((neighbor = GetNeighbor(macAddr)) != NULL && neighbor->GetRloc16() != sourceAddress.GetRloc16()) - { - RemoveNeighbor(*neighbor); - } - // Leader Data SuccessOrExit(error = Tlv::GetTlv(aMessage, Tlv::kLeaderData, sizeof(leaderData), leaderData)); VerifyOrExit(leaderData.IsValid(), error = OT_ERROR_PARSE); @@ -1196,7 +1191,7 @@ otError MleRouter::HandleAdvertisement(const Message &aMessage, const Ip6::Messa error = OT_ERROR_DROP); } - if (mRole == OT_DEVICE_ROLE_CHILD && (mParent.GetExtAddress() == macAddr || !IsFullThreadDevice())) + if (mRole == OT_DEVICE_ROLE_CHILD && (aNeighbor == &mParent || !IsFullThreadDevice())) { ExitNow(); } @@ -1215,6 +1210,8 @@ otError MleRouter::HandleAdvertisement(const Message &aMessage, const Ip6::Messa } else if (leaderData.GetLeaderRouterId() != GetLeaderId()) { + VerifyOrExit(aNeighbor && aNeighbor->IsStateValid()); + if (mRole != OT_DEVICE_ROLE_CHILD) { otLogInfoMle("Leader ID mismatch"); @@ -1232,7 +1229,7 @@ otError MleRouter::HandleAdvertisement(const Message &aMessage, const Ip6::Messa Get().HandleTimeSyncMessage(aMessage); #endif - if (IsFullThreadDevice() && + if (IsFullThreadDevice() && (aNeighbor && aNeighbor->IsStateValid()) && ((mRouterTable.GetActiveRouterCount() == 0) || (static_cast(route.GetRouterIdSequence() - mRouterTable.GetRouterIdSequence()) > 0))) { @@ -1280,18 +1277,11 @@ otError MleRouter::HandleAdvertisement(const Message &aMessage, const Ip6::Messa ExitNow(); case OT_DEVICE_ROLE_CHILD: - router = (macAddr == mParent.GetExtAddress()) ? &mParent : mRouterTable.GetRouter(routerId); - VerifyOrExit(router != NULL); - - if (router->IsStateValid() && IsFullThreadDevice() && (mRouterSelectionJitterTimeout == 0) && - (mRouterTable.GetActiveRouterCount() < mRouterUpgradeThreshold)) + if (aNeighbor == &mParent) { - mRouterSelectionJitterTimeout = 1 + Random::NonCrypto::GetUint8InRange(0, mRouterSelectionJitter); - ExitNow(); - } + // MLE Advertisement from parent + router = &mParent; - if (router == &mParent) - { if (mParent.GetRloc16() != sourceAddress.GetRloc16()) { BecomeDetached(); @@ -1300,7 +1290,16 @@ otError MleRouter::HandleAdvertisement(const Message &aMessage, const Ip6::Messa if (IsFullThreadDevice()) { - Router *leader = mRouterTable.GetLeader(); + Router *leader; + + if ((mRouterSelectionJitterTimeout == 0) && + (mRouterTable.GetActiveRouterCount() < mRouterUpgradeThreshold)) + { + mRouterSelectionJitterTimeout = 1 + Random::NonCrypto::GetUint8InRange(0, mRouterSelectionJitter); + ExitNow(); + } + + leader = mRouterTable.GetLeader(); if (leader != NULL) { @@ -1333,17 +1332,24 @@ otError MleRouter::HandleAdvertisement(const Message &aMessage, const Ip6::Messa } } } - else if (IsFullThreadDevice() && !router->IsStateValid() && !router->IsStateLinkRequest() && - (mRouterTable.GetActiveLinkCount() < OPENTHREAD_CONFIG_MLE_CHILD_ROUTER_LINKS)) + else { - router->SetExtAddress(macAddr); - router->GetLinkInfo().Clear(); - router->GetLinkInfo().AddRss(Get().GetNoiseFloor(), linkInfo->mRss); - router->ResetLinkFailures(); - router->SetLastHeard(TimerMilli::GetNow()); - router->SetState(Neighbor::kStateLinkRequest); - SendLinkRequest(router); - ExitNow(error = OT_ERROR_NO_ROUTE); + // MLE Advertisement not from parent, but from some other neighboring router + router = mRouterTable.GetRouter(routerId); + VerifyOrExit(router != NULL); + + if (IsFullThreadDevice() && !router->IsStateValid() && !router->IsStateLinkRequest() && + (mRouterTable.GetActiveLinkCount() < OPENTHREAD_CONFIG_MLE_CHILD_ROUTER_LINKS)) + { + router->SetExtAddress(macAddr); + router->GetLinkInfo().Clear(); + router->GetLinkInfo().AddRss(Get().GetNoiseFloor(), linkInfo->mRss); + router->ResetLinkFailures(); + router->SetLastHeard(TimerMilli::GetNow()); + router->SetState(Neighbor::kStateLinkRequest); + SendLinkRequest(router); + ExitNow(error = OT_ERROR_NO_ROUTE); + } } router->SetLastHeard(TimerMilli::GetNow()); @@ -1403,6 +1409,12 @@ otError MleRouter::HandleAdvertisement(const Message &aMessage, const Ip6::Messa #endif exit: + if (aNeighbor && aNeighbor->GetRloc16() != sourceAddress.GetRloc16()) + { + // Remove stale neighbors + RemoveNeighbor(*aNeighbor); + } + return error; } diff --git a/src/core/thread/mle_router_ftd.hpp b/src/core/thread/mle_router_ftd.hpp index bb88a82ba..a226ac76b 100644 --- a/src/core/thread/mle_router_ftd.hpp +++ b/src/core/thread/mle_router_ftd.hpp @@ -688,7 +688,7 @@ private: const Ip6::MessageInfo &aMessageInfo, uint32_t aKeySequence, Neighbor * aNeighbor); - otError HandleAdvertisement(const Message &aMessage, const Ip6::MessageInfo &aMessageInfo); + otError HandleAdvertisement(const Message &aMessage, const Ip6::MessageInfo &aMessageInfo, Neighbor *); otError HandleParentRequest(const Message &aMessage, const Ip6::MessageInfo &aMessageInfo); otError HandleChildIdRequest(const Message &aMessage, const Ip6::MessageInfo &aMessageInfo, uint32_t aKeySequence); otError HandleChildUpdateRequest(const Message & aMessage, diff --git a/src/core/thread/mle_router_mtd.hpp b/src/core/thread/mle_router_mtd.hpp index 9ca7bf68a..6da39d818 100644 --- a/src/core/thread/mle_router_mtd.hpp +++ b/src/core/thread/mle_router_mtd.hpp @@ -139,7 +139,7 @@ private: { return OT_ERROR_DROP; } - otError HandleAdvertisement(const Message &, const Ip6::MessageInfo &) { return OT_ERROR_DROP; } + otError HandleAdvertisement(const Message &, const Ip6::MessageInfo &, Neighbor *) { return OT_ERROR_DROP; } otError HandleParentRequest(const Message &, const Ip6::MessageInfo &) { return OT_ERROR_DROP; } otError HandleChildIdRequest(const Message &, const Ip6::MessageInfo &, uint32_t) { return OT_ERROR_DROP; } otError HandleChildUpdateRequest(const Message &, const Ip6::MessageInfo &, uint32_t) { return OT_ERROR_DROP; }