From e802770b36321510e4fdd21161bd18f8acc0411d Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 14 Feb 2022 10:47:14 -0500 Subject: [PATCH 1/4] Assert that the Matter stack lock is held while reading/writing attributes. Several changes here: 1) Add an assert that when attributes are being read/written the Matter stack lock is held, so the read/write does not race with interaction model messages touching the attribute store. 2) Implement the Matter stack lock assertion bits on FreeRTOS. 3) Add FreeRTOS to the set of configurations that actually enable Matter stack lock assertions. 4) For the configurations that enable the assertions, change the default behavior from "log" to "fatal", so we actually notice the --- src/app/util/attribute-storage.cpp | 3 +++ .../GenericPlatformManagerImpl_FreeRTOS.cpp | 15 +++++++++++++++ .../GenericPlatformManagerImpl_FreeRTOS.h | 4 ++++ src/platform/BUILD.gn | 4 ++-- 4 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index a732e3513ab486..30cde0131670b1 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -47,6 +47,7 @@ #include #include #include +#include #include #include @@ -549,6 +550,8 @@ bool emAfMatchAttribute(const EmberAfCluster * cluster, const EmberAfAttributeMe EmberAfStatus emAfReadOrWriteAttribute(EmberAfAttributeSearchRecord * attRecord, const EmberAfAttributeMetadata ** metadata, uint8_t * buffer, uint16_t readLength, bool write) { + assertChipStackLockedByCurrentThread(); + uint16_t attributeOffsetIndex = 0; for (uint8_t ep = 0; ep < emberAfEndpointCount(); ep++) diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp index 58fbb1837d6d6f..3e5a4afe1b79fe 100644 --- a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp +++ b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp @@ -104,6 +104,21 @@ void GenericPlatformManagerImpl_FreeRTOS::_UnlockChipStack(void) xSemaphoreGive(mChipStackLock); } +template +bool GenericPlatformManagerImpl_FreeRTOS::_IsChipStackLockedByCurrentThread() const +{ + // If we have not started our event loop yet, return true because in that + // case we can't be racing against the (not yet started) event loop. + // + // Similarly, if mChipStackLock has not been created yet, might as well + // return true. + ChipLogError(DeviceLayer, "LOCKY: %d %d %d %d", mEventLoopTask == nullptr, mChipStackLock == nullptr, + xSemaphoreGetMutexHolder(mChipStackLock) == nullptr, + xSemaphoreGetMutexHolder(mChipStackLock) == xTaskGetCurrentTaskHandle()); + return (mEventLoopTask == nullptr) || (mChipStackLock == nullptr) || + (xSemaphoreGetMutexHolder(mChipStackLock) == xTaskGetCurrentTaskHandle()); +} + template CHIP_ERROR GenericPlatformManagerImpl_FreeRTOS::_PostEvent(const ChipDeviceEvent * event) { diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.h b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.h index 4b701546711ae7..c8a8e35139929a 100644 --- a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.h +++ b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.h @@ -78,6 +78,10 @@ class GenericPlatformManagerImpl_FreeRTOS : public GenericPlatformManagerImpl Date: Mon, 14 Feb 2022 11:43:20 -0500 Subject: [PATCH 2/4] Remove stray debugging log. --- .../platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp index 3e5a4afe1b79fe..ae6fb88721e6cb 100644 --- a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp +++ b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp @@ -112,9 +112,6 @@ bool GenericPlatformManagerImpl_FreeRTOS::_IsChipStackLockedByCurrent // // Similarly, if mChipStackLock has not been created yet, might as well // return true. - ChipLogError(DeviceLayer, "LOCKY: %d %d %d %d", mEventLoopTask == nullptr, mChipStackLock == nullptr, - xSemaphoreGetMutexHolder(mChipStackLock) == nullptr, - xSemaphoreGetMutexHolder(mChipStackLock) == xTaskGetCurrentTaskHandle()); return (mEventLoopTask == nullptr) || (mChipStackLock == nullptr) || (xSemaphoreGetMutexHolder(mChipStackLock) == xTaskGetCurrentTaskHandle()); } From 4cf5ad8dd8ee0a4300d3cd84f57201a706bc8e3d Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 14 Feb 2022 15:08:28 -0500 Subject: [PATCH 3/4] Make p6 and qpg CI compile. --- config/qpg/chip-gn/args.gni | 4 ++++ examples/all-clusters-app/p6/args.gni | 6 ++++++ examples/lighting-app/p6/args.gni | 6 ++++++ examples/lighting-app/qpg/args.gni | 4 ++++ examples/lock-app/p6/args.gni | 6 ++++++ examples/lock-app/qpg/args.gni | 4 ++++ examples/persistent-storage/qpg/args.gni | 6 ++++++ .../internal/GenericPlatformManagerImpl_FreeRTOS.cpp | 9 +++++++++ .../internal/GenericPlatformManagerImpl_FreeRTOS.h | 2 +- 9 files changed, 46 insertions(+), 1 deletion(-) diff --git a/config/qpg/chip-gn/args.gni b/config/qpg/chip-gn/args.gni index 1b34dc0ed37713..bbaa8868de6df9 100644 --- a/config/qpg/chip-gn/args.gni +++ b/config/qpg/chip-gn/args.gni @@ -32,6 +32,10 @@ qpg_cc = "arm-none-eabi-gcc" qpg_cxx = "arm-none-eabi-g++" chip_mdns = "platform" +# Disable lock tracking, since our FreeRTOS configuration does not set +# INCLUDE_xSemaphoreGetMutexHolder +chip_stack_lock_tracking = "none" + chip_project_config_include_dirs = [ "//examples/platform/qpg/project_include/" ] chip_project_config_include = "" diff --git a/examples/all-clusters-app/p6/args.gni b/examples/all-clusters-app/p6/args.gni index 2a93385cf36bd6..43a75f96144ea5 100644 --- a/examples/all-clusters-app/p6/args.gni +++ b/examples/all-clusters-app/p6/args.gni @@ -18,3 +18,9 @@ import("${chip_root}/src/platform/P6/args.gni") p6_target_project = get_label_info(":all_clusters_app_sdk_sources", "label_no_toolchain") + +declare_args() { + # Disable lock tracking, since our FreeRTOS configuration does not set + # INCLUDE_xSemaphoreGetMutexHolder + chip_stack_lock_tracking = "none" +} diff --git a/examples/lighting-app/p6/args.gni b/examples/lighting-app/p6/args.gni index f239efee01e5da..5f9b284655b922 100644 --- a/examples/lighting-app/p6/args.gni +++ b/examples/lighting-app/p6/args.gni @@ -18,3 +18,9 @@ import("${chip_root}/src/platform/P6/args.gni") p6_target_project = get_label_info(":lighting_app_sdk_sources", "label_no_toolchain") + +declare_args() { + # Disable lock tracking, since our FreeRTOS configuration does not set + # INCLUDE_xSemaphoreGetMutexHolder + chip_stack_lock_tracking = "none" +} diff --git a/examples/lighting-app/qpg/args.gni b/examples/lighting-app/qpg/args.gni index cba75e2ec1cc81..748cff2ce4a916 100644 --- a/examples/lighting-app/qpg/args.gni +++ b/examples/lighting-app/qpg/args.gni @@ -20,6 +20,10 @@ qpg_sdk_target = get_label_info(":sdk", "label_no_toolchain") declare_args() { chip_enable_ota_requestor = true + + # Disable lock tracking, since our FreeRTOS configuration does not set + # INCLUDE_xSemaphoreGetMutexHolder + chip_stack_lock_tracking = "none" } pw_log_BACKEND = "${chip_root}/src/lib/support/pw_log_chip" diff --git a/examples/lock-app/p6/args.gni b/examples/lock-app/p6/args.gni index a83cf1abdcb38e..c0b6f98e878997 100644 --- a/examples/lock-app/p6/args.gni +++ b/examples/lock-app/p6/args.gni @@ -18,3 +18,9 @@ import("${chip_root}/src/platform/P6/args.gni") p6_target_project = get_label_info(":lock_app_sdk_sources", "label_no_toolchain") + +declare_args() { + # Disable lock tracking, since our FreeRTOS configuration does not set + # INCLUDE_xSemaphoreGetMutexHolder + chip_stack_lock_tracking = "none" +} diff --git a/examples/lock-app/qpg/args.gni b/examples/lock-app/qpg/args.gni index cba75e2ec1cc81..748cff2ce4a916 100644 --- a/examples/lock-app/qpg/args.gni +++ b/examples/lock-app/qpg/args.gni @@ -20,6 +20,10 @@ qpg_sdk_target = get_label_info(":sdk", "label_no_toolchain") declare_args() { chip_enable_ota_requestor = true + + # Disable lock tracking, since our FreeRTOS configuration does not set + # INCLUDE_xSemaphoreGetMutexHolder + chip_stack_lock_tracking = "none" } pw_log_BACKEND = "${chip_root}/src/lib/support/pw_log_chip" diff --git a/examples/persistent-storage/qpg/args.gni b/examples/persistent-storage/qpg/args.gni index 4918e5af9de27b..3e9a1662c0299b 100644 --- a/examples/persistent-storage/qpg/args.gni +++ b/examples/persistent-storage/qpg/args.gni @@ -17,3 +17,9 @@ import("//build_overrides/chip.gni") import("${chip_root}/examples/platform/qpg/args.gni") qpg_sdk_target = get_label_info(":sdk", "label_no_toolchain") chip_enable_openthread = false + +declare_args() { + # Disable lock tracking, since our FreeRTOS configuration does not set + # INCLUDE_xSemaphoreGetMutexHolder + chip_stack_lock_tracking = "none" +} diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp index ae6fb88721e6cb..4234bad77513d5 100644 --- a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp +++ b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp @@ -104,9 +104,17 @@ void GenericPlatformManagerImpl_FreeRTOS::_UnlockChipStack(void) xSemaphoreGive(mChipStackLock); } +#if CHIP_STACK_LOCK_TRACKING_ENABLED template bool GenericPlatformManagerImpl_FreeRTOS::_IsChipStackLockedByCurrentThread() const { + // We can't check for INCLUDE_xTaskGetCurrentTaskHandle because it's often + // _not_ set, but xTaskGetCurrentTaskHandle works anyway because + // configUSE_MUTEXES is set. So in practice, xTaskGetCurrentTaskHandle can + // be assumed to be available here. +#if INCLUDE_xSemaphoreGetMutexHolder != 1 +#error Must either set INCLUDE_xSemaphoreGetMutexHolder = 1 in FreeRTOSConfig.h or set chip_stack_lock_tracking = "none" in Matter gn configuration. +#endif // If we have not started our event loop yet, return true because in that // case we can't be racing against the (not yet started) event loop. // @@ -115,6 +123,7 @@ bool GenericPlatformManagerImpl_FreeRTOS::_IsChipStackLockedByCurrent return (mEventLoopTask == nullptr) || (mChipStackLock == nullptr) || (xSemaphoreGetMutexHolder(mChipStackLock) == xTaskGetCurrentTaskHandle()); } +#endif // CHIP_STACK_LOCK_TRACKING_ENABLED template CHIP_ERROR GenericPlatformManagerImpl_FreeRTOS::_PostEvent(const ChipDeviceEvent * event) diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.h b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.h index c8a8e35139929a..3ad68b42ab197f 100644 --- a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.h +++ b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.h @@ -80,7 +80,7 @@ class GenericPlatformManagerImpl_FreeRTOS : public GenericPlatformManagerImpl Date: Mon, 14 Feb 2022 18:16:57 -0500 Subject: [PATCH 4/4] Also assert in MatterReportingAttributeChangeCallback. --- src/app/util/ember-compatibility-functions.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp index e8d4fe138be76e..03b97fe0d4683a 100644 --- a/src/app/util/ember-compatibility-functions.cpp +++ b/src/app/util/ember-compatibility-functions.cpp @@ -42,6 +42,7 @@ #include #include #include +#include #include #include @@ -1029,6 +1030,10 @@ void MatterReportingAttributeChangeCallback(EndpointId endpoint, ClusterId clust void MatterReportingAttributeChangeCallback(EndpointId endpoint, ClusterId clusterId, AttributeId attributeId) { + // Attribute writes have asserted this already, but this assert should catch + // applications notifying about changes from their end. + assertChipStackLockedByCurrentThread(); + ClusterInfo info; info.mClusterId = clusterId; info.mAttributeId = attributeId;