Skip to content

os/: Add IPv6 multicast support and fix BLE callback#7273

Open
abhinav-s235 wants to merge 1 commit intoSamsung:masterfrom
abhinav-s235:matter_fix
Open

os/: Add IPv6 multicast support and fix BLE callback#7273
abhinav-s235 wants to merge 1 commit intoSamsung:masterfrom
abhinav-s235:matter_fix

Conversation

@abhinav-s235
Copy link
Copy Markdown
Contributor

  • IPv6 multicast group join/leave socket options (IPV6_JOIN_GROUP, IPV6_LEAVE_GROUP)

  • BLE callback parameter fix in ble_tizenrt_service.c

  • PREFERENCE_ERROR_NONE enum addition

  • Network interface name retrieval fix in netmgr_ioctl_netdev.c

- IPv6 multicast group join/leave socket options (IPV6_JOIN_GROUP, IPV6_LEAVE_GROUP)

- BLE callback parameter fix in ble_tizenrt_service.c

- PREFERENCE_ERROR_NONE enum addition

- Network interface name retrieval fix in netmgr_ioctl_netdev.c
@seokhun-eom24
Copy link
Copy Markdown
Contributor

Automated nightly Codex PR review
PR: #7273 — os/: Add IPv6 multicast support and fix BLE callback
PR URL: #7273
GitHub updatedAt: 2026-04-15T12:38:19Z
Refreshed (UTC): 2026-04-15 17:38:49 UTC
Refreshed (KST): 2026-04-16 02:38:49 KST
Comment model: single evolving Codex-authored PR comment

PR #7273 — os/: Add IPv6 multicast support and fix BLE callback

This review done by codex. AI reviews can be inaccurate.
UTC: 2026-04-15 17:36 UTC
KST (UTC+9): 2026-04-16 02:36 KST


Repository: Samsung/TizenRT

Base → Head: masterpr/7273

HEAD Commit: 5fc7548aa687ee5ac91eca3b469a41582630fdcb

Scope: Reviewed the GitHub PR diff for the 5 files shown on PR #7273. The fetched pr/7273 branch also contains older ancestor commits that are not part of the GitHub PR diff, so the findings below are scoped to the PR patch shown by GitHub.


🔎 Review Summary

Category Status
Build / Compile Status
Functional Correctness
Validation Coverage ⚠️

Final Verdict: ❗ Request Changes


🚨 Must-Fix Issues

1. The RTL8730E CCCD callback now corrupts the callback ABI instead of fixing it
Item Details
Location os/board/rtl8730e/src/component/bluetooth/example/ble_peripheral/ble_tizenrt_service.c:95
Severity High
Type Functional correctness

Problem
The new CCCD callback call swaps the callback payload into the wrong parameter slot. trble_server_cb_t expects (type, conn, handle, void *arg, uint16_t result, uint16_t pending), but the patch now passes p_cccd_ind->value as arg and forces result to 0. That turns a 16-bit CCCD value into an invalid pointer-sized void *, drops the real profile->arg, and changes the callback contract only for this RTL8730E path.

Impact

  • Existing server callbacks that dereference or compare their registered arg lose their context on every CCCD change.
  • Any callback that interprets result as the CCCD state now always sees 0, so enable/disable handling can silently break.
  • This diverges from the sibling Realtek BLE server path, increasing cross-chip behavior drift.

Evidence

  • os/include/tinyara/net/if/ble.h:314trble_server_cb_t defines the fourth parameter as void *arg and the fifth as uint16_t result.
  • os/board/rtl8730e/src/component/bluetooth/example/ble_peripheral/ble_tizenrt_service.c:95 — the patch now calls cb(..., p_cccd_ind->value, 0, 0).
  • os/board/rtl8730e/src/component/bluetooth/example/ble_peripheral/ble_tizenrt_service.c:540 — the registered callback context is still stored in profile->arg.
  • os/board/rtl8721csm/src/component/common/bluetooth/realtek/sdk/example/ble_scatternet/ble_tizenrt_scatternet_app.c:1980 — the sibling path still passes p_cha_info->arg in the arg slot.

