[v6,3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal

Commit Message

YonglongLi June 29, 2021, 1:41 a.m. UTC

according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
ADD_ADDR/echo-ADD_ADDR option

add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option

Signed-off-by: Yonglong Li <[e-mail]>
---
 include/net/mptcp.h  |  3 ++-
 net/mptcp/options.c  | 65 +++++++++++++++++++++++++++++++---------------------
 net/mptcp/pm.c       | 33 +++++++++++---------------
 net/mptcp/protocol.h | 23 ++++++++++++-------
 4 files changed, 69 insertions(+), 55 deletions(-)

Comments

Geliang Tang June 29, 2021, 5:58 a.m. UTC | #1

Yonglong Li <[e-mail]> 于2021年6月29日周二 上午9:42写道：
>
> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> ADD_ADDR/echo-ADD_ADDR option
>
> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>
> Signed-off-by: Yonglong Li <[e-mail]>
> ---
>  include/net/mptcp.h  |  3 ++-
>  net/mptcp/options.c  | 65 +++++++++++++++++++++++++++++++---------------------
>  net/mptcp/pm.c       | 33 +++++++++++---------------
>  net/mptcp/protocol.h | 23 ++++++++++++-------
>  4 files changed, 69 insertions(+), 55 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index d61bbbf..d2c6ebe 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -61,7 +61,8 @@ struct mptcp_out_options {
>         u64 sndr_key;
>         u64 rcvr_key;
>         u64 ahmac;
> -       struct mptcp_addr_info addr;
> +       struct mptcp_addr_info local;
> +       struct mptcp_addr_info remote;
>         struct mptcp_rm_list rm_list;
>         u8 join_id;
>         u8 backup;
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..1707bec 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>         bool drop_other_suboptions = false;
>         unsigned int opt_size = *size;
> -       bool echo;
> -       bool port;
> -       int len;
> +       u8 add_addr, flags = 0xff;
> +       int len = 0;
>
> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> -            mptcp_pm_should_add_signal_port(msk) ||
> -            mptcp_pm_should_add_signal_echo(msk)) &&
> +       if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
> +               return false;

This add_addr argument is useless, let's drop it.

And here add back mptcp_pm_should_add_signal check here. The original code
called mptcp_pm_should_add_signal twice for double check, once out of pm
lock, once under pm lock. We should keep it.

> +
> +       if ((mptcp_pm_should_add_signal_echo(msk) ||
> +            (mptcp_pm_should_add_signal_addr(msk) &&
> +             (opts->local.family == AF_INET6 || opts->local.port))) &&
>             skb && skb_is_tcp_pure_ack(skb)) {
>                 pr_debug("drop other suboptions");
>                 opts->suboptions = 0;
> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>                 drop_other_suboptions = true;
>         }
>
> -       if (!mptcp_pm_should_add_signal(msk) ||
> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> -               return false;
>
> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
> +       if (mptcp_pm_should_add_signal_echo(msk)) {
> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> +       } else {
> +               opts->ahmac = add_addr_generate_hmac(msk->local_key,
> +                                                    msk->remote_key,
> +                                                    &opts->local);

Keep this ahmac generating code after opts->suboptions set just like the
original code, since ahmac is the more expensive to populate. If remaining
length isn't enough, no need to set ahmac.

> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> +       }
> +
> +       len = mptcp_add_addr_len(opts);
>         if (remaining < len)
>                 return false;
>
> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>         if (drop_other_suboptions)
>                 *size -= opt_size;
>         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> -       if (!echo) {
> -               opts->ahmac = add_addr_generate_hmac(msk->local_key,
> -                                                    msk->remote_key,
> -                                                    &opts->addr);
> -       }
> -       pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> -                opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> +
> +       spin_lock_bh(&msk->pm.lock);
> +       WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> +       spin_unlock_bh(&msk->pm.lock);

addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
set it again. I thinks this trunk and all the flags set above should be
dropped.

> +
> +       pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> +                add_addr, (opts->ahmac == 0), opts->local.id,
> +                opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>
>         return true;
>  }

The whole function is something like this:
'''
        struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
        struct mptcp_sock *msk = mptcp_sk(subflow->conn);
        bool drop_other_suboptions = false;
        unsigned int opt_size = *size;
        int len;

        if (!mptcp_pm_should_add_signal(msk) ||
            !mptcp_pm_add_addr_signal(msk, remaining, opts))
                return false;

        if ((mptcp_pm_should_add_signal_echo(msk) ||
             (mptcp_pm_should_add_signal_addr(msk) &&
              (opts->local.family == AF_INET6 || opts->local.port))) &&
            skb && skb_is_tcp_pure_ack(skb)) {
                pr_debug("drop other suboptions");
                opts->suboptions = 0;
                opts->ext_copy.use_ack = 0;
                opts->ext_copy.use_map = 0;
                remaining += opt_size;
                drop_other_suboptions = true;
        }

        len = mptcp_add_addr_len(opts);
        if (remaining < len)
                return false;

        *size = len;
        if (drop_other_suboptions)
                *size -= opt_size;
        opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
        if (mptcp_pm_should_add_signal_addr(msk)) {
                opts->ahmac = add_addr_generate_hmac(msk->local_key,
                                                     msk->remote_key,
                                                     &opts->local);
        }

        pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
                 msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
                 opts->ahmac, ntohs(opts->local.port),
opts->remote.id, ntohs(opts->remote.port));

        return true;
'''

> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>
>  mp_capable_done:
>         if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> +               struct mptcp_addr_info *addr = &opts->remote;

We can simplify it like this:
         struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
&opts->remote;

>                 u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>                 u8 echo = MPTCP_ADDR_ECHO;
>
> +               if (opts->ahmac)
> +                       addr = &opts->local;

And this trunk can be dropped.

> +
>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> -               if (opts->addr.family == AF_INET6)
> +               if (addr->family == AF_INET6)
>                         len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>  #endif
>
> -               if (opts->addr.port)
> +               if (addr->port)
>                         len += TCPOLEN_MPTCP_PORT_LEN;
>
>                 if (opts->ahmac) {
> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>                 }
>
>                 *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> -                                     len, echo, opts->addr.id);
> -               if (opts->addr.family == AF_INET) {
> -                       memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> +                                     len, echo, addr->id);
> +               if (addr->family == AF_INET) {
> +                       memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
>                         ptr += 1;
>                 }
>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> -               else if (opts->addr.family == AF_INET6) {
> -                       memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
> +               else if (addr->family == AF_INET6) {
> +                       memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
>                         ptr += 4;
>                 }
>  #endif
>
> -               if (!opts->addr.port) {
> +               if (!addr->port) {
>                         if (opts->ahmac) {
>                                 put_unaligned_be64(opts->ahmac, ptr);
>                                 ptr += 2;
>                         }
>                 } else {
> -                       u16 port = ntohs(opts->addr.port);
> +                       u16 port = ntohs(addr->port);
>
>                         if (opts->ahmac) {
>                                 u8 *bptr = (u8 *)ptr;
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index cf873e9..9c621293 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>
>  /* path manager helpers */
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port)
> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> +                             u8 *add_addr)

Drop this add_addr argument.

>  {
> -       u8 add_addr;
> -       int ret = false;
> -
>         spin_lock_bh(&msk->pm.lock);
>
> -       /* double check after the lock is acquired */
> -       if (!mptcp_pm_should_add_signal(msk))
> -               goto out_unlock;

Keep this double check code.

> -
> -       *echo = mptcp_pm_should_add_signal_echo(msk);
> -       *port = mptcp_pm_should_add_signal_port(msk);
> -
> -       if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> -               goto out_unlock;

Keep this length double check code too.

> +       if (!mptcp_pm_should_add_signal(msk)) {
> +               spin_unlock_bh(&msk->pm.lock);
> +               return false;
> +       }
>
> -       *saddr = msk->pm.local;
> -       add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
-       WRITE_ONCE(msk->pm.addr_signal, add_addr);

This code is just added in patch 1, I think we should keep it. And no need
to write addr_signal again in mptcp_established_options_add_addr.

> -       ret = true;
> +       opts->local = msk->pm.local;
> +       opts->remote = msk->pm.remote;
> +       *add_addr = msk->pm.addr_signal;
>
> -out_unlock:
>         spin_unlock_bh(&msk->pm.lock);
> -       return ret;

Keep this out_unlock code.

> +
> +       if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> +               mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);

Could we use mptcp_pm_add_addr_send_ack here instead of open coding?

I'm no sure why we need this two lines, and why you use '&&' here. Do you
mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?

> +       return true;
>  }

The whole function is something like this:
'''
        int ret = false;
        u8 add_addr;

        spin_lock_bh(&msk->pm.lock);

        /* double check after the lock is acquired */
        if (!mptcp_pm_should_add_signal(msk))
                goto out_unlock;

        if (remaining < mptcp_add_addr_len(opts))
                goto out_unlock;

        opts->local = msk->pm.local;
        opts->remote = msk->pm.remote;
        if (mptcp_pm_should_add_signal_echo(msk))
                add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
        else
                add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
        WRITE_ONCE(msk->pm.addr_signal, add_addr);
        ret = true;

out_unlock:
        spin_unlock_bh(&msk->pm.lock);
        if (ret && mptcp_pm_should_add_signal_echo(msk) &&
mptcp_pm_should_add_signal_addr(msk))
                mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
        return ret;
'''

>
>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index a0b0ec0..0bfbbdef 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
>         return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
>  }
>
> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
>  {
> -       u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> +       u8 len = 0;
> +       struct mptcp_addr_info *addr = &opts->remote;

We can simplify it like this:
         struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
&opts->remote;

And keep the orignal code unchanged.

>
> -       if (family == AF_INET6)
> -               len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> -       if (!echo)
> +       if (opts->ahmac) {
> +               addr = &opts->local;
>                 len += MPTCPOPT_THMAC_LEN;
> +       }
> +
> +       if (addr->family == AF_INET6)
> +               len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> +       else
> +               len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
> +
>         /* account for 2 trailing 'nop' options */
> -       if (port)
> +       if (addr->port)
>                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>
>         return len;

The whole function is something like this:
'''
        struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
&opts->remote;
        u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;

        if (addr->family == AF_INET6)
                len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
        if (opts->ahmac)
                len += MPTCPOPT_THMAC_LEN;
        /* account for 2 trailing 'nop' options */
        if (addr->port)
                len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;

        return len;
'''

Thanks.
-Geliang

> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
>         return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
>  }
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port);
> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> +                             u8 *add_addr);
>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>                              struct mptcp_rm_list *rm_list);
>  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> --
> 1.8.3.1
>
>

Geliang Tang June 29, 2021, 6:05 a.m. UTC | #2

Geliang Tang <[e-mail]> 于2021年6月29日周二 下午1:58写道：
>
> Yonglong Li <[e-mail]> 于2021年6月29日周二 上午9:42写道：
> >
> > according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> > ADD_ADDR/echo-ADD_ADDR option
> >
> > add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >
> > Signed-off-by: Yonglong Li <[e-mail]>
> > ---
> >  include/net/mptcp.h  |  3 ++-
> >  net/mptcp/options.c  | 65 +++++++++++++++++++++++++++++++---------------------
> >  net/mptcp/pm.c       | 33 +++++++++++---------------
> >  net/mptcp/protocol.h | 23 ++++++++++++-------
> >  4 files changed, 69 insertions(+), 55 deletions(-)
> >
> > diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> > index d61bbbf..d2c6ebe 100644
> > --- a/include/net/mptcp.h
> > +++ b/include/net/mptcp.h
> > @@ -61,7 +61,8 @@ struct mptcp_out_options {
> >         u64 sndr_key;
> >         u64 rcvr_key;
> >         u64 ahmac;
> > -       struct mptcp_addr_info addr;
> > +       struct mptcp_addr_info local;
> > +       struct mptcp_addr_info remote;
> >         struct mptcp_rm_list rm_list;
> >         u8 join_id;
> >         u8 backup;
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index 1aec016..1707bec 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >         bool drop_other_suboptions = false;
> >         unsigned int opt_size = *size;
> > -       bool echo;
> > -       bool port;
> > -       int len;
> > +       u8 add_addr, flags = 0xff;
> > +       int len = 0;
> >
> > -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> > -            mptcp_pm_should_add_signal_port(msk) ||
> > -            mptcp_pm_should_add_signal_echo(msk)) &&
> > +       if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
> > +               return false;
>
> This add_addr argument is useless, let's drop it.
>
> And here add back mptcp_pm_should_add_signal check here. The original code
> called mptcp_pm_should_add_signal twice for double check, once out of pm
> lock, once under pm lock. We should keep it.
>
> > +
> > +       if ((mptcp_pm_should_add_signal_echo(msk) ||
> > +            (mptcp_pm_should_add_signal_addr(msk) &&
> > +             (opts->local.family == AF_INET6 || opts->local.port))) &&
> >             skb && skb_is_tcp_pure_ack(skb)) {
> >                 pr_debug("drop other suboptions");
> >                 opts->suboptions = 0;
> > @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >                 drop_other_suboptions = true;
> >         }
> >
> > -       if (!mptcp_pm_should_add_signal(msk) ||
> > -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> > -               return false;
> >
> > -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
> > +       if (mptcp_pm_should_add_signal_echo(msk)) {
> > +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> > +       } else {
> > +               opts->ahmac = add_addr_generate_hmac(msk->local_key,
> > +                                                    msk->remote_key,
> > +                                                    &opts->local);
>
> Keep this ahmac generating code after opts->suboptions set just like the
> original code, since ahmac is the more expensive to populate. If remaining
> length isn't enough, no need to set ahmac.
>
> > +               flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> > +       }
> > +
> > +       len = mptcp_add_addr_len(opts);
> >         if (remaining < len)
> >                 return false;
> >
> > @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >         if (drop_other_suboptions)
> >                 *size -= opt_size;
> >         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> > -       if (!echo) {
> > -               opts->ahmac = add_addr_generate_hmac(msk->local_key,
> > -                                                    msk->remote_key,
> > -                                                    &opts->addr);
> > -       }
> > -       pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> > -                opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> > +
> > +       spin_lock_bh(&msk->pm.lock);
> > +       WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> > +       spin_unlock_bh(&msk->pm.lock);
>
> addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
> set it again. I thinks this trunk and all the flags set above should be
> dropped.
>
> > +
> > +       pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> > +                add_addr, (opts->ahmac == 0), opts->local.id,
> > +                opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
> >
> >         return true;
> >  }
>
> The whole function is something like this:
> '''
>         struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>         bool drop_other_suboptions = false;
>         unsigned int opt_size = *size;
>         int len;
>
>         if (!mptcp_pm_should_add_signal(msk) ||
>             !mptcp_pm_add_addr_signal(msk, remaining, opts))
>                 return false;
>
>         if ((mptcp_pm_should_add_signal_echo(msk) ||
>              (mptcp_pm_should_add_signal_addr(msk) &&
>               (opts->local.family == AF_INET6 || opts->local.port))) &&
>             skb && skb_is_tcp_pure_ack(skb)) {
>                 pr_debug("drop other suboptions");
>                 opts->suboptions = 0;
>                 opts->ext_copy.use_ack = 0;
>                 opts->ext_copy.use_map = 0;
>                 remaining += opt_size;
>                 drop_other_suboptions = true;
>         }
>
>         len = mptcp_add_addr_len(opts);
>         if (remaining < len)
>                 return false;
>
>         *size = len;
>         if (drop_other_suboptions)
>                 *size -= opt_size;
>         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>         if (mptcp_pm_should_add_signal_addr(msk)) {
>                 opts->ahmac = add_addr_generate_hmac(msk->local_key,
>                                                      msk->remote_key,
>                                                      &opts->local);
>         }
>

Sorry, no need to add this blank line here.

>         pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
> ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
>                  msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
>                  opts->ahmac, ntohs(opts->local.port),
> opts->remote.id, ntohs(opts->remote.port));
>
>         return true;
> '''
>
> > @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >
> >  mp_capable_done:
> >         if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> > +               struct mptcp_addr_info *addr = &opts->remote;
>
> We can simplify it like this:
>          struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> &opts->remote;
>
> >                 u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >                 u8 echo = MPTCP_ADDR_ECHO;
> >
> > +               if (opts->ahmac)
> > +                       addr = &opts->local;
>
> And this trunk can be dropped.
>
> > +
> >  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> > -               if (opts->addr.family == AF_INET6)
> > +               if (addr->family == AF_INET6)
> >                         len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >  #endif
> >
> > -               if (opts->addr.port)
> > +               if (addr->port)
> >                         len += TCPOLEN_MPTCP_PORT_LEN;
> >
> >                 if (opts->ahmac) {
> > @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >                 }
> >
> >                 *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> > -                                     len, echo, opts->addr.id);
> > -               if (opts->addr.family == AF_INET) {
> > -                       memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> > +                                     len, echo, addr->id);
> > +               if (addr->family == AF_INET) {
> > +                       memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
> >                         ptr += 1;
> >                 }
> >  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> > -               else if (opts->addr.family == AF_INET6) {
> > -                       memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
> > +               else if (addr->family == AF_INET6) {
> > +                       memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
> >                         ptr += 4;
> >                 }
> >  #endif
> >
> > -               if (!opts->addr.port) {
> > +               if (!addr->port) {
> >                         if (opts->ahmac) {
> >                                 put_unaligned_be64(opts->ahmac, ptr);
> >                                 ptr += 2;
> >                         }
> >                 } else {
> > -                       u16 port = ntohs(opts->addr.port);
> > +                       u16 port = ntohs(addr->port);
> >
> >                         if (opts->ahmac) {
> >                                 u8 *bptr = (u8 *)ptr;
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index cf873e9..9c621293 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >
> >  /* path manager helpers */
> >
> > -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > -                             struct mptcp_addr_info *saddr, bool *echo, bool *port)
> > +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> > +                             u8 *add_addr)
>
> Drop this add_addr argument.
>
> >  {
> > -       u8 add_addr;
> > -       int ret = false;
> > -
> >         spin_lock_bh(&msk->pm.lock);
> >
> > -       /* double check after the lock is acquired */
> > -       if (!mptcp_pm_should_add_signal(msk))
> > -               goto out_unlock;
>
> Keep this double check code.
>
> > -
> > -       *echo = mptcp_pm_should_add_signal_echo(msk);
> > -       *port = mptcp_pm_should_add_signal_port(msk);
> > -
> > -       if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> > -               goto out_unlock;
>
> Keep this length double check code too.
>
> > +       if (!mptcp_pm_should_add_signal(msk)) {
> > +               spin_unlock_bh(&msk->pm.lock);
> > +               return false;
> > +       }
> >
> > -       *saddr = msk->pm.local;
> > -       add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> -       WRITE_ONCE(msk->pm.addr_signal, add_addr);
>
> This code is just added in patch 1, I think we should keep it. And no need
> to write addr_signal again in mptcp_established_options_add_addr.
>
> > -       ret = true;
> > +       opts->local = msk->pm.local;
> > +       opts->remote = msk->pm.remote;
> > +       *add_addr = msk->pm.addr_signal;
> >
> > -out_unlock:
> >         spin_unlock_bh(&msk->pm.lock);
> > -       return ret;
>
> Keep this out_unlock code.
>
> > +
> > +       if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> > +               mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>
> Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
>
> I'm no sure why we need this two lines, and why you use '&&' here. Do you
> mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?
>
> > +       return true;
> >  }
>
> The whole function is something like this:
> '''
>         int ret = false;
>         u8 add_addr;
>
>         spin_lock_bh(&msk->pm.lock);
>
>         /* double check after the lock is acquired */
>         if (!mptcp_pm_should_add_signal(msk))
>                 goto out_unlock;
>
>         if (remaining < mptcp_add_addr_len(opts))
>                 goto out_unlock;
>
>         opts->local = msk->pm.local;
>         opts->remote = msk->pm.remote;
>         if (mptcp_pm_should_add_signal_echo(msk))
>                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
>         else
>                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
>         WRITE_ONCE(msk->pm.addr_signal, add_addr);
>         ret = true;
>
> out_unlock:
>         spin_unlock_bh(&msk->pm.lock);
>         if (ret && mptcp_pm_should_add_signal_echo(msk) &&
> mptcp_pm_should_add_signal_addr(msk))
>                 mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>         return ret;
> '''
>
> >
> >  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index a0b0ec0..0bfbbdef 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
> >         return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
> >  }
> >
> > -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> > +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
> >  {
> > -       u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> > +       u8 len = 0;
> > +       struct mptcp_addr_info *addr = &opts->remote;
>
> We can simplify it like this:
>          struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> &opts->remote;
>
> And keep the orignal code unchanged.
>
> >
> > -       if (family == AF_INET6)
> > -               len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> > -       if (!echo)
> > +       if (opts->ahmac) {
> > +               addr = &opts->local;
> >                 len += MPTCPOPT_THMAC_LEN;
> > +       }
> > +
> > +       if (addr->family == AF_INET6)
> > +               len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> > +       else
> > +               len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
> > +
> >         /* account for 2 trailing 'nop' options */
> > -       if (port)
> > +       if (addr->port)
> >                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >
> >         return len;
>
> The whole function is something like this:
> '''
>         struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> &opts->remote;
>         u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>
>         if (addr->family == AF_INET6)
>                 len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>         if (opts->ahmac)
>                 len += MPTCPOPT_THMAC_LEN;
>         /* account for 2 trailing 'nop' options */
>         if (addr->port)
>                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>
>         return len;
> '''
>
> Thanks.
> -Geliang
>
> > @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
> >         return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
> >  }
> >
> > -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > -                             struct mptcp_addr_info *saddr, bool *echo, bool *port);
> > +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> > +                             u8 *add_addr);
> >  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >                              struct mptcp_rm_list *rm_list);
> >  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> > --
> > 1.8.3.1
> >
> >

