[network-manager-app] pending dataset support over ubus - #73309
[network-manager-app] pending dataset support over ubus#73309LorbusChris wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new MatterUbusService to publish a "matter" ubus object for onboarding and commissioning window control, and updates the OpenThread ubus border router delegate to support pending datasets and asynchronous provisioning/deprovisioning. The review feedback highlights several critical issues, including potential null pointer dereferences in SetActiveDataset and HandleOpenWindow, a potential use-after-free or incorrect state transition in RevertActiveDataset due to an uncleared callback, and a potential compilation failure in HandleOpenWindow due to strict type safety.
cbf69ed to
e4737a4
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the MatterUbusService to expose onboarding information and commissioning window controls via ubus, and enhances the OpenThread ubus border router delegate to support pending datasets, asynchronous provisioning, and reverting active datasets. The review feedback highlights three main improvement opportunities: ensuring mActiveDataset is cleared and reported even if ubus_invoke_async fails during a revert to prevent inconsistent local state; increasing the QR code buffer size in MatterUbusService to 128 bytes to avoid potential buffer size errors; and conditionally triggering the pending dataset attribute change notification only when the dataset is successfully updated or cleared.
97023d3 to
65025df
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the MatterUbusService to expose onboarding information and commissioning window control via ubus, integrates it into the network manager application, and enhances the OpenThread ubus border router delegate to support asynchronous provisioning, deprovisioning, and pending dataset management. The review feedback highlights several critical areas for improvement: addressing potential Use-After-Free or race conditions by tracking and aborting in-flight asynchronous requests, validating parameters before use in GetDataset, preventing data truncation when opening the commissioning window, and ensuring consistent state management by resetting mActivationPending on provisioning failure and only clearing local state after a successful asynchronous deprovisioning invocation.
65025df to
7de5d04
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new MatterUbusService to expose Matter onboarding information and commissioning window control via ubus, integrates it into the Linux network-manager-app, and updates the Thread Border Router delegates to support pending datasets, asynchronous provisioning/deprovisioning, and reverting active datasets. The review feedback highlights critical issues with asynchronous ubus invocations in ThreadBROpenThreadUbus.cpp, where registering callbacks after ubus_invoke_async introduces a race condition, and calling ubus_complete_request_async manually is redundant and potentially corruptive. Additionally, a potential use-after-free vulnerability in the callback context deletion was identified, and a type mismatch in MatterUbusService.cpp where System::Clock::Seconds16 should be used instead of System::Clock::Seconds32 was pointed out.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the MatterUbusService to expose onboarding information and commissioning window control over ubus, and updates the OpenThread ubus border router delegate to support asynchronous dataset provisioning, pending datasets, and reverting active datasets. The review feedback highlights a critical safety concern: if the OpenThreadUbusBorderRouterDelegate is destroyed or reverted while an asynchronous provision request is still in-flight, it could lead to a Use-After-Free (UAF) or dangling pointer. The reviewer suggests tracking the active request via a new mPendingProvision member, implementing a destructor to abort and clean up the request, and updating RevertActiveDataset accordingly.
|
PR #73309: Size comparison from 8f38beb to 7de5d04aa11a4d142ff52427f1f9fc608bbbc1a9 Full report (12 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, realtek, stm32)
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new MatterUbusService to publish a "matter" ubus object for local commissioning control, and enhances the OpenThread ubus border router delegate to support pending datasets, asynchronous provisioning, and dataset reversion. Feedback was provided on ThreadBROpenThreadUbus.cpp to address a potential use-after-free (UAF) vulnerability when a provision request is in-flight during delegate destruction, and to optimize attribute change notifications by avoiding redundant reports when the pending dataset has not actually changed.
b4b0c5b to
0dd5a44
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the MatterUbusService to expose onboarding information and commissioning window control over ubus, and updates the OpenThread ubus border router delegate to support pending datasets, asynchronous provisioning/deprovisioning, and dataset reversion. The review feedback highlights critical thread-safety issues across multiple ubus handlers and callbacks where the CHIP stack lock must be acquired before accessing Matter APIs or modifying delegate state. Additionally, a compilation error was identified in HandleOpenWindow due to a type mismatch when passing the timeout parameter.
|
PR #73309: Size comparison from 72ecdc2 to 0dd5a445f0b4e85efad7eaed92720999919c9f82 Full report (3 builds for realtek, stm32)
|
0dd5a44 to
ee50078
Compare
|
PR #73309: Size comparison from 72ecdc2 to ee50078 Full report (33 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #73309 +/- ##
=======================================
Coverage 56.31% 56.31%
=======================================
Files 1869 1869
Lines 121761 121761
Branches 14301 14301
=======================================
Hits 68571 68571
Misses 53190 53190 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
PR #73309: Size comparison from 9463786 to c6ffb47 Full report (12 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, realtek, stm32)
|
c6ffb47 to
aa52470
Compare
|
PR #73309: Size comparison from 9463786 to aa52470 Full report (33 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
@coderabbitai review |
|
aa52470 to
8fe1864
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
PR #73309: Size comparison from b791201 to 8fe1864 Full report (6 builds for cc32xx, nrfconnect, realtek, stm32)
|
Patch 038 replaces the demo stubs behind the NIM device type with live router state: the Wi-Fi Network Management cluster shares the LAN access point credentials read from netifd (guest networks excluded; wifi_share, wifi_network and wifi_iface in /etc/config/matter adjust or disable the sharing), Thread Network Diagnostics is fed from otbr-agent over ubus, the Thread Network Directory is seeded with the border router own network, and Ethernet Network Diagnostics reports the uplink counters. The service gains a procd reload trigger on wireless/network config changes that pokes the daemon over ubus, and the ubusd ACL grants the matter user read access to exactly network.wireless status. Applying a new ubusd ACL needs a ubusd restart, which breaks clients holding the old connection (rpcd, lighttpd, foris) - a reboot after install is the clean path. Patch 035 first puts the example's OpenWrt integration behind a backend interface, so the patches that follow land as backend code rather than as conditional blocks in main.cpp; it is proposed upstream on its own, stacked on project-chip/connectedhomeip#73309. Assisted-By: Claude Fable 5
Patch 038 replaces the demo stubs behind the NIM device type with live router state: the Wi-Fi Network Management cluster shares the LAN access point credentials read from netifd (guest networks excluded; wifi_share, wifi_network and wifi_iface in /etc/config/matter adjust or disable the sharing), Thread Network Diagnostics is fed from otbr-agent over ubus, the Thread Network Directory is seeded with the border router own network, and Ethernet Network Diagnostics reports the uplink counters. The service gains a procd reload trigger on wireless/network config changes that pokes the daemon over ubus, and the ubusd ACL grants the matter user read access to exactly network.wireless status. Applying a new ubusd ACL needs a ubusd restart, which breaks clients holding the old connection (rpcd, lighttpd, foris) - a reboot after install is the clean path. Patch 035 first puts the example's OpenWrt integration behind a backend interface, so the patches that follow land as backend code rather than as conditional blocks in main.cpp; it is proposed upstream on its own, stacked on project-chip/connectedhomeip#73309. Assisted-By: Claude Fable 5 Signed-off-by: Christian Glombek <c.glombek@cosa.systems>
openthread-br: the host groundwork is now the single patch 121, since openthread/ot-br-posix#3489 was squashed to one commit at its reviewer's request; the ubus methods 130-136 are regenerated on top of it, unchanged in content. The nftables series 300-303 follows openthread/ot-br-posix#3325 as of 2026-08-23: a build with the backend now installs a marker file the legacy firewall scripts read, so the two cannot drift apart when the OTBR_NFTABLES environment variable is forgotten. PKG_RELEASE bumped so the rebuilt package is picked up. matter-netman: pinned to connectedhomeip master of 2026-08-21 (b7912017), which carries project-chip/connectedhomeip#73343, so its patch goes. The setup-passcode log patch goes too, its upstream PR having been closed. The carried patches are regenerated from the upstream branches as they stand: 035 puts the example's OpenWrt integration behind a backend interface (stacked on project-chip/connectedhomeip#73309), and the NIM patches that follow land as backend code rather than as conditional blocks in main.cpp; 036-038 shift into the freed slots, 039-046 keep their numbers. The same files ship in openwrt/packages and project-chip/matter-openwrt#53. Assisted-By: Claude Fable 5 Signed-off-by: Christian Glombek <c.glombek@cosa.systems>
openthread-br: the host groundwork is now the single patch 121, since openthread/ot-br-posix#3489 was squashed to one commit at its reviewer's request; the ubus methods 130-136 are regenerated on top of it, unchanged in content. The nftables series 300-303 follows openthread/ot-br-posix#3325 as of 2026-08-23: a build with the backend now installs a marker file the legacy firewall scripts read, so the two cannot drift apart when the OTBR_NFTABLES environment variable is forgotten. PKG_RELEASE bumped so the rebuilt package is picked up. matter-netman: pinned to connectedhomeip master of 2026-08-21 (b7912017), which carries project-chip/connectedhomeip#73343, so its patch goes. The setup-passcode log patch goes too, its upstream PR having been closed. The carried patches are regenerated from the upstream branches as they stand: 035 puts the example's OpenWrt integration behind a backend interface (stacked on project-chip/connectedhomeip#73309), and the NIM patches that follow land as backend code rather than as conditional blocks in main.cpp; 036-038 shift into the freed slots, 039-046 keep their numbers. The same files ship in openwrt/packages and project-chip/matter-openwrt#53. Assisted-By: Claude Fable 5 Signed-off-by: Christian Glombek <c.glombek@cosa.systems> (cherry picked from commit c2dfe5d)
openthread-br: the host groundwork is now the single patch 121, since openthread/ot-br-posix#3489 was squashed to one commit at its reviewer's request; the ubus methods 130-136 are regenerated on top of it, unchanged in content. The nftables series 300-303 follows openthread/ot-br-posix#3325 as of 2026-08-23: a build with the backend now installs a marker file the legacy firewall scripts read, so the two cannot drift apart when the OTBR_NFTABLES environment variable is forgotten. PKG_RELEASE bumped so the rebuilt package is picked up. matter-netman: pinned to connectedhomeip master of 2026-08-21 (b7912017), which carries project-chip/connectedhomeip#73343, so its patch goes. The setup-passcode log patch goes too, its upstream PR having been closed. The carried patches are regenerated from the upstream branches as they stand: 035 puts the example's OpenWrt integration behind a backend interface (stacked on project-chip/connectedhomeip#73309), and the NIM patches that follow land as backend code rather than as conditional blocks in main.cpp; 036-038 shift into the freed slots, 039-046 keep their numbers. The same files ship in openwrt/packages and project-chip/matter-openwrt#53. Assisted-By: Claude Fable 5 Signed-off-by: Christian Glombek <c.glombek@cosa.systems>
openthread-br: the host groundwork is now the single patch 121, since openthread/ot-br-posix#3489 was squashed to one commit at its reviewer's request; the ubus methods 130-136 are regenerated on top of it, unchanged in content. The nftables series 300-303 follows openthread/ot-br-posix#3325 as of 2026-08-23: a build with the backend now installs a marker file the legacy firewall scripts read, so the two cannot drift apart when the OTBR_NFTABLES environment variable is forgotten. PKG_RELEASE bumped so the rebuilt package is picked up. matter-netman: pinned to connectedhomeip master of 2026-08-21 (b7912017), which carries project-chip/connectedhomeip#73343, so its patch goes. The setup-passcode log patch goes too, its upstream PR having been closed. The carried patches are regenerated from the upstream branches as they stand: 035 puts the example's OpenWrt integration behind a backend interface (stacked on project-chip/connectedhomeip#73309), and the NIM patches that follow land as backend code rather than as conditional blocks in main.cpp; 036-038 shift into the freed slots, 039-046 keep their numbers. The same files ship in openwrt/packages and project-chip/matter-openwrt#53. Assisted-By: Claude Fable 5 Signed-off-by: Christian Glombek <c.glombek@cosa.systems> (cherry picked from commit 2358537)
Patch 038 replaces the demo stubs behind the NIM device type with live router state: the Wi-Fi Network Management cluster shares the LAN access point credentials read from netifd (guest networks excluded; wifi_share, wifi_network and wifi_iface in /etc/config/matter adjust or disable the sharing), Thread Network Diagnostics is fed from otbr-agent over ubus, the Thread Network Directory is seeded with the border router own network, and Ethernet Network Diagnostics reports the uplink counters. The service gains a procd reload trigger on wireless/network config changes that pokes the daemon over ubus, and the ubusd ACL grants the matter user read access to exactly network.wireless status. Applying a new ubusd ACL needs a ubusd restart, which breaks clients holding the old connection (rpcd, lighttpd, foris) - a reboot after install is the clean path. Patch 035 first puts the example's OpenWrt integration behind a backend interface, so the patches that follow land as backend code rather than as conditional blocks in main.cpp; it is proposed upstream on its own, stacked on project-chip/connectedhomeip#73309. Assisted-By: Claude Fable 5
The fake border router returned NOT_IMPLEMENTED, so fail-safe rollback could not be exercised without real hardware. SetActiveDataset is only accepted when no dataset is configured, so reverting means returning to the unconfigured state rather than restoring a previous dataset. Clear it and report the timestamp change. Assisted-By: Claude Opus 5
The ubus delegate advertised no PANChange support and answered SetPendingDataset and RevertActiveDataset with NOT_IMPLEMENTED, so a controller could form a Thread network through it but never change one that was already running, and a fail-safe expiry left the border router holding a dataset the controller had abandoned. otbr now exposes the matching operations over ubus, so: - GetPanChangeSupported() returns true, which adds the PANChange feature bit and SetPendingDatasetRequest to the accepted command list. - SetPendingDataset() invokes set_pending, which schedules a migration; every node switches when the dataset's delay timer expires. - RevertActiveDataset() invokes deprovision, which detaches and erases the dataset. Note this is not leave, which factory resets the instance. - The pending dataset reported by status and by the pending_dataset_changed notification is cached and served through GetDataset(), so PendingDatasetTimestamp and GetPendingDatasetRequest work. An empty payload means the migration completed, and clears it. Assisted-By: Claude Opus 5
otbr does not reply to provision until the device has attached, and deprovision detaches gracefully before erasing, so both replies can be tens of seconds away. The delegate invoked them with a blocking ubus_invoke and a two-second timeout: SetActiveDataset always reported FAILURE to the Matter controller while the border router went on to form the network anyway, and the fail-safe path risked the same. Invoke both asynchronously. Activation success is already driven by the device_role_changed notification once the device attaches; the reply now only matters when provision is rejected outright, which fails the activation immediately. Revert clears the cached state right away and lets the notifications resync, since returning to unprovisioned is the outcome either way. Assisted-By: Claude Opus 5
The daemon's onboarding code only lives in its log, and once the device is commissioned nothing on the router itself can pair it with another controller: the initial code stops being valid and opening a window takes a Matter administrator. Publish a "matter" ubus object with the device's own view of all of this: - status: fabric count, commissioning window state, and the onboarding payload (manual pairing code, QR code, ids), so a router UI can show the real thing instead of deriving it from configuration files. - open_commissioning_window / close_commissioning_window: local control of a basic commissioning window, during which the device's own onboarding code authenticates, so the code the UI shows is usable even after the device has been commissioned. The object is re-published after ubus reconnects; ubusd needs a publish ACL for it, which the packaging installs. Assisted-By: Claude Opus 5
Two commissioning-flow fixes surfaced by pairing the border router with an iOS controller: The fail-safe expiry handler in the TBRM cluster reverts the active dataset unconditionally, for every expired fail-safe. A commissioning attempt that fails for unrelated reasons, such as an attestation policy rejection, therefore wiped a Thread network the router had been provisioned with outside Matter. Track whether an uncommitted dataset activation exists and make RevertActiveDataset a no-op otherwise, which is what reverting means. The commissionable DNS-SD advertisement carried no device name, so commissioners offered a generic "Matter Accessory" placeholder when pairing. Advertise the configured name. Assisted-By: Claude Opus 5
8fe1864 to
19b112e
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
PR #73309: Size comparison from 6b6737b to 19b112e Full report (34 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
openthread-br: the host groundwork is now the single patch 121, since openthread/ot-br-posix#3489 was squashed to one commit at its reviewer's request; the ubus methods 130-136 are regenerated on top of it, unchanged in content. The nftables series 300-303 follows openthread/ot-br-posix#3325 as of 2026-08-23: a build with the backend now installs a marker file the legacy firewall scripts read, so the two cannot drift apart when the OTBR_NFTABLES environment variable is forgotten. PKG_RELEASE bumped so the rebuilt package is picked up. matter-netman: pinned to connectedhomeip master of 2026-08-21 (b7912017), which carries project-chip/connectedhomeip#73343, so its patch goes. The setup-passcode log patch goes too, its upstream PR having been closed. The carried patches are regenerated from the upstream branches as they stand: 035 puts the example's OpenWrt integration behind a backend interface (stacked on project-chip/connectedhomeip#73309), and the NIM patches that follow land as backend code rather than as conditional blocks in main.cpp; 036-038 shift into the freed slots, 039-046 keep their numbers. The same files ship in openwrt/packages and project-chip/matter-openwrt#53. Assisted-By: Claude Fable 5 Signed-off-by: Christian Glombek <c.glombek@cosa.systems> (cherry picked from commit 2358537)
openthread-br: the host groundwork is now the single patch 121, since openthread/ot-br-posix#3489 was squashed to one commit at its reviewer's request; the ubus methods 130-136 are regenerated on top of it, unchanged in content. The nftables series 300-303 follows openthread/ot-br-posix#3325 as of 2026-08-23: a build with the backend now installs a marker file the legacy firewall scripts read, so the two cannot drift apart when the OTBR_NFTABLES environment variable is forgotten. PKG_RELEASE bumped so the rebuilt package is picked up. matter-netman: pinned to connectedhomeip master of 2026-08-21 (b7912017), which carries project-chip/connectedhomeip#73343, so its patch goes. The setup-passcode log patch goes too, its upstream PR having been closed. The carried patches are regenerated from the upstream branches as they stand: 035 puts the example's OpenWrt integration behind a backend interface (stacked on project-chip/connectedhomeip#73309), and the NIM patches that follow land as backend code rather than as conditional blocks in main.cpp; 036-038 shift into the freed slots, 039-046 keep their numbers. The same files ship in openwrt/packages and project-chip/matter-openwrt#53. Assisted-By: Claude Fable 5 Signed-off-by: Christian Glombek <c.glombek@cosa.systems>
openthread-br: the host groundwork is now the single patch 121, since openthread/ot-br-posix#3489 was squashed to one commit at its reviewer's request; the ubus methods 130-136 are regenerated on top of it, unchanged in content. The nftables series 300-303 follows openthread/ot-br-posix#3325 as of 2026-08-23: a build with the backend now installs a marker file the legacy firewall scripts read, so the two cannot drift apart when the OTBR_NFTABLES environment variable is forgotten. PKG_RELEASE bumped so the rebuilt package is picked up. matter-netman: pinned to connectedhomeip master of 2026-08-21 (b7912017), which carries project-chip/connectedhomeip#73343, so its patch goes. The setup-passcode log patch goes too, its upstream PR having been closed. The carried patches are regenerated from the upstream branches as they stand: 035 puts the example's OpenWrt integration behind a backend interface (stacked on project-chip/connectedhomeip#73309), and the NIM patches that follow land as backend code rather than as conditional blocks in main.cpp; 036-038 shift into the freed slots, 039-046 keep their numbers. The same files ship in openwrt/packages and project-chip/matter-openwrt#53. Assisted-By: Claude Fable 5 Signed-off-by: Christian Glombek <c.glombek@cosa.systems> (cherry picked from commit 2358537)
openthread-br: the host groundwork is now the single patch 121, since openthread/ot-br-posix#3489 was squashed to one commit at its reviewer's request; the ubus methods 130-136 are regenerated on top of it, unchanged in content. The nftables series 300-303 follows openthread/ot-br-posix#3325 as of 2026-08-23: a build with the backend now installs a marker file the legacy firewall scripts read, so the two cannot drift apart when the OTBR_NFTABLES environment variable is forgotten. PKG_RELEASE bumped so the rebuilt package is picked up. matter-netman: pinned to connectedhomeip master of 2026-08-21 (b7912017), which carries project-chip/connectedhomeip#73343, so its patch goes. The setup-passcode log patch goes too, its upstream PR having been closed. The carried patches are regenerated from the upstream branches as they stand: 035 puts the example's OpenWrt integration behind a backend interface (stacked on project-chip/connectedhomeip#73309), and the NIM patches that follow land as backend code rather than as conditional blocks in main.cpp; 036-038 shift into the freed slots, 039-046 keep their numbers. The same files ship in openwrt/packages and project-chip/matter-openwrt#53. Assisted-By: Claude Fable 5 Signed-off-by: Christian Glombek <c.glombek@cosa.systems>
Summary
The Linux network-manager example can already delegate Thread management to an external otbr-agent over ubus on OpenWrt. This series makes that delegation handle the full network lifecycle:
RevertActiveDatasetin the fake delegate (parity for testing)SetPendingDatasetRequestforwards to otbr'sset_pending; completion is tracked via thepending_dataset_changednotificationmatterubus object: commissioning-window control, onboarding state for a web UI, and areload_wifipokeThe otbr-side ubus counterpart is being upstreamed to openthread/ot-br-posix (openthread/ot-br-posix#3489 plus a follow-up series; author coordination in project-chip/matter-openwrt#53, where these commits ship as the matter-netman 030–035 patches).
Requires: openthread/ot-br-posix#3489
Requires: ot-br-posix patches 100-106,110 from https://github.com/project-chip/matter-openwrt/tree/main/third_party/openthread-br/patches (PR to be opened by @ksperling-apple)
Suggested reviewer: @ksperling-apple
Related issues
project-chip/matter-openwrt#53
Testing
In daily production use on an OpenWrt (Turris Omnia) border router as a Matter NIM device: commissioned into a Home Assistant fabric; Thread Border Router Management reads/writes;
RevertActiveDatasetunder the fail-safe; and a live whole-network Thread migration (~30 devices) driven through the pending-dataset path end to end.matter-network-manager-appbuilt from this branch with GN (host Linux).