Better IPv6 handling of AddrAny (which maps to IPv4 if enabled in lwip) #10564

andy31415 · 2021-10-15T14:24:21Z

Problem

IPv6 listening not performed properly when IPv4 is disabled, generally the 'force address type' inside UDP bind handling is hackish:

chip only has a single 'AddrAny' constant. LWIP with IPv4 enabled maps IP_ADDR_ANY to the IPv4 constant
this results in 'toLwipAddr' to generate the IPv4 listen address and that stays as such when IPv4 is disabled (very counter intuitive)
once an IPv4 is set, the GetPCB validation done during 'send' will detect ipv4 and refuse to send packets over the UDP interface.

Change overview

force returning the IPv6 ANY any when IPv4 is disabled in CHIP
revise the hack for IPv6 when IPv4 is enabled for compilation: we still likely want to explicitly use IP6_ADDR_ANY instead of relying on using the v4 address and changing the type (even if it may be equivalent, it is not as clear to prove).

Testing

Manually tested on separate IPv6 validation branch: ESP32 was able to send IPv6 broadcasts at boot with matter mDNS packets and was also able to reply to mDNS queries (sporadically though - I am still debugging this as I suspect some problems with joining or using multicast groups but have yet to determine the side)

…disabled and enabled

todo · 2021-10-15T14:24:23Z

IPAddress ANY has only one constant state, however addrType

connectedhomeip/src/inet/UDPEndPoint.cpp

Lines 138 to 148 in 7814da6

    
           // TODO: IPAddress ANY has only one constant state, however addrType 
        
           // has separate IPV4 and IPV6 'any' settings. This tries to correct 
        
           // for this as LWIP default if IPv4 is compiled in is to consider 
        
           // 'any == any_v4' 
        
           // 
        
           // We may want to consider having separate AnyV4 and AnyV6 constants 
        
           // inside CHIP to resolve this ambiguity 
        
           if (addr.Type() == kIPAddressType_Any) && (addrType == kIPAddressType_IPv6)) { 
        
               ipAddr = *IP6_ADDR_ANY; 
        
           }

This comment was generated by todo based on a `TODO` comment in `7814da6` in #10564. cc @andy31415.

github-actions · 2021-10-15T15:04:52Z

Size increase report for "gn_qpg-example-build" from f2b4dbd

File	Section	File	VM
chip-qpg6100-lighting-example.out	.text	32	32

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_line,0,42
.debug_info,0,35
.text,32,32
.debug_abbrev,0,20
.debug_str,0,2
.debug_loc,0,-15
[Unmapped],0,-32

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

github-actions · 2021-10-15T15:07:49Z

PR #10564: Size comparison from f2b4dbd to 9baf5d1

20 builds

platform	target	config	section	`f2b4dbd`	`9baf5d1`	change
efr32	lighting-app	BRD4161A	.bss	118020	118020	0
			.data	1800	1800	0
			.text	782732	782764	32
	lock-app	BRD4161A	.bss	115892	115892	0
			.data	1760	1760	0
			.text	761988	762020	32
	window-app	BRD4161A	.bss	116212	116212	0
			.data	1764	1764	0
			.text	762896	762928	32
	lighting-app	BRD4161A+rpc	.bss	131348	131348	0
			.data	1852	1852	0
			.text	762476	762508	32
k32w	lock-app	k32w061+debug	.bss	69052	69052	0
			.data	1864	1864	0
			.text	515444	515460	16
	shell	k32w061+debug	.bss	55080	55080	0
			.data	672	672	0
			.text	357172	357204	32
	lighting-app	k32w061+se05x+release	.bss	78560	78560	0
			.data	1900	1900	0
			.text	614116	614148	32
linux	all-clusters-app	debug	.bss	52144	52144	0
			.data	978	978	0
			.data.rel.ro	58368	58368	0
			.dynamic	592	592	0
			.got	4072	4072	0
			.init	27	27	0
			.init_array	504	504	0
			.rodata	135637	135637	0
			.text	1322082	1322082	0
	chip-tool	debug	.bss	17552	17552	0
			.data	1584	1584	0
			.data.rel.ro	83264	83264	0
			.dynamic	592	592	0
			.got	4328	4328	0
			.init	27	27	0
			.init_array	416	416	0
			.rodata	174432	174432	0
			.text	2422901	2422901	0
	ota-provider-app	debug	.bss	37472	37472	0
			.data	752	752	0
			.data.rel.ro	23144	23144	0
			.dynamic	592	592	0
			.got	4008	4008	0
			.init	27	27	0
			.init_array	440	440	0
			.rodata	109720	109720	0
			.text	1009730	1009730	0
	ota-requestor-app	debug	.bss	205728	205728	0
			.data	752	752	0
			.data.rel.ro	24440	24440	0
			.dynamic	592	592	0
			.got	4136	4136	0
			.init	27	27	0
			.init_array	512	512	0
			.rodata	127768	127768	0
			.text	1129394	1129394	0
	shell	debug	.bss	16072	16072	0
			.data	242	242	0
			.data.rel.ro	35120	35120	0
			.dynamic	592	592	0
			.got	3496	3496	0
			.init	27	27	0
			.init_array	336	336	0
			.rodata	71855	71855	0
			.text	568898	568898	0
	tv-app	debug	.bss	216368	216368	0
			.data	2032	2032	0
			.data.rel.ro	55504	55504	0
			.dynamic	592	592	0
			.got	4400	4400	0
			.init	27	27	0
			.init_array	608	608	0
			.rodata	151336	151336	0
			.text	1464034	1464034	0
	bridge-app	debug+rpc	.bss	52880	52880	0
			.data	976	976	0
			.data.rel.ro	25464	25464	0
			.dynamic	592	592	0
			.got	3944	3944	0
			.init	27	27	0
			.init_array	400	400	0
			.rodata	110388	110388	0
			.text	1051349	1051349	0
	lighting-app	debug+rpc	.bss	42200	42200	0
			.data	1106	1106	0
			.data.rel.ro	52208	52208	0
			.dynamic	608	608	0
			.got	4104	4104	0
			.init	27	27	0
			.init_array	528	528	0
			.rodata	127537	127537	0
			.text	`1252498`	`1252498`	0