YonglongLi June 29, 2021, 7:01 a.m. UTC | #3

Hi Geiliang, Thanks for your reviews.

On 2021/6/29 13:58, Geliang Tang wrote:
> Yonglong Li <[e-mail]> 于2021年6月29日周二 上午9:42写道：
>>
>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>> ADD_ADDR/echo-ADD_ADDR option
>>
>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>
>> Signed-off-by: Yonglong Li <[e-mail]>
>> ---
>>  include/net/mptcp.h  |  3 ++-
>>  net/mptcp/options.c  | 65 +++++++++++++++++++++++++++++++---------------------
>>  net/mptcp/pm.c       | 33 +++++++++++---------------
>>  net/mptcp/protocol.h | 23 ++++++++++++-------
>>  4 files changed, 69 insertions(+), 55 deletions(-)
>>
>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
>> index d61bbbf..d2c6ebe 100644
>> --- a/include/net/mptcp.h
>> +++ b/include/net/mptcp.h
>> @@ -61,7 +61,8 @@ struct mptcp_out_options {
>>         u64 sndr_key;
>>         u64 rcvr_key;
>>         u64 ahmac;
>> -       struct mptcp_addr_info addr;
>> +       struct mptcp_addr_info local;
>> +       struct mptcp_addr_info remote;
>>         struct mptcp_rm_list rm_list;
>>         u8 join_id;
>>         u8 backup;
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index 1aec016..1707bec 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>         bool drop_other_suboptions = false;
>>         unsigned int opt_size = *size;
>> -       bool echo;
>> -       bool port;
>> -       int len;
>> +       u8 add_addr, flags = 0xff;
>> +       int len = 0;
>>
>> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>> -            mptcp_pm_should_add_signal_port(msk) ||
>> -            mptcp_pm_should_add_signal_echo(msk)) &&
>> +       if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
>> +               return false;
> 
> This add_addr argument is useless, let's drop it.
> 
we can use add_addr use in debug log.

> And here add back mptcp_pm_should_add_signal check here. The original code
> called mptcp_pm_should_add_signal twice for double check, once out of pm
> lock, once under pm lock. We should keep it.
Sorry, I think double check is not necessary. does we need double check?

> 
>> +
>> +       if ((mptcp_pm_should_add_signal_echo(msk) ||
>> +            (mptcp_pm_should_add_signal_addr(msk) &&
>> +             (opts->local.family == AF_INET6 || opts->local.port))) &&
>>             skb && skb_is_tcp_pure_ack(skb)) {
>>                 pr_debug("drop other suboptions");
>>                 opts->suboptions = 0;
>> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>                 drop_other_suboptions = true;
>>         }
>>
>> -       if (!mptcp_pm_should_add_signal(msk) ||
>> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>> -               return false;
>>
>> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
>> +       if (mptcp_pm_should_add_signal_echo(msk)) {
>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>> +       } else {
>> +               opts->ahmac = add_addr_generate_hmac(msk->local_key,
>> +                                                    msk->remote_key,
>> +                                                    &opts->local);
> 
> Keep this ahmac generating code after opts->suboptions set just like the
> original code, since ahmac is the more expensive to populate. If remaining
> length isn't enough, no need to set ahmac.

because mptcp_add_addr_len(opts) will use ahmac to calculate len of opts, so I think Keep this ahmac
generating code after opts->suboptions set is not ok.

> 
>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>> +       }
>> +
>> +       len = mptcp_add_addr_len(opts);
>>         if (remaining < len)
>>                 return false;
>>
>> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>         if (drop_other_suboptions)
>>                 *size -= opt_size;
>>         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>> -       if (!echo) {
>> -               opts->ahmac = add_addr_generate_hmac(msk->local_key,
>> -                                                    msk->remote_key,
>> -                                                    &opts->addr);
>> -       }
>> -       pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
>> -                opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
>> +
>> +       spin_lock_bh(&msk->pm.lock);
>> +       WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
>> +       spin_unlock_bh(&msk->pm.lock);
> 
> addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
> set it again. I thinks this trunk and all the flags set above should be
> dropped.

Because MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO maybe set at the same time.
So i think we should only unset one flag.

> 
>> +
>> +       pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
>> +                add_addr, (opts->ahmac == 0), opts->local.id,
>> +                opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>>
>>         return true;
>>  }
> 
> The whole function is something like this:
> '''
>         struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>         bool drop_other_suboptions = false;
>         unsigned int opt_size = *size;
>         int len;
> 
>         if (!mptcp_pm_should_add_signal(msk) ||
>             !mptcp_pm_add_addr_signal(msk, remaining, opts))
>                 return false;
> 
>         if ((mptcp_pm_should_add_signal_echo(msk) ||
>              (mptcp_pm_should_add_signal_addr(msk) &&
>               (opts->local.family == AF_INET6 || opts->local.port))) &&
>             skb && skb_is_tcp_pure_ack(skb)) {
>                 pr_debug("drop other suboptions");
>                 opts->suboptions = 0;
>                 opts->ext_copy.use_ack = 0;
>                 opts->ext_copy.use_map = 0;
>                 remaining += opt_size;
>                 drop_other_suboptions = true;
>         }
> 
>         len = mptcp_add_addr_len(opts);
>         if (remaining < len)
>                 return false;
> 
>         *size = len;
>         if (drop_other_suboptions)
>                 *size -= opt_size;
>         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>         if (mptcp_pm_should_add_signal_addr(msk)) {
>                 opts->ahmac = add_addr_generate_hmac(msk->local_key,
>                                                      msk->remote_key,
>                                                      &opts->local);
>         }
> 
>         pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
> ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
>                  msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
>                  opts->ahmac, ntohs(opts->local.port),
> opts->remote.id, ntohs(opts->remote.port));
> 
>         return true;
> '''
> 
>> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>
>>  mp_capable_done:
>>         if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
>> +               struct mptcp_addr_info *addr = &opts->remote;
> 
> We can simplify it like this:
>          struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> &opts->remote;
> 
>>                 u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>                 u8 echo = MPTCP_ADDR_ECHO;
>>
>> +               if (opts->ahmac)
>> +                       addr = &opts->local;
> 
> And this trunk can be dropped.
> 
>> +
>>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>> -               if (opts->addr.family == AF_INET6)
>> +               if (addr->family == AF_INET6)
>>                         len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>  #endif
>>
>> -               if (opts->addr.port)
>> +               if (addr->port)
>>                         len += TCPOLEN_MPTCP_PORT_LEN;
>>
>>                 if (opts->ahmac) {
>> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>                 }
>>
>>                 *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
>> -                                     len, echo, opts->addr.id);
>> -               if (opts->addr.family == AF_INET) {
>> -                       memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
>> +                                     len, echo, addr->id);
>> +               if (addr->family == AF_INET) {
>> +                       memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
>>                         ptr += 1;
>>                 }
>>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>> -               else if (opts->addr.family == AF_INET6) {
>> -                       memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
>> +               else if (addr->family == AF_INET6) {
>> +                       memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
>>                         ptr += 4;
>>                 }
>>  #endif
>>
>> -               if (!opts->addr.port) {
>> +               if (!addr->port) {
>>                         if (opts->ahmac) {
>>                                 put_unaligned_be64(opts->ahmac, ptr);
>>                                 ptr += 2;
>>                         }
>>                 } else {
>> -                       u16 port = ntohs(opts->addr.port);
>> +                       u16 port = ntohs(addr->port);
>>
>>                         if (opts->ahmac) {
>>                                 u8 *bptr = (u8 *)ptr;
>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>> index cf873e9..9c621293 100644
>> --- a/net/mptcp/pm.c
>> +++ b/net/mptcp/pm.c
>> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>
>>  /* path manager helpers */
>>
>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port)
>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
>> +                             u8 *add_addr)
> 
> Drop this add_addr argument.
> 
>>  {
>> -       u8 add_addr;
>> -       int ret = false;
>> -
>>         spin_lock_bh(&msk->pm.lock);
>>
>> -       /* double check after the lock is acquired */
>> -       if (!mptcp_pm_should_add_signal(msk))
>> -               goto out_unlock;
> 
> Keep this double check code.
> 
>> -
>> -       *echo = mptcp_pm_should_add_signal_echo(msk);
>> -       *port = mptcp_pm_should_add_signal_port(msk);
>> -
>> -       if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
>> -               goto out_unlock;
> 
> Keep this length double check code too.
> 
>> +       if (!mptcp_pm_should_add_signal(msk)) {
>> +               spin_unlock_bh(&msk->pm.lock);
>> +               return false;
>> +       }
>>
>> -       *saddr = msk->pm.local;
>> -       add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> -       WRITE_ONCE(msk->pm.addr_signal, add_addr);
> 
> This code is just added in patch 1, I think we should keep it. And no need
> to write addr_signal again in mptcp_established_options_add_addr.
> 
>> -       ret = true;
>> +       opts->local = msk->pm.local;
>> +       opts->remote = msk->pm.remote;
>> +       *add_addr = msk->pm.addr_signal;
>>
>> -out_unlock:
>>         spin_unlock_bh(&msk->pm.lock);
>> -       return ret;
> 
> Keep this out_unlock code.
> 
>> +
>> +       if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
>> +               mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> 
> Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
> 
> I'm no sure why we need this two lines, and why you use '&&' here. Do you
> mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?
> 
>> +       return true;
>>  }
> 
> The whole function is something like this:
> '''
>         int ret = false;
>         u8 add_addr;
> 
>         spin_lock_bh(&msk->pm.lock);
> 
>         /* double check after the lock is acquired */
>         if (!mptcp_pm_should_add_signal(msk))
>                 goto out_unlock;
> 
>         if (remaining < mptcp_add_addr_len(opts))
>                 goto out_unlock;
> 
>         opts->local = msk->pm.local;
>         opts->remote = msk->pm.remote;
>         if (mptcp_pm_should_add_signal_echo(msk))
>                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
>         else
>                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
>         WRITE_ONCE(msk->pm.addr_signal, add_addr);
>         ret = true;
> 
> out_unlock:
>         spin_unlock_bh(&msk->pm.lock);
>         if (ret && mptcp_pm_should_add_signal_echo(msk) &&
> mptcp_pm_should_add_signal_addr(msk))
>                 mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>         return ret;
> '''
> 
>>
>>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index a0b0ec0..0bfbbdef 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
>>         return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
>>  }
>>
>> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
>> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
>>  {
>> -       u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>> +       u8 len = 0;
>> +       struct mptcp_addr_info *addr = &opts->remote;
> 
> We can simplify it like this:
>          struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> &opts->remote;
> 
> And keep the orignal code unchanged.
> 
>>
>> -       if (family == AF_INET6)
>> -               len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>> -       if (!echo)
>> +       if (opts->ahmac) {
>> +               addr = &opts->local;
>>                 len += MPTCPOPT_THMAC_LEN;
>> +       }
>> +
>> +       if (addr->family == AF_INET6)
>> +               len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>> +       else
>> +               len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>> +
>>         /* account for 2 trailing 'nop' options */
>> -       if (port)
>> +       if (addr->port)
>>                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>
>>         return len;
> 
> The whole function is something like this:
> '''
>         struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> &opts->remote;
>         u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> 
>         if (addr->family == AF_INET6)
>                 len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>         if (opts->ahmac)
>                 len += MPTCPOPT_THMAC_LEN;
>         /* account for 2 trailing 'nop' options */
>         if (addr->port)
>                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> 
>         return len;
> '''
> 
> Thanks.
> -Geliang
> 
>> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
>>         return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
>>  }
>>
>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port);
>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
>> +                             u8 *add_addr);
>>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>                              struct mptcp_rm_list *rm_list);
>>  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
>> --
>> 1.8.3.1
>>
>>
> 
>

Geliang Tang June 29, 2021, 7:35 a.m. UTC | #4