Required Fix
Files to edit:

  • os/board/rtl8730e/src/component/bluetooth/example/ble_peripheral/ble_tizenrt_service.cble_tizenrt_srv_callback

Change outline:

  • Restore p_cha_info->arg as the callback's fourth argument.
  • Propagate the CCCD bitmask through the result argument if that is the intended signal for CCCD updates.
  • Keep the calling convention aligned with trble_server_cb_t and the sibling RTL8721 implementation.

Example patch:

diff --git a/os/board/rtl8730e/src/component/bluetooth/example/ble_peripheral/ble_tizenrt_service.c b/os/board/rtl8730e/src/component/bluetooth/example/ble_peripheral/ble_tizenrt_service.c
@@
-				p_cha_info->cb(TRBLE_ATTR_CB_CCCD, p_cccd_ind->conn_handle, p_cha_info->abs_handle, p_cccd_ind->value, 0, 0);
+				p_cha_info->cb(TRBLE_ATTR_CB_CCCD,
+						p_cccd_ind->conn_handle,
+						p_cha_info->abs_handle,
+						p_cha_info->arg,
+						p_cccd_ind->value,
+						0);

Inference:

  • The use of result for the CCCD bitmask is inferred from the pre-patch call shape and the BK7239N path that stores CCCD state in an event result field; confirm the intended public callback contract before merging.

Validation Method

  • Register a BLE characteristic callback with a non-NULL context pointer and assert that the same pointer is received on a CCCD write.
  • Toggle notify/indicate from a peer and verify that the callback receives the expected CCCD bitmask instead of 0.
2. `IPV6_JOIN_GROUP` / `IPV6_LEAVE_GROUP` ignores the requested interface and operates on every IPv6 netif
Item Details
Location os/net/lwip/src/api/sockets.c:2746
Severity High
Type Functional correctness

Problem
The new IPv6 multicast socket option path ignores struct ipv6_mreq.ipv6mr_interface and always calls mld6_joingroup(IP6_ADDR_ANY6, ...) / mld6_leavegroup(IP6_ADDR_ANY6, ...). In lwIP, IP6_ADDR_ANY6 means "apply to all matching netifs", so one socket option call can join or leave the group on every IPv6-capable interface instead of the interface requested by the caller.

Impact

  • A socket that intends to join a multicast group on one interface can unexpectedly program MLD and MAC filters on all IPv6 interfaces.
  • A leave on one socket can tear down membership that another interface still needs, because the leave path is equally broad.
  • This is a concrete behavioral mismatch with the new API contract: the PR adds ipv6mr_interface, but the implementation never reads it.

Evidence

  • os/net/lwip/src/include/lwip/sockets.h:350-353struct ipv6_mreq includes ipv6mr_interface.
  • os/net/lwip/src/api/sockets.c:2753-2756 — the new code hardcodes IP6_ADDR_ANY6 for both join and leave.
  • os/net/lwip/src/core/ipv6/mld6.c:313-318mld6_joingroup() loops over netif_list and joins any interface when srcaddr is "any".
  • os/net/lwip/src/api/api_msg.c:1831-1835 — the existing netconn_join_leave_group() path already supports passing a specific IPv6 netif address down to MLD.

Required Fix
Files to edit:

  • os/net/lwip/src/api/sockets.clwip_setsockopt_impl

Change outline:

  • Resolve ipv6mr_interface to the intended netif instead of hardcoding IP6_ADDR_ANY6.
  • Pass the specific IPv6 netif address into the existing join/leave helper path so the operation affects only the requested interface.
  • Reject invalid nonzero interface indexes with EADDRNOTAVAIL instead of silently broadening the request.

Example patch:

