* [MPTCP] Re: [bug report] the test cases for signaling invalid addresses don't work
@ 2021-02-03 17:24 Paolo Abeni
0 siblings, 0 replies; 2+ messages in thread
From: Paolo Abeni @ 2021-02-03 17:24 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 3205 bytes --]
On Wed, 2021-02-03 at 19:30 +0800, Geliang Tang wrote:
> I added two test cases for signaling multi addresses today:
>
> # signal addresses
> reset
> ip netns exec $ns1 ./pm_nl_ctl limits 3 3
> ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
> ip netns exec $ns1 ./pm_nl_ctl add 10.0.3.1 flags signal
> ip netns exec $ns1 ./pm_nl_ctl add 10.0.4.1 flags signal
> ip netns exec $ns2 ./pm_nl_ctl limits 3 3
> run_tests $ns1 $ns2 10.0.1.1
> chk_join_nr "signal addresses" 3 3 3
> chk_add_nr 3 3
>
> # signal invalid addresses
> reset
> ip netns exec $ns1 ./pm_nl_ctl limits 3 3
> ip netns exec $ns1 ./pm_nl_ctl add 10.0.12.1 flags signal
> ip netns exec $ns1 ./pm_nl_ctl add 10.0.13.1 flags signal
> ip netns exec $ns1 ./pm_nl_ctl add 10.0.14.1 flags signal
> ip netns exec $ns2 ./pm_nl_ctl limits 3 3
> run_tests $ns1 $ns2 10.0.1.1
> chk_join_nr "signal invalid addresses" 0 0 0
> chk_add_nr 1 1
>
> The 1st one works well, the 2nd one added three invalid addresses, but
> only got 1 ADD_ADDR and 1 ECHO. I thought it should be 0 ADD_ADDR or
> 3 ADD_ADDRs.
>
> In the 2nd test case, mptcp_pm_create_subflow_or_signal_addr was only
> triggered once when the msk is established. Since the first address is an
> invalid one, the subflow for it cannot be created successfully. So
> mptcp_pm_nl_subflow_established could not be invoked. The last two
> addresses didn't have a chance to be announced. I think this behavior is
> incorrect.
>
> I have several questions about this:
>
> 1. Is this an known issue? Is it worth creating a new issue on github and
> fixing it?
I think filing a github issue is the correct thing to do, so that we
don't lose track of this in the ML.
Even if I must admit it looks a low prio item to me.
> 2. What do we expect for the 2nd test case, 0 ADD_ADDR or 3 ADD_ADDRs?
> 3. Should we skip announcing the invalid addresses?
> 4. Or should we change the logic of mptcp_pm_create_subflow_or_signal_addr
> to announce all the invalid addresses?
I think the "correct" behaviour would be announcing 3 addresses.
Setting and maintaining correct list of addresses is up to the user-
space/path-manger.
The kernel could teorerically watch out for local IP address changes,
but even announcing only IPs that are locally available would not
guarantee the related subflow to be established - for a gazilion of
reasons, last but not least the address could be removed by the admin
after being announced and before subflow creation.
Overall I think the kernel should not filter the addresses to be
annonced in any way.
Eventually we could change how add_addr events are scheduled, e.g.
moving to the next address after that the previous one has been echoed
by the peer.
As I said I think it's not an huge priority, we have planty of bugs to
fix first ;) So I would just keep the current result, possibly adding a
TODO/FIXME note in the test and/or in the relevant kernel code.
All the above IMHO.
Cheers,
Paolo
^ permalink raw reply [flat|nested] 2+ messages in thread
* [MPTCP] Re: [bug report] the test cases for signaling invalid addresses don't work
@ 2021-02-04 3:15 Geliang Tang
0 siblings, 0 replies; 2+ messages in thread
From: Geliang Tang @ 2021-02-04 3:15 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 3576 bytes --]
Hi Paolo,
Paolo Abeni <pabeni(a)redhat.com> 于2021年2月4日周四 上午1:24写道:
>
> On Wed, 2021-02-03 at 19:30 +0800, Geliang Tang wrote:
> > I added two test cases for signaling multi addresses today:
> >
> > # signal addresses
> > reset
> > ip netns exec $ns1 ./pm_nl_ctl limits 3 3
> > ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
> > ip netns exec $ns1 ./pm_nl_ctl add 10.0.3.1 flags signal
> > ip netns exec $ns1 ./pm_nl_ctl add 10.0.4.1 flags signal
> > ip netns exec $ns2 ./pm_nl_ctl limits 3 3
> > run_tests $ns1 $ns2 10.0.1.1
> > chk_join_nr "signal addresses" 3 3 3
> > chk_add_nr 3 3
> >
> > # signal invalid addresses
> > reset
> > ip netns exec $ns1 ./pm_nl_ctl limits 3 3
> > ip netns exec $ns1 ./pm_nl_ctl add 10.0.12.1 flags signal
> > ip netns exec $ns1 ./pm_nl_ctl add 10.0.13.1 flags signal
> > ip netns exec $ns1 ./pm_nl_ctl add 10.0.14.1 flags signal
> > ip netns exec $ns2 ./pm_nl_ctl limits 3 3
> > run_tests $ns1 $ns2 10.0.1.1
> > chk_join_nr "signal invalid addresses" 0 0 0
> > chk_add_nr 1 1
> >
> > The 1st one works well, the 2nd one added three invalid addresses, but
> > only got 1 ADD_ADDR and 1 ECHO. I thought it should be 0 ADD_ADDR or
> > 3 ADD_ADDRs.
> >
> > In the 2nd test case, mptcp_pm_create_subflow_or_signal_addr was only
> > triggered once when the msk is established. Since the first address is an
> > invalid one, the subflow for it cannot be created successfully. So
> > mptcp_pm_nl_subflow_established could not be invoked. The last two
> > addresses didn't have a chance to be announced. I think this behavior is
> > incorrect.
> >
> > I have several questions about this:
> >
> > 1. Is this an known issue? Is it worth creating a new issue on github and
> > fixing it?
>
> I think filing a github issue is the correct thing to do, so that we
> don't lose track of this in the ML.
>
> Even if I must admit it looks a low prio item to me.
>
> > 2. What do we expect for the 2nd test case, 0 ADD_ADDR or 3 ADD_ADDRs?
> > 3. Should we skip announcing the invalid addresses?
> > 4. Or should we change the logic of mptcp_pm_create_subflow_or_signal_addr
> > to announce all the invalid addresses?
>
> I think the "correct" behaviour would be announcing 3 addresses.
> Setting and maintaining correct list of addresses is up to the user-
> space/path-manger.
>
> The kernel could teorerically watch out for local IP address changes,
> but even announcing only IPs that are locally available would not
> guarantee the related subflow to be established - for a gazilion of
> reasons, last but not least the address could be removed by the admin
> after being announced and before subflow creation.
>
> Overall I think the kernel should not filter the addresses to be
> annonced in any way.
>
> Eventually we could change how add_addr events are scheduled, e.g.
> moving to the next address after that the previous one has been echoed
> by the peer.
>
> As I said I think it's not an huge priority, we have planty of bugs to
> fix first ;) So I would just keep the current result, possibly adding a
> TODO/FIXME note in the test and/or in the relevant kernel code.
>
> All the above IMHO.
>
Thanks for your suggestions. I had created issues #151 for this and
assigned it to myself. I'll try to fix it recently.
-Geliang
> Cheers,
>
> Paolo
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-02-04 3:15 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-03 17:24 [MPTCP] Re: [bug report] the test cases for signaling invalid addresses don't work Paolo Abeni
2021-02-04 3:15 Geliang Tang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.