Yonglong Li <[e-mail]> 于2021年6月29日周二 下午3:02写道：
>
>
> Hi Geiliang, Thanks for your reviews.
>
> On 2021/6/29 13:58, Geliang Tang wrote:
> > Yonglong Li <[e-mail]> 于2021年6月29日周二 上午9:42写道：
> >>
> >> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> >> ADD_ADDR/echo-ADD_ADDR option
> >>
> >> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >>
> >> Signed-off-by: Yonglong Li <[e-mail]>
> >> ---
> >>  include/net/mptcp.h  |  3 ++-
> >>  net/mptcp/options.c  | 65 +++++++++++++++++++++++++++++++---------------------
> >>  net/mptcp/pm.c       | 33 +++++++++++---------------
> >>  net/mptcp/protocol.h | 23 ++++++++++++-------
> >>  4 files changed, 69 insertions(+), 55 deletions(-)
> >>
> >> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> >> index d61bbbf..d2c6ebe 100644
> >> --- a/include/net/mptcp.h
> >> +++ b/include/net/mptcp.h
> >> @@ -61,7 +61,8 @@ struct mptcp_out_options {
> >>         u64 sndr_key;
> >>         u64 rcvr_key;
> >>         u64 ahmac;
> >> -       struct mptcp_addr_info addr;
> >> +       struct mptcp_addr_info local;
> >> +       struct mptcp_addr_info remote;
> >>         struct mptcp_rm_list rm_list;
> >>         u8 join_id;
> >>         u8 backup;
> >> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >> index 1aec016..1707bec 100644
> >> --- a/net/mptcp/options.c
> >> +++ b/net/mptcp/options.c
> >> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >>         bool drop_other_suboptions = false;
> >>         unsigned int opt_size = *size;
> >> -       bool echo;
> >> -       bool port;
> >> -       int len;
> >> +       u8 add_addr, flags = 0xff;
> >> +       int len = 0;
> >>
> >> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> >> -            mptcp_pm_should_add_signal_port(msk) ||
> >> -            mptcp_pm_should_add_signal_echo(msk)) &&
> >> +       if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
> >> +               return false;
> >
> > This add_addr argument is useless, let's drop it.
> >
> we can use add_addr use in debug log.

I think it's not worth adding a new argument just for debugging.

>
> > And here add back mptcp_pm_should_add_signal check here. The original code
> > called mptcp_pm_should_add_signal twice for double check, once out of pm
> > lock, once under pm lock. We should keep it.
> Sorry, I think double check is not necessary. does we need double check?

I think we should keep the original logic here. If we want to drop this
double check or something, we should do it in another patch, don't mix too
much things in one patch.

>
> >
> >> +
> >> +       if ((mptcp_pm_should_add_signal_echo(msk) ||
> >> +            (mptcp_pm_should_add_signal_addr(msk) &&
> >> +             (opts->local.family == AF_INET6 || opts->local.port))) &&
> >>             skb && skb_is_tcp_pure_ack(skb)) {
> >>                 pr_debug("drop other suboptions");
> >>                 opts->suboptions = 0;
> >> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>                 drop_other_suboptions = true;
> >>         }
> >>
> >> -       if (!mptcp_pm_should_add_signal(msk) ||
> >> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> >> -               return false;
> >>
> >> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
> >> +       if (mptcp_pm_should_add_signal_echo(msk)) {
> >> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> >> +       } else {
> >> +               opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >> +                                                    msk->remote_key,
> >> +                                                    &opts->local);
> >
> > Keep this ahmac generating code after opts->suboptions set just like the
> > original code, since ahmac is the more expensive to populate. If remaining
> > length isn't enough, no need to set ahmac.
>
> because mptcp_add_addr_len(opts) will use ahmac to calculate len of opts, so I think Keep this ahmac
> generating code after opts->suboptions set is not ok.

So we should use mptcp_pm_should_add_signal_addr instead of opts->ahmac in
mptcp_add_addr_len.

>
> >
> >> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> >> +       }
> >> +
> >> +       len = mptcp_add_addr_len(opts);
> >>         if (remaining < len)
> >>                 return false;
> >>
> >> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>         if (drop_other_suboptions)
> >>                 *size -= opt_size;
> >>         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >> -       if (!echo) {
> >> -               opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >> -                                                    msk->remote_key,
> >> -                                                    &opts->addr);
> >> -       }
> >> -       pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> >> -                opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> >> +
> >> +       spin_lock_bh(&msk->pm.lock);
> >> +       WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> >> +       spin_unlock_bh(&msk->pm.lock);
> >
> > addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
> > set it again. I thinks this trunk and all the flags set above should be
> > dropped.
>
> Because MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO maybe set at the same time.
> So i think we should only unset one flag.

We can only unset one flag in mptcp_pm_add_addr_signal, see my comment in
patch 1.

-Geliang

>
> >
> >> +
> >> +       pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> >> +                add_addr, (opts->ahmac == 0), opts->local.id,
> >> +                opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
> >>
> >>         return true;
> >>  }
> >
> > The whole function is something like this:
> > '''
> >         struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> >         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >         bool drop_other_suboptions = false;
> >         unsigned int opt_size = *size;
> >         int len;
> >
> >         if (!mptcp_pm_should_add_signal(msk) ||
> >             !mptcp_pm_add_addr_signal(msk, remaining, opts))
> >                 return false;
> >
> >         if ((mptcp_pm_should_add_signal_echo(msk) ||
> >              (mptcp_pm_should_add_signal_addr(msk) &&
> >               (opts->local.family == AF_INET6 || opts->local.port))) &&
> >             skb && skb_is_tcp_pure_ack(skb)) {
> >                 pr_debug("drop other suboptions");
> >                 opts->suboptions = 0;
> >                 opts->ext_copy.use_ack = 0;
> >                 opts->ext_copy.use_map = 0;
> >                 remaining += opt_size;
> >                 drop_other_suboptions = true;
> >         }
> >
> >         len = mptcp_add_addr_len(opts);
> >         if (remaining < len)
> >                 return false;
> >
> >         *size = len;
> >         if (drop_other_suboptions)
> >                 *size -= opt_size;
> >         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >         if (mptcp_pm_should_add_signal_addr(msk)) {
> >                 opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >                                                      msk->remote_key,
> >                                                      &opts->local);
> >         }
> >
> >         pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
> > ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> >                  msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
> >                  opts->ahmac, ntohs(opts->local.port),
> > opts->remote.id, ntohs(opts->remote.port));
> >
> >         return true;
> > '''
> >
> >> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>
> >>  mp_capable_done:
> >>         if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> >> +               struct mptcp_addr_info *addr = &opts->remote;
> >
> > We can simplify it like this:
> >          struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> > &opts->remote;
> >
> >>                 u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>                 u8 echo = MPTCP_ADDR_ECHO;
> >>
> >> +               if (opts->ahmac)
> >> +                       addr = &opts->local;
> >
> > And this trunk can be dropped.
> >
> >> +
> >>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >> -               if (opts->addr.family == AF_INET6)
> >> +               if (addr->family == AF_INET6)
> >>                         len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>  #endif
> >>
> >> -               if (opts->addr.port)
> >> +               if (addr->port)
> >>                         len += TCPOLEN_MPTCP_PORT_LEN;
> >>
> >>                 if (opts->ahmac) {
> >> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>                 }
> >>
> >>                 *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> >> -                                     len, echo, opts->addr.id);
> >> -               if (opts->addr.family == AF_INET) {
> >> -                       memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> >> +                                     len, echo, addr->id);
> >> +               if (addr->family == AF_INET) {
> >> +                       memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
> >>                         ptr += 1;
> >>                 }
> >>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >> -               else if (opts->addr.family == AF_INET6) {
> >> -                       memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
> >> +               else if (addr->family == AF_INET6) {
> >> +                       memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
> >>                         ptr += 4;
> >>                 }
> >>  #endif
> >>
> >> -               if (!opts->addr.port) {
> >> +               if (!addr->port) {
> >>                         if (opts->ahmac) {
> >>                                 put_unaligned_be64(opts->ahmac, ptr);
> >>                                 ptr += 2;
> >>                         }
> >>                 } else {
> >> -                       u16 port = ntohs(opts->addr.port);
> >> +                       u16 port = ntohs(addr->port);
> >>
> >>                         if (opts->ahmac) {
> >>                                 u8 *bptr = (u8 *)ptr;
> >> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> >> index cf873e9..9c621293 100644
> >> --- a/net/mptcp/pm.c
> >> +++ b/net/mptcp/pm.c
> >> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >>
> >>  /* path manager helpers */
> >>
> >> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port)
> >> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> >> +                             u8 *add_addr)
> >
> > Drop this add_addr argument.
> >
> >>  {
> >> -       u8 add_addr;
> >> -       int ret = false;
> >> -
> >>         spin_lock_bh(&msk->pm.lock);
> >>
> >> -       /* double check after the lock is acquired */
> >> -       if (!mptcp_pm_should_add_signal(msk))
> >> -               goto out_unlock;
> >
> > Keep this double check code.
> >
> >> -
> >> -       *echo = mptcp_pm_should_add_signal_echo(msk);
> >> -       *port = mptcp_pm_should_add_signal_port(msk);
> >> -
> >> -       if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> >> -               goto out_unlock;
> >
> > Keep this length double check code too.
> >
> >> +       if (!mptcp_pm_should_add_signal(msk)) {
> >> +               spin_unlock_bh(&msk->pm.lock);
> >> +               return false;
> >> +       }
> >>
> >> -       *saddr = msk->pm.local;
> >> -       add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> > -       WRITE_ONCE(msk->pm.addr_signal, add_addr);
> >
> > This code is just added in patch 1, I think we should keep it. And no need
> > to write addr_signal again in mptcp_established_options_add_addr.
> >
> >> -       ret = true;
> >> +       opts->local = msk->pm.local;
> >> +       opts->remote = msk->pm.remote;
> >> +       *add_addr = msk->pm.addr_signal;
> >>
> >> -out_unlock:
> >>         spin_unlock_bh(&msk->pm.lock);
> >> -       return ret;
> >
> > Keep this out_unlock code.
> >
> >> +
> >> +       if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> >> +               mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> >
> > Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
> >
> > I'm no sure why we need this two lines, and why you use '&&' here. Do you
> > mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?
> >
> >> +       return true;
> >>  }
> >
> > The whole function is something like this:
> > '''
> >         int ret = false;
> >         u8 add_addr;
> >
> >         spin_lock_bh(&msk->pm.lock);
> >
> >         /* double check after the lock is acquired */
> >         if (!mptcp_pm_should_add_signal(msk))
> >                 goto out_unlock;
> >
> >         if (remaining < mptcp_add_addr_len(opts))
> >                 goto out_unlock;
> >
> >         opts->local = msk->pm.local;
> >         opts->remote = msk->pm.remote;
> >         if (mptcp_pm_should_add_signal_echo(msk))
> >                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
> >         else
> >                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
> >         WRITE_ONCE(msk->pm.addr_signal, add_addr);
> >         ret = true;
> >
> > out_unlock:
> >         spin_unlock_bh(&msk->pm.lock);
> >         if (ret && mptcp_pm_should_add_signal_echo(msk) &&
> > mptcp_pm_should_add_signal_addr(msk))
> >                 mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> >         return ret;
> > '''
> >
> >>
> >>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> >> index a0b0ec0..0bfbbdef 100644
> >> --- a/net/mptcp/protocol.h
> >> +++ b/net/mptcp/protocol.h
> >> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
> >>         return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
> >>  }
> >>
> >> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> >> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
> >>  {
> >> -       u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >> +       u8 len = 0;
> >> +       struct mptcp_addr_info *addr = &opts->remote;
> >
> > We can simplify it like this:
> >          struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> > &opts->remote;
> >
> > And keep the orignal code unchanged.
> >
> >>
> >> -       if (family == AF_INET6)
> >> -               len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >> -       if (!echo)
> >> +       if (opts->ahmac) {
> >> +               addr = &opts->local;
> >>                 len += MPTCPOPT_THMAC_LEN;
> >> +       }
> >> +
> >> +       if (addr->family == AF_INET6)
> >> +               len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >> +       else
> >> +               len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >> +
> >>         /* account for 2 trailing 'nop' options */
> >> -       if (port)
> >> +       if (addr->port)
> >>                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >>
> >>         return len;
> >
> > The whole function is something like this:
> > '''
> >         struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> > &opts->remote;
> >         u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >
> >         if (addr->family == AF_INET6)
> >                 len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >         if (opts->ahmac)
> >                 len += MPTCPOPT_THMAC_LEN;
> >         /* account for 2 trailing 'nop' options */
> >         if (addr->port)
> >                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >
> >         return len;
> > '''
> >
> > Thanks.
> > -Geliang
> >
> >> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
> >>         return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
> >>  }
> >>
> >> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port);
> >> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> >> +                             u8 *add_addr);
> >>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>                              struct mptcp_rm_list *rm_list);
> >>  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> >> --
> >> 1.8.3.1
> >>
> >>
> >
> >
>
> --
> Li YongLong

YonglongLi June 29, 2021, 7:54 a.m. UTC | #5

On 2021/6/29 15:35, Geliang Tang wrote:
> Yonglong Li <[e-mail]> 于2021年6月29日周二 下午3:02写道：
>>
>>
>> Hi Geiliang, Thanks for your reviews.
>>
>> On 2021/6/29 13:58, Geliang Tang wrote:
>>> Yonglong Li <[e-mail]> 于2021年6月29日周二 上午9:42写道：
>>>>
>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>>>> ADD_ADDR/echo-ADD_ADDR option
>>>>
>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>>>
>>>> Signed-off-by: Yonglong Li <[e-mail]>
>>>> ---
>>>>  include/net/mptcp.h  |  3 ++-
>>>>  net/mptcp/options.c  | 65 +++++++++++++++++++++++++++++++---------------------
>>>>  net/mptcp/pm.c       | 33 +++++++++++---------------
>>>>  net/mptcp/protocol.h | 23 ++++++++++++-------
>>>>  4 files changed, 69 insertions(+), 55 deletions(-)
>>>>
>>>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
>>>> index d61bbbf..d2c6ebe 100644
>>>> --- a/include/net/mptcp.h
>>>> +++ b/include/net/mptcp.h
>>>> @@ -61,7 +61,8 @@ struct mptcp_out_options {
>>>>         u64 sndr_key;
>>>>         u64 rcvr_key;
>>>>         u64 ahmac;
>>>> -       struct mptcp_addr_info addr;
>>>> +       struct mptcp_addr_info local;
>>>> +       struct mptcp_addr_info remote;
>>>>         struct mptcp_rm_list rm_list;
>>>>         u8 join_id;
>>>>         u8 backup;
>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>> index 1aec016..1707bec 100644
>>>> --- a/net/mptcp/options.c
>>>> +++ b/net/mptcp/options.c
>>>> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>>         bool drop_other_suboptions = false;
>>>>         unsigned int opt_size = *size;
>>>> -       bool echo;
>>>> -       bool port;
>>>> -       int len;
>>>> +       u8 add_addr, flags = 0xff;
>>>> +       int len = 0;
>>>>
>>>> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>>>> -            mptcp_pm_should_add_signal_port(msk) ||
>>>> -            mptcp_pm_should_add_signal_echo(msk)) &&
>>>> +       if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
>>>> +               return false;
>>>
>>> This add_addr argument is useless, let's drop it.
>>>
>> we can use add_addr use in debug log.
> 
> I think it's not worth adding a new argument just for debugging.
agree.

> 
>>
>>> And here add back mptcp_pm_should_add_signal check here. The original code
>>> called mptcp_pm_should_add_signal twice for double check, once out of pm
>>> lock, once under pm lock. We should keep it.
>> Sorry, I think double check is not necessary. does we need double check?
> 
> I think we should keep the original logic here. If we want to drop this
> double check or something, we should do it in another patch, don't mix too
> much things in one patch.
agree.

> 
>>
>>>
>>>> +
>>>> +       if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>> +            (mptcp_pm_should_add_signal_addr(msk) &&
>>>> +             (opts->local.family == AF_INET6 || opts->local.port))) &&
>>>>             skb && skb_is_tcp_pure_ack(skb)) {
>>>>                 pr_debug("drop other suboptions");
>>>>                 opts->suboptions = 0;
>>>> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>                 drop_other_suboptions = true;
>>>>         }
>>>>
>>>> -       if (!mptcp_pm_should_add_signal(msk) ||
>>>> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>>>> -               return false;
>>>>
>>>> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
>>>> +       if (mptcp_pm_should_add_signal_echo(msk)) {
>>>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>>>> +       } else {
>>>> +               opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>> +                                                    msk->remote_key,
>>>> +                                                    &opts->local);
>>>
>>> Keep this ahmac generating code after opts->suboptions set just like the
>>> original code, since ahmac is the more expensive to populate. If remaining
>>> length isn't enough, no need to set ahmac.
>>
>> because mptcp_add_addr_len(opts) will use ahmac to calculate len of opts, so I think Keep this ahmac
>> generating code after opts->suboptions set is not ok.
> 
> So we should use mptcp_pm_should_add_signal_addr instead of opts->ahmac in
> mptcp_add_addr_len.
agree.

> 
>>
>>>
>>>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>>>> +       }
>>>> +
>>>> +       len = mptcp_add_addr_len(opts);
>>>>         if (remaining < len)
>>>>                 return false;
>>>>
>>>> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>         if (drop_other_suboptions)
>>>>                 *size -= opt_size;
>>>>         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>> -       if (!echo) {
>>>> -               opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>> -                                                    msk->remote_key,
>>>> -                                                    &opts->addr);
>>>> -       }
>>>> -       pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
>>>> -                opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
>>>> +
>>>> +       spin_lock_bh(&msk->pm.lock);
>>>> +       WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
>>>> +       spin_unlock_bh(&msk->pm.lock);
>>>
>>> addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
>>> set it again. I thinks this trunk and all the flags set above should be
>>> dropped.
>>
>> Because MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO maybe set at the same time.
>> So i think we should only unset one flag.
> 
> We can only unset one flag in mptcp_pm_add_addr_signal, see my comment in
> patch 1.