diff --git a/os/net/lwip/src/api/sockets.c b/os/net/lwip/src/api/sockets.c
@@
-			if (optname == IPV6_JOIN_GROUP) {
-				err_ret = mld6_joingroup(IP6_ADDR_ANY6, &imr->ipv6mr_multiaddr);
-			} else {
-				err_ret = mld6_leavegroup(IP6_ADDR_ANY6, &imr->ipv6mr_multiaddr);
-			}
+			ip_addr_t multi_addr;
+			ip_addr_t if_addr;
+			struct netif *netif = NULL;
+
+			inet6_addr_to_ip6addr(ip_2_ip6(&multi_addr), &imr->ipv6mr_multiaddr);
+			IP_SET_TYPE_VAL(multi_addr, IPADDR_TYPE_V6);
+
+			if (imr->ipv6mr_interface != 0) {
+				netif = lwip_ifindex_to_netif(imr->ipv6mr_interface);
+				if (netif == NULL) {
+					err = EADDRNOTAVAIL;
+					break;
+				}
+				ip_addr_copy_from_ip6(if_addr, *netif_ip6_addr(netif, 0));
+			} else {
+				ip_addr_set_any(IPADDR_TYPE_V6, &if_addr);
+			}
+
+			err_ret = netconn_join_leave_group(sock->conn, &multi_addr, &if_addr,
+					optname == IPV6_JOIN_GROUP ? NETCONN_JOIN : NETCONN_LEAVE);

Inference:

  • lwip_ifindex_to_netif() is illustrative. This tree does not currently expose a direct helper by that name, so the final patch likely needs a small local helper over netif_list plus the repo's netif numbering convention.

Validation Method

  • Bring up two IPv6 netifs, join a multicast group with a nonzero interface index, and verify that only the targeted netif gets MLD state and MAC filter updates.
  • Leave the group from one interface while the other stays subscribed and confirm the second interface remains joined.
3. The IPv6 multicast setsockopt path removed the existing socket-type and optlen guard
Item Details
Location os/net/lwip/src/api/sockets.c:2751
Severity High
Type Safety

Problem
The new code comments out LWIP_SOCKOPT_CHECK_OPTLEN_CONN_PCB_TYPE(sock, optlen, struct ipv6_mreq, NETCONN_UDP_IPV6) and then unconditionally casts optval to struct ipv6_mreq *. That removes the same guard the IPv4 multicast path uses, so malformed optlen values or non-UDP IPv6 sockets can fall into the group-management path.

Impact

  • A short setsockopt() buffer can trigger an out-of-bounds read from optval when the code reads imr->ipv6mr_multiaddr.
  • TCP/raw IPv6 sockets can now reach MLD group-management logic even though the API is intended for UDP multicast sockets.
  • This creates a user-visible contract regression: IPv4 membership options return EINVAL / ENOPROTOOPT in these cases, but the new IPv6 path no longer does.

Evidence

  • os/net/lwip/src/api/sockets.c:2751 — the validation macro is present only as a commented-out line.
  • os/net/lwip/src/api/sockets.c:2748-2749optval is cast immediately to const struct ipv6_mreq *.
  • os/net/lwip/src/api/sockets.c:2653-2661 — the IPv4 membership path performs LWIP_SOCKOPT_CHECK_OPTLEN_CONN_PCB_TYPE(..., struct ip_mreq, NETCONN_UDP) before dereferencing the option payload.
  • os/net/lwip/src/api/sockets.c:188-197 — the macro enforces both minimum buffer length and socket type.

Required Fix
Files to edit:

  • os/net/lwip/src/api/sockets.clwip_setsockopt_impl

Change outline:

  • Restore the LWIP_SOCKOPT_CHECK_OPTLEN_CONN_PCB_TYPE(..., struct ipv6_mreq, NETCONN_UDP_IPV6) guard before touching optval.
  • Keep the IPv6 implementation aligned with the IPv4 multicast path's validation behavior.
  • Return EINVAL for short option payloads and ENOPROTOOPT for non-UDP IPv6 sockets before any MLD side effects occur.

