From 10cc734e177481d616ae85b996b3af2aea0b875d Mon Sep 17 00:00:00 2001 From: "Deomid \"rojer\" Ryabkov" Date: Mon, 4 Sep 2023 18:31:59 +0100 Subject: [PATCH] UDPEndPointImplLwIP: Support LWIP_TCPIP_CORE_LOCKING=0 Wrap calls to LwIP APIs in `tcpip_api_call()`, as required. When `LWIP_TCPIP_CORE_LOCKING` is enabled, this internally becomes `LOCK_TCPIP_CORE/UNLOCK_TCPIP_CORE` and when it isn't, it posts a message to the TCPIP task to run the function. Added CHIP stack locking to the UDP receive function. --- src/inet/BUILD.gn | 1 + src/inet/EndPointStateLwIP.cpp | 85 +++++++++++++ src/inet/EndPointStateLwIP.h | 7 +- src/inet/TCPEndPointImplLwIP.cpp | 7 ++ src/inet/UDPEndPointImplLwIP.cpp | 208 ++++++++++++++----------------- 5 files changed, 191 insertions(+), 117 deletions(-) create mode 100644 src/inet/EndPointStateLwIP.cpp diff --git a/src/inet/BUILD.gn b/src/inet/BUILD.gn index f8f81f94e3c44a..1ba8ea5687e2e3 100644 --- a/src/inet/BUILD.gn +++ b/src/inet/BUILD.gn @@ -103,6 +103,7 @@ static_library("inet") { ] if (chip_system_config_use_lwip) { + sources += [ "EndPointStateLwIP.cpp" ] public_deps += [ "${chip_root}/src/lwip" ] } diff --git a/src/inet/EndPointStateLwIP.cpp b/src/inet/EndPointStateLwIP.cpp new file mode 100644 index 00000000000000..34fba243353701 --- /dev/null +++ b/src/inet/EndPointStateLwIP.cpp @@ -0,0 +1,85 @@ +/* + * + * Copyright (c) 2023 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include +#include + +#include + +namespace chip { +namespace Inet { + +err_t EndPointStateLwIP::RunOnTCPIPRet(std::function fn) +{ + assertChipStackLockedByCurrentThread(); +#if LWIP_TCPIP_CORE_LOCKING + err_t err; + LOCK_TCPIP_CORE(); + err = fn(); + UNLOCK_TCPIP_CORE(); + return err; +#else + // Post a message to the TCPIP task and wait for it to run. + static sys_sem_t sTCPIPSem; + static bool sTCPIPSemInited = false; + if (!sTCPIPSemInited) + { + err_t err = sys_sem_new(&sTCPIPSem, 0); + if (err != ERR_OK) + { + return err; + } + sTCPIPSemInited = true; + } + + // tcpip_callback takes a C function pointer, so we can't pass a capturing lambda to it. + // Just store the state the function we pass to it needs in statics. + // This should be safe, since that function will execute before we return and there is no + // re-entry into this method. + static std::function sTCPIPFunction; + static err_t sTCPIPFunctionResult; + VerifyOrDie(sTCPIPFunction == nullptr); + + sTCPIPFunction = fn; + const err_t err = tcpip_callback( + [](void * aCtx) { + sTCPIPFunctionResult = sTCPIPFunction(); + sys_sem_signal(&sTCPIPSem); + }, + nullptr); + if (err != ERR_OK) + { + return err; + } + sys_arch_sem_wait(&sTCPIPSem, 0); + sTCPIPFunction = nullptr; + return sTCPIPFunctionResult; +#endif +} + +void EndPointStateLwIP::RunOnTCPIP(std::function fn) +{ + VerifyOrDie(RunOnTCPIPRet([&fn]() { + fn(); + return ERR_OK; + }) == ERR_OK); +} + +} // namespace Inet +} // namespace chip diff --git a/src/inet/EndPointStateLwIP.h b/src/inet/EndPointStateLwIP.h index d42f613931484e..ddc862d65ef386 100644 --- a/src/inet/EndPointStateLwIP.h +++ b/src/inet/EndPointStateLwIP.h @@ -22,8 +22,9 @@ #pragma once -#include +#include +#include #include struct udp_pcb; @@ -46,6 +47,10 @@ class DLL_EXPORT EndPointStateLwIP UDP = 1, TCP = 2 } mLwIPEndPointType; + + // Synchronously runs a function within the TCPIP task's context. + static void RunOnTCPIP(std::function); + static err_t RunOnTCPIPRet(std::function); }; } // namespace Inet diff --git a/src/inet/TCPEndPointImplLwIP.cpp b/src/inet/TCPEndPointImplLwIP.cpp index 757b7d8d5c95de..a80253643864b8 100644 --- a/src/inet/TCPEndPointImplLwIP.cpp +++ b/src/inet/TCPEndPointImplLwIP.cpp @@ -37,6 +37,13 @@ #include #include +static_assert(LWIP_VERSION_MAJOR > 1, "CHIP requires LwIP 2.0 or later"); + +#if !(CHIP_DEVICE_LAYER_TARGET_BL602 || CHIP_DEVICE_LAYER_TARGET_BL702 || CHIP_DEVICE_LAYER_TARGET_BL702L) +// TODO: Update to use RunOnTCPIP. +static_assert(LWIP_TCPIP_CORE_LOCKING, "CHIP requires config LWIP_TCPIP_CORE_LOCKING enabled"); +#endif + namespace chip { namespace Inet { diff --git a/src/inet/UDPEndPointImplLwIP.cpp b/src/inet/UDPEndPointImplLwIP.cpp index 8db3d7e593e2b0..f808bf5ed9bb1c 100755 --- a/src/inet/UDPEndPointImplLwIP.cpp +++ b/src/inet/UDPEndPointImplLwIP.cpp @@ -36,15 +36,10 @@ #include #include #include -#include #include static_assert(LWIP_VERSION_MAJOR > 1, "CHIP requires LwIP 2.0 or later"); -#if !(CHIP_DEVICE_LAYER_TARGET_BL602 || CHIP_DEVICE_LAYER_TARGET_BL702 || CHIP_DEVICE_LAYER_TARGET_BL702L) -static_assert(LWIP_TCPIP_CORE_LOCKING, "CHIP requires config LWIP_TCPIP_CORE_LOCKING enabled"); -#endif - #if !defined(RAW_FLAGS_MULTICAST_LOOP) || !defined(UDP_FLAGS_MULTICAST_LOOP) || !defined(raw_clear_flags) || \ !defined(raw_set_flags) || !defined(udp_clear_flags) || !defined(udp_set_flags) #define HAVE_LWIP_MULTICAST_LOOP 0 @@ -66,27 +61,11 @@ static_assert(LWIP_TCPIP_CORE_LOCKING, "CHIP requires config LWIP_TCPIP_CORE_LOC namespace chip { namespace Inet { -namespace { -/** - * @brief - * RAII locking for LwIP core to simplify management of - * LOCK_TCPIP_CORE()/UNLOCK_TCPIP_CORE() calls. - */ -class ScopedLwIPLock -{ -public: - ScopedLwIPLock() { LOCK_TCPIP_CORE(); } - ~ScopedLwIPLock() { UNLOCK_TCPIP_CORE(); } -}; -} // anonymous namespace - EndpointQueueFilter * UDPEndPointImplLwIP::sQueueFilter = nullptr; CHIP_ERROR UDPEndPointImplLwIP::BindImpl(IPAddressType addressType, const IPAddress & address, uint16_t port, InterfaceId interfaceId) { - ScopedLwIPLock lwipLock; - // Make sure we have the appropriate type of PCB. CHIP_ERROR res = GetPCB(addressType); @@ -99,7 +78,7 @@ CHIP_ERROR UDPEndPointImplLwIP::BindImpl(IPAddressType addressType, const IPAddr if (res == CHIP_NO_ERROR) { - res = chip::System::MapErrorLwIP(udp_bind(mUDP, &ipAddr, port)); + res = chip::System::MapErrorLwIP(RunOnTCPIPRet([this, &ipAddr, port]() { return udp_bind(mUDP, &ipAddr, port); })); } if (res == CHIP_NO_ERROR) @@ -112,10 +91,7 @@ CHIP_ERROR UDPEndPointImplLwIP::BindImpl(IPAddressType addressType, const IPAddr CHIP_ERROR UDPEndPointImplLwIP::BindInterfaceImpl(IPAddressType addrType, InterfaceId intfId) { - // A lock is required because the LwIP thread may be referring to intf_filter, - // while this code running in the Inet application is potentially modifying it. // NOTE: this only supports LwIP interfaces whose number is no bigger than 9. - ScopedLwIPLock lwipLock; // Make sure we have the appropriate type of PCB. CHIP_ERROR err = GetPCB(addrType); @@ -139,16 +115,16 @@ CHIP_ERROR UDPEndPointImplLwIP::LwIPBindInterface(struct udp_pcb * aUDP, Interfa } } - udp_bind_netif(aUDP, netifp); + RunOnTCPIP([aUDP, netifp]() { udp_bind_netif(aUDP, netifp); }); return CHIP_NO_ERROR; } InterfaceId UDPEndPointImplLwIP::GetBoundInterface() const { - ScopedLwIPLock lwipLock; - #if HAVE_LWIP_UDP_BIND_NETIF - return InterfaceId(netif_get_by_index(mUDP->netif_idx)); + struct netif * netif; + RunOnTCPIP([this, &netif]() { netif = netif_get_by_index(mUDP->netif_idx); }); + return InterfaceId(netif); #else return InterfaceId(mUDP->intf_filter); #endif @@ -161,14 +137,14 @@ uint16_t UDPEndPointImplLwIP::GetBoundPort() const CHIP_ERROR UDPEndPointImplLwIP::ListenImpl() { - ScopedLwIPLock lwipLock; - - udp_recv(mUDP, LwIPReceiveUDPMessage, this); + RunOnTCPIP([this]() { udp_recv(mUDP, LwIPReceiveUDPMessage, this); }); return CHIP_NO_ERROR; } CHIP_ERROR UDPEndPointImplLwIP::SendMsgImpl(const IPPacketInfo * pktInfo, System::PacketBufferHandle && msg) { + assertChipStackLockedByCurrentThread(); + const IPAddress & destAddr = pktInfo->DestAddress; if (!msg.HasSoleOwnership()) @@ -185,50 +161,47 @@ CHIP_ERROR UDPEndPointImplLwIP::SendMsgImpl(const IPPacketInfo * pktInfo, System CHIP_ERROR res = CHIP_NO_ERROR; err_t lwipErr = ERR_VAL; - // Adding a scope here to unlock the LwIP core when the lock is no longer required. + // Make sure we have the appropriate type of PCB based on the destination address. + res = GetPCB(destAddr.Type()); + if (res != CHIP_NO_ERROR) { - ScopedLwIPLock lwipLock; - - // Make sure we have the appropriate type of PCB based on the destination address. - res = GetPCB(destAddr.Type()); - if (res != CHIP_NO_ERROR) - { - return res; - } + return res; + } - // Send the message to the specified address/port. - // If an outbound interface has been specified, call a specific version of the UDP sendto() - // function that accepts the target interface. - // If a source address has been specified, temporarily override the local_ip of the PCB. - // This results in LwIP using the given address being as the source address for the generated - // packet, as if the PCB had been bound to that address. - const IPAddress & srcAddr = pktInfo->SrcAddress; - const uint16_t & destPort = pktInfo->DestPort; - const InterfaceId & intfId = pktInfo->Interface; + // Send the message to the specified address/port. + // If an outbound interface has been specified, call a specific version of the UDP sendto() + // function that accepts the target interface. + // If a source address has been specified, temporarily override the local_ip of the PCB. + // This results in LwIP using the given address being as the source address for the generated + // packet, as if the PCB had been bound to that address. + const IPAddress & srcAddr = pktInfo->SrcAddress; + const uint16_t & destPort = pktInfo->DestPort; + const InterfaceId & intfId = pktInfo->Interface; - ip_addr_t lwipSrcAddr = srcAddr.ToLwIPAddr(); - ip_addr_t lwipDestAddr = destAddr.ToLwIPAddr(); + ip_addr_t lwipSrcAddr = srcAddr.ToLwIPAddr(); + ip_addr_t lwipDestAddr = destAddr.ToLwIPAddr(); - ip_addr_t boundAddr; - ip_addr_copy(boundAddr, mUDP->local_ip); + ip_addr_t boundAddr; + ip_addr_copy(boundAddr, mUDP->local_ip); - if (!ip_addr_isany(&lwipSrcAddr)) - { - ip_addr_copy(mUDP->local_ip, lwipSrcAddr); - } + if (!ip_addr_isany(&lwipSrcAddr)) + { + ip_addr_copy(mUDP->local_ip, lwipSrcAddr); + } + lwipErr = RunOnTCPIPRet([this, &intfId, &msg, &lwipDestAddr, destPort]() { if (intfId.IsPresent()) { - lwipErr = udp_sendto_if(mUDP, System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(msg), &lwipDestAddr, destPort, - intfId.GetPlatformInterface()); + return udp_sendto_if(mUDP, System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(msg), &lwipDestAddr, destPort, + intfId.GetPlatformInterface()); } else { - lwipErr = udp_sendto(mUDP, System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(msg), &lwipDestAddr, destPort); + return udp_sendto(mUDP, System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(msg), &lwipDestAddr, destPort); } + }); - ip_addr_copy(mUDP->local_ip, boundAddr); - } + ip_addr_copy(mUDP->local_ip, boundAddr); if (lwipErr != ERR_OK) { @@ -240,30 +213,31 @@ CHIP_ERROR UDPEndPointImplLwIP::SendMsgImpl(const IPPacketInfo * pktInfo, System void UDPEndPointImplLwIP::CloseImpl() { - ScopedLwIPLock lwipLock; + assertChipStackLockedByCurrentThread(); // Since UDP PCB is released synchronously here, but UDP endpoint itself might have to wait // for destruction asynchronously, there could be more allocated UDP endpoints than UDP PCBs. - if (mUDP != nullptr) + if (mUDP == nullptr) { - udp_remove(mUDP); - mUDP = nullptr; - mLwIPEndPointType = LwIPEndPointType::Unknown; - - // If there is a UDPEndPointImplLwIP::LwIPReceiveUDPMessage - // event pending in the event queue (SystemLayer::ScheduleLambda), we - // schedule a release call to the end of the queue, to ensure that the - // queued pointer to UDPEndPointImplLwIP is not dangling. - if (mDelayReleaseCount != 0) + return; + } + RunOnTCPIP([this]() { udp_remove(mUDP); }); + mUDP = nullptr; + mLwIPEndPointType = LwIPEndPointType::Unknown; + + // If there is a UDPEndPointImplLwIP::LwIPReceiveUDPMessage + // event pending in the event queue (SystemLayer::ScheduleLambda), we + // schedule a release call to the end of the queue, to ensure that the + // queued pointer to UDPEndPointImplLwIP is not dangling. + if (mDelayReleaseCount != 0) + { + Retain(); + CHIP_ERROR err = GetSystemLayer().ScheduleLambda([this] { Release(); }); + if (err != CHIP_NO_ERROR) { - Retain(); - CHIP_ERROR err = GetSystemLayer().ScheduleLambda([this] { Release(); }); - if (err != CHIP_NO_ERROR) - { - ChipLogError(Inet, "Unable to schedule lambda: %" CHIP_ERROR_FORMAT, err.Format()); - // There is nothing we can do here, accept the chance of racing - Release(); - } + ChipLogError(Inet, "Unable to schedule lambda: %" CHIP_ERROR_FORMAT, err.Format()); + // There is nothing we can do here, accept the chance of racing + Release(); } } } @@ -306,7 +280,7 @@ void UDPEndPointImplLwIP::HandleDataReceived(System::PacketBufferHandle && msg, CHIP_ERROR UDPEndPointImplLwIP::GetPCB(IPAddressType addrType) { - // IMPORTANT: This method MUST be called with the LwIP stack LOCKED! + assertChipStackLockedByCurrentThread(); // If a PCB hasn't been allocated yet... if (mUDP == nullptr) @@ -314,12 +288,12 @@ CHIP_ERROR UDPEndPointImplLwIP::GetPCB(IPAddressType addrType) // Allocate a PCB of the appropriate type. if (addrType == IPAddressType::kIPv6) { - mUDP = udp_new_ip_type(IPADDR_TYPE_V6); + RunOnTCPIP([this]() { mUDP = udp_new_ip_type(IPADDR_TYPE_V6); }); } #if INET_CONFIG_ENABLE_IPV4 else if (addrType == IPAddressType::kIPv4) { - mUDP = udp_new_ip_type(IPADDR_TYPE_V4); + RunOnTCPIP([this]() { mUDP = udp_new_ip_type(IPADDR_TYPE_V4); }); } #endif // INET_CONFIG_ENABLE_IPV4 else @@ -474,26 +448,25 @@ CHIP_ERROR UDPEndPointImplLwIP::IPv4JoinLeaveMulticastGroupImpl(InterfaceId aInt { #if LWIP_IPV4 && LWIP_IGMP const ip4_addr_t lIPv4Address = aAddress.ToIPv4(); - err_t lStatus; - + struct netif * lNetif = nullptr; + if (aInterfaceId.IsPresent()) { - ScopedLwIPLock lwipLock; + lNetif = FindNetifFromInterfaceId(aInterfaceId); + VerifyOrReturnError(lNetif != nullptr, INET_ERROR_UNKNOWN_INTERFACE); + } - if (aInterfaceId.IsPresent()) + err_t lStatus = RunOnTCPIPRet([lNetif, &lIPv4Address, join]() { + if (lNetif != nullptr) { - - struct netif * const lNetif = FindNetifFromInterfaceId(aInterfaceId); - VerifyOrReturnError(lNetif != nullptr, INET_ERROR_UNKNOWN_INTERFACE); - - lStatus = join ? igmp_joingroup_netif(lNetif, &lIPv4Address) // - : igmp_leavegroup_netif(lNetif, &lIPv4Address); + return join ? igmp_joingroup_netif(lNetif, &lIPv4Address) // + : igmp_leavegroup_netif(lNetif, &lIPv4Address); } else { - lStatus = join ? igmp_joingroup(IP4_ADDR_ANY4, &lIPv4Address) // - : igmp_leavegroup(IP4_ADDR_ANY4, &lIPv4Address); + return join ? igmp_joingroup(IP4_ADDR_ANY4, &lIPv4Address) // + : igmp_leavegroup(IP4_ADDR_ANY4, &lIPv4Address); } - } + }); if (lStatus == ERR_MEM) { @@ -510,24 +483,25 @@ CHIP_ERROR UDPEndPointImplLwIP::IPv6JoinLeaveMulticastGroupImpl(InterfaceId aInt { #ifdef HAVE_IPV6_MULTICAST const ip6_addr_t lIPv6Address = aAddress.ToIPv6(); - err_t lStatus; - + struct netif * lNetif = nullptr; + if (aInterfaceId.IsPresent()) { - ScopedLwIPLock lwipLock; + lNetif = FindNetifFromInterfaceId(aInterfaceId); + VerifyOrReturnError(lNetif != nullptr, INET_ERROR_UNKNOWN_INTERFACE); + } - if (aInterfaceId.IsPresent()) + err_t lStatus = RunOnTCPIPRet([lNetif, &lIPv6Address, join]() { + if (lNetif != nullptr) { - struct netif * const lNetif = FindNetifFromInterfaceId(aInterfaceId); - VerifyOrReturnError(lNetif != nullptr, INET_ERROR_UNKNOWN_INTERFACE); - lStatus = join ? mld6_joingroup_netif(lNetif, &lIPv6Address) // - : mld6_leavegroup_netif(lNetif, &lIPv6Address); + return join ? mld6_joingroup_netif(lNetif, &lIPv6Address) // + : mld6_leavegroup_netif(lNetif, &lIPv6Address); } else { - lStatus = join ? mld6_joingroup(IP6_ADDR_ANY6, &lIPv6Address) // - : mld6_leavegroup(IP6_ADDR_ANY6, &lIPv6Address); + return join ? mld6_joingroup(IP6_ADDR_ANY6, &lIPv6Address) // + : mld6_leavegroup(IP6_ADDR_ANY6, &lIPv6Address); } - } + }); if (lStatus == ERR_MEM) { @@ -544,18 +518,20 @@ struct netif * UDPEndPointImplLwIP::FindNetifFromInterfaceId(InterfaceId aInterf { struct netif * lRetval = nullptr; + RunOnTCPIP([aInterfaceId, &lRetval]() { #if defined(NETIF_FOREACH) - NETIF_FOREACH(lRetval) - { - if (lRetval == aInterfaceId.GetPlatformInterface()) + NETIF_FOREACH(lRetval) { - break; + if (lRetval == aInterfaceId.GetPlatformInterface()) + { + break; + } } - } #else // defined(NETIF_FOREACH) - for (lRetval = netif_list; lRetval != nullptr && lRetval != aInterfaceId.GetPlatformInterface(); lRetval = lRetval->next) - ; + for (lRetval = netif_list; lRetval != nullptr && lRetval != aInterfaceId.GetPlatformInterface(); lRetval = lRetval->next) + ; #endif // defined(NETIF_FOREACH) + }); return (lRetval); }