Setting both the 'signal' and 'subflow' flags is very likely a mistake
from the user, and would bring unexpected behaviours as mentioned in
some bug reports on the MPTCP project.
It is then important to guide the users by printing a warning on stderr.
Users can ignore this message if they have an atypical environment where
the initiator of a connection might accept the creation of additional
subflows from the other peer.
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
ip/ipmptcp.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
index 9847f95b..0f09cf75 100644
--- a/ip/ipmptcp.c
+++ b/ip/ipmptcp.c
@@ -179,6 +179,15 @@ static int mptcp_parse_opt(int argc, char **argv, struct nlmsghdr *n, int cmd)
if (adding && port && !(flags & MPTCP_PM_ADDR_FLAG_SIGNAL))
invarg("flags must have signal when using port", "port");
+ if (adding && (flags & MPTCP_PM_ADDR_FLAG_SIGNAL) &&
+ (flags & MPTCP_PM_ADDR_FLAG_SUBFLOW))
+ fprintf(stderr,
+ "Warning: Setting both the 'signal' and 'subflow' flags is usually a mistake:\n"
+ " 'signal' is used to announce additional addresses, typically for the\n"
+ " server side, while 'subflow' is used to create new paths, typically\n"
+ " for the client side. It is not typical to both initiate, and accept\n"
+ " new connections with the same peer. Continuing as requested.\n");
+
if (setting && id_set && port)
invarg("port can't be used with id", "port");
--
2.43.0On Wed, 5 Jun 2024, Matthieu Baerts (NGI0) wrote:
> Setting both the 'signal' and 'subflow' flags is very likely a mistake
> from the user, and would bring unexpected behaviours as mentioned in
> some bug reports on the MPTCP project.
>
> It is then important to guide the users by printing a warning on stderr.
> Users can ignore this message if they have an atypical environment where
> the initiator of a connection might accept the creation of additional
> subflows from the other peer.
>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> ip/ipmptcp.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
> index 9847f95b..0f09cf75 100644
> --- a/ip/ipmptcp.c
> +++ b/ip/ipmptcp.c
> @@ -179,6 +179,15 @@ static int mptcp_parse_opt(int argc, char **argv, struct nlmsghdr *n, int cmd)
> if (adding && port && !(flags & MPTCP_PM_ADDR_FLAG_SIGNAL))
> invarg("flags must have signal when using port", "port");
>
> + if (adding && (flags & MPTCP_PM_ADDR_FLAG_SIGNAL) &&
> + (flags & MPTCP_PM_ADDR_FLAG_SUBFLOW))
> + fprintf(stderr,
> + "Warning: Setting both the 'signal' and 'subflow' flags is usually a mistake:\n"
> + " 'signal' is used to announce additional addresses, typically for the\n"
> + " server side, while 'subflow' is used to create new paths, typically\n"
> + " for the client side. It is not typical to both initiate, and accept\n"
> + " new connections with the same peer. Continuing as requested.\n");
> +
Hi Matthieu -
For this patch and the next one, I think it would be better to update the
man page with information about typical usage rather than printing this
error message. Seems like the ip tool doesn't have other informational
run-time messages like this (especially ones this long), and there's no
'-q' flag to silence them for non-typical cases.
- Mat
> if (setting && id_set && port)
> invarg("port can't be used with id", "port");
>
>
> --
> 2.43.0
>
>
>
Hi Mat,
On 06/06/2024 21:14, Mat Martineau wrote:
> On Wed, 5 Jun 2024, Matthieu Baerts (NGI0) wrote:
>
>> Setting both the 'signal' and 'subflow' flags is very likely a mistake
>> from the user, and would bring unexpected behaviours as mentioned in
>> some bug reports on the MPTCP project.
>>
>> It is then important to guide the users by printing a warning on stderr.
>> Users can ignore this message if they have an atypical environment where
>> the initiator of a connection might accept the creation of additional
>> subflows from the other peer.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> ip/ipmptcp.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
>> index 9847f95b..0f09cf75 100644
>> --- a/ip/ipmptcp.c
>> +++ b/ip/ipmptcp.c
>> @@ -179,6 +179,15 @@ static int mptcp_parse_opt(int argc, char **argv,
>> struct nlmsghdr *n, int cmd)
>> if (adding && port && !(flags & MPTCP_PM_ADDR_FLAG_SIGNAL))
>> invarg("flags must have signal when using port", "port");
>>
>> + if (adding && (flags & MPTCP_PM_ADDR_FLAG_SIGNAL) &&
>> + (flags & MPTCP_PM_ADDR_FLAG_SUBFLOW))
>> + fprintf(stderr,
>> + "Warning: Setting both the 'signal' and 'subflow' flags
>> is usually a mistake:\n"
>> + " 'signal' is used to announce additional
>> addresses, typically for the\n"
>> + " server side, while 'subflow' is used to create
>> new paths, typically\n"
>> + " for the client side. It is not typical to both
>> initiate, and accept\n"
>> + " new connections with the same peer. Continuing
>> as requested.\n");
>> +
>
> Hi Matthieu -
Thank you for the review!
> For this patch and the next one, I think it would be better to update
> the man page with information about typical usage rather than printing
> this error message. Seems like the ip tool doesn't have other
> informational run-time messages like this (especially ones this long),
> and there's no '-q' flag to silence them for non-typical cases.
That was my first intension, but I think only adding such info in the
man page will not prevent frustrations, and bug reports. I mean: that
supposes people are (fully and carefully) reading the man pages, and not
just 'ip mptcp help' :)
I thought it was OK to let people ignoring this message, because the
action will still be done. But what about adding a new option
'nowarning' option then?
ip mptcp endpoint add (...) [ nowarning ]
If people know what they are doing, they can add this not to have the
warning, while not fully ignoring what is printing to stderr.
If not, I'm not sure people will notice what they are doing is very
likely wrong.
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
On Fri, 7 Jun 2024, Matthieu Baerts wrote:
> Hi Mat,
>
> On 06/06/2024 21:14, Mat Martineau wrote:
>> On Wed, 5 Jun 2024, Matthieu Baerts (NGI0) wrote:
>>
>>> Setting both the 'signal' and 'subflow' flags is very likely a mistake
>>> from the user, and would bring unexpected behaviours as mentioned in
>>> some bug reports on the MPTCP project.
>>>
>>> It is then important to guide the users by printing a warning on stderr.
>>> Users can ignore this message if they have an atypical environment where
>>> the initiator of a connection might accept the creation of additional
>>> subflows from the other peer.
>>>
>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>> ---
>>> ip/ipmptcp.c | 9 +++++++++
>>> 1 file changed, 9 insertions(+)
>>>
>>> diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
>>> index 9847f95b..0f09cf75 100644
>>> --- a/ip/ipmptcp.c
>>> +++ b/ip/ipmptcp.c
>>> @@ -179,6 +179,15 @@ static int mptcp_parse_opt(int argc, char **argv,
>>> struct nlmsghdr *n, int cmd)
>>> if (adding && port && !(flags & MPTCP_PM_ADDR_FLAG_SIGNAL))
>>> invarg("flags must have signal when using port", "port");
>>>
>>> + if (adding && (flags & MPTCP_PM_ADDR_FLAG_SIGNAL) &&
>>> + (flags & MPTCP_PM_ADDR_FLAG_SUBFLOW))
>>> + fprintf(stderr,
>>> + "Warning: Setting both the 'signal' and 'subflow' flags
>>> is usually a mistake:\n"
>>> + " 'signal' is used to announce additional
>>> addresses, typically for the\n"
>>> + " server side, while 'subflow' is used to create
>>> new paths, typically\n"
>>> + " for the client side. It is not typical to both
>>> initiate, and accept\n"
>>> + " new connections with the same peer. Continuing
>>> as requested.\n");
>>> +
>>
>> Hi Matthieu -
>
> Thank you for the review!
>
>> For this patch and the next one, I think it would be better to update
>> the man page with information about typical usage rather than printing
>> this error message. Seems like the ip tool doesn't have other
>> informational run-time messages like this (especially ones this long),
>> and there's no '-q' flag to silence them for non-typical cases.
> That was my first intension, but I think only adding such info in the
> man page will not prevent frustrations, and bug reports. I mean: that
> supposes people are (fully and carefully) reading the man pages, and not
> just 'ip mptcp help' :)
Hi Matthieu -
Ok, maybe info could be added at runtime (like you suggest below) and also
in the man page?
>
> I thought it was OK to let people ignoring this message, because the
> action will still be done. But what about adding a new option
> 'nowarning' option then?
>
> ip mptcp endpoint add (...) [ nowarning ]
>
> If people know what they are doing, they can add this not to have the
> warning, while not fully ignoring what is printing to stderr.
>
My main concern is to have a way to silence it, so "nowarning" fine with
me. Then it would be up to the iproute maintainers :)
> If not, I'm not sure people will notice what they are doing is very
> likely wrong.
It is definely helpful information for new users to get them on the right
track faster, and also avoid extra support load.
- Mat
Hi Mat,
Thank you for your reply!
On 07/06/2024 18:59, Mat Martineau wrote:
> On Fri, 7 Jun 2024, Matthieu Baerts wrote:
>
>> Hi Mat,
>>
>> On 06/06/2024 21:14, Mat Martineau wrote:
>>> On Wed, 5 Jun 2024, Matthieu Baerts (NGI0) wrote:
>>>
>>>> Setting both the 'signal' and 'subflow' flags is very likely a mistake
>>>> from the user, and would bring unexpected behaviours as mentioned in
>>>> some bug reports on the MPTCP project.
>>>>
>>>> It is then important to guide the users by printing a warning on
>>>> stderr.
>>>> Users can ignore this message if they have an atypical environment
>>>> where
>>>> the initiator of a connection might accept the creation of additional
>>>> subflows from the other peer.
>>>>
>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>> ---
>>>> ip/ipmptcp.c | 9 +++++++++
>>>> 1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
>>>> index 9847f95b..0f09cf75 100644
>>>> --- a/ip/ipmptcp.c
>>>> +++ b/ip/ipmptcp.c
>>>> @@ -179,6 +179,15 @@ static int mptcp_parse_opt(int argc, char **argv,
>>>> struct nlmsghdr *n, int cmd)
>>>> if (adding && port && !(flags & MPTCP_PM_ADDR_FLAG_SIGNAL))
>>>> invarg("flags must have signal when using port", "port");
>>>>
>>>> + if (adding && (flags & MPTCP_PM_ADDR_FLAG_SIGNAL) &&
>>>> + (flags & MPTCP_PM_ADDR_FLAG_SUBFLOW))
>>>> + fprintf(stderr,
>>>> + "Warning: Setting both the 'signal' and 'subflow' flags
>>>> is usually a mistake:\n"
>>>> + " 'signal' is used to announce additional
>>>> addresses, typically for the\n"
>>>> + " server side, while 'subflow' is used to create
>>>> new paths, typically\n"
>>>> + " for the client side. It is not typical to both
>>>> initiate, and accept\n"
>>>> + " new connections with the same peer. Continuing
>>>> as requested.\n");
>>>> +
>>>
>>> Hi Matthieu -
>>
>> Thank you for the review!
>>
>>> For this patch and the next one, I think it would be better to update
>>> the man page with information about typical usage rather than printing
>>> this error message. Seems like the ip tool doesn't have other
>>> informational run-time messages like this (especially ones this long),
>>> and there's no '-q' flag to silence them for non-typical cases.
>> That was my first intension, but I think only adding such info in the
>> man page will not prevent frustrations, and bug reports. I mean: that
>> supposes people are (fully and carefully) reading the man pages, and not
>> just 'ip mptcp help' :)
>
> Hi Matthieu -
>
> Ok, maybe info could be added at runtime (like you suggest below) and
> also in the man page?
Yes, we can add info on both sides.
>> I thought it was OK to let people ignoring this message, because the
>> action will still be done. But what about adding a new option
>> 'nowarning' option then?
>>
>> ip mptcp endpoint add (...) [ nowarning ]
>>
>> If people know what they are doing, they can add this not to have the
>> warning, while not fully ignoring what is printing to stderr.
>>
>
> My main concern is to have a way to silence it, so "nowarning" fine with
> me. Then it would be up to the iproute maintainers :)
I looked at the code of the in-kernel PM (but I may have missed cases):
it looks like when both 'signal' and 'subflow' are set, the 'subflow'
flag is somehow ignored. So does it make sense to allow them together?
For the 'patch 4/4', I don't see what would be the use case, except if
only 'backup' is set. We can update that.
Maybe we should (also) modify the kernel, not to allow cases that don't
make sense?
I added a topic -- with more details -- for the next meeting [1].
[1] https://annuel2.framapad.org/p/mptcp_upstreaming_20240612
>> If not, I'm not sure people will notice what they are doing is very
>> likely wrong.
>
> It is definely helpful information for new users to get them on the
> right track faster, and also avoid extra support load.
Exactly :)
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
Hi Mat,
On 10/06/2024 13:00, Matthieu Baerts wrote:
> Hi Mat,
>
> Thank you for your reply!
>
> On 07/06/2024 18:59, Mat Martineau wrote:
>> On Fri, 7 Jun 2024, Matthieu Baerts wrote:
>>
>>> Hi Mat,
>>>
>>> On 06/06/2024 21:14, Mat Martineau wrote:
>>>> On Wed, 5 Jun 2024, Matthieu Baerts (NGI0) wrote:
>>>>
>>>>> Setting both the 'signal' and 'subflow' flags is very likely a mistake
>>>>> from the user, and would bring unexpected behaviours as mentioned in
>>>>> some bug reports on the MPTCP project.
>>>>>
>>>>> It is then important to guide the users by printing a warning on
>>>>> stderr.
>>>>> Users can ignore this message if they have an atypical environment
>>>>> where
>>>>> the initiator of a connection might accept the creation of additional
>>>>> subflows from the other peer.
>>>>>
>>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>>> ---
>>>>> ip/ipmptcp.c | 9 +++++++++
>>>>> 1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
>>>>> index 9847f95b..0f09cf75 100644
>>>>> --- a/ip/ipmptcp.c
>>>>> +++ b/ip/ipmptcp.c
>>>>> @@ -179,6 +179,15 @@ static int mptcp_parse_opt(int argc, char **argv,
>>>>> struct nlmsghdr *n, int cmd)
>>>>> if (adding && port && !(flags & MPTCP_PM_ADDR_FLAG_SIGNAL))
>>>>> invarg("flags must have signal when using port", "port");
>>>>>
>>>>> + if (adding && (flags & MPTCP_PM_ADDR_FLAG_SIGNAL) &&
>>>>> + (flags & MPTCP_PM_ADDR_FLAG_SUBFLOW))
>>>>> + fprintf(stderr,
>>>>> + "Warning: Setting both the 'signal' and 'subflow' flags
>>>>> is usually a mistake:\n"
>>>>> + " 'signal' is used to announce additional
>>>>> addresses, typically for the\n"
>>>>> + " server side, while 'subflow' is used to create
>>>>> new paths, typically\n"
>>>>> + " for the client side. It is not typical to both
>>>>> initiate, and accept\n"
>>>>> + " new connections with the same peer. Continuing
>>>>> as requested.\n");
>>>>> +
>>>>
>>>> Hi Matthieu -
>>>
>>> Thank you for the review!
>>>
>>>> For this patch and the next one, I think it would be better to update
>>>> the man page with information about typical usage rather than printing
>>>> this error message. Seems like the ip tool doesn't have other
>>>> informational run-time messages like this (especially ones this long),
>>>> and there's no '-q' flag to silence them for non-typical cases.
>>> That was my first intension, but I think only adding such info in the
>>> man page will not prevent frustrations, and bug reports. I mean: that
>>> supposes people are (fully and carefully) reading the man pages, and not
>>> just 'ip mptcp help' :)
>>
>> Hi Matthieu -
>>
>> Ok, maybe info could be added at runtime (like you suggest below) and
>> also in the man page?
>
> Yes, we can add info on both sides.
>
>>> I thought it was OK to let people ignoring this message, because the
>>> action will still be done. But what about adding a new option
>>> 'nowarning' option then?
>>>
>>> ip mptcp endpoint add (...) [ nowarning ]
>>>
>>> If people know what they are doing, they can add this not to have the
>>> warning, while not fully ignoring what is printing to stderr.
>>>
>>
>> My main concern is to have a way to silence it, so "nowarning" fine with
>> me. Then it would be up to the iproute maintainers :)
>
> I looked at the code of the in-kernel PM (but I may have missed cases):
> it looks like when both 'signal' and 'subflow' are set, the 'subflow'
> flag is somehow ignored. So does it make sense to allow them together?
>
> For the 'patch 4/4', I don't see what would be the use case, except if
> only 'backup' is set. We can update that.
>
> Maybe we should (also) modify the kernel, not to allow cases that don't
> make sense?
>
> I added a topic -- with more details -- for the next meeting [1].
>
> [1] https://annuel2.framapad.org/p/mptcp_upstreaming_20240612
After the discussion we had, and the series I just sent [1], do you
think we still need the last two patches adding warnings? Is it enough
with the man update?
There is still the case where people might not set any flags, but it
looks like an issue on their side, a bit like adding an IP without the
netmask, and having issues because of that.
All these patches still apply on the latest 'main' branch from the
iproute2 repo.
[1]
https://lore.kernel.org/r/20240621-mptcp-pm-avail-v1-0-b692d5eb89b5@kernel.org
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
© 2016 - 2024 Red Hat, Inc.