| Message ID | bbe531db-6d59-d341-16ed-f48b1fd43d2a@163.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | mptcp: create listening socket only if signal flag is set on | expand |
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 > >
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 >> >>
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 > >> > >> >
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 > > >> > > >> > >
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 >>>> >>>> >>
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 >>>>> >>>>> >>>
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 > >>>>> > >>>>> > >>> > >
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 >>>>>>> >>>>>>> >>>>> >> >>
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
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 >
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
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
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
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");