if change like this. there is a issue: if remaining len checking is not ok and return false, The ADD_ADDR/ECHO event will
be clear. So I think we should make sure ADD_ADDR/ECHO option will add in packet before clean flags. WDYT?

> 
> -Geliang
> 
>>
>>>
>>>> +
>>>> +       pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
>>>> +                add_addr, (opts->ahmac == 0), opts->local.id,
>>>> +                opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>>>>
>>>>         return true;
>>>>  }
>>>
>>> The whole function is something like this:
>>> '''
>>>         struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>         bool drop_other_suboptions = false;
>>>         unsigned int opt_size = *size;
>>>         int len;
>>>
>>>         if (!mptcp_pm_should_add_signal(msk) ||
>>>             !mptcp_pm_add_addr_signal(msk, remaining, opts))
>>>                 return false;
>>>
>>>         if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>              (mptcp_pm_should_add_signal_addr(msk) &&
>>>               (opts->local.family == AF_INET6 || opts->local.port))) &&
>>>             skb && skb_is_tcp_pure_ack(skb)) {
>>>                 pr_debug("drop other suboptions");
>>>                 opts->suboptions = 0;
>>>                 opts->ext_copy.use_ack = 0;
>>>                 opts->ext_copy.use_map = 0;
>>>                 remaining += opt_size;
>>>                 drop_other_suboptions = true;
>>>         }
>>>
>>>         len = mptcp_add_addr_len(opts);
>>>         if (remaining < len)
>>>                 return false;
>>>
>>>         *size = len;
>>>         if (drop_other_suboptions)
>>>                 *size -= opt_size;
>>>         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>         if (mptcp_pm_should_add_signal_addr(msk)) {
>>>                 opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>                                                      msk->remote_key,
>>>                                                      &opts->local);
>>>         }
>>>
>>>         pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
>>> ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
>>>                  msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
>>>                  opts->ahmac, ntohs(opts->local.port),
>>> opts->remote.id, ntohs(opts->remote.port));
>>>
>>>         return true;
>>> '''
>>>
>>>> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>>>
>>>>  mp_capable_done:
>>>>         if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
>>>> +               struct mptcp_addr_info *addr = &opts->remote;
>>>
>>> We can simplify it like this:
>>>          struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>> &opts->remote;
>>>
>>>>                 u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>                 u8 echo = MPTCP_ADDR_ECHO;
>>>>
>>>> +               if (opts->ahmac)
>>>> +                       addr = &opts->local;
>>>
>>> And this trunk can be dropped.
>>>
>>>> +
>>>>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>>> -               if (opts->addr.family == AF_INET6)
>>>> +               if (addr->family == AF_INET6)
>>>>                         len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>  #endif
>>>>
>>>> -               if (opts->addr.port)
>>>> +               if (addr->port)
>>>>                         len += TCPOLEN_MPTCP_PORT_LEN;
>>>>
>>>>                 if (opts->ahmac) {
>>>> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>>>                 }
>>>>
>>>>                 *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
>>>> -                                     len, echo, opts->addr.id);
>>>> -               if (opts->addr.family == AF_INET) {
>>>> -                       memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
>>>> +                                     len, echo, addr->id);
>>>> +               if (addr->family == AF_INET) {
>>>> +                       memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
>>>>                         ptr += 1;
>>>>                 }
>>>>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>>> -               else if (opts->addr.family == AF_INET6) {
>>>> -                       memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
>>>> +               else if (addr->family == AF_INET6) {
>>>> +                       memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
>>>>                         ptr += 4;
>>>>                 }
>>>>  #endif
>>>>
>>>> -               if (!opts->addr.port) {
>>>> +               if (!addr->port) {
>>>>                         if (opts->ahmac) {
>>>>                                 put_unaligned_be64(opts->ahmac, ptr);
>>>>                                 ptr += 2;
>>>>                         }
>>>>                 } else {
>>>> -                       u16 port = ntohs(opts->addr.port);
>>>> +                       u16 port = ntohs(addr->port);
>>>>
>>>>                         if (opts->ahmac) {
>>>>                                 u8 *bptr = (u8 *)ptr;
>>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>>> index cf873e9..9c621293 100644
>>>> --- a/net/mptcp/pm.c
>>>> +++ b/net/mptcp/pm.c
>>>> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>>>
>>>>  /* path manager helpers */
>>>>
>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port)
>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
>>>> +                             u8 *add_addr)
>>>
>>> Drop this add_addr argument.
>>>
>>>>  {
>>>> -       u8 add_addr;
>>>> -       int ret = false;
>>>> -
>>>>         spin_lock_bh(&msk->pm.lock);
>>>>
>>>> -       /* double check after the lock is acquired */
>>>> -       if (!mptcp_pm_should_add_signal(msk))
>>>> -               goto out_unlock;
>>>
>>> Keep this double check code.
>>>
>>>> -
>>>> -       *echo = mptcp_pm_should_add_signal_echo(msk);
>>>> -       *port = mptcp_pm_should_add_signal_port(msk);
>>>> -
>>>> -       if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
>>>> -               goto out_unlock;
>>>
>>> Keep this length double check code too.
>>>
>>>> +       if (!mptcp_pm_should_add_signal(msk)) {
>>>> +               spin_unlock_bh(&msk->pm.lock);
>>>> +               return false;
>>>> +       }
>>>>
>>>> -       *saddr = msk->pm.local;
>>>> -       add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
>>> -       WRITE_ONCE(msk->pm.addr_signal, add_addr);
>>>
>>> This code is just added in patch 1, I think we should keep it. And no need
>>> to write addr_signal again in mptcp_established_options_add_addr.
>>>
>>>> -       ret = true;
>>>> +       opts->local = msk->pm.local;
>>>> +       opts->remote = msk->pm.remote;
>>>> +       *add_addr = msk->pm.addr_signal;
>>>>
>>>> -out_unlock:
>>>>         spin_unlock_bh(&msk->pm.lock);
>>>> -       return ret;
>>>
>>> Keep this out_unlock code.
>>>
>>>> +
>>>> +       if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
>>>> +               mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>>>
>>> Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
>>>
>>> I'm no sure why we need this two lines, and why you use '&&' here. Do you
>>> mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?
>>>
>>>> +       return true;
>>>>  }
>>>
>>> The whole function is something like this:
>>> '''
>>>         int ret = false;
>>>         u8 add_addr;
>>>
>>>         spin_lock_bh(&msk->pm.lock);
>>>
>>>         /* double check after the lock is acquired */
>>>         if (!mptcp_pm_should_add_signal(msk))
>>>                 goto out_unlock;
>>>
>>>         if (remaining < mptcp_add_addr_len(opts))
>>>                 goto out_unlock;
>>>
>>>         opts->local = msk->pm.local;
>>>         opts->remote = msk->pm.remote;
>>>         if (mptcp_pm_should_add_signal_echo(msk))
>>>                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
>>>         else
>>>                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
>>>         WRITE_ONCE(msk->pm.addr_signal, add_addr);
>>>         ret = true;
>>>
>>> out_unlock:
>>>         spin_unlock_bh(&msk->pm.lock);
>>>         if (ret && mptcp_pm_should_add_signal_echo(msk) &&
>>> mptcp_pm_should_add_signal_addr(msk))
>>>                 mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>>>         return ret;
>>> '''
>>>
>>>>
>>>>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>>> index a0b0ec0..0bfbbdef 100644
>>>> --- a/net/mptcp/protocol.h
>>>> +++ b/net/mptcp/protocol.h
>>>> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
>>>>         return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
>>>>  }
>>>>
>>>> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
>>>> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
>>>>  {
>>>> -       u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>> +       u8 len = 0;
>>>> +       struct mptcp_addr_info *addr = &opts->remote;
>>>
>>> We can simplify it like this:
>>>          struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>> &opts->remote;
>>>
>>> And keep the orignal code unchanged.
>>>
>>>>
>>>> -       if (family == AF_INET6)
>>>> -               len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>> -       if (!echo)
>>>> +       if (opts->ahmac) {
>>>> +               addr = &opts->local;
>>>>                 len += MPTCPOPT_THMAC_LEN;
>>>> +       }
>>>> +
>>>> +       if (addr->family == AF_INET6)
>>>> +               len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>> +       else
>>>> +               len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>> +
>>>>         /* account for 2 trailing 'nop' options */
>>>> -       if (port)
>>>> +       if (addr->port)
>>>>                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>>>
>>>>         return len;
>>>
>>> The whole function is something like this:
>>> '''
>>>         struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>> &opts->remote;
>>>         u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>
>>>         if (addr->family == AF_INET6)
>>>                 len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>         if (opts->ahmac)
>>>                 len += MPTCPOPT_THMAC_LEN;
>>>         /* account for 2 trailing 'nop' options */
>>>         if (addr->port)
>>>                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>>
>>>         return len;
>>> '''
>>>
>>> Thanks.
>>> -Geliang
>>>
>>>> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
>>>>         return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
>>>>  }
>>>>
>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port);
>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
>>>> +                             u8 *add_addr);
>>>>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>                              struct mptcp_rm_list *rm_list);
>>>>  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
>>>> --
>>>> 1.8.3.1
>>>>
>>>>
>>>
>>>
>>
>> --
>> Li YongLong
>

Geliang Tang June 29, 2021, 8:25 a.m. UTC | #6

Yonglong Li <[e-mail]> 于2021年6月29日周二 下午3:54写道：
>
>
>
> On 2021/6/29 15:35, Geliang Tang wrote:
> > Yonglong Li <[e-mail]> 于2021年6月29日周二 下午3:02写道：
> >>
> >>
> >> Hi Geiliang, Thanks for your reviews.
> >>
> >> On 2021/6/29 13:58, Geliang Tang wrote:
> >>> Yonglong Li <[e-mail]> 于2021年6月29日周二 上午9:42写道：
> >>>>
> >>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> >>>> ADD_ADDR/echo-ADD_ADDR option
> >>>>
> >>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >>>>
> >>>> Signed-off-by: Yonglong Li <[e-mail]>
> >>>> ---
> >>>>  include/net/mptcp.h  |  3 ++-
> >>>>  net/mptcp/options.c  | 65 +++++++++++++++++++++++++++++++---------------------
> >>>>  net/mptcp/pm.c       | 33 +++++++++++---------------
> >>>>  net/mptcp/protocol.h | 23 ++++++++++++-------
> >>>>  4 files changed, 69 insertions(+), 55 deletions(-)
> >>>>
> >>>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> >>>> index d61bbbf..d2c6ebe 100644
> >>>> --- a/include/net/mptcp.h
> >>>> +++ b/include/net/mptcp.h
> >>>> @@ -61,7 +61,8 @@ struct mptcp_out_options {
> >>>>         u64 sndr_key;
> >>>>         u64 rcvr_key;
> >>>>         u64 ahmac;
> >>>> -       struct mptcp_addr_info addr;
> >>>> +       struct mptcp_addr_info local;
> >>>> +       struct mptcp_addr_info remote;
> >>>>         struct mptcp_rm_list rm_list;
> >>>>         u8 join_id;
> >>>>         u8 backup;
> >>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>>> index 1aec016..1707bec 100644
> >>>> --- a/net/mptcp/options.c
> >>>> +++ b/net/mptcp/options.c
> >>>> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >>>>         bool drop_other_suboptions = false;
> >>>>         unsigned int opt_size = *size;
> >>>> -       bool echo;
> >>>> -       bool port;
> >>>> -       int len;
> >>>> +       u8 add_addr, flags = 0xff;
> >>>> +       int len = 0;
> >>>>
> >>>> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> >>>> -            mptcp_pm_should_add_signal_port(msk) ||
> >>>> -            mptcp_pm_should_add_signal_echo(msk)) &&
> >>>> +       if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
> >>>> +               return false;
> >>>
> >>> This add_addr argument is useless, let's drop it.
> >>>
> >> we can use add_addr use in debug log.
> >
> > I think it's not worth adding a new argument just for debugging.
> agree.
>
> >
> >>
> >>> And here add back mptcp_pm_should_add_signal check here. The original code
> >>> called mptcp_pm_should_add_signal twice for double check, once out of pm
> >>> lock, once under pm lock. We should keep it.
> >> Sorry, I think double check is not necessary. does we need double check?
> >
> > I think we should keep the original logic here. If we want to drop this
> > double check or something, we should do it in another patch, don't mix too
> > much things in one patch.
> agree.
>
> >
> >>
> >>>
> >>>> +
> >>>> +       if ((mptcp_pm_should_add_signal_echo(msk) ||
> >>>> +            (mptcp_pm_should_add_signal_addr(msk) &&
> >>>> +             (opts->local.family == AF_INET6 || opts->local.port))) &&
> >>>>             skb && skb_is_tcp_pure_ack(skb)) {
> >>>>                 pr_debug("drop other suboptions");
> >>>>                 opts->suboptions = 0;
> >>>> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>                 drop_other_suboptions = true;
> >>>>         }
> >>>>
> >>>> -       if (!mptcp_pm_should_add_signal(msk) ||
> >>>> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> >>>> -               return false;
> >>>>
> >>>> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
> >>>> +       if (mptcp_pm_should_add_signal_echo(msk)) {
> >>>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>> +       } else {
> >>>> +               opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >>>> +                                                    msk->remote_key,
> >>>> +                                                    &opts->local);
> >>>
> >>> Keep this ahmac generating code after opts->suboptions set just like the
> >>> original code, since ahmac is the more expensive to populate. If remaining
> >>> length isn't enough, no need to set ahmac.
> >>
> >> because mptcp_add_addr_len(opts) will use ahmac to calculate len of opts, so I think Keep this ahmac
> >> generating code after opts->suboptions set is not ok.
> >
> > So we should use mptcp_pm_should_add_signal_addr instead of opts->ahmac in
> > mptcp_add_addr_len.
> agree.
>
> >
> >>
> >>>
> >>>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> >>>> +       }
> >>>> +
> >>>> +       len = mptcp_add_addr_len(opts);
> >>>>         if (remaining < len)
> >>>>                 return false;
> >>>>
> >>>> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>         if (drop_other_suboptions)
> >>>>                 *size -= opt_size;
> >>>>         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >>>> -       if (!echo) {
> >>>> -               opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >>>> -                                                    msk->remote_key,
> >>>> -                                                    &opts->addr);
> >>>> -       }
> >>>> -       pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> >>>> -                opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> >>>> +
> >>>> +       spin_lock_bh(&msk->pm.lock);
> >>>> +       WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> >>>> +       spin_unlock_bh(&msk->pm.lock);
> >>>
> >>> addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
> >>> set it again. I thinks this trunk and all the flags set above should be
> >>> dropped.
> >>
> >> Because MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO maybe set at the same time.
> >> So i think we should only unset one flag.
> >
> > We can only unset one flag in mptcp_pm_add_addr_signal, see my comment in
> > patch 1.
>
> if change like this. there is a issue: if remaining len checking is not ok and return false, The ADD_ADDR/ECHO event will
> be clear. So I think we should make sure ADD_ADDR/ECHO option will add in packet before clean flags. WDYT?
>

You're right, let's clear it in mptcp_established_options_add_addr.
Furthermore, we should do the same thing for RM_ADDR, clear rm_addr in
mptcp_established_options_rm_addr too.

If so, patch 1 will become useless. Let's drop it.

-Geliang



