Re: [PATCH] mptcp: create listening socket only if signal flag is set on

[Jianguo Wu <wujianguo106@163.com>]

On 2021/4/23 16:21, Matthieu Baerts wrote:
> Hi Jianguo, Geliang,
> 
> Thank you for the patches and reviews!
> 
> On 23/04/2021 09:41, Jianguo Wu wrote:
>>
>>
>> On 2021/4/23 15:27, Geliang Tang wrote:
>>> Geliang Tang <geliangtang@gmail.com> 于2021年4月23日周五 下午3:17写道：
>>>>
>>>> Hi Jianguo,
>>>>
>>>> Jianguo Wu <wujianguo106@163.com> 于2021年4月23日周五 下午2:39写道：
>>>>>
>>>>>
>>>>>
>>>>> On 2021/4/23 12:33, Geliang Tang wrote:
>>>>>> Hi Jianguo,
>>>>>>
>>>>>> Thanks for your patch.
>>>>>>
>>>>>> Jianguo Wu <wujianguo106@163.com> 于2021年4月22日周四 下午6:30写道：
>>>>>>>
>>>>>>> From: Jianguo Wu <wujianguo@chinatelecom.cn>
>>>>>>>
>>>>>>> PM announces address to remote host when signal flag is set,
>>>>>>> so create listening socket only if signal flag is set.
>>>>>>
>>>>>> This check has done in the user-space, here are these lines in pm_nl_ctl.c:
>>>>>>
>>>>>> 274                 } else if (!strcmp(argv[arg], "port")) {
>>>>>> 275                         u_int16_t port;
>>>>>> 276
>>>>>> 277                         if (++arg >= argc)
>>>>>> 278                                 error(1, 0, " missing port value");
>>>>>> 279                         if (!(flags & MPTCP_PM_ADDR_FLAG_SIGNAL))
>>>>>> 280                                 error(1, 0, " flags must be signal
>>>>>> when using port");
>>>>>>
>>>>>
>>>>> Hi Geliang, Thanks for point out this, I was use 'ip mptcp' to add address, It doesn't seem to have done that check.
>>>>> Is it worth to do check in kernel to make sure there is a signal flag before create listening socket?
>>>>
>>>> I think it's better to check this in the user-space, you can write a patch
>>>> to iproute2 to add this check in 'ip mptcp'.
>>>
>>> Or add this check in mptcp_pm_parse_addr, something like:
>>>
>>> $ git diff net/mptcp/pm_netlink.c
>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>> index 6ba040897738..9cd4a4180a2a 100644
>>> --- a/net/mptcp/pm_netlink.c
>>> +++ b/net/mptcp/pm_netlink.c
>>> @@ -971,8 +971,14 @@ static int mptcp_pm_parse_addr(struct nlattr
>>> *attr, struct genl_info *info,
>>>         if (tb[MPTCP_PM_ADDR_ATTR_FLAGS])
>>>                 entry->flags = nla_get_u32(tb[MPTCP_PM_ADDR_ATTR_FLAGS]);
>>>
>>> -       if (tb[MPTCP_PM_ADDR_ATTR_PORT])
>>> +       if (tb[MPTCP_PM_ADDR_ATTR_PORT]) {
>>> +               if (!(entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
>>> +                       NL_SET_ERR_MSG_ATTR("");
>>> +                       return -EINVAL;
>>> +               }
>>> +
>>>                 entry->addr.port =
>>> htons(nla_get_u16(tb[MPTCP_PM_ADDR_ATTR_PORT]));
>>> +       }
>>>
>>>         return 0;
>>>  }
>>>
>>> WDYT?
>>>
>>
>> I think this is better, we can't assume that all user-space tools works correctly.
> 
> I was thinking a bit about that. With the proposed modifications, it
> means that a server will have to send an ADD_ADDR if it wants to accept
> subflows on another port. I guess there will be use-cases where we will
> not want to send ADD_ADDR because we are in a controlled environment or
> we don't want to announce a public ADD_ADDR+port on a specific network.
> 

Hi Matt, do you mean there will be use-cases where using ADD_ADDR+port as local address to initiate
a outgoing subflow join, but not send ADD_ADDR?

> Do we have to support that? Maybe safer to restrict actions for now and
> allow that later if there are needs, no?
> 
> In other words, I guess we should accept the proposed modifications.
> Any other points of view? :)
> 
> Cheers,
> Matt
>