[MPTCP] Re: [PATCH mptcp-next v4 3/3] selftests: mptcp: add IPv4-mapped IPv6 testcases

[MPTCP] Re: [PATCH mptcp-next v4 3/3] selftests: mptcp: add IPv4-mapped IPv6 testcases

[Geliang Tang]

[-- Attachment #1: Type: text/plain, Size: 2820 bytes --]

Hi Matt,

Matthieu Baerts <matthieu.baerts(a)tessares.net> 于2020年12月21日周一 下午7:49写道：
>
> Hi Geliang,
>
> On 21/12/2020 11:16, Geliang Tang wrote:
> > Hi Matt,
> >
> > Matthieu Baerts <matthieu.baerts(a)tessares.net> 于2020年12月19日周六 下午9:07写道：
> >>
> >> From: Geliang Tang <geliangtang(a)gmail.com>
> >>
> >> Here, we make sure we support IPv4-mapped in IPv6 addresses in different
> >> contexts:
>
> (...)
>
> >> +# no subflow IPv6 to v4 address
> >> +reset
> >> +ip netns exec $ns1 ./pm_nl_ctl limits 0 1
> >> +ip netns exec $ns2 ./pm_nl_ctl limits 0 1
> >> +ip netns exec $ns2 ./pm_nl_ctl add dead:beef:2::10.0.3.2 flags subflow
> >
> > dead:beef:2::10.0.3.2 is a invalid address, should we use dead:beef:3::2 here?
>
> I used "dead:beef:2::10.0.3.2" on purpose just to have a valid and
> usable IPv4 address at the end of the v6 one. If we try to use the IPv4
> address at the end of this v6 one, we will send the SYN+MPJ to the
> correct host (no routing issue). With the previous version of this
> patchset (v3), this was creating a subflow with 10.0.3.2!
>

Sorry, I didn't realize you did it on purpose, please keep it here.

> I should certainly add a new comment here.
> Maybe you would prefer an additional test with "dead:beef:3::2" as well?

Please add this additional test.

>
> >> +run_tests $ns1 $ns2 "10.0.1.1"
> >
> > We can drop the quotation marks here, only the IPv4-mapped address need
> > the quotation marks.
>
> Yes, I usually add quotes in Bash for each string but I see we didn't do
> that for the other tests, I will remove that!
>
> > And add "0 0 0 slow" behind it, since the test need to slow down when use
> > the IPv6 address.
>
> Do I need to do that here as well even if we don't create v6 subflows?

I think this can be consistent with other IPv6 tests, but it's up to you :)

Thanks.

-Geliang

>
> >> +chk_join_nr "no JOIN with different families v4-v6" 0 0 0
> >
> > This string is too long, it break the alignment of the output. Can we use
> > "diff" here instead of "different"?
>
> Good point, I didn't check that! I will short "different", good idea!
>
> >> +
> >> +# no subflow IPv4 to v6 address
> >> +reset
> >> +ip netns exec $ns1 ./pm_nl_ctl limits 0 1
> >> +ip netns exec $ns2 ./pm_nl_ctl limits 0 1
> >> +ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow
> >> +run_tests $ns1 $ns2 dead:beef:1::1
> >
> > Add "0 0 0 slow" here.
>
> Same here, is it needed if we don't create any IPv6 subflow?
>
> >> +chk_join_nr "no JOIN with different families v6-v4" 0 0 0
> >
> > This string is too long too.
>
> Will do in v5!
>
> Thanks for the review!
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

[MPTCP] Re: [PATCH mptcp-next v4 3/3] selftests: mptcp: add IPv4-mapped IPv6 testcases

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 2370 bytes --]

Hi Geliang,

On 21/12/2020 11:16, Geliang Tang wrote:
> Hi Matt,
> 
> Matthieu Baerts <matthieu.baerts(a)tessares.net> 于2020年12月19日周六 下午9:07写道：
>>
>> From: Geliang Tang <geliangtang(a)gmail.com>
>>
>> Here, we make sure we support IPv4-mapped in IPv6 addresses in different
>> contexts:

(...)

>> +# no subflow IPv6 to v4 address
>> +reset
>> +ip netns exec $ns1 ./pm_nl_ctl limits 0 1
>> +ip netns exec $ns2 ./pm_nl_ctl limits 0 1
>> +ip netns exec $ns2 ./pm_nl_ctl add dead:beef:2::10.0.3.2 flags subflow
> 
> dead:beef:2::10.0.3.2 is a invalid address, should we use dead:beef:3::2 here?

I used "dead:beef:2::10.0.3.2" on purpose just to have a valid and 
usable IPv4 address at the end of the v6 one. If we try to use the IPv4 
address at the end of this v6 one, we will send the SYN+MPJ to the 
correct host (no routing issue). With the previous version of this 
patchset (v3), this was creating a subflow with 10.0.3.2!

I should certainly add a new comment here.
Maybe you would prefer an additional test with "dead:beef:3::2" as well?

>> +run_tests $ns1 $ns2 "10.0.1.1"
> 
> We can drop the quotation marks here, only the IPv4-mapped address need
> the quotation marks.

Yes, I usually add quotes in Bash for each string but I see we didn't do 
that for the other tests, I will remove that!

> And add "0 0 0 slow" behind it, since the test need to slow down when use
> the IPv6 address.

Do I need to do that here as well even if we don't create v6 subflows?

>> +chk_join_nr "no JOIN with different families v4-v6" 0 0 0
> 
> This string is too long, it break the alignment of the output. Can we use
> "diff" here instead of "different"?

Good point, I didn't check that! I will short "different", good idea!