> >
> > -Geliang
> >
> >>
> >>>
> >>>> +
> >>>> +       pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> >>>> +                add_addr, (opts->ahmac == 0), opts->local.id,
> >>>> +                opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
> >>>>
> >>>>         return true;
> >>>>  }
> >>>
> >>> The whole function is something like this:
> >>> '''
> >>>         struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> >>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >>>         bool drop_other_suboptions = false;
> >>>         unsigned int opt_size = *size;
> >>>         int len;
> >>>
> >>>         if (!mptcp_pm_should_add_signal(msk) ||
> >>>             !mptcp_pm_add_addr_signal(msk, remaining, opts))
> >>>                 return false;
> >>>
> >>>         if ((mptcp_pm_should_add_signal_echo(msk) ||
> >>>              (mptcp_pm_should_add_signal_addr(msk) &&
> >>>               (opts->local.family == AF_INET6 || opts->local.port))) &&
> >>>             skb && skb_is_tcp_pure_ack(skb)) {
> >>>                 pr_debug("drop other suboptions");
> >>>                 opts->suboptions = 0;
> >>>                 opts->ext_copy.use_ack = 0;
> >>>                 opts->ext_copy.use_map = 0;
> >>>                 remaining += opt_size;
> >>>                 drop_other_suboptions = true;
> >>>         }
> >>>
> >>>         len = mptcp_add_addr_len(opts);
> >>>         if (remaining < len)
> >>>                 return false;
> >>>
> >>>         *size = len;
> >>>         if (drop_other_suboptions)
> >>>                 *size -= opt_size;
> >>>         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >>>         if (mptcp_pm_should_add_signal_addr(msk)) {
> >>>                 opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >>>                                                      msk->remote_key,
> >>>                                                      &opts->local);
> >>>         }
> >>>
> >>>         pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
> >>> ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> >>>                  msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
> >>>                  opts->ahmac, ntohs(opts->local.port),
> >>> opts->remote.id, ntohs(opts->remote.port));
> >>>
> >>>         return true;
> >>> '''
> >>>
> >>>> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>>>
> >>>>  mp_capable_done:
> >>>>         if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> >>>> +               struct mptcp_addr_info *addr = &opts->remote;
> >>>
> >>> We can simplify it like this:
> >>>          struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> >>> &opts->remote;
> >>>
> >>>>                 u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>>                 u8 echo = MPTCP_ADDR_ECHO;
> >>>>
> >>>> +               if (opts->ahmac)
> >>>> +                       addr = &opts->local;
> >>>
> >>> And this trunk can be dropped.
> >>>
> >>>> +
> >>>>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >>>> -               if (opts->addr.family == AF_INET6)
> >>>> +               if (addr->family == AF_INET6)
> >>>>                         len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>>  #endif
> >>>>
> >>>> -               if (opts->addr.port)
> >>>> +               if (addr->port)
> >>>>                         len += TCPOLEN_MPTCP_PORT_LEN;
> >>>>
> >>>>                 if (opts->ahmac) {
> >>>> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>>>                 }
> >>>>
> >>>>                 *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> >>>> -                                     len, echo, opts->addr.id);
> >>>> -               if (opts->addr.family == AF_INET) {
> >>>> -                       memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> >>>> +                                     len, echo, addr->id);
> >>>> +               if (addr->family == AF_INET) {
> >>>> +                       memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
> >>>>                         ptr += 1;
> >>>>                 }
> >>>>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >>>> -               else if (opts->addr.family == AF_INET6) {
> >>>> -                       memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
> >>>> +               else if (addr->family == AF_INET6) {
> >>>> +                       memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
> >>>>                         ptr += 4;
> >>>>                 }
> >>>>  #endif
> >>>>
> >>>> -               if (!opts->addr.port) {
> >>>> +               if (!addr->port) {
> >>>>                         if (opts->ahmac) {
> >>>>                                 put_unaligned_be64(opts->ahmac, ptr);
> >>>>                                 ptr += 2;
> >>>>                         }
> >>>>                 } else {
> >>>> -                       u16 port = ntohs(opts->addr.port);
> >>>> +                       u16 port = ntohs(addr->port);
> >>>>
> >>>>                         if (opts->ahmac) {
> >>>>                                 u8 *bptr = (u8 *)ptr;
> >>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> >>>> index cf873e9..9c621293 100644
> >>>> --- a/net/mptcp/pm.c
> >>>> +++ b/net/mptcp/pm.c
> >>>> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >>>>
> >>>>  /* path manager helpers */
> >>>>
> >>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port)
> >>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> >>>> +                             u8 *add_addr)
> >>>
> >>> Drop this add_addr argument.
> >>>
> >>>>  {
> >>>> -       u8 add_addr;
> >>>> -       int ret = false;
> >>>> -
> >>>>         spin_lock_bh(&msk->pm.lock);
> >>>>
> >>>> -       /* double check after the lock is acquired */
> >>>> -       if (!mptcp_pm_should_add_signal(msk))
> >>>> -               goto out_unlock;
> >>>
> >>> Keep this double check code.
> >>>
> >>>> -
> >>>> -       *echo = mptcp_pm_should_add_signal_echo(msk);
> >>>> -       *port = mptcp_pm_should_add_signal_port(msk);
> >>>> -
> >>>> -       if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> >>>> -               goto out_unlock;
> >>>
> >>> Keep this length double check code too.
> >>>
> >>>> +       if (!mptcp_pm_should_add_signal(msk)) {
> >>>> +               spin_unlock_bh(&msk->pm.lock);
> >>>> +               return false;
> >>>> +       }
> >>>>
> >>>> -       *saddr = msk->pm.local;
> >>>> -       add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> >>> -       WRITE_ONCE(msk->pm.addr_signal, add_addr);
> >>>
> >>> This code is just added in patch 1, I think we should keep it. And no need
> >>> to write addr_signal again in mptcp_established_options_add_addr.
> >>>
> >>>> -       ret = true;
> >>>> +       opts->local = msk->pm.local;
> >>>> +       opts->remote = msk->pm.remote;
> >>>> +       *add_addr = msk->pm.addr_signal;
> >>>>
> >>>> -out_unlock:
> >>>>         spin_unlock_bh(&msk->pm.lock);
> >>>> -       return ret;
> >>>
> >>> Keep this out_unlock code.
> >>>
> >>>> +
> >>>> +       if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> >>>> +               mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> >>>
> >>> Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
> >>>
> >>> I'm no sure why we need this two lines, and why you use '&&' here. Do you
> >>> mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?
> >>>
> >>>> +       return true;
> >>>>  }
> >>>
> >>> The whole function is something like this:
> >>> '''
> >>>         int ret = false;
> >>>         u8 add_addr;
> >>>
> >>>         spin_lock_bh(&msk->pm.lock);
> >>>
> >>>         /* double check after the lock is acquired */
> >>>         if (!mptcp_pm_should_add_signal(msk))
> >>>                 goto out_unlock;
> >>>
> >>>         if (remaining < mptcp_add_addr_len(opts))
> >>>                 goto out_unlock;
> >>>
> >>>         opts->local = msk->pm.local;
> >>>         opts->remote = msk->pm.remote;
> >>>         if (mptcp_pm_should_add_signal_echo(msk))
> >>>                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>         else
> >>>                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
> >>>         WRITE_ONCE(msk->pm.addr_signal, add_addr);
> >>>         ret = true;
> >>>
> >>> out_unlock:
> >>>         spin_unlock_bh(&msk->pm.lock);
> >>>         if (ret && mptcp_pm_should_add_signal_echo(msk) &&
> >>> mptcp_pm_should_add_signal_addr(msk))
> >>>                 mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> >>>         return ret;
> >>> '''
> >>>
> >>>>
> >>>>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> >>>> index a0b0ec0..0bfbbdef 100644
> >>>> --- a/net/mptcp/protocol.h
> >>>> +++ b/net/mptcp/protocol.h
> >>>> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
> >>>>         return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
> >>>>  }
> >>>>
> >>>> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> >>>> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
> >>>>  {
> >>>> -       u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>> +       u8 len = 0;
> >>>> +       struct mptcp_addr_info *addr = &opts->remote;
> >>>
> >>> We can simplify it like this:
> >>>          struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> >>> &opts->remote;
> >>>
> >>> And keep the orignal code unchanged.
> >>>
> >>>>
> >>>> -       if (family == AF_INET6)
> >>>> -               len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>> -       if (!echo)
> >>>> +       if (opts->ahmac) {
> >>>> +               addr = &opts->local;
> >>>>                 len += MPTCPOPT_THMAC_LEN;
> >>>> +       }
> >>>> +
> >>>> +       if (addr->family == AF_INET6)
> >>>> +               len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>> +       else
> >>>> +               len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>> +
> >>>>         /* account for 2 trailing 'nop' options */
> >>>> -       if (port)
> >>>> +       if (addr->port)
> >>>>                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >>>>
> >>>>         return len;
> >>>
> >>> The whole function is something like this:
> >>> '''
> >>>         struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> >>> &opts->remote;
> >>>         u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>
> >>>         if (addr->family == AF_INET6)
> >>>                 len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>         if (opts->ahmac)
> >>>                 len += MPTCPOPT_THMAC_LEN;
> >>>         /* account for 2 trailing 'nop' options */
> >>>         if (addr->port)
> >>>                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >>>
> >>>         return len;
> >>> '''
> >>>
> >>> Thanks.
> >>> -Geliang
> >>>
> >>>> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
> >>>>         return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
> >>>>  }
> >>>>
> >>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port);
> >>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> >>>> +                             u8 *add_addr);
> >>>>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>                              struct mptcp_rm_list *rm_list);
> >>>>  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> >>>> --
> >>>> 1.8.3.1
> >>>>
> >>>>
> >>>
> >>>
> >>
> >> --
> >> Li YongLong
> >
>
> --
> Li YongLong
>

YonglongLi June 30, 2021, 1:30 a.m. UTC | #7

On 2021/6/29 16:25, Geliang Tang wrote:
> Yonglong Li <[e-mail]> 于2021年6月29日周二 下午3:54写道：
>>
>>
>>
>> On 2021/6/29 15:35, Geliang Tang wrote:
>>> Yonglong Li <[e-mail]> 于2021年6月29日周二 下午3:02写道：
>>>>
>>>>
>>>> Hi Geiliang, Thanks for your reviews.
>>>>
>>>> On 2021/6/29 13:58, Geliang Tang wrote:
>>>>> Yonglong Li <[e-mail]> 于2021年6月29日周二 上午9:42写道：
>>>>>>
>>>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>>>>>> ADD_ADDR/echo-ADD_ADDR option
>>>>>>
>>>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>>>>>
>>>>>> Signed-off-by: Yonglong Li <[e-mail]>
>>>>>> ---
>>>>>>  include/net/mptcp.h  |  3 ++-
>>>>>>  net/mptcp/options.c  | 65 +++++++++++++++++++++++++++++++---------------------
>>>>>>  net/mptcp/pm.c       | 33 +++++++++++---------------
>>>>>>  net/mptcp/protocol.h | 23 ++++++++++++-------
>>>>>>  4 files changed, 69 insertions(+), 55 deletions(-)
>>>>>>
>>>>>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
>>>>>> index d61bbbf..d2c6ebe 100644
>>>>>> --- a/include/net/mptcp.h
>>>>>> +++ b/include/net/mptcp.h
>>>>>> @@ -61,7 +61,8 @@ struct mptcp_out_options {
>>>>>>         u64 sndr_key;
>>>>>>         u64 rcvr_key;
>>>>>>         u64 ahmac;
>>>>>> -       struct mptcp_addr_info addr;
>>>>>> +       struct mptcp_addr_info local;
>>>>>> +       struct mptcp_addr_info remote;
>>>>>>         struct mptcp_rm_list rm_list;
>>>>>>         u8 join_id;
>>>>>>         u8 backup;
>>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>>>> index 1aec016..1707bec 100644
>>>>>> --- a/net/mptcp/options.c
>>>>>> +++ b/net/mptcp/options.c
>>>>>> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>>>>         bool drop_other_suboptions = false;
>>>>>>         unsigned int opt_size = *size;
>>>>>> -       bool echo;
>>>>>> -       bool port;
>>>>>> -       int len;
>>>>>> +       u8 add_addr, flags = 0xff;
>>>>>> +       int len = 0;
>>>>>>
>>>>>> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>>>>>> -            mptcp_pm_should_add_signal_port(msk) ||
>>>>>> -            mptcp_pm_should_add_signal_echo(msk)) &&
>>>>>> +       if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
>>>>>> +               return false;
>>>>>
>>>>> This add_addr argument is useless, let's drop it.
>>>>>
>>>> we can use add_addr use in debug log.
>>>
>>> I think it's not worth adding a new argument just for debugging.
>> agree.
>>
>>>
>>>>
>>>>> And here add back mptcp_pm_should_add_signal check here. The original code
>>>>> called mptcp_pm_should_add_signal twice for double check, once out of pm
>>>>> lock, once under pm lock. We should keep it.
>>>> Sorry, I think double check is not necessary. does we need double check?
>>>
>>> I think we should keep the original logic here. If we want to drop this
>>> double check or something, we should do it in another patch, don't mix too
>>> much things in one patch.
>> agree.
>>
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> +       if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>>>> +            (mptcp_pm_should_add_signal_addr(msk) &&
>>>>>> +             (opts->local.family == AF_INET6 || opts->local.port))) &&
>>>>>>             skb && skb_is_tcp_pure_ack(skb)) {
>>>>>>                 pr_debug("drop other suboptions");
>>>>>>                 opts->suboptions = 0;
>>>>>> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>>                 drop_other_suboptions = true;
>>>>>>         }
>>>>>>
>>>>>> -       if (!mptcp_pm_should_add_signal(msk) ||
>>>>>> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>>>>>> -               return false;
>>>>>>
>>>>>> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
>>>>>> +       if (mptcp_pm_should_add_signal_echo(msk)) {
>>>>>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>>> +       } else {
>>>>>> +               opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>>>> +                                                    msk->remote_key,
>>>>>> +                                                    &opts->local);
>>>>>
>>>>> Keep this ahmac generating code after opts->suboptions set just like the
>>>>> original code, since ahmac is the more expensive to populate. If remaining
>>>>> length isn't enough, no need to set ahmac.
>>>>
>>>> because mptcp_add_addr_len(opts) will use ahmac to calculate len of opts, so I think Keep this ahmac
>>>> generating code after opts->suboptions set is not ok.
>>>
>>> So we should use mptcp_pm_should_add_signal_addr instead of opts->ahmac in
>>> mptcp_add_addr_len.
>> agree.
>>
>>>
>>>>
>>>>>
>>>>>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>>>>>> +       }
>>>>>> +
>>>>>> +       len = mptcp_add_addr_len(opts);
>>>>>>         if (remaining < len)
>>>>>>                 return false;
>>>>>>
>>>>>> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>>         if (drop_other_suboptions)
>>>>>>                 *size -= opt_size;
>>>>>>         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>>>> -       if (!echo) {
>>>>>> -               opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>>>> -                                                    msk->remote_key,
>>>>>> -                                                    &opts->addr);
>>>>>> -       }
>>>>>> -       pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
>>>>>> -                opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
>>>>>> +
>>>>>> +       spin_lock_bh(&msk->pm.lock);
>>>>>> +       WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
>>>>>> +       spin_unlock_bh(&msk->pm.lock);
>>>>>
>>>>> addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
>>>>> set it again. I thinks this trunk and all the flags set above should be
>>>>> dropped.
>>>>
>>>> Because MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO maybe set at the same time.
>>>> So i think we should only unset one flag.
>>>
>>> We can only unset one flag in mptcp_pm_add_addr_signal, see my comment in
>>> patch 1.
>>
>> if change like this. there is a issue: if remaining len checking is not ok and return false, The ADD_ADDR/ECHO event will
>> be clear. So I think we should make sure ADD_ADDR/ECHO option will add in packet before clean flags. WDYT?
>>
> 
> You're right, let's clear it in mptcp_established_options_add_addr.
> Furthermore, we should do the same thing for RM_ADDR, clear rm_addr in
> mptcp_established_options_rm_addr too.
> 
> If so, patch 1 will become useless. Let's drop it.
> 
> -Geliang
> I think RM_ADDR doesn't have this issue. Because mptcp_pm_rm_addr_signal() check the failed case.

