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

[Geliang Tang <geliangtang@gmail.com>]

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