p6	lock-app	default	.bss	68216	68216	0
			.data	2416	2416	0
			.heap	962712	962712	0
			.text	1126544	1126560	16
qpg	lighting-app	qpg6100+debug	.bss	53536	53536	0
			.data	996	996	0
			.text	486352	486384	32
	lock-app	qpg6100+debug	.bss	52488	52488	0
			.data	952	952	0
			.text	462544	462576	32
	persistent-storage-app	qpg6100+debug	.bss	17778	17778	0
			.data	280	280	0
			.text	102704	102704	0
telink	lighting-app	tlsr9518adk80d	bss	70984	70984	0
			noinit	33216	33216	0
			text	458376	458376	0

2 builds

platform	target	config	section	`f2b4dbd`	`9baf5d1`
mbed	lighting-app	CY8CPROTO_062_4343W+release	.bss	172148	172148
			.data	5464	5464
			.heap	858832	858832
			.text	1220072	1220072
	lock-app	CY8CPROTO_062_4343W+release	.bss	171084	171084
			.data	5432	5432
			.heap	859928	859928
			.text	1198056	1198056

12 builds

platform	target	config	section	`f2b4dbd`	`9baf5d1`	change	% change
esp32	all-clusters-app	c3devkit	.dram0.bss	60264	60264	0	0.0
			.dram0.data	16208	16208	0	0.0
			.flash.rodata	198632	198632	0	0.0
			.flash.text	869794	869798	4	0.0
			.iram0.text	57330	57330	0	0.0
		m5stack	.dram0.bss	62760	62760	0	0.0
			.dram0.data	32084	32084	0	0.0
			.flash.rodata	207076	207076	0	0.0
			.flash.text	900503	900423	-80	-0.0
			.iram0.text	125115	125115	0	0.0
nrfconnect	lighting-app	nrf52840dk_nrf52840	bss	112316	112316	0	0.0
			rodata	97576	97576	0	0.0
			text	577696	577696	0	0.0
	lock-app	nrf52840dk_nrf52840	bss	111348	111348	0	0.0
			rodata	94072	94072	0	0.0
			text	559136	559136	0	0.0
	pigweed-app	nrf52840dk_nrf52840	bss	51772	51772	0	0.0
			rodata	45772	45772	0	0.0
			text	339392	339392	0	0.0
	pump-app	nrf52840dk_nrf52840	bss	111416	111416	0	0.0
			rodata	95052	95052	0	0.0
			text	562308	562308	0	0.0
	pump-controller-app	nrf52840dk_nrf52840	bss	111356	111356	0	0.0
			rodata	94132	94132	0	0.0
			text	558912	558912	0	0.0
	shell	nrf52840dk_nrf52840	bss	107316	107316	0	0.0
			rodata	71640	71640	0	0.0
			text	518908	518908	0	0.0
	lighting-app	nrf52840dk_nrf52840+rpc	bss	108556	108556	0	0.0
			rodata	88360	88360	0	0.0
			text	550900	550900	0	0.0
		nrf5340dk_nrf5340_cpuapp	bss	113688	113688	0	0.0
			rodata	92816	92816	0	0.0
			text	507156	507156	0	0.0
	lock-app	nrf5340dk_nrf5340_cpuapp	bss	112720	112720	0	0.0
			rodata	89332	89332	0	0.0
			text	488592	488592	0	0.0
	shell	nrf5340dk_nrf5340_cpuapp	bss	108300	108300	0	0.0
			rodata	66284	66284	0	0.0
			text	439504	439504	0	0.0

msandstedt · 2021-10-15T15:18:08Z

The sdk seems pretty set on having the single IPAddress::Any. But I think that's fine. However, wouldn't proper behavior be:

IPv4-only stack -> set to INADDR_ANY
IPv6-only stack -> set to in6addr_any
dual stack -> set to in6addr_any, disable IPv6_ONLY