> 
> 
>>>
>>> -Geliang
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> +       pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
>>>>>> +                add_addr, (opts->ahmac == 0), opts->local.id,
>>>>>> +                opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>>>>>>
>>>>>>         return true;
>>>>>>  }
>>>>>
>>>>> The whole function is something like this:
>>>>> '''
>>>>>         struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>>>>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>>>         bool drop_other_suboptions = false;
>>>>>         unsigned int opt_size = *size;
>>>>>         int len;
>>>>>
>>>>>         if (!mptcp_pm_should_add_signal(msk) ||
>>>>>             !mptcp_pm_add_addr_signal(msk, remaining, opts))
>>>>>                 return false;
>>>>>
>>>>>         if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>>>              (mptcp_pm_should_add_signal_addr(msk) &&
>>>>>               (opts->local.family == AF_INET6 || opts->local.port))) &&
>>>>>             skb && skb_is_tcp_pure_ack(skb)) {
>>>>>                 pr_debug("drop other suboptions");
>>>>>                 opts->suboptions = 0;
>>>>>                 opts->ext_copy.use_ack = 0;
>>>>>                 opts->ext_copy.use_map = 0;
>>>>>                 remaining += opt_size;
>>>>>                 drop_other_suboptions = true;
>>>>>         }
>>>>>
>>>>>         len = mptcp_add_addr_len(opts);
>>>>>         if (remaining < len)
>>>>>                 return false;
>>>>>
>>>>>         *size = len;
>>>>>         if (drop_other_suboptions)
>>>>>                 *size -= opt_size;
>>>>>         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>>>         if (mptcp_pm_should_add_signal_addr(msk)) {
>>>>>                 opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>>>                                                      msk->remote_key,
>>>>>                                                      &opts->local);
>>>>>         }
>>>>>
>>>>>         pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
>>>>> ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
>>>>>                  msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
>>>>>                  opts->ahmac, ntohs(opts->local.port),
>>>>> opts->remote.id, ntohs(opts->remote.port));
>>>>>
>>>>>         return true;
>>>>> '''
>>>>>
>>>>>> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>>>>>
>>>>>>  mp_capable_done:
>>>>>>         if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
>>>>>> +               struct mptcp_addr_info *addr = &opts->remote;
>>>>>
>>>>> We can simplify it like this:
>>>>>          struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>>>> &opts->remote;
>>>>>
>>>>>>                 u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>>                 u8 echo = MPTCP_ADDR_ECHO;
>>>>>>
>>>>>> +               if (opts->ahmac)
>>>>>> +                       addr = &opts->local;
>>>>>
>>>>> And this trunk can be dropped.
>>>>>
>>>>>> +
>>>>>>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>>>>> -               if (opts->addr.family == AF_INET6)
>>>>>> +               if (addr->family == AF_INET6)
>>>>>>                         len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>>  #endif
>>>>>>
>>>>>> -               if (opts->addr.port)
>>>>>> +               if (addr->port)
>>>>>>                         len += TCPOLEN_MPTCP_PORT_LEN;
>>>>>>
>>>>>>                 if (opts->ahmac) {
>>>>>> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>>>>>                 }
>>>>>>
>>>>>>                 *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
>>>>>> -                                     len, echo, opts->addr.id);
>>>>>> -               if (opts->addr.family == AF_INET) {
>>>>>> -                       memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
>>>>>> +                                     len, echo, addr->id);
>>>>>> +               if (addr->family == AF_INET) {
>>>>>> +                       memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
>>>>>>                         ptr += 1;
>>>>>>                 }
>>>>>>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>>>>> -               else if (opts->addr.family == AF_INET6) {
>>>>>> -                       memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
>>>>>> +               else if (addr->family == AF_INET6) {
>>>>>> +                       memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
>>>>>>                         ptr += 4;
>>>>>>                 }
>>>>>>  #endif
>>>>>>
>>>>>> -               if (!opts->addr.port) {
>>>>>> +               if (!addr->port) {
>>>>>>                         if (opts->ahmac) {
>>>>>>                                 put_unaligned_be64(opts->ahmac, ptr);
>>>>>>                                 ptr += 2;
>>>>>>                         }
>>>>>>                 } else {
>>>>>> -                       u16 port = ntohs(opts->addr.port);
>>>>>> +                       u16 port = ntohs(addr->port);
>>>>>>
>>>>>>                         if (opts->ahmac) {
>>>>>>                                 u8 *bptr = (u8 *)ptr;
>>>>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>>>>> index cf873e9..9c621293 100644
>>>>>> --- a/net/mptcp/pm.c
>>>>>> +++ b/net/mptcp/pm.c
>>>>>> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>>>>>
>>>>>>  /* path manager helpers */
>>>>>>
>>>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port)
>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
>>>>>> +                             u8 *add_addr)
>>>>>
>>>>> Drop this add_addr argument.
>>>>>
>>>>>>  {
>>>>>> -       u8 add_addr;
>>>>>> -       int ret = false;
>>>>>> -
>>>>>>         spin_lock_bh(&msk->pm.lock);
>>>>>>
>>>>>> -       /* double check after the lock is acquired */
>>>>>> -       if (!mptcp_pm_should_add_signal(msk))
>>>>>> -               goto out_unlock;
>>>>>
>>>>> Keep this double check code.
>>>>>
>>>>>> -
>>>>>> -       *echo = mptcp_pm_should_add_signal_echo(msk);
>>>>>> -       *port = mptcp_pm_should_add_signal_port(msk);
>>>>>> -
>>>>>> -       if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
>>>>>> -               goto out_unlock;
>>>>>
>>>>> Keep this length double check code too.
>>>>>
>>>>>> +       if (!mptcp_pm_should_add_signal(msk)) {
>>>>>> +               spin_unlock_bh(&msk->pm.lock);
>>>>>> +               return false;
>>>>>> +       }
>>>>>>
>>>>>> -       *saddr = msk->pm.local;
>>>>>> -       add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
>>>>> -       WRITE_ONCE(msk->pm.addr_signal, add_addr);
>>>>>
>>>>> This code is just added in patch 1, I think we should keep it. And no need
>>>>> to write addr_signal again in mptcp_established_options_add_addr.
>>>>>
>>>>>> -       ret = true;
>>>>>> +       opts->local = msk->pm.local;
>>>>>> +       opts->remote = msk->pm.remote;
>>>>>> +       *add_addr = msk->pm.addr_signal;
>>>>>>
>>>>>> -out_unlock:
>>>>>>         spin_unlock_bh(&msk->pm.lock);
>>>>>> -       return ret;
>>>>>
>>>>> Keep this out_unlock code.
>>>>>
>>>>>> +
>>>>>> +       if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
>>>>>> +               mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>>>>>
>>>>> Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
>>>>>
>>>>> I'm no sure why we need this two lines, and why you use '&&' here. Do you
>>>>> mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?
>>>>>
>>>>>> +       return true;
>>>>>>  }
>>>>>
>>>>> The whole function is something like this:
>>>>> '''
>>>>>         int ret = false;
>>>>>         u8 add_addr;
>>>>>
>>>>>         spin_lock_bh(&msk->pm.lock);
>>>>>
>>>>>         /* double check after the lock is acquired */
>>>>>         if (!mptcp_pm_should_add_signal(msk))
>>>>>                 goto out_unlock;
>>>>>
>>>>>         if (remaining < mptcp_add_addr_len(opts))
>>>>>                 goto out_unlock;
>>>>>
>>>>>         opts->local = msk->pm.local;
>>>>>         opts->remote = msk->pm.remote;
>>>>>         if (mptcp_pm_should_add_signal_echo(msk))
>>>>>                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>>         else
>>>>>                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
>>>>>         WRITE_ONCE(msk->pm.addr_signal, add_addr);
>>>>>         ret = true;
>>>>>
>>>>> out_unlock:
>>>>>         spin_unlock_bh(&msk->pm.lock);
>>>>>         if (ret && mptcp_pm_should_add_signal_echo(msk) &&
>>>>> mptcp_pm_should_add_signal_addr(msk))
>>>>>                 mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>>>>>         return ret;
>>>>> '''
>>>>>
>>>>>>
>>>>>>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>>>>> index a0b0ec0..0bfbbdef 100644
>>>>>> --- a/net/mptcp/protocol.h
>>>>>> +++ b/net/mptcp/protocol.h
>>>>>> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
>>>>>>         return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
>>>>>>  }
>>>>>>
>>>>>> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
>>>>>> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
>>>>>>  {
>>>>>> -       u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>> +       u8 len = 0;
>>>>>> +       struct mptcp_addr_info *addr = &opts->remote;
>>>>>
>>>>> We can simplify it like this:
>>>>>          struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>>>> &opts->remote;
>>>>>
>>>>> And keep the orignal code unchanged.
>>>>>
>>>>>>
>>>>>> -       if (family == AF_INET6)
>>>>>> -               len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>> -       if (!echo)
>>>>>> +       if (opts->ahmac) {
>>>>>> +               addr = &opts->local;
>>>>>>                 len += MPTCPOPT_THMAC_LEN;
>>>>>> +       }
>>>>>> +
>>>>>> +       if (addr->family == AF_INET6)
>>>>>> +               len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>> +       else
>>>>>> +               len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>> +
>>>>>>         /* account for 2 trailing 'nop' options */
>>>>>> -       if (port)
>>>>>> +       if (addr->port)
>>>>>>                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>>>>>
>>>>>>         return len;
>>>>>
>>>>> The whole function is something like this:
>>>>> '''
>>>>>         struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>>>> &opts->remote;
>>>>>         u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>
>>>>>         if (addr->family == AF_INET6)
>>>>>                 len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>         if (opts->ahmac)
>>>>>                 len += MPTCPOPT_THMAC_LEN;
>>>>>         /* account for 2 trailing 'nop' options */
>>>>>         if (addr->port)
>>>>>                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>>>>
>>>>>         return len;
>>>>> '''
>>>>>
>>>>> Thanks.
>>>>> -Geliang
>>>>>
>>>>>> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
>>>>>>         return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
>>>>>>  }
>>>>>>
>>>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port);
>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
>>>>>> +                             u8 *add_addr);
>>>>>>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>>                              struct mptcp_rm_list *rm_list);
>>>>>>  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
>>>>>> --
>>>>>> 1.8.3.1
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>> --
>>>> Li YongLong
>>>
>>
>> --
>> Li YongLong
>>
>

Geliang Tang June 30, 2021, 2:05 a.m. UTC | #8

Yonglong Li <[e-mail]> 于2021年6月30日周三 上午9:30写道：
>
>
>
> On 2021/6/29 16:25, Geliang Tang wrote:
> > Yonglong Li <[e-mail]> 于2021年6月29日周二 下午3:54写道：
> >>
> >>
> >>
> >> On 2021/6/29 15:35, Geliang Tang wrote:
> >>> Yonglong Li <[e-mail]> 于2021年6月29日周二 下午3:02写道：
> >>>>
> >>>>
> >>>> Hi Geiliang, Thanks for your reviews.
> >>>>
> >>>> On 2021/6/29 13:58, Geliang Tang wrote:
> >>>>> Yonglong Li <[e-mail]> 于2021年6月29日周二 上午9:42写道：
> >>>>>>
> >>>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> >>>>>> ADD_ADDR/echo-ADD_ADDR option
> >>>>>>
> >>>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >>>>>>
> >>>>>> Signed-off-by: Yonglong Li <[e-mail]>
> >>>>>> ---
> >>>>>>  include/net/mptcp.h  |  3 ++-
> >>>>>>  net/mptcp/options.c  | 65 +++++++++++++++++++++++++++++++---------------------
> >>>>>>  net/mptcp/pm.c       | 33 +++++++++++---------------
> >>>>>>  net/mptcp/protocol.h | 23 ++++++++++++-------
> >>>>>>  4 files changed, 69 insertions(+), 55 deletions(-)
> >>>>>>
> >>>>>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> >>>>>> index d61bbbf..d2c6ebe 100644
> >>>>>> --- a/include/net/mptcp.h
> >>>>>> +++ b/include/net/mptcp.h
> >>>>>> @@ -61,7 +61,8 @@ struct mptcp_out_options {
> >>>>>>         u64 sndr_key;
> >>>>>>         u64 rcvr_key;
> >>>>>>         u64 ahmac;
> >>>>>> -       struct mptcp_addr_info addr;
> >>>>>> +       struct mptcp_addr_info local;
> >>>>>> +       struct mptcp_addr_info remote;
> >>>>>>         struct mptcp_rm_list rm_list;
> >>>>>>         u8 join_id;
> >>>>>>         u8 backup;
> >>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>>>>> index 1aec016..1707bec 100644
> >>>>>> --- a/net/mptcp/options.c
> >>>>>> +++ b/net/mptcp/options.c
> >>>>>> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >>>>>>         bool drop_other_suboptions = false;
> >>>>>>         unsigned int opt_size = *size;
> >>>>>> -       bool echo;
> >>>>>> -       bool port;
> >>>>>> -       int len;
> >>>>>> +       u8 add_addr, flags = 0xff;
> >>>>>> +       int len = 0;
> >>>>>>
> >>>>>> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> >>>>>> -            mptcp_pm_should_add_signal_port(msk) ||
> >>>>>> -            mptcp_pm_should_add_signal_echo(msk)) &&
> >>>>>> +       if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
> >>>>>> +               return false;
> >>>>>
> >>>>> This add_addr argument is useless, let's drop it.
> >>>>>
> >>>> we can use add_addr use in debug log.
> >>>
> >>> I think it's not worth adding a new argument just for debugging.
> >> agree.
> >>
> >>>
> >>>>
> >>>>> And here add back mptcp_pm_should_add_signal check here. The original code
> >>>>> called mptcp_pm_should_add_signal twice for double check, once out of pm
> >>>>> lock, once under pm lock. We should keep it.
> >>>> Sorry, I think double check is not necessary. does we need double check?
> >>>
> >>> I think we should keep the original logic here. If we want to drop this
> >>> double check or something, we should do it in another patch, don't mix too
> >>> much things in one patch.
> >> agree.
> >>
> >>>
> >>>>
> >>>>>
> >>>>>> +
> >>>>>> +       if ((mptcp_pm_should_add_signal_echo(msk) ||
> >>>>>> +            (mptcp_pm_should_add_signal_addr(msk) &&
> >>>>>> +             (opts->local.family == AF_INET6 || opts->local.port))) &&
> >>>>>>             skb && skb_is_tcp_pure_ack(skb)) {
> >>>>>>                 pr_debug("drop other suboptions");
> >>>>>>                 opts->suboptions = 0;
> >>>>>> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>>>                 drop_other_suboptions = true;
> >>>>>>         }
> >>>>>>
> >>>>>> -       if (!mptcp_pm_should_add_signal(msk) ||
> >>>>>> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> >>>>>> -               return false;
> >>>>>>
> >>>>>> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
> >>>>>> +       if (mptcp_pm_should_add_signal_echo(msk)) {
> >>>>>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>>>> +       } else {
> >>>>>> +               opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >>>>>> +                                                    msk->remote_key,
> >>>>>> +                                                    &opts->local);
> >>>>>
> >>>>> Keep this ahmac generating code after opts->suboptions set just like the
> >>>>> original code, since ahmac is the more expensive to populate. If remaining
> >>>>> length isn't enough, no need to set ahmac.
> >>>>
> >>>> because mptcp_add_addr_len(opts) will use ahmac to calculate len of opts, so I think Keep this ahmac
> >>>> generating code after opts->suboptions set is not ok.
> >>>
> >>> So we should use mptcp_pm_should_add_signal_addr instead of opts->ahmac in
> >>> mptcp_add_addr_len.
> >> agree.
> >>
> >>>
> >>>>
> >>>>>
> >>>>>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> >>>>>> +       }
> >>>>>> +
> >>>>>> +       len = mptcp_add_addr_len(opts);
> >>>>>>         if (remaining < len)
> >>>>>>                 return false;
> >>>>>>
> >>>>>> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>>>         if (drop_other_suboptions)
> >>>>>>                 *size -= opt_size;
> >>>>>>         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >>>>>> -       if (!echo) {
> >>>>>> -               opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >>>>>> -                                                    msk->remote_key,
> >>>>>> -                                                    &opts->addr);
> >>>>>> -       }
> >>>>>> -       pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> >>>>>> -                opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> >>>>>> +
> >>>>>> +       spin_lock_bh(&msk->pm.lock);
> >>>>>> +       WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> >>>>>> +       spin_unlock_bh(&msk->pm.lock);
> >>>>>
> >>>>> addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
> >>>>> set it again. I thinks this trunk and all the flags set above should be
> >>>>> dropped.
> >>>>
> >>>> Because MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO maybe set at the same time.
> >>>> So i think we should only unset one flag.
> >>>
> >>> We can only unset one flag in mptcp_pm_add_addr_signal, see my comment in
> >>> patch 1.
> >>
> >> if change like this. there is a issue: if remaining len checking is not ok and return false, The ADD_ADDR/ECHO event will
> >> be clear. So I think we should make sure ADD_ADDR/ECHO option will add in packet before clean flags. WDYT?
> >>
> >
> > You're right, let's clear it in mptcp_established_options_add_addr.
> > Furthermore, we should do the same thing for RM_ADDR, clear rm_addr in
> > mptcp_established_options_rm_addr too.
> >
> > If so, patch 1 will become useless. Let's drop it.
> >
> > -Geliang
> > I think RM_ADDR doesn't have this issue. Because mptcp_pm_rm_addr_signal() check the failed case.

If so, how about doing the same thing as RM_ADDR to check the failed case
in mptcp_pm_add_addr_signal too.

I think we should use the same logic for ADD_ADDR and RM_ADDR.

