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

[Geliang Tang <geliangtang@gmail.com>]

Yonglong Li <liyonglong@chinatelecom.cn> 于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 <liyonglong@chinatelecom.cn>
> ---
>  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
>
>