>> +
>> +# no subflow IPv4 to v6 address
>> +reset
>> +ip netns exec $ns1 ./pm_nl_ctl limits 0 1
>> +ip netns exec $ns2 ./pm_nl_ctl limits 0 1
>> +ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow
>> +run_tests $ns1 $ns2 dead:beef:1::1
> 
> Add "0 0 0 slow" here.

Same here, is it needed if we don't create any IPv6 subflow?

>> +chk_join_nr "no JOIN with different families v6-v4" 0 0 0
> 
> This string is too long too.

Will do in v5!

Thanks for the review!

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

[MPTCP] Re: [PATCH mptcp-next v4 3/3] selftests: mptcp: add IPv4-mapped IPv6 testcases

[Geliang Tang]

[-- Attachment #1: Type: text/plain, Size: 4303 bytes --]

Hi Matt,

Matthieu Baerts <matthieu.baerts(a)tessares.net> 于2020年12月19日周六 下午9:07写道：
>
> From: Geliang Tang <geliangtang(a)gmail.com>
>
> Here, we make sure we support IPv4-mapped in IPv6 addresses in different
> contexts:
>
> - a v4-mapped address is received by the PM and can be used as v4.
> - a v4 address is received by the PM and can be used even with a v4
>   mapped socket.
>
> We also make sure we don't try to establish subflows between v4 and v6
> addresses, e.g. if a real v6 address ends with a valid v4 address.
>
> Co-developed-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
> Signed-off-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index f74cd993b168..eff813819e6a 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -790,6 +790,73 @@ chk_join_nr "remove subflow and signal IPv6" 2 2 2
>  chk_add_nr 1 1
>  chk_rm_nr 1 1
>
> +# subflow IPv4-mapped to IPv4-mapped
> +reset
> +ip netns exec $ns1 ./pm_nl_ctl limits 0 1
> +ip netns exec $ns2 ./pm_nl_ctl limits 0 1
> +ip netns exec $ns2 ./pm_nl_ctl add "::ffff:10.0.3.2" flags subflow
> +run_tests $ns1 $ns2 "::ffff:10.0.1.1"
> +chk_join_nr "single subflow IPv4-mapped" 1 1 1
> +
> +# signal address IPv4-mapped with IPv4-mapped sk
> +reset
> +ip netns exec $ns1 ./pm_nl_ctl limits 0 1
> +ip netns exec $ns2 ./pm_nl_ctl limits 1 1
> +ip netns exec $ns1 ./pm_nl_ctl add "::ffff:10.0.2.1" flags signal
> +run_tests $ns1 $ns2 "::ffff:10.0.1.1"
> +chk_join_nr "signal address IPv4-mapped" 1 1 1
> +chk_add_nr 1 1
> +
> +# subflow v4-map-v6
> +reset
> +ip netns exec $ns1 ./pm_nl_ctl limits 0 1
> +ip netns exec $ns2 ./pm_nl_ctl limits 0 1
> +ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow
> +run_tests $ns1 $ns2 "::ffff:10.0.1.1"
> +chk_join_nr "single subflow v4-map-v6" 1 1 1
> +
> +# signal address v4-map-v6
> +reset
> +ip netns exec $ns1 ./pm_nl_ctl limits 0 1
> +ip netns exec $ns2 ./pm_nl_ctl limits 1 1
> +ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
> +run_tests $ns1 $ns2 "::ffff:10.0.1.1"
> +chk_join_nr "signal address v4-map-v6" 1 1 1
> +chk_add_nr 1 1
> +
> +# subflow v6-map-v4
> +reset
> +ip netns exec $ns1 ./pm_nl_ctl limits 0 1
> +ip netns exec $ns2 ./pm_nl_ctl limits 0 1
> +ip netns exec $ns2 ./pm_nl_ctl add "::ffff:10.0.3.2" flags subflow
> +run_tests $ns1 $ns2 10.0.1.1
> +chk_join_nr "single subflow v6-map-v4" 1 1 1
> +
> +# signal address v6-map-v4
> +reset
> +ip netns exec $ns1 ./pm_nl_ctl limits 0 1
> +ip netns exec $ns2 ./pm_nl_ctl limits 1 1
> +ip netns exec $ns1 ./pm_nl_ctl add "::ffff:10.0.2.1" flags signal
> +run_tests $ns1 $ns2 10.0.1.1
> +chk_join_nr "signal address v6-map-v4" 1 1 1
> +chk_add_nr 1 1
> +
> +# no subflow IPv6 to v4 address
> +reset
> +ip netns exec $ns1 ./pm_nl_ctl limits 0 1
> +ip netns exec $ns2 ./pm_nl_ctl limits 0 1
> +ip netns exec $ns2 ./pm_nl_ctl add dead:beef:2::10.0.3.2 flags subflow

dead:beef:2::10.0.3.2 is a invalid address, should we use dead:beef:3::2 here?

> +run_tests $ns1 $ns2 "10.0.1.1"

We can drop the quotation marks here, only the IPv4-mapped address need
the quotation marks.

And add "0 0 0 slow" behind it, since the test need to slow down when use
the IPv6 address.

> +chk_join_nr "no JOIN with different families v4-v6" 0 0 0

This string is too long, it break the alignment of the output. Can we use
"diff" here instead of "different"?

> +
> +# no subflow IPv4 to v6 address
> +reset
> +ip netns exec $ns1 ./pm_nl_ctl limits 0 1
> +ip netns exec $ns2 ./pm_nl_ctl limits 0 1
> +ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow
> +run_tests $ns1 $ns2 dead:beef:1::1

Add "0 0 0 slow" here.

> +chk_join_nr "no JOIN with different families v6-v4" 0 0 0

This string is too long too.



-Geliang

> +
>  # single subflow, backup
>  reset
>  ip netns exec $ns1 ./pm_nl_ctl limits 0 1
> --
> 2.29.2
>