>
> >
> >
> >>>
> >>> -Geliang
> >>>
> >>>>
> >>>>>
> >>>>>> +
> >>>>>> +       pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> >>>>>> +                add_addr, (opts->ahmac == 0), opts->local.id,
> >>>>>> +                opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
> >>>>>>
> >>>>>>         return true;
> >>>>>>  }
> >>>>>
> >>>>> The whole function is something like this:
> >>>>> '''
> >>>>>         struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> >>>>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >>>>>         bool drop_other_suboptions = false;
> >>>>>         unsigned int opt_size = *size;
> >>>>>         int len;
> >>>>>
> >>>>>         if (!mptcp_pm_should_add_signal(msk) ||
> >>>>>             !mptcp_pm_add_addr_signal(msk, remaining, opts))
> >>>>>                 return false;
> >>>>>
> >>>>>         if ((mptcp_pm_should_add_signal_echo(msk) ||
> >>>>>              (mptcp_pm_should_add_signal_addr(msk) &&
> >>>>>               (opts->local.family == AF_INET6 || opts->local.port))) &&
> >>>>>             skb && skb_is_tcp_pure_ack(skb)) {
> >>>>>                 pr_debug("drop other suboptions");
> >>>>>                 opts->suboptions = 0;
> >>>>>                 opts->ext_copy.use_ack = 0;
> >>>>>                 opts->ext_copy.use_map = 0;
> >>>>>                 remaining += opt_size;
> >>>>>                 drop_other_suboptions = true;
> >>>>>         }
> >>>>>
> >>>>>         len = mptcp_add_addr_len(opts);
> >>>>>         if (remaining < len)
> >>>>>                 return false;
> >>>>>
> >>>>>         *size = len;
> >>>>>         if (drop_other_suboptions)
> >>>>>                 *size -= opt_size;
> >>>>>         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >>>>>         if (mptcp_pm_should_add_signal_addr(msk)) {
> >>>>>                 opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >>>>>                                                      msk->remote_key,
> >>>>>                                                      &opts->local);
> >>>>>         }
> >>>>>
> >>>>>         pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
> >>>>> ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> >>>>>                  msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
> >>>>>                  opts->ahmac, ntohs(opts->local.port),
> >>>>> opts->remote.id, ntohs(opts->remote.port));
> >>>>>
> >>>>>         return true;
> >>>>> '''
> >>>>>
> >>>>>> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>>>>>
> >>>>>>  mp_capable_done:
> >>>>>>         if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> >>>>>> +               struct mptcp_addr_info *addr = &opts->remote;
> >>>>>
> >>>>> We can simplify it like this:
> >>>>>          struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> >>>>> &opts->remote;
> >>>>>
> >>>>>>                 u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>>>>                 u8 echo = MPTCP_ADDR_ECHO;
> >>>>>>
> >>>>>> +               if (opts->ahmac)
> >>>>>> +                       addr = &opts->local;
> >>>>>
> >>>>> And this trunk can be dropped.
> >>>>>
> >>>>>> +
> >>>>>>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >>>>>> -               if (opts->addr.family == AF_INET6)
> >>>>>> +               if (addr->family == AF_INET6)
> >>>>>>                         len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>>>>  #endif
> >>>>>>
> >>>>>> -               if (opts->addr.port)
> >>>>>> +               if (addr->port)
> >>>>>>                         len += TCPOLEN_MPTCP_PORT_LEN;
> >>>>>>
> >>>>>>                 if (opts->ahmac) {
> >>>>>> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>>>>>                 }
> >>>>>>
> >>>>>>                 *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> >>>>>> -                                     len, echo, opts->addr.id);
> >>>>>> -               if (opts->addr.family == AF_INET) {
> >>>>>> -                       memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> >>>>>> +                                     len, echo, addr->id);
> >>>>>> +               if (addr->family == AF_INET) {
> >>>>>> +                       memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
> >>>>>>                         ptr += 1;
> >>>>>>                 }
> >>>>>>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >>>>>> -               else if (opts->addr.family == AF_INET6) {
> >>>>>> -                       memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
> >>>>>> +               else if (addr->family == AF_INET6) {
> >>>>>> +                       memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
> >>>>>>                         ptr += 4;
> >>>>>>                 }
> >>>>>>  #endif
> >>>>>>
> >>>>>> -               if (!opts->addr.port) {
> >>>>>> +               if (!addr->port) {
> >>>>>>                         if (opts->ahmac) {
> >>>>>>                                 put_unaligned_be64(opts->ahmac, ptr);
> >>>>>>                                 ptr += 2;
> >>>>>>                         }
> >>>>>>                 } else {
> >>>>>> -                       u16 port = ntohs(opts->addr.port);
> >>>>>> +                       u16 port = ntohs(addr->port);
> >>>>>>
> >>>>>>                         if (opts->ahmac) {
> >>>>>>                                 u8 *bptr = (u8 *)ptr;
> >>>>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> >>>>>> index cf873e9..9c621293 100644
> >>>>>> --- a/net/mptcp/pm.c
> >>>>>> +++ b/net/mptcp/pm.c
> >>>>>> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >>>>>>
> >>>>>>  /* path manager helpers */
> >>>>>>
> >>>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>>> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port)
> >>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> >>>>>> +                             u8 *add_addr)
> >>>>>
> >>>>> Drop this add_addr argument.
> >>>>>
> >>>>>>  {
> >>>>>> -       u8 add_addr;
> >>>>>> -       int ret = false;
> >>>>>> -
> >>>>>>         spin_lock_bh(&msk->pm.lock);
> >>>>>>
> >>>>>> -       /* double check after the lock is acquired */
> >>>>>> -       if (!mptcp_pm_should_add_signal(msk))
> >>>>>> -               goto out_unlock;
> >>>>>
> >>>>> Keep this double check code.
> >>>>>
> >>>>>> -
> >>>>>> -       *echo = mptcp_pm_should_add_signal_echo(msk);
> >>>>>> -       *port = mptcp_pm_should_add_signal_port(msk);
> >>>>>> -
> >>>>>> -       if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> >>>>>> -               goto out_unlock;
> >>>>>
> >>>>> Keep this length double check code too.
> >>>>>
> >>>>>> +       if (!mptcp_pm_should_add_signal(msk)) {
> >>>>>> +               spin_unlock_bh(&msk->pm.lock);
> >>>>>> +               return false;
> >>>>>> +       }
> >>>>>>
> >>>>>> -       *saddr = msk->pm.local;
> >>>>>> -       add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> >>>>> -       WRITE_ONCE(msk->pm.addr_signal, add_addr);
> >>>>>
> >>>>> This code is just added in patch 1, I think we should keep it. And no need
> >>>>> to write addr_signal again in mptcp_established_options_add_addr.
> >>>>>
> >>>>>> -       ret = true;
> >>>>>> +       opts->local = msk->pm.local;
> >>>>>> +       opts->remote = msk->pm.remote;
> >>>>>> +       *add_addr = msk->pm.addr_signal;
> >>>>>>
> >>>>>> -out_unlock:
> >>>>>>         spin_unlock_bh(&msk->pm.lock);
> >>>>>> -       return ret;
> >>>>>
> >>>>> Keep this out_unlock code.
> >>>>>
> >>>>>> +
> >>>>>> +       if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> >>>>>> +               mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> >>>>>
> >>>>> Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
> >>>>>
> >>>>> I'm no sure why we need this two lines, and why you use '&&' here. Do you
> >>>>> mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?

Please move these two lines into a new patch, and describe why we need it
in the commit log.

Thanks.
-Geliang

> >>>>>
> >>>>>> +       return true;
> >>>>>>  }
> >>>>>
> >>>>> The whole function is something like this:
> >>>>> '''
> >>>>>         int ret = false;
> >>>>>         u8 add_addr;
> >>>>>
> >>>>>         spin_lock_bh(&msk->pm.lock);
> >>>>>
> >>>>>         /* double check after the lock is acquired */
> >>>>>         if (!mptcp_pm_should_add_signal(msk))
> >>>>>                 goto out_unlock;
> >>>>>
> >>>>>         if (remaining < mptcp_add_addr_len(opts))
> >>>>>                 goto out_unlock;
> >>>>>
> >>>>>         opts->local = msk->pm.local;
> >>>>>         opts->remote = msk->pm.remote;
> >>>>>         if (mptcp_pm_should_add_signal_echo(msk))
> >>>>>                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>>>         else
> >>>>>                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
> >>>>>         WRITE_ONCE(msk->pm.addr_signal, add_addr);
> >>>>>         ret = true;
> >>>>>
> >>>>> out_unlock:
> >>>>>         spin_unlock_bh(&msk->pm.lock);
> >>>>>         if (ret && mptcp_pm_should_add_signal_echo(msk) &&
> >>>>> mptcp_pm_should_add_signal_addr(msk))
> >>>>>                 mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> >>>>>         return ret;
> >>>>> '''
> >>>>>
> >>>>>>
> >>>>>>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> >>>>>> index a0b0ec0..0bfbbdef 100644
> >>>>>> --- a/net/mptcp/protocol.h
> >>>>>> +++ b/net/mptcp/protocol.h
> >>>>>> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
> >>>>>>         return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
> >>>>>>  }
> >>>>>>
> >>>>>> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> >>>>>> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
> >>>>>>  {
> >>>>>> -       u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>>>> +       u8 len = 0;
> >>>>>> +       struct mptcp_addr_info *addr = &opts->remote;
> >>>>>
> >>>>> We can simplify it like this:
> >>>>>          struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> >>>>> &opts->remote;
> >>>>>
> >>>>> And keep the orignal code unchanged.
> >>>>>
> >>>>>>
> >>>>>> -       if (family == AF_INET6)
> >>>>>> -               len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>>>> -       if (!echo)
> >>>>>> +       if (opts->ahmac) {
> >>>>>> +               addr = &opts->local;
> >>>>>>                 len += MPTCPOPT_THMAC_LEN;
> >>>>>> +       }
> >>>>>> +
> >>>>>> +       if (addr->family == AF_INET6)
> >>>>>> +               len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>>>> +       else
> >>>>>> +               len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>>>> +
> >>>>>>         /* account for 2 trailing 'nop' options */
> >>>>>> -       if (port)
> >>>>>> +       if (addr->port)
> >>>>>>                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >>>>>>
> >>>>>>         return len;
> >>>>>
> >>>>> The whole function is something like this:
> >>>>> '''
> >>>>>         struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> >>>>> &opts->remote;
> >>>>>         u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>>>
> >>>>>         if (addr->family == AF_INET6)
> >>>>>                 len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>>>         if (opts->ahmac)
> >>>>>                 len += MPTCPOPT_THMAC_LEN;
> >>>>>         /* account for 2 trailing 'nop' options */
> >>>>>         if (addr->port)
> >>>>>                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >>>>>
> >>>>>         return len;
> >>>>> '''
> >>>>>
> >>>>> Thanks.
> >>>>> -Geliang
> >>>>>
> >>>>>> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
> >>>>>>         return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
> >>>>>>  }
> >>>>>>
> >>>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>>> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port);
> >>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> >>>>>> +                             u8 *add_addr);
> >>>>>>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>>>                              struct mptcp_rm_list *rm_list);
> >>>>>>  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> >>>>>> --
> >>>>>> 1.8.3.1
> >>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>> --
> >>>> Li YongLong
> >>>
> >>
> >> --
> >> Li YongLong
> >>
> >
>
> --
> Li YongLong

YonglongLi June 30, 2021, 6:50 a.m. UTC | #9

On 2021/6/30 10:05, Geliang Tang wrote:
> Yonglong Li <[e-mail]> 于2021年6月30日周三 上午9:30写道：
>>
>>
>>
>> On 2021/6/29 16:25, Geliang Tang wrote:
>>> Yonglong Li <[e-mail]> 于2021年6月29日周二 下午3:54写道：
>>>>
>>>>
>>>>
>>>> On 2021/6/29 15:35, Geliang Tang wrote:
>>>>> Yonglong Li <[e-mail]> 于2021年6月29日周二 下午3:02写道：
>>>>>>
>>>>>>
>>>>>> Hi Geiliang, Thanks for your reviews.
>>>>>>
>>>>>> On 2021/6/29 13:58, Geliang Tang wrote:
>>>>>>> Yonglong Li <[e-mail]> 于2021年6月29日周二 上午9:42写道：
>>>>>>>>
>>>>>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>>>>>>>> ADD_ADDR/echo-ADD_ADDR option
>>>>>>>>
>>>>>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>>>>>>>
>>>>>>>> Signed-off-by: Yonglong Li <[e-mail]>
>>>>>>>> ---
>>>>>>>>  include/net/mptcp.h  |  3 ++-
>>>>>>>>  net/mptcp/options.c  | 65 +++++++++++++++++++++++++++++++---------------------
>>>>>>>>  net/mptcp/pm.c       | 33 +++++++++++---------------
>>>>>>>>  net/mptcp/protocol.h | 23 ++++++++++++-------
>>>>>>>>  4 files changed, 69 insertions(+), 55 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
>>>>>>>> index d61bbbf..d2c6ebe 100644
>>>>>>>> --- a/include/net/mptcp.h
>>>>>>>> +++ b/include/net/mptcp.h
>>>>>>>> @@ -61,7 +61,8 @@ struct mptcp_out_options {
>>>>>>>>         u64 sndr_key;
>>>>>>>>         u64 rcvr_key;
>>>>>>>>         u64 ahmac;
>>>>>>>> -       struct mptcp_addr_info addr;
>>>>>>>> +       struct mptcp_addr_info local;
>>>>>>>> +       struct mptcp_addr_info remote;
>>>>>>>>         struct mptcp_rm_list rm_list;
>>>>>>>>         u8 join_id;
>>>>>>>>         u8 backup;
>>>>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>>>>>> index 1aec016..1707bec 100644
>>>>>>>> --- a/net/mptcp/options.c
>>>>>>>> +++ b/net/mptcp/options.c
>>>>>>>> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>>>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>>>>>>         bool drop_other_suboptions = false;
>>>>>>>>         unsigned int opt_size = *size;
>>>>>>>> -       bool echo;
>>>>>>>> -       bool port;
>>>>>>>> -       int len;
>>>>>>>> +       u8 add_addr, flags = 0xff;
>>>>>>>> +       int len = 0;
>>>>>>>>
>>>>>>>> -       if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>>>>>>>> -            mptcp_pm_should_add_signal_port(msk) ||
>>>>>>>> -            mptcp_pm_should_add_signal_echo(msk)) &&
>>>>>>>> +       if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
>>>>>>>> +               return false;
>>>>>>>
>>>>>>> This add_addr argument is useless, let's drop it.
>>>>>>>
>>>>>> we can use add_addr use in debug log.
>>>>>
>>>>> I think it's not worth adding a new argument just for debugging.
>>>> agree.
>>>>
>>>>>
>>>>>>
>>>>>>> And here add back mptcp_pm_should_add_signal check here. The original code
>>>>>>> called mptcp_pm_should_add_signal twice for double check, once out of pm
>>>>>>> lock, once under pm lock. We should keep it.
>>>>>> Sorry, I think double check is not necessary. does we need double check?
>>>>>
>>>>> I think we should keep the original logic here. If we want to drop this
>>>>> double check or something, we should do it in another patch, don't mix too
>>>>> much things in one patch.
>>>> agree.
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +       if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>>>>>> +            (mptcp_pm_should_add_signal_addr(msk) &&
>>>>>>>> +             (opts->local.family == AF_INET6 || opts->local.port))) &&
>>>>>>>>             skb && skb_is_tcp_pure_ack(skb)) {
>>>>>>>>                 pr_debug("drop other suboptions");
>>>>>>>>                 opts->suboptions = 0;
>>>>>>>> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>>>>                 drop_other_suboptions = true;
>>>>>>>>         }
>>>>>>>>
>>>>>>>> -       if (!mptcp_pm_should_add_signal(msk) ||
>>>>>>>> -           !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>>>>>>>> -               return false;
>>>>>>>>
>>>>>>>> -       len = mptcp_add_addr_len(opts->addr.family, echo, port);
>>>>>>>> +       if (mptcp_pm_should_add_signal_echo(msk)) {
>>>>>>>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>>>>> +       } else {
>>>>>>>> +               opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>>>>>> +                                                    msk->remote_key,
>>>>>>>> +                                                    &opts->local);
>>>>>>>
>>>>>>> Keep this ahmac generating code after opts->suboptions set just like the
>>>>>>> original code, since ahmac is the more expensive to populate. If remaining
>>>>>>> length isn't enough, no need to set ahmac.
>>>>>>
>>>>>> because mptcp_add_addr_len(opts) will use ahmac to calculate len of opts, so I think Keep this ahmac
>>>>>> generating code after opts->suboptions set is not ok.
>>>>>
>>>>> So we should use mptcp_pm_should_add_signal_addr instead of opts->ahmac in
>>>>> mptcp_add_addr_len.
>>>> agree.
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> +               flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>>>>>>>> +       }
>>>>>>>> +
>>>>>>>> +       len = mptcp_add_addr_len(opts);
>>>>>>>>         if (remaining < len)
>>>>>>>>                 return false;
>>>>>>>>
>>>>>>>> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>>>>         if (drop_other_suboptions)
>>>>>>>>                 *size -= opt_size;
>>>>>>>>         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>>>>>> -       if (!echo) {
>>>>>>>> -               opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>>>>>> -                                                    msk->remote_key,
>>>>>>>> -                                                    &opts->addr);
>>>>>>>> -       }
>>>>>>>> -       pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
>>>>>>>> -                opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
>>>>>>>> +
>>>>>>>> +       spin_lock_bh(&msk->pm.lock);
>>>>>>>> +       WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
>>>>>>>> +       spin_unlock_bh(&msk->pm.lock);
>>>>>>>
>>>>>>> addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
>>>>>>> set it again. I thinks this trunk and all the flags set above should be
>>>>>>> dropped.
>>>>>>
>>>>>> Because MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO maybe set at the same time.
>>>>>> So i think we should only unset one flag.
>>>>>
>>>>> We can only unset one flag in mptcp_pm_add_addr_signal, see my comment in
>>>>> patch 1.
>>>>
>>>> if change like this. there is a issue: if remaining len checking is not ok and return false, The ADD_ADDR/ECHO event will
>>>> be clear. So I think we should make sure ADD_ADDR/ECHO option will add in packet before clean flags. WDYT?
>>>>
>>>
>>> You're right, let's clear it in mptcp_established_options_add_addr.
>>> Furthermore, we should do the same thing for RM_ADDR, clear rm_addr in
>>> mptcp_established_options_rm_addr too.
>>>
>>> If so, patch 1 will become useless. Let's drop it.
>>>
>>> -Geliang
>>> I think RM_ADDR doesn't have this issue. Because mptcp_pm_rm_addr_signal() check the failed case.
> 
> If so, how about doing the same thing as RM_ADDR to check the failed case
> in mptcp_pm_add_addr_signal too.
> 
> I think we should use the same logic for ADD_ADDR and RM_ADDR.

Agree. I will prepare next patch.