Example patch:

diff --git a/os/net/lwip/src/api/sockets.c b/os/net/lwip/src/api/sockets.c
@@
-			//LWIP_SOCKOPT_CHECK_OPTLEN_CONN_PCB_TYPE(sock, optlen, struct ipv6_mreq, NETCONN_UDP_IPV6);
+			LWIP_SOCKOPT_CHECK_OPTLEN_CONN_PCB_TYPE(sock, optlen, struct ipv6_mreq, NETCONN_UDP_IPV6);

Inference:

  • None. The tree already uses this exact guard for the IPv4 membership path in the same function.

Validation Method

  • Call setsockopt(IPPROTO_IPV6, IPV6_JOIN_GROUP, ...) on a TCP IPv6 socket and confirm it returns ENOPROTOOPT.
  • Call it on a UDP IPv6 socket with optlen < sizeof(struct ipv6_mreq) and confirm it returns EINVAL without touching MLD state.

🟡 Nice-to-Have Improvements

1. Add focused regression coverage for the two new behavior changes that CI does not exercise directly
Item Details
Location os/net/lwip/src/api/sockets.c, os/board/rtl8730e/src/component/bluetooth/example/ble_peripheral/ble_tizenrt_service.c
Severity Medium
Type Validation

Problem
The PR's CI checks are green, but the two risky paths added here are interface-specific IPv6 multicast membership and Realtek CCCD callback argument propagation. Neither path is obviously covered by the existing board build jobs alone.

Impact

  • Merge confidence remains lower than the green build status suggests.
  • Regressions in callback argument ordering or multicast-interface selection can survive until runtime integration.

Recommended Action

  • Add one targeted BLE regression that validates CCCD callback argument ordering on RTL8730E.
  • Add one targeted IPv6 multicast regression that verifies interface-specific join/leave behavior and invalid-argument handling.

Expected Output

  • A small validation matrix or test log showing:
Area Example Target / Check Result
RTL8730E BLE CCCD callback receives registered arg and expected bitmask Pass
lwIP IPv6 IPV6_JOIN_GROUP honors ipv6mr_interface on a UDP IPv6 socket Pass
lwIP IPv6 short/non-UDP setsockopt() inputs return the expected errno Pass

✅ Notable Improvements

✔ The new socket-option constants match the standard Linux values

  • IPV6_JOIN_GROUP and IPV6_LEAVE_GROUP are added as 20 and 21, which matches the common userspace ABI and reduces porting friction for IPv6 multicast applications.

✔ The preference header now exposes an explicit success value

  • PREFERENCE_ERROR_NONE = 0 makes the preference error enum self-describing and aligns the public header with the framework documentation that treats success as zero.

✔ The netmgr SIOCGIFNAME path now consistently returns a running interface name

  • Removing the req->ifr_name[0] == 0 gate fixes the previous "leave caller-provided garbage in place" behavior and makes the helper actually populate the ioctl result.

🧾 Final Assessment

Must-Fix Summary

  • The BLE callback change is not a safe callback-parameter fix; it changes the callback ABI and drops the registered context pointer.
  • The new IPv6 multicast support is not merge-ready because it ignores the requested interface and skips the existing length/type validation guard.

Nice-to-Have Summary

  • After the blocking fixes land, add focused runtime coverage for RTL8730E CCCD callbacks and IPv6 multicast join/leave semantics so future refactors do not regress these paths silently.

Residual Risk

  • Current CI results are green, but they do not disprove the three code-path issues above because all three are runtime-behavior problems behind successful compilation.

📌 Final Verdict

❗ Request Changes

The PR has useful intent, but the BLE callback fix regresses the callback contract and the IPv6 multicast implementation is currently too broad and under-validated for merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants