[ncp] add border routing InfraIf recv ICMP6 ND #10755

Irving-cl · 2024-09-26T01:54:26Z

This PR implements receiving InfraIf Icmp6 Nd messages on NCP.

This is to support border routing for NCP case. See this openthread/ot-br-posix#2398 (comment) for the whole picture of this series of changes.

In this PR, a new spinel property SPINEL_PROP_INFRA_IF_RECV_ICMP6_ND is added for the host to pass the ICMP6 ND messages to the NCP.

size-report · 2024-09-26T01:59:22Z

Size Report of OpenThread

Merging #10755 into main(6e46e2e).

name	branch	text	data	bss	total
ot-cli-ftd	main	469976	856	66636	537468
	#10755	469976	856	66636	537468
	+/-	0	0	0	0
ot-ncp-ftd	main	438468	760	61864	501092
	#10755	438468	760	61864	501092
	+/-	0	0	0	0
libopenthread-ftd.a	main	238132	95	40422	278649
	#10755	238132	95	40422	278649
	+/-	0	0	0	0
libopenthread-cli-ftd.a	main	58802	0	8075	66877
	#10755	58802	0	8075	66877
	+/-	0	0	0	0
libopenthread-ncp-ftd.a	main	32605	0	5924	38529
	#10755	32605	0	5924	38529
	+/-	0	0	0	0
ot-cli-mtd	main	366744	760	51452	418956
	#10755	366744	760	51452	418956
	+/-	0	0	0	0
ot-ncp-mtd	main	349388	760	46696	396844
	#10755	349388	760	46696	396844
	+/-	0	0	0	0
libopenthread-mtd.a	main	160201	0	25254	185455
	#10755	160201	0	25254	185455
	+/-	0	0	0	0
libopenthread-cli-mtd.a	main	39893	0	8059	47952
	#10755	39893	0	8059	47952
	+/-	0	0	0	0
libopenthread-ncp-mtd.a	main	25413	0	5924	31337
	#10755	25413	0	5924	31337
	+/-	0	0	0	0
ot-cli-ftd-br	main	555128	864	131500	687492
	#10755	555128	864	131500	687492
	+/-	0	0	0	0
libopenthread-ftd-br.a	main	327362	100	105262	432724
	#10755	327362	100	105262	432724
	+/-	0	0	0	0
libopenthread-cli-ftd-br.a	main	72930	0	8099	81029
	#10755	72930	0	8099	81029
	+/-	0	0	0	0
ot-rcp	main	62768	564	20788	84120
	#10755	62768	564	20788	84120
	+/-	0	0	0	0
libopenthread-rcp.a	main	9750	0	5060	14810
	#10755	9750	0	5060	14810
	+/-	0	0	0	0
libopenthread-radio.a	main	19338	0	222	19560
	#10755	19338	0	222	19560
	+/-	0	0	0	0

codecov · 2024-09-26T03:02:18Z

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 71.65%. Comparing base (6e46e2e) to head (76c6f17).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/ncp/ncp_base_ftd.cpp	0.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10755      +/-   ##
==========================================
- Coverage   74.57%   71.65%   -2.92%     
==========================================
  Files         609      607       -2     
  Lines       84911    89163    +4252     
==========================================
+ Hits        63321    63889     +568     
- Misses      21590    25274    +3684

Files with missing lines	Coverage Δ
src/ncp/ncp_base.hpp	`9.78% <ø> (-90.22%)`	⬇️
src/ncp/ncp_base_dispatcher.cpp	`100.00% <ø> (ø)`
src/ncp/platform/infra_if.cpp	`33.33% <ø> (ø)`
src/ncp/ncp_base_ftd.cpp	`5.41% <0.00%> (-0.10%)`	⬇️

... and 188 files with indirect coverage changes

zhanglongxia

LGTM

bukepo · 2024-09-26T08:03:16Z

src/lib/spinel/spinel.h

+     * `6`: The IP6 source address of the received message.
+     * `d`: The data of the received message.
+     */
+    SPINEL_PROP_INFRA_IF_RECV_ICMP6_ND = SPINEL_PROP_INFRA_IF__BEGIN + 2,


should we make this general ICMPv6 packets?

Suggested change

SPINEL_PROP_INFRA_IF_RECV_ICMP6_ND = SPINEL_PROP_INFRA_IF__BEGIN + 2,

SPINEL_PROP_INFRA_IF_RECV_ICMP6 = SPINEL_PROP_INFRA_IF__BEGIN + 2,

I don't think so. Because after NCP receives this message, otPlatInfraIfRecvIcmp6Nd should be called which only handles ND messages.

If we want to support other Icmp6 messages in the future, we will need to add extra parsing and handling code in NCP (check if this is a ND or other ICMP6 messages), which introduces extra dependency and adds the complexity of NCP. Because OT core can already handle these, we should let OT core handle them. Thoughts?

@bukepo As discussed, use this property for all ICMP6 messages instead of only for ND messages. Added a field in this property for the ICMP6 message type.

Sounds okay to me but I am just curios what are some use-cases of non-ND type ICMPv6 messages from infra-if that may be useful?

For now there is no use case of non-ND type ICMPv6 messages from infra-if. I think we can probably leave the extensibility here for the future.

@abtink Currently, a dedicated host socket is created for joining any multicast group that a node in Thread network subscribes to(see code). It is a trick to leverage Linux kernel's existing MLDv2 implementation to send and process MLDv2 messages. However, the trick actually causes the host itself to subscribe to those multicast groups while the host itself probably isn't interested in them. As a result, unwanted multicast traffic is delivered to the input chain of the host's IPv6 stack. A more elegant way may be handling the MLDv2 messages by OpenThread itself, which avoids creating that host socket. That would require OT to receive and send MLDv2 messages which are also based on ICMPv6.

abtink

LGTM. Thanks.

One suggestion below about adding #if guard?

src/ncp/ncp_base_ftd.cpp

abtink · 2024-09-27T07:40:24Z

src/lib/spinel/spinel.h

+     * `6`: The IP6 source address of the received message.
+     * `d`: The data of the received message.
+     */
+    SPINEL_PROP_INFRA_IF_RECV_ICMP6_ND = SPINEL_PROP_INFRA_IF__BEGIN + 2,


Sounds okay to me but I am just curios what are some use-cases of non-ND type ICMPv6 messages from infra-if that may be useful?

bukepo · 2024-09-27T15:09:53Z

src/ncp/ncp_base_ftd.cpp

+    SuccessOrExit(error = mDecoder.ReadIp6Address(address));
+    SuccessOrExit(error = mDecoder.ReadData(icmp6Data, len));
+
+    if (msgType == SPINEL_IPV6_ICMP_TYPE_ND)


this is probably unnecessary, as otPlatInfraIfRecvIcmp6Nd will check the message type. having it may add extra work to the host.

Currently this line is unnecessary. But without it, the code seem confusing. As we defined SPINEL_PROP_INFRA_IF_RECV_ICMP6, it could be potentially non ND messages. If we simply call otPlatInfraIfRecvIcmp6Nd, the code is kind of weird. If we have other ICMP6 case in the future, we should use a switch here. Thoughts? @bukepo

otPlatInfraIfRecvIcmp6Nd() needs to check whether the packet is ND anyway. If you check the current code, it doesn't verify whether it's ND before delivering them to this API either. We just trust the kernel will do the right thing.

Make sense. We can update otPlatInfraIfRecvIcmp6Nd to otPlatInfraIfRecvIcmp6 when there are other icmp6 messages. I added a comment here to avoid confusion.

otPlatInfraIfRecvIcmp6Nd() explicitly states that the platform should call it with ND messages:

/** * The infra interface driver calls this method to notify OpenThread * that an ICMPv6 Neighbor Discovery message is received.

If a platform implementation doesn't adhere to this, I recommend updating or fixing the platform code.

We can update otPlatInfraIfRecvIcmp6Nd to otPlatInfraIfRecvIcmp6

We cannot and should not rename or change existing APIs to ensure backward compatibility. OT public/platform APIs are our contract with vendors/users. We need a very strong justification for any changes and only allow changes after exploring all other options/solutions.

I'll keep that in mind. We won't and shouldn't change otPlatInfraIfRecvIcmp6Nd for this PR or in the future. I didn't think it clearly enough earlier.

Irving-cl added comp: ncp P2 comp: spinel labels Sep 26, 2024

Irving-cl requested review from bukepo and zhanglongxia September 26, 2024 01:54

Irving-cl mentioned this pull request Sep 26, 2024

NCP support in ot-br-posix openthread/ot-br-posix#2398

Open

17 tasks

zhanglongxia approved these changes Sep 26, 2024

View reviewed changes

bukepo reviewed Sep 26, 2024

View reviewed changes

Irving-cl requested a review from bukepo September 27, 2024 03:54

Irving-cl force-pushed the border_routing_ncp_infraif_recv_icmp6nd branch 3 times, most recently from f88cd26 to 6ff51db Compare September 27, 2024 07:28

abtink approved these changes Sep 27, 2024

View reviewed changes

Irving-cl force-pushed the border_routing_ncp_infraif_recv_icmp6nd branch 2 times, most recently from 8640e2f to 6fdf90a Compare September 27, 2024 09:57

bukepo reviewed Sep 27, 2024

View reviewed changes

Irving-cl force-pushed the border_routing_ncp_infraif_recv_icmp6nd branch from 6fdf90a to 2596ec6 Compare September 29, 2024 02:34

Irving-cl requested a review from bukepo September 29, 2024 02:36

Irving-cl force-pushed the border_routing_ncp_infraif_recv_icmp6nd branch from 2596ec6 to 15862a2 Compare September 29, 2024 02:36

bukepo approved these changes Sep 29, 2024

View reviewed changes

Irving-cl force-pushed the border_routing_ncp_infraif_recv_icmp6nd branch from 15862a2 to 2610928 Compare September 29, 2024 04:14

Irving-cl requested a review from jwhui September 29, 2024 06:02

Irving-cl force-pushed the border_routing_ncp_infraif_recv_icmp6nd branch from 2610928 to 953bfb9 Compare September 30, 2024 02:20

[ncp] add border routing InfraIf recv ICMP6 ND

76c6f17

Irving-cl force-pushed the border_routing_ncp_infraif_recv_icmp6nd branch from 953bfb9 to 76c6f17 Compare September 30, 2024 03:20

jwhui approved these changes Sep 30, 2024

View reviewed changes

jwhui merged commit a6d6073 into openthread:main Sep 30, 2024
103 checks passed

Irving-cl deleted the border_routing_ncp_infraif_recv_icmp6nd branch October 8, 2024 03:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ncp] add border routing InfraIf recv ICMP6 ND #10755

[ncp] add border routing InfraIf recv ICMP6 ND #10755

Irving-cl commented Sep 26, 2024

size-report bot commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading

zhanglongxia left a comment

bukepo Sep 26, 2024

Irving-cl Sep 27, 2024

Irving-cl Sep 27, 2024

abtink Sep 27, 2024

Irving-cl Sep 27, 2024

bukepo Sep 27, 2024

abtink left a comment

abtink Sep 27, 2024

bukepo Sep 27, 2024 •

edited

Loading

Irving-cl Sep 29, 2024

bukepo Sep 29, 2024

Irving-cl Sep 29, 2024

abtink Sep 29, 2024

Irving-cl Sep 30, 2024 •

edited

Loading

	SPINEL_PROP_INFRA_IF_RECV_ICMP6_ND = SPINEL_PROP_INFRA_IF__BEGIN + 2,
	SPINEL_PROP_INFRA_IF_RECV_ICMP6 = SPINEL_PROP_INFRA_IF__BEGIN + 2,

[ncp] add border routing InfraIf recv ICMP6 ND #10755

[ncp] add border routing InfraIf recv ICMP6 ND #10755

Conversation

Irving-cl commented Sep 26, 2024

size-report bot commented Sep 26, 2024 • edited Loading

Size Report of OpenThread

codecov bot commented Sep 26, 2024 • edited Loading

Codecov Report

zhanglongxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abtink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bukepo Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Irving-cl Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

size-report bot commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading

bukepo Sep 27, 2024 •

edited

Loading

Irving-cl Sep 30, 2024 •

edited

Loading