> 
>>
>>>
>>>
>>>>>
>>>>> -Geliang
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +       pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
>>>>>>>> +                add_addr, (opts->ahmac == 0), opts->local.id,
>>>>>>>> +                opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>>>>>>>>
>>>>>>>>         return true;
>>>>>>>>  }
>>>>>>>
>>>>>>> The whole function is something like this:
>>>>>>> '''
>>>>>>>         struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>>>>>>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>>>>>         bool drop_other_suboptions = false;
>>>>>>>         unsigned int opt_size = *size;
>>>>>>>         int len;
>>>>>>>
>>>>>>>         if (!mptcp_pm_should_add_signal(msk) ||
>>>>>>>             !mptcp_pm_add_addr_signal(msk, remaining, opts))
>>>>>>>                 return false;
>>>>>>>
>>>>>>>         if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>>>>>              (mptcp_pm_should_add_signal_addr(msk) &&
>>>>>>>               (opts->local.family == AF_INET6 || opts->local.port))) &&
>>>>>>>             skb && skb_is_tcp_pure_ack(skb)) {
>>>>>>>                 pr_debug("drop other suboptions");
>>>>>>>                 opts->suboptions = 0;
>>>>>>>                 opts->ext_copy.use_ack = 0;
>>>>>>>                 opts->ext_copy.use_map = 0;
>>>>>>>                 remaining += opt_size;
>>>>>>>                 drop_other_suboptions = true;
>>>>>>>         }
>>>>>>>
>>>>>>>         len = mptcp_add_addr_len(opts);
>>>>>>>         if (remaining < len)
>>>>>>>                 return false;
>>>>>>>
>>>>>>>         *size = len;
>>>>>>>         if (drop_other_suboptions)
>>>>>>>                 *size -= opt_size;
>>>>>>>         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>>>>>         if (mptcp_pm_should_add_signal_addr(msk)) {
>>>>>>>                 opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>>>>>                                                      msk->remote_key,
>>>>>>>                                                      &opts->local);
>>>>>>>         }
>>>>>>>
>>>>>>>         pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
>>>>>>> ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
>>>>>>>                  msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
>>>>>>>                  opts->ahmac, ntohs(opts->local.port),
>>>>>>> opts->remote.id, ntohs(opts->remote.port));
>>>>>>>
>>>>>>>         return true;
>>>>>>> '''
>>>>>>>
>>>>>>>> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>>>>>>>
>>>>>>>>  mp_capable_done:
>>>>>>>>         if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
>>>>>>>> +               struct mptcp_addr_info *addr = &opts->remote;
>>>>>>>
>>>>>>> We can simplify it like this:
>>>>>>>          struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>>>>>> &opts->remote;
>>>>>>>
>>>>>>>>                 u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>>>>                 u8 echo = MPTCP_ADDR_ECHO;
>>>>>>>>
>>>>>>>> +               if (opts->ahmac)
>>>>>>>> +                       addr = &opts->local;
>>>>>>>
>>>>>>> And this trunk can be dropped.
>>>>>>>
>>>>>>>> +
>>>>>>>>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>>>>>>> -               if (opts->addr.family == AF_INET6)
>>>>>>>> +               if (addr->family == AF_INET6)
>>>>>>>>                         len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>>>>  #endif
>>>>>>>>
>>>>>>>> -               if (opts->addr.port)
>>>>>>>> +               if (addr->port)
>>>>>>>>                         len += TCPOLEN_MPTCP_PORT_LEN;
>>>>>>>>
>>>>>>>>                 if (opts->ahmac) {
>>>>>>>> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>>>>>>>                 }
>>>>>>>>
>>>>>>>>                 *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
>>>>>>>> -                                     len, echo, opts->addr.id);
>>>>>>>> -               if (opts->addr.family == AF_INET) {
>>>>>>>> -                       memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
>>>>>>>> +                                     len, echo, addr->id);
>>>>>>>> +               if (addr->family == AF_INET) {
>>>>>>>> +                       memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
>>>>>>>>                         ptr += 1;
>>>>>>>>                 }
>>>>>>>>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>>>>>>> -               else if (opts->addr.family == AF_INET6) {
>>>>>>>> -                       memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
>>>>>>>> +               else if (addr->family == AF_INET6) {
>>>>>>>> +                       memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
>>>>>>>>                         ptr += 4;
>>>>>>>>                 }
>>>>>>>>  #endif
>>>>>>>>
>>>>>>>> -               if (!opts->addr.port) {
>>>>>>>> +               if (!addr->port) {
>>>>>>>>                         if (opts->ahmac) {
>>>>>>>>                                 put_unaligned_be64(opts->ahmac, ptr);
>>>>>>>>                                 ptr += 2;
>>>>>>>>                         }
>>>>>>>>                 } else {
>>>>>>>> -                       u16 port = ntohs(opts->addr.port);
>>>>>>>> +                       u16 port = ntohs(addr->port);
>>>>>>>>
>>>>>>>>                         if (opts->ahmac) {
>>>>>>>>                                 u8 *bptr = (u8 *)ptr;
>>>>>>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>>>>>>> index cf873e9..9c621293 100644
>>>>>>>> --- a/net/mptcp/pm.c
>>>>>>>> +++ b/net/mptcp/pm.c
>>>>>>>> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>>>>>>>
>>>>>>>>  /* path manager helpers */
>>>>>>>>
>>>>>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>>>> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port)
>>>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
>>>>>>>> +                             u8 *add_addr)
>>>>>>>
>>>>>>> Drop this add_addr argument.
>>>>>>>
>>>>>>>>  {
>>>>>>>> -       u8 add_addr;
>>>>>>>> -       int ret = false;
>>>>>>>> -
>>>>>>>>         spin_lock_bh(&msk->pm.lock);
>>>>>>>>
>>>>>>>> -       /* double check after the lock is acquired */
>>>>>>>> -       if (!mptcp_pm_should_add_signal(msk))
>>>>>>>> -               goto out_unlock;
>>>>>>>
>>>>>>> Keep this double check code.
>>>>>>>
>>>>>>>> -
>>>>>>>> -       *echo = mptcp_pm_should_add_signal_echo(msk);
>>>>>>>> -       *port = mptcp_pm_should_add_signal_port(msk);
>>>>>>>> -
>>>>>>>> -       if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
>>>>>>>> -               goto out_unlock;
>>>>>>>
>>>>>>> Keep this length double check code too.
>>>>>>>
>>>>>>>> +       if (!mptcp_pm_should_add_signal(msk)) {
>>>>>>>> +               spin_unlock_bh(&msk->pm.lock);
>>>>>>>> +               return false;
>>>>>>>> +       }
>>>>>>>>
>>>>>>>> -       *saddr = msk->pm.local;
>>>>>>>> -       add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
>>>>>>> -       WRITE_ONCE(msk->pm.addr_signal, add_addr);
>>>>>>>
>>>>>>> This code is just added in patch 1, I think we should keep it. And no need
>>>>>>> to write addr_signal again in mptcp_established_options_add_addr.
>>>>>>>
>>>>>>>> -       ret = true;
>>>>>>>> +       opts->local = msk->pm.local;
>>>>>>>> +       opts->remote = msk->pm.remote;
>>>>>>>> +       *add_addr = msk->pm.addr_signal;
>>>>>>>>
>>>>>>>> -out_unlock:
>>>>>>>>         spin_unlock_bh(&msk->pm.lock);
>>>>>>>> -       return ret;
>>>>>>>
>>>>>>> Keep this out_unlock code.
>>>>>>>
>>>>>>>> +
>>>>>>>> +       if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
>>>>>>>> +               mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>>>>>>>
>>>>>>> Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
>>>>>>>
>>>>>>> I'm no sure why we need this two lines, and why you use '&&' here. Do you
>>>>>>> mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?
> 
> Please move these two lines into a new patch, and describe why we need it
> in the commit log.
> 
> Thanks.
> -Geliang
> 
>>>>>>>
>>>>>>>> +       return true;
>>>>>>>>  }
>>>>>>>
>>>>>>> The whole function is something like this:
>>>>>>> '''
>>>>>>>         int ret = false;
>>>>>>>         u8 add_addr;
>>>>>>>
>>>>>>>         spin_lock_bh(&msk->pm.lock);
>>>>>>>
>>>>>>>         /* double check after the lock is acquired */
>>>>>>>         if (!mptcp_pm_should_add_signal(msk))
>>>>>>>                 goto out_unlock;
>>>>>>>
>>>>>>>         if (remaining < mptcp_add_addr_len(opts))
>>>>>>>                 goto out_unlock;
>>>>>>>
>>>>>>>         opts->local = msk->pm.local;
>>>>>>>         opts->remote = msk->pm.remote;
>>>>>>>         if (mptcp_pm_should_add_signal_echo(msk))
>>>>>>>                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>>>>         else
>>>>>>>                 add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
>>>>>>>         WRITE_ONCE(msk->pm.addr_signal, add_addr);
>>>>>>>         ret = true;
>>>>>>>
>>>>>>> out_unlock:
>>>>>>>         spin_unlock_bh(&msk->pm.lock);
>>>>>>>         if (ret && mptcp_pm_should_add_signal_echo(msk) &&
>>>>>>> mptcp_pm_should_add_signal_addr(msk))
>>>>>>>                 mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>>>>>>>         return ret;
>>>>>>> '''
>>>>>>>
>>>>>>>>
>>>>>>>>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>>>>>>> index a0b0ec0..0bfbbdef 100644
>>>>>>>> --- a/net/mptcp/protocol.h
>>>>>>>> +++ b/net/mptcp/protocol.h
>>>>>>>> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
>>>>>>>>         return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
>>>>>>>>  }
>>>>>>>>
>>>>>>>> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
>>>>>>>> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
>>>>>>>>  {
>>>>>>>> -       u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>>>> +       u8 len = 0;
>>>>>>>> +       struct mptcp_addr_info *addr = &opts->remote;
>>>>>>>
>>>>>>> We can simplify it like this:
>>>>>>>          struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>>>>>> &opts->remote;
>>>>>>>
>>>>>>> And keep the orignal code unchanged.
>>>>>>>
>>>>>>>>
>>>>>>>> -       if (family == AF_INET6)
>>>>>>>> -               len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>>>> -       if (!echo)
>>>>>>>> +       if (opts->ahmac) {
>>>>>>>> +               addr = &opts->local;
>>>>>>>>                 len += MPTCPOPT_THMAC_LEN;
>>>>>>>> +       }
>>>>>>>> +
>>>>>>>> +       if (addr->family == AF_INET6)
>>>>>>>> +               len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>>>> +       else
>>>>>>>> +               len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>>>> +
>>>>>>>>         /* account for 2 trailing 'nop' options */
>>>>>>>> -       if (port)
>>>>>>>> +       if (addr->port)
>>>>>>>>                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>>>>>>>
>>>>>>>>         return len;
>>>>>>>
>>>>>>> The whole function is something like this:
>>>>>>> '''
>>>>>>>         struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>>>>>> &opts->remote;
>>>>>>>         u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>>>
>>>>>>>         if (addr->family == AF_INET6)
>>>>>>>                 len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>>>         if (opts->ahmac)
>>>>>>>                 len += MPTCPOPT_THMAC_LEN;
>>>>>>>         /* account for 2 trailing 'nop' options */
>>>>>>>         if (addr->port)
>>>>>>>                 len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>>>>>>
>>>>>>>         return len;
>>>>>>> '''
>>>>>>>
>>>>>>> Thanks.
>>>>>>> -Geliang
>>>>>>>
>>>>>>>> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
>>>>>>>>         return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
>>>>>>>>  }
>>>>>>>>
>>>>>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>>>> -                             struct mptcp_addr_info *saddr, bool *echo, bool *port);
>>>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
>>>>>>>> +                             u8 *add_addr);
>>>>>>>>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>>>>                              struct mptcp_rm_list *rm_list);
>>>>>>>>  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
>>>>>>>> --
>>>>>>>> 1.8.3.1
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Li YongLong
>>>>>
>>>>
>>>> --
>>>> Li YongLong
>>>>
>>>
>>
>> --
>> Li YongLong
>

Patch

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index d61bbbf..d2c6ebe 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -61,7 +61,8 @@  struct mptcp_out_options {
 	u64 sndr_key;
 	u64 rcvr_key;
 	u64 ahmac;
-	struct mptcp_addr_info addr;
+	struct mptcp_addr_info local;
+	struct mptcp_addr_info remote;
 	struct mptcp_rm_list rm_list;
 	u8 join_id;
 	u8 backup;
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 1aec016..1707bec 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -655,13 +655,15 @@  static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
 	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
 	bool drop_other_suboptions = false;
 	unsigned int opt_size = *size;
-	bool echo;
-	bool port;
-	int len;
+	u8 add_addr, flags = 0xff;
+	int len = 0;
 
-	if ((mptcp_pm_should_add_signal_ipv6(msk) ||
-	     mptcp_pm_should_add_signal_port(msk) ||
-	     mptcp_pm_should_add_signal_echo(msk)) &&
+	if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
+		return false;
+
+	if ((mptcp_pm_should_add_signal_echo(msk) ||
+	     (mptcp_pm_should_add_signal_addr(msk) &&
+	      (opts->local.family == AF_INET6 || opts->local.port))) &&
 	    skb && skb_is_tcp_pure_ack(skb)) {
 		pr_debug("drop other suboptions");
 		opts->suboptions = 0;
@@ -671,11 +673,17 @@  static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
 		drop_other_suboptions = true;
 	}
 
-	if (!mptcp_pm_should_add_signal(msk) ||
-	    !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
-		return false;
 
-	len = mptcp_add_addr_len(opts->addr.family, echo, port);
+	if (mptcp_pm_should_add_signal_echo(msk)) {
+		flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
+	} else {
+		opts->ahmac = add_addr_generate_hmac(msk->local_key,
+						     msk->remote_key,
+						     &opts->local);
+		flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
+	}
+
+	len = mptcp_add_addr_len(opts);
 	if (remaining < len)
 		return false;
 
@@ -683,13 +691,14 @@  static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
 	if (drop_other_suboptions)
 		*size -= opt_size;
 	opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
-	if (!echo) {
-		opts->ahmac = add_addr_generate_hmac(msk->local_key,
-						     msk->remote_key,
-						     &opts->addr);
-	}
-	pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
-		 opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
+
+	spin_lock_bh(&msk->pm.lock);
+	WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
+	spin_unlock_bh(&msk->pm.lock);
+
+	pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
+		 add_addr, (opts->ahmac == 0), opts->local.id,
+		 opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
 
 	return true;
 }
@@ -1229,15 +1238,19 @@  void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 
 mp_capable_done:
 	if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
+		struct mptcp_addr_info *addr = &opts->remote;
 		u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
 		u8 echo = MPTCP_ADDR_ECHO;
 
+		if (opts->ahmac)
+			addr = &opts->local;
+
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
-		if (opts->addr.family == AF_INET6)
+		if (addr->family == AF_INET6)
 			len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
 #endif
 
-		if (opts->addr.port)
+		if (addr->port)
 			len += TCPOLEN_MPTCP_PORT_LEN;
 
 		if (opts->ahmac) {
@@ -1246,25 +1259,25 @@  void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 		}
 
 		*ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
-				      len, echo, opts->addr.id);
-		if (opts->addr.family == AF_INET) {
-			memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
+				      len, echo, addr->id);
+		if (addr->family == AF_INET) {
+			memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
 			ptr += 1;
 		}
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
-		else if (opts->addr.family == AF_INET6) {
-			memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
+		else if (addr->family == AF_INET6) {
+			memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
 			ptr += 4;
 		}
 #endif
 
-		if (!opts->addr.port) {
+		if (!addr->port) {
 			if (opts->ahmac) {
 				put_unaligned_be64(opts->ahmac, ptr);
 				ptr += 2;
 			}
 		} else {
-			u16 port = ntohs(opts->addr.port);
+			u16 port = ntohs(addr->port);
 
 			if (opts->ahmac) {
 				u8 *bptr = (u8 *)ptr;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index cf873e9..9c621293 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -253,32 +253,25 @@  void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
 
 /* path manager helpers */
 
-bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
-			      struct mptcp_addr_info *saddr, bool *echo, bool *port)
+bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
+			      u8 *add_addr)
 {
-	u8 add_addr;
-	int ret = false;
-
 	spin_lock_bh(&msk->pm.lock);
 
-	/* double check after the lock is acquired */
-	if (!mptcp_pm_should_add_signal(msk))
-		goto out_unlock;
-
-	*echo = mptcp_pm_should_add_signal_echo(msk);
-	*port = mptcp_pm_should_add_signal_port(msk);
-
-	if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
-		goto out_unlock;
+	if (!mptcp_pm_should_add_signal(msk)) {
+		spin_unlock_bh(&msk->pm.lock);
+		return false;
+	}
 
-	*saddr = msk->pm.local;
-	add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
-	WRITE_ONCE(msk->pm.addr_signal, add_addr);
-	ret = true;
+	opts->local = msk->pm.local;
+	opts->remote = msk->pm.remote;
+	*add_addr = msk->pm.addr_signal;
 
-out_unlock:
 	spin_unlock_bh(&msk->pm.lock);
-	return ret;
+
+	if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
+		mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
+	return true;
 }
 
 bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index a0b0ec0..0bfbbdef 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -737,16 +737,23 @@  static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
 	return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
 }
 
-static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
+static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
 {
-	u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
+	u8 len = 0;
+	struct mptcp_addr_info *addr = &opts->remote;
 
-	if (family == AF_INET6)
-		len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
-	if (!echo)
+	if (opts->ahmac) {
+		addr = &opts->local;
 		len += MPTCPOPT_THMAC_LEN;
+	}
+
+	if (addr->family == AF_INET6)
+		len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
+	else
+		len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
+
 	/* account for 2 trailing 'nop' options */
-	if (port)
+	if (addr->port)
 		len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
 
 	return len;
@@ -760,8 +767,8 @@  static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
 	return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
 }
 
-bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
-			      struct mptcp_addr_info *saddr, bool *echo, bool *port);
+bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
+			      u8 *add_addr);
 bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 			     struct mptcp_rm_list *rm_list);
 int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);