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

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

[Jianguo Wu]

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.

Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")
Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
---
 net/mptcp/pm_netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index becc32b..dbd6910 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1025,7 +1025,7 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
 	}

 	*entry = addr;
-	if (entry->addr.port) {
+	if (entry->addr.port && (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
 		ret = mptcp_pm_nl_create_listen_socket(skb->sk, entry);
 		if (ret) {
 			GENL_SET_ERR_MSG(info, "create listen socket error");
-- 
1.8.3.1

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

[Geliang Tang]

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");

-Geliang

>
> Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")
> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
> ---
>  net/mptcp/pm_netlink.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index becc32b..dbd6910 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1025,7 +1025,7 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
>         }
>
>         *entry = addr;
> -       if (entry->addr.port) {
> +       if (entry->addr.port && (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
>                 ret = mptcp_pm_nl_create_listen_socket(skb->sk, entry);
>                 if (ret) {
>                         GENL_SET_ERR_MSG(info, "create listen socket error");
> --
> 1.8.3.1
>
>

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

[Jianguo Wu]

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?

> -Geliang
> 
>>
>> Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")
>> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
>> ---
>>  net/mptcp/pm_netlink.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index becc32b..dbd6910 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
>> @@ -1025,7 +1025,7 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
>>         }
>>
>>         *entry = addr;
>> -       if (entry->addr.port) {
>> +       if (entry->addr.port && (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
>>                 ret = mptcp_pm_nl_create_listen_socket(skb->sk, entry);
>>                 if (ret) {
>>                         GENL_SET_ERR_MSG(info, "create listen socket error");
>> --
>> 1.8.3.1
>>
>>

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

[Geliang Tang]

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'.

Thanks.

-Geliang

>
> > -Geliang
> >
> >>
> >> Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")
> >> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
> >> ---
> >>  net/mptcp/pm_netlink.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> >> index becc32b..dbd6910 100644
> >> --- a/net/mptcp/pm_netlink.c
> >> +++ b/net/mptcp/pm_netlink.c
> >> @@ -1025,7 +1025,7 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
> >>         }
> >>
> >>         *entry = addr;
> >> -       if (entry->addr.port) {
> >> +       if (entry->addr.port && (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
> >>                 ret = mptcp_pm_nl_create_listen_socket(skb->sk, entry);
> >>                 if (ret) {
> >>                         GENL_SET_ERR_MSG(info, "create listen socket error");
> >> --
> >> 1.8.3.1
> >>
> >>
>

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

[Geliang Tang]

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?

-Geliang


>
> Thanks.
>
> -Geliang
>
> >
> > > -Geliang
> > >
> > >>
> > >> Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")
> > >> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
> > >> ---
> > >>  net/mptcp/pm_netlink.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > >> index becc32b..dbd6910 100644
> > >> --- a/net/mptcp/pm_netlink.c
> > >> +++ b/net/mptcp/pm_netlink.c
> > >> @@ -1025,7 +1025,7 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
> > >>         }
> > >>
> > >>         *entry = addr;
> > >> -       if (entry->addr.port) {
> > >> +       if (entry->addr.port && (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
> > >>                 ret = mptcp_pm_nl_create_listen_socket(skb->sk, entry);
> > >>                 if (ret) {
> > >>                         GENL_SET_ERR_MSG(info, "create listen socket error");
> > >> --
> > >> 1.8.3.1
> > >>
> > >>
> >

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

[Jianguo Wu]

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.

> -Geliang
> 
> 
>>
>> Thanks.
>>
>> -Geliang
>>
>>>
>>>> -Geliang
>>>>
>>>>>
>>>>> Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")
>>>>> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
>>>>> ---
>>>>>  net/mptcp/pm_netlink.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>>>> index becc32b..dbd6910 100644
>>>>> --- a/net/mptcp/pm_netlink.c
>>>>> +++ b/net/mptcp/pm_netlink.c
>>>>> @@ -1025,7 +1025,7 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
>>>>>         }
>>>>>
>>>>>         *entry = addr;
>>>>> -       if (entry->addr.port) {
>>>>> +       if (entry->addr.port && (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
>>>>>                 ret = mptcp_pm_nl_create_listen_socket(skb->sk, entry);
>>>>>                 if (ret) {
>>>>>                         GENL_SET_ERR_MSG(info, "create listen socket error");
>>>>> --
>>>>> 1.8.3.1
>>>>>
>>>>>
>>>

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

[Geliang Tang]

Jianguo Wu <wujianguo106@163.com> 于2021年4月23日周五 下午3:41写道：
>
>
>
> 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.

You can add this check both in mptcp_pm_parse_addr and in 'ip mptcp'.

-Geliang

>
> > -Geliang
> >
> >
> >>
> >> Thanks.
> >>
> >> -Geliang
> >>
> >>>
> >>>> -Geliang
> >>>>
> >>>>>
> >>>>> Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")
> >>>>> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
> >>>>> ---
> >>>>>  net/mptcp/pm_netlink.c | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> >>>>> index becc32b..dbd6910 100644
> >>>>> --- a/net/mptcp/pm_netlink.c
> >>>>> +++ b/net/mptcp/pm_netlink.c
> >>>>> @@ -1025,7 +1025,7 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
> >>>>>         }
> >>>>>
> >>>>>         *entry = addr;
> >>>>> -       if (entry->addr.port) {
> >>>>> +       if (entry->addr.port && (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
> >>>>>                 ret = mptcp_pm_nl_create_listen_socket(skb->sk, entry);
> >>>>>                 if (ret) {
> >>>>>                         GENL_SET_ERR_MSG(info, "create listen socket error");
> >>>>> --
> >>>>> 1.8.3.1
> >>>>>
> >>>>>
> >>>
>
>

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

[Jianguo Wu]

On 2021/4/23 15:47, Geliang Tang wrote:
> Jianguo Wu <wujianguo106@163.com> 于2021年4月23日周五 下午3:41写道：
>>
>>
>>
>> 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.
> 
> You can add this check both in mptcp_pm_parse_addr and in 'ip mptcp'.
> 

Ok, I will send patches soon, Thanks:)

> -Geliang
> 
>>
>>> -Geliang
>>>
>>>
>>>>
>>>> Thanks.
>>>>
>>>> -Geliang
>>>>
>>>>>
>>>>>> -Geliang
>>>>>>
>>>>>>>
>>>>>>> Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")
>>>>>>> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
>>>>>>> ---
>>>>>>>  net/mptcp/pm_netlink.c | 2 +-
>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>>>>>> index becc32b..dbd6910 100644
>>>>>>> --- a/net/mptcp/pm_netlink.c
>>>>>>> +++ b/net/mptcp/pm_netlink.c
>>>>>>> @@ -1025,7 +1025,7 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
>>>>>>>         }
>>>>>>>
>>>>>>>         *entry = addr;
>>>>>>> -       if (entry->addr.port) {
>>>>>>> +       if (entry->addr.port && (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
>>>>>>>                 ret = mptcp_pm_nl_create_listen_socket(skb->sk, entry);
>>>>>>>                 if (ret) {
>>>>>>>                         GENL_SET_ERR_MSG(info, "create listen socket error");
>>>>>>> --
>>>>>>> 1.8.3.1
>>>>>>>
>>>>>>>
>>>>>
>>
>>

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

[Matthieu Baerts]

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.

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
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

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

[Jianguo Wu]

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
> 

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

[Matthieu Baerts]

Hi Jianguo,

On 23/04/2021 10:38, Jianguo Wu wrote:
> On 2021/4/23 16:21, Matthieu Baerts wrote:
>> On 23/04/2021 09:41, Jianguo Wu wrote:
>>> 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?

That would definitively not be a common use-case but we will see if some
people are interested by that.

Anyhow, I think it is better to start by well covering requested
use-cases and avoid "strange" behaviours, e.g. not announcing the
address but doing bind.

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

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

[Geliang Tang]

Hi Matt,

Matthieu Baerts <matthieu.baerts@tessares.net> 于2021年4月23日周五 下午4:21写道：
>
> 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.
>
> 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? :)

I think the proposed modifications is not enough.

If the port is set with non-signal flag (like subflow), we should not only
stop creating listening socket, but also stop appending this address.

Since our code can't deal with the ADD_SUBFLOW+port case (like
"pm_nl_ctl add 10.0.0.1 flag subflow port 1000"). Local address's port will
re-set as the main socket's port number in our code. So set port to a local
address is useless, we should add the flag check to avoid this case.

I think the flag check should be added both in mptcp_pm_parse_addr and in
'ip mptcp'. Let's waiting for Jianguo's new patches.

Thanks.

-Geliang

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

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

[Matthieu Baerts]

Hi Geliang,

On 23/04/2021 10:59, Geliang Tang wrote:
> Matthieu Baerts <matthieu.baerts@tessares.net> 于2021年4月23日周五 下午4:21写道：
>> On 23/04/2021 09:41, Jianguo Wu wrote:
>>> 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.
>>
>> 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? :)
> 
> I think the proposed modifications is not enough.
> 
> If the port is set with non-signal flag (like subflow), we should not only
> stop creating listening socket, but also stop appending this address.
> 
> Since our code can't deal with the ADD_SUBFLOW+port case (like
> "pm_nl_ctl add 10.0.0.1 flag subflow port 1000"). Local address's port will
> re-set as the main socket's port number in our code. So set port to a local
> address is useless, we should add the flag check to avoid this case.

Indeed, that seems to make sense!

Thanks for checking this!

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

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

[Jianguo Wu]

On 2021/4/23 15:17, Geliang Tang wrote:
> 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'.
> 

OK, thanks!

> Thanks.
> 
> -Geliang
> 
>>
>>> -Geliang
>>>
>>>>
>>>> Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")
>>>> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
>>>> ---
>>>>  net/mptcp/pm_netlink.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>>> index becc32b..dbd6910 100644
>>>> --- a/net/mptcp/pm_netlink.c
>>>> +++ b/net/mptcp/pm_netlink.c
>>>> @@ -1025,7 +1025,7 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
>>>>         }
>>>>
>>>>         *entry = addr;
>>>> -       if (entry->addr.port) {
>>>> +       if (entry->addr.port && (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
>>>>                 ret = mptcp_pm_nl_create_listen_socket(skb->sk, entry);
>>>>                 if (ret) {
>>>>                         GENL_SET_ERR_MSG(info, "create listen socket error");
>>>> --
>>>> 1.8.3.1
>>>>
>>>>
>>