github-actions · 2021-10-15T15:50:03Z

Size increase report for "esp32-example-build" from f2b4dbd

File	Section	File	VM
chip-all-clusters-app.elf	.flash.text	4	4

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_line,0,24
.debug_info,0,18
.debug_abbrev,0,14
.debug_frame,0,4
.flash.text,4,4
.riscv.attributes,0,2
.shstrtab,0,1
[Unmapped],0,-4
.debug_str,0,-6
.debug_loc,0,-16
.symtab,0,-16
.strtab,0,-61

andy31415 · 2021-10-15T16:16:00Z

The sdk seems pretty set on having the single IPAddress::Any. But I think that's fine. However, wouldn't proper behavior be:

IPv4-only stack -> set to INADDR_ANY

IPv6-only stack -> set to in6addr_any

dual stack -> set to in6addr_any, disable IPv6_ONLY

We explicitly do not have IPv4 only stack support due to matter spec. Ideal goal is to always have an IPv6 only stack and I was debugging that when finding this problem.

The issue I have is that when listening for mDNS, it seems I know exactly on what I need to listen (we iterate through all IP addresses and listen on v4/v6 accordingly) and that is important because we have to join the correct multicast group that is also IPv4/v6 specific.

I am guessing the 'addrType' parameter seems like a workaround acknowledging that ::Any was insufficient for listening purposes. Any is however used as a "don't care/undefined" for things like source addresses or our internal DNS discovery setting (saying find any address), so it is not easy to remove. Ideally I think we should not be using ::Any as a bitfield of v4 + v6, however if we are moving towards supporting IPv6 only stacks the hassle to fix this may be too much.

msandstedt · 2021-10-18T15:50:21Z

The sdk seems pretty set on having the single IPAddress::Any. But I think that's fine. However, wouldn't proper behavior be:

IPv4-only stack -> set to INADDR_ANY

IPv6-only stack -> set to in6addr_any

dual stack -> set to in6addr_any, disable IPv6_ONLY

We explicitly do not have IPv4 only stack support due to matter spec. Ideal goal is to always have an IPv6 only stack and I was debugging that when finding this problem.

I am just trying to make the case that buildable configurations should actually work. There's nothing mysterious about dual-stack. It's just more work and more code space. In this case, a single socket bound to any address seems like it has a very obvious meaning in a dual-stack build: it should accept both IPv4 and IPv6 traffic.

But yes, obviously the sdk must support binding to IP6_ADDR_ANY first and foremost, and so this PR makes sense.

Better IPv6 handling of any: support both LWIP conversion if IPv6 is …

7814da6

…disabled and enabled

boring-cyborg bot added the inet label Oct 15, 2021

pullapprove bot requested review from wbschiller, woody-apple, yufengwangca and yunhanw-google October 15, 2021 14:25

pullapprove bot added the review - pending label Oct 15, 2021

kpschoedel approved these changes Oct 15, 2021

View reviewed changes

andy31415 added 2 commits October 15, 2021 10:35

Remove previous settype code

1dc7e89

Fix typo

9baf5d1

yunhanw-google approved these changes Oct 15, 2021

View reviewed changes

jmartinez-silabs approved these changes Oct 16, 2021

View reviewed changes

msandstedt approved these changes Oct 18, 2021

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels Oct 18, 2021

andy31415 merged commit 5987dab into project-chip:master Oct 18, 2021

todo bot mentioned this pull request Oct 18, 2021

IPAddress ANY has only one constant state, however addrType #10617

Closed

andy31415 deleted the ipv6_any_fixes branch October 28, 2021 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better IPv6 handling of AddrAny (which maps to IPv4 if enabled in lwip) #10564

Better IPv6 handling of AddrAny (which maps to IPv4 if enabled in lwip) #10564

andy31415 commented Oct 15, 2021

todo bot commented Oct 15, 2021

github-actions bot commented Oct 15, 2021

github-actions bot commented Oct 15, 2021 •

edited

Loading

msandstedt commented Oct 15, 2021

github-actions bot commented Oct 15, 2021

andy31415 commented Oct 15, 2021

msandstedt commented Oct 18, 2021

Better IPv6 handling of AddrAny (which maps to IPv4 if enabled in lwip) #10564

Better IPv6 handling of AddrAny (which maps to IPv4 if enabled in lwip) #10564

Conversation

andy31415 commented Oct 15, 2021

Problem

Change overview

Testing

todo bot commented Oct 15, 2021

IPAddress ANY has only one constant state, however addrType

This comment was generated by todo based on a TODO comment in 7814da6 in #10564. cc @andy31415.

github-actions bot commented Oct 15, 2021

github-actions bot commented Oct 15, 2021 • edited Loading

msandstedt commented Oct 15, 2021

github-actions bot commented Oct 15, 2021

andy31415 commented Oct 15, 2021

msandstedt commented Oct 18, 2021

This comment was generated by todo based on a `TODO` comment in `7814da6` in #10564. cc @andy31415.

github-actions bot commented Oct 15, 2021 •

edited

Loading