* Re: [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-17 9:14 ` [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
@ 2021-06-17 12:37 ` Geliang Tang
2021-06-18 1:10 ` Yonglong Li
2021-06-17 19:22 ` kernel test robot
2021-06-18 0:25 ` Mat Martineau
2 siblings, 1 reply; 13+ messages in thread
From: Geliang Tang @ 2021-06-17 12:37 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Paolo Abeni, Matthieu Baerts, Mat Martineau
Hi Yonglong,
Thanks for this patch set.
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月17日周四 下午5:15写道：
>
> 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>
> ---
> net/mptcp/options.c | 161 +++++++++++++++++++++++++++++++++------------------
> net/mptcp/pm.c | 30 +++-------
> net/mptcp/protocol.h | 13 +++--
> 3 files changed, 122 insertions(+), 82 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..3ecf2c6 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,43 +655,72 @@ 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;
> + struct mptcp_addr_info remote;
> + struct mptcp_addr_info local;
> + int ret = false;
> + u8 add_addr, flags;
> int len;
>
> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> - mptcp_pm_should_add_signal_port(msk) ||
> - mptcp_pm_should_add_signal_echo(msk)) &&
> - 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;
> - }
> -
> - 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 (remaining < len)
> - return false;
> -
> - *size = len;
> - if (drop_other_suboptions)
> - *size -= opt_size;
> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> - if (!echo) {
> + if (!mptcp_pm_should_add_signal(msk))
> + goto out;
> +
> + *size = 0;
> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> + if (mptcp_pm_should_add_signal_echo(msk)) {
> + if (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(remote.family, true, !!remote.port);
> + if (remaining < len && mptcp_pm_should_add_signal_addr(msk))
> + goto add_addr;
> + else if (remaining < len)
> + goto out;
> + remaining -= len;
> + *size += len;
> + opts->remote = remote;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> + opts->remote.id, ntohs(opts->remote.port), add_addr);
> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
> +add_addr:
> + if ((local.family == AF_INET6 || 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(local.family, false, !!local.port);
> + if (remaining < len)
> + goto out;
> + *size += len;
> + opts->addr = local;
> opts->ahmac = add_addr_generate_hmac(msk->local_key,
> msk->remote_key,
> &opts->addr);
> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> + pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
> + opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
> }
There are some duplicate codes here between the
mptcp_pm_should_add_signal_echo(msk) trunk and the
mptcp_pm_should_add_signal_addr(msk) trunk, could you please simply them
into one trunk?
> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
>
> - return true;
> + if (drop_other_suboptions)
> + *size -= opt_size;
> + spin_lock_bh(&msk->pm.lock);
> + WRITE_ONCE(msk->pm.addr_signal, flags | msk->pm.addr_signal);
> + spin_unlock_bh(&msk->pm.lock);
> + ret = true;
> +
> +out:
> + return ret;
> }
>
> static bool mptcp_established_options_rm_addr(struct sock *sk,
> @@ -1230,21 +1259,18 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> mp_capable_done:
> if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> - u8 echo = MPTCP_ADDR_ECHO;
> + u8 echo = 0;
>
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> if (opts->addr.family == AF_INET6)
> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> #endif
>
> + len += sizeof(opts->ahmac);
> +
> if (opts->addr.port)
> len += TCPOLEN_MPTCP_PORT_LEN;
>
> - if (opts->ahmac) {
> - len += sizeof(opts->ahmac);
> - echo = 0;
> - }
> -
> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> len, echo, opts->addr.id);
> if (opts->addr.family == AF_INET) {
> @@ -1259,30 +1285,55 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> #endif
>
> if (!opts->addr.port) {
> - if (opts->ahmac) {
> - put_unaligned_be64(opts->ahmac, ptr);
> - ptr += 2;
> - }
> + put_unaligned_be64(opts->ahmac, ptr);
> + ptr += 2;
> } else {
> u16 port = ntohs(opts->addr.port);
> + u8 *bptr = (u8 *)ptr;
>
> - if (opts->ahmac) {
> - u8 *bptr = (u8 *)ptr;
> + put_unaligned_be16(port, bptr);
> + bptr += 2;
> + put_unaligned_be64(opts->ahmac, bptr);
> + bptr += 8;
> + put_unaligned_be16(TCPOPT_NOP << 8 |
> + TCPOPT_NOP, bptr);
>
> - put_unaligned_be16(port, bptr);
> - bptr += 2;
> - put_unaligned_be64(opts->ahmac, bptr);
> - bptr += 8;
> - put_unaligned_be16(TCPOPT_NOP << 8 |
> - TCPOPT_NOP, bptr);
> + ptr += 3;
> + }
> + }
>
> - ptr += 3;
> - } else {
> - put_unaligned_be32(port << 16 |
> - TCPOPT_NOP << 8 |
> - TCPOPT_NOP, ptr);
> - ptr += 1;
> - }
> + if (OPTION_MPTCP_ADD_ECHO & opts->suboptions) {
> + u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> + u8 echo = MPTCP_ADDR_ECHO;
> +
> +#if IS_ENABLED(CONFIG_MPTCP_IPV6)
> + if (opts->remote.family == AF_INET6)
> + len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> +#endif
> +
> + if (opts->remote.port)
> + len += TCPOLEN_MPTCP_PORT_LEN;
> +
> + *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> + len, echo, opts->remote.id);
> + if (opts->remote.family == AF_INET) {
> + memcpy((u8 *)ptr, (u8 *)&opts->remote.addr.s_addr, 4);
> + ptr += 1;
> + }
> +#if IS_ENABLED(CONFIG_MPTCP_IPV6)
> + else if (opts->remote.family == AF_INET6) {
> + memcpy((u8 *)ptr, opts->remote.addr6.s6_addr, 16);
> + ptr += 4;
> + }
> +#endif
> +
> + if (opts->remote.port) {
> + u16 port = ntohs(opts->remote.port);
> +
> + put_unaligned_be32(port << 16 |
> + TCPOPT_NOP << 8 |
> + TCPOPT_NOP, ptr);
> + ptr += 1;
> }
> }
And the same here between the OPTION_MPTCP_ADD_ADDR trunk and the
OPTION_MPTCP_ADD_ECHO trunk.
Thanks.
-Geliang
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 74be6d7..a62d4a5 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -22,7 +22,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
>
> lockdep_assert_held(&msk->pm.lock);
>
> - if (add_addr) {
> + if (add_addr &
> + (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> pr_warn("addr_signal error, add_addr=%d", add_addr);
> return -EINVAL;
> }
> @@ -252,32 +253,19 @@ 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)
> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> + struct mptcp_addr_info *daddr, 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;
> -
> *saddr = msk->pm.local;
> - add_addr = msk->pm.addr_signal & BIT(MPTCP_RM_ADDR_SIGNAL);
> - WRITE_ONCE(msk->pm.addr_signal, add_addr);
> - ret = true;
> + *daddr = 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);
> }
>
> 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..90fb532 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -22,10 +22,11 @@
> #define OPTION_MPTCP_MPJ_SYNACK BIT(4)
> #define OPTION_MPTCP_MPJ_ACK BIT(5)
> #define OPTION_MPTCP_ADD_ADDR BIT(6)
> -#define OPTION_MPTCP_RM_ADDR BIT(7)
> -#define OPTION_MPTCP_FASTCLOSE BIT(8)
> -#define OPTION_MPTCP_PRIO BIT(9)
> -#define OPTION_MPTCP_RST BIT(10)
> +#define OPTION_MPTCP_ADD_ECHO BIT(7)
> +#define OPTION_MPTCP_RM_ADDR BIT(8)
> +#define OPTION_MPTCP_FASTCLOSE BIT(9)
> +#define OPTION_MPTCP_PRIO BIT(10)
> +#define OPTION_MPTCP_RST BIT(11)
>
> /* MPTCP option subtypes */
> #define MPTCPOPT_MP_CAPABLE 0
> @@ -760,8 +761,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);
> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> + struct mptcp_addr_info *daddr, 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
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-17 12:37 ` Geliang Tang
@ 2021-06-18 1:10 ` Yonglong Li
0 siblings, 0 replies; 13+ messages in thread
From: Yonglong Li @ 2021-06-18 1:10 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp
Hi Geliang,
Thanks for your review. I will simply the code and send v4 patch.
On 2021/6/17 20:37, Geliang Tang wrote:
>> &opts->addr);
>> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>> + pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
>> + opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
>> }
> There are some duplicate codes here between the
> mptcp_pm_should_add_signal_echo(msk) trunk and the
> mptcp_pm_should_add_signal_addr(msk) trunk, could you please simply them
> into one trunk?
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-17 9:14 ` [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
@ 2021-06-17 19:22 ` kernel test robot
2021-06-17 19:22 ` kernel test robot
2021-06-18 0:25 ` Mat Martineau
2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-06-17 19:22 UTC (permalink / raw)
To: Yonglong Li, mptcp
Cc: kbuild-all, clang-built-linux, pabeni, matthieu.baerts,
mathew.j.martineau, geliangtang, Yonglong Li
[-- Attachment #1: Type: text/plain, Size: 9971 bytes --]
Hi Yonglong,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mptcp/export]
[also build test WARNING on linus/master v5.13-rc6 next-20210617]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Yonglong-Li/mptcp-fix-conflicts-when-using-pm-add_signal-in-ADD_ADDR-echo-and-RM_ADDR-process/20210617-171559
base: https://github.com/multipath-tcp/mptcp_net-next.git export
config: x86_64-randconfig-a015-20210617 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 64720f57bea6a6bf033feef4a5751ab9c0c3b401)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/dcb008513c667a57c48dd885599f2d760c8cf7eb
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Yonglong-Li/mptcp-fix-conflicts-when-using-pm-add_signal-in-ADD_ADDR-echo-and-RM_ADDR-process/20210617-171559
git checkout dcb008513c667a57c48dd885599f2d760c8cf7eb
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
net/mptcp/options.c:567:21: warning: parameter 'remaining' set but not used [-Wunused-but-set-parameter]
unsigned int remaining,
^
>> net/mptcp/options.c:698:9: warning: variable 'flags' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
} else if (mptcp_pm_should_add_signal_addr(msk)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:56:28: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:58:30: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
net/mptcp/options.c:726:34: note: uninitialized use occurs here
WRITE_ONCE(msk->pm.addr_signal, flags | msk->pm.addr_signal);
^~~~~
include/asm-generic/rwonce.h:61:18: note: expanded from macro 'WRITE_ONCE'
__WRITE_ONCE(x, val); \
^~~
include/asm-generic/rwonce.h:55:33: note: expanded from macro '__WRITE_ONCE'
*(volatile typeof(x) *)&(x) = (val); \
^~~
net/mptcp/options.c:698:9: note: remove the 'if' if its condition is always true
} else if (mptcp_pm_should_add_signal_addr(msk)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:56:23: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
net/mptcp/options.c:669:20: note: initialize the variable 'flags' to silence this warning
u8 add_addr, flags;
^
= '\0'
2 warnings generated.
vim +698 net/mptcp/options.c
563
564 static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
565 bool snd_data_fin_enable,
566 unsigned int *size,
> 567 unsigned int remaining,
568 struct mptcp_out_options *opts)
569 {
570 struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
571 struct mptcp_sock *msk = mptcp_sk(subflow->conn);
572 unsigned int dss_size = 0;
573 struct mptcp_ext *mpext;
574 unsigned int ack_size;
575 bool ret = false;
576 u64 ack_seq;
577
578 opts->csum_reqd = READ_ONCE(msk->csum_enabled);
579 mpext = skb ? mptcp_get_ext(skb) : NULL;
580
581 if (!skb || (mpext && mpext->use_map) || snd_data_fin_enable) {
582 unsigned int map_size = TCPOLEN_MPTCP_DSS_BASE + TCPOLEN_MPTCP_DSS_MAP64;
583
584 if (mpext) {
585 if (opts->csum_reqd)
586 map_size += TCPOLEN_MPTCP_DSS_CHECKSUM;
587
588 opts->ext_copy = *mpext;
589 }
590
591 remaining -= map_size;
592 dss_size = map_size;
593 if (skb && snd_data_fin_enable)
594 mptcp_write_data_fin(subflow, skb, &opts->ext_copy);
595 ret = true;
596 }
597
598 /* passive sockets msk will set the 'can_ack' after accept(), even
599 * if the first subflow may have the already the remote key handy
600 */
601 opts->ext_copy.use_ack = 0;
602 if (!READ_ONCE(msk->can_ack)) {
603 *size = ALIGN(dss_size, 4);
604 return ret;
605 }
606
607 ack_seq = READ_ONCE(msk->ack_seq);
608 if (READ_ONCE(msk->use_64bit_ack)) {
609 ack_size = TCPOLEN_MPTCP_DSS_ACK64;
610 opts->ext_copy.data_ack = ack_seq;
611 opts->ext_copy.ack64 = 1;
612 } else {
613 ack_size = TCPOLEN_MPTCP_DSS_ACK32;
614 opts->ext_copy.data_ack32 = (uint32_t)ack_seq;
615 opts->ext_copy.ack64 = 0;
616 }
617 opts->ext_copy.use_ack = 1;
618 WRITE_ONCE(msk->old_wspace, __mptcp_space((struct sock *)msk));
619
620 /* Add kind/length/subtype/flag overhead if mapping is not populated */
621 if (dss_size == 0)
622 ack_size += TCPOLEN_MPTCP_DSS_BASE;
623
624 dss_size += ack_size;
625
626 *size = ALIGN(dss_size, 4);
627 return true;
628 }
629
630 static u64 add_addr_generate_hmac(u64 key1, u64 key2,
631 struct mptcp_addr_info *addr)
632 {
633 u16 port = ntohs(addr->port);
634 u8 hmac[SHA256_DIGEST_SIZE];
635 u8 msg[19];
636 int i = 0;
637
638 msg[i++] = addr->id;
639 if (addr->family == AF_INET) {
640 memcpy(&msg[i], &addr->addr.s_addr, 4);
641 i += 4;
642 }
643 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
644 else if (addr->family == AF_INET6) {
645 memcpy(&msg[i], &addr->addr6.s6_addr, 16);
646 i += 16;
647 }
648 #endif
649 msg[i++] = port >> 8;
650 msg[i++] = port & 0xFF;
651
652 mptcp_crypto_hmac_sha(key1, key2, msg, i, hmac);
653
654 return get_unaligned_be64(&hmac[SHA256_DIGEST_SIZE - sizeof(u64)]);
655 }
656
657 static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *skb,
658 unsigned int *size,
659 unsigned int remaining,
660 struct mptcp_out_options *opts)
661 {
662 struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
663 struct mptcp_sock *msk = mptcp_sk(subflow->conn);
664 bool drop_other_suboptions = false;
665 unsigned int opt_size = *size;
666 struct mptcp_addr_info remote;
667 struct mptcp_addr_info local;
668 int ret = false;
669 u8 add_addr, flags;
670 int len;
671
672 if (!mptcp_pm_should_add_signal(msk))
673 goto out;
674
675 *size = 0;
676 mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
677 if (mptcp_pm_should_add_signal_echo(msk)) {
678 if (skb && skb_is_tcp_pure_ack(skb)) {
679 pr_debug("drop other suboptions");
680 opts->suboptions = 0;
681 opts->ext_copy.use_ack = 0;
682 opts->ext_copy.use_map = 0;
683 remaining += opt_size;
684 drop_other_suboptions = true;
685 }
686 len = mptcp_add_addr_len(remote.family, true, !!remote.port);
687 if (remaining < len && mptcp_pm_should_add_signal_addr(msk))
688 goto add_addr;
689 else if (remaining < len)
690 goto out;
691 remaining -= len;
692 *size += len;
693 opts->remote = remote;
694 flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
695 opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
696 pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
697 opts->remote.id, ntohs(opts->remote.port), add_addr);
> 698 } else if (mptcp_pm_should_add_signal_addr(msk)) {
699 add_addr:
700 if ((local.family == AF_INET6 || local.port) && skb &&
701 skb_is_tcp_pure_ack(skb)) {
702 pr_debug("drop other suboptions");
703 opts->suboptions = 0;
704 opts->ext_copy.use_ack = 0;
705 opts->ext_copy.use_map = 0;
706 remaining += opt_size;
707 drop_other_suboptions = true;
708 }
709 len = mptcp_add_addr_len(local.family, false, !!local.port);
710 if (remaining < len)
711 goto out;
712 *size += len;
713 opts->addr = local;
714 opts->ahmac = add_addr_generate_hmac(msk->local_key,
715 msk->remote_key,
716 &opts->addr);
717 opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
718 flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
719 pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
720 opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
721 }
722
723 if (drop_other_suboptions)
724 *size -= opt_size;
725 spin_lock_bh(&msk->pm.lock);
726 WRITE_ONCE(msk->pm.addr_signal, flags | msk->pm.addr_signal);
727 spin_unlock_bh(&msk->pm.lock);
728 ret = true;
729
730 out:
731 return ret;
732 }
733
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31950 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
@ 2021-06-17 19:22 ` kernel test robot
0 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-06-17 19:22 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 10215 bytes --]
Hi Yonglong,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mptcp/export]
[also build test WARNING on linus/master v5.13-rc6 next-20210617]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Yonglong-Li/mptcp-fix-conflicts-when-using-pm-add_signal-in-ADD_ADDR-echo-and-RM_ADDR-process/20210617-171559
base: https://github.com/multipath-tcp/mptcp_net-next.git export
config: x86_64-randconfig-a015-20210617 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 64720f57bea6a6bf033feef4a5751ab9c0c3b401)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/dcb008513c667a57c48dd885599f2d760c8cf7eb
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Yonglong-Li/mptcp-fix-conflicts-when-using-pm-add_signal-in-ADD_ADDR-echo-and-RM_ADDR-process/20210617-171559
git checkout dcb008513c667a57c48dd885599f2d760c8cf7eb
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
net/mptcp/options.c:567:21: warning: parameter 'remaining' set but not used [-Wunused-but-set-parameter]
unsigned int remaining,
^
>> net/mptcp/options.c:698:9: warning: variable 'flags' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
} else if (mptcp_pm_should_add_signal_addr(msk)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:56:28: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:58:30: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
net/mptcp/options.c:726:34: note: uninitialized use occurs here
WRITE_ONCE(msk->pm.addr_signal, flags | msk->pm.addr_signal);
^~~~~
include/asm-generic/rwonce.h:61:18: note: expanded from macro 'WRITE_ONCE'
__WRITE_ONCE(x, val); \
^~~
include/asm-generic/rwonce.h:55:33: note: expanded from macro '__WRITE_ONCE'
*(volatile typeof(x) *)&(x) = (val); \
^~~
net/mptcp/options.c:698:9: note: remove the 'if' if its condition is always true
} else if (mptcp_pm_should_add_signal_addr(msk)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:56:23: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
net/mptcp/options.c:669:20: note: initialize the variable 'flags' to silence this warning
u8 add_addr, flags;
^
= '\0'
2 warnings generated.
vim +698 net/mptcp/options.c
563
564 static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
565 bool snd_data_fin_enable,
566 unsigned int *size,
> 567 unsigned int remaining,
568 struct mptcp_out_options *opts)
569 {
570 struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
571 struct mptcp_sock *msk = mptcp_sk(subflow->conn);
572 unsigned int dss_size = 0;
573 struct mptcp_ext *mpext;
574 unsigned int ack_size;
575 bool ret = false;
576 u64 ack_seq;
577
578 opts->csum_reqd = READ_ONCE(msk->csum_enabled);
579 mpext = skb ? mptcp_get_ext(skb) : NULL;
580
581 if (!skb || (mpext && mpext->use_map) || snd_data_fin_enable) {
582 unsigned int map_size = TCPOLEN_MPTCP_DSS_BASE + TCPOLEN_MPTCP_DSS_MAP64;
583
584 if (mpext) {
585 if (opts->csum_reqd)
586 map_size += TCPOLEN_MPTCP_DSS_CHECKSUM;
587
588 opts->ext_copy = *mpext;
589 }
590
591 remaining -= map_size;
592 dss_size = map_size;
593 if (skb && snd_data_fin_enable)
594 mptcp_write_data_fin(subflow, skb, &opts->ext_copy);
595 ret = true;
596 }
597
598 /* passive sockets msk will set the 'can_ack' after accept(), even
599 * if the first subflow may have the already the remote key handy
600 */
601 opts->ext_copy.use_ack = 0;
602 if (!READ_ONCE(msk->can_ack)) {
603 *size = ALIGN(dss_size, 4);
604 return ret;
605 }
606
607 ack_seq = READ_ONCE(msk->ack_seq);
608 if (READ_ONCE(msk->use_64bit_ack)) {
609 ack_size = TCPOLEN_MPTCP_DSS_ACK64;
610 opts->ext_copy.data_ack = ack_seq;
611 opts->ext_copy.ack64 = 1;
612 } else {
613 ack_size = TCPOLEN_MPTCP_DSS_ACK32;
614 opts->ext_copy.data_ack32 = (uint32_t)ack_seq;
615 opts->ext_copy.ack64 = 0;
616 }
617 opts->ext_copy.use_ack = 1;
618 WRITE_ONCE(msk->old_wspace, __mptcp_space((struct sock *)msk));
619
620 /* Add kind/length/subtype/flag overhead if mapping is not populated */
621 if (dss_size == 0)
622 ack_size += TCPOLEN_MPTCP_DSS_BASE;
623
624 dss_size += ack_size;
625
626 *size = ALIGN(dss_size, 4);
627 return true;
628 }
629
630 static u64 add_addr_generate_hmac(u64 key1, u64 key2,
631 struct mptcp_addr_info *addr)
632 {
633 u16 port = ntohs(addr->port);
634 u8 hmac[SHA256_DIGEST_SIZE];
635 u8 msg[19];
636 int i = 0;
637
638 msg[i++] = addr->id;
639 if (addr->family == AF_INET) {
640 memcpy(&msg[i], &addr->addr.s_addr, 4);
641 i += 4;
642 }
643 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
644 else if (addr->family == AF_INET6) {
645 memcpy(&msg[i], &addr->addr6.s6_addr, 16);
646 i += 16;
647 }
648 #endif
649 msg[i++] = port >> 8;
650 msg[i++] = port & 0xFF;
651
652 mptcp_crypto_hmac_sha(key1, key2, msg, i, hmac);
653
654 return get_unaligned_be64(&hmac[SHA256_DIGEST_SIZE - sizeof(u64)]);
655 }
656
657 static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *skb,
658 unsigned int *size,
659 unsigned int remaining,
660 struct mptcp_out_options *opts)
661 {
662 struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
663 struct mptcp_sock *msk = mptcp_sk(subflow->conn);
664 bool drop_other_suboptions = false;
665 unsigned int opt_size = *size;
666 struct mptcp_addr_info remote;
667 struct mptcp_addr_info local;
668 int ret = false;
669 u8 add_addr, flags;
670 int len;
671
672 if (!mptcp_pm_should_add_signal(msk))
673 goto out;
674
675 *size = 0;
676 mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
677 if (mptcp_pm_should_add_signal_echo(msk)) {
678 if (skb && skb_is_tcp_pure_ack(skb)) {
679 pr_debug("drop other suboptions");
680 opts->suboptions = 0;
681 opts->ext_copy.use_ack = 0;
682 opts->ext_copy.use_map = 0;
683 remaining += opt_size;
684 drop_other_suboptions = true;
685 }
686 len = mptcp_add_addr_len(remote.family, true, !!remote.port);
687 if (remaining < len && mptcp_pm_should_add_signal_addr(msk))
688 goto add_addr;
689 else if (remaining < len)
690 goto out;
691 remaining -= len;
692 *size += len;
693 opts->remote = remote;
694 flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
695 opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
696 pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
697 opts->remote.id, ntohs(opts->remote.port), add_addr);
> 698 } else if (mptcp_pm_should_add_signal_addr(msk)) {
699 add_addr:
700 if ((local.family == AF_INET6 || local.port) && skb &&
701 skb_is_tcp_pure_ack(skb)) {
702 pr_debug("drop other suboptions");
703 opts->suboptions = 0;
704 opts->ext_copy.use_ack = 0;
705 opts->ext_copy.use_map = 0;
706 remaining += opt_size;
707 drop_other_suboptions = true;
708 }
709 len = mptcp_add_addr_len(local.family, false, !!local.port);
710 if (remaining < len)
711 goto out;
712 *size += len;
713 opts->addr = local;
714 opts->ahmac = add_addr_generate_hmac(msk->local_key,
715 msk->remote_key,
716 &opts->addr);
717 opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
718 flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
719 pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
720 opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
721 }
722
723 if (drop_other_suboptions)
724 *size -= opt_size;
725 spin_lock_bh(&msk->pm.lock);
726 WRITE_ONCE(msk->pm.addr_signal, flags | msk->pm.addr_signal);
727 spin_unlock_bh(&msk->pm.lock);
728 ret = true;
729
730 out:
731 return ret;
732 }
733
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31950 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-17 9:14 ` [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
2021-06-17 12:37 ` Geliang Tang
2021-06-17 19:22 ` kernel test robot
@ 2021-06-18 0:25 ` Mat Martineau
2021-06-18 1:24 ` Yonglong Li
2 siblings, 1 reply; 13+ messages in thread
From: Mat Martineau @ 2021-06-18 0:25 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, pabeni, matthieu.baerts, geliangtang
On Thu, 17 Jun 2021, Yonglong Li wrote:
> 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>
> ---
> net/mptcp/options.c | 161 +++++++++++++++++++++++++++++++++------------------
> net/mptcp/pm.c | 30 +++-------
> net/mptcp/protocol.h | 13 +++--
> 3 files changed, 122 insertions(+), 82 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..3ecf2c6 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,43 +655,72 @@ 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;
> + struct mptcp_addr_info remote;
> + struct mptcp_addr_info local;
> + int ret = false;
> + u8 add_addr, flags;
> int len;
>
> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> - mptcp_pm_should_add_signal_port(msk) ||
> - mptcp_pm_should_add_signal_echo(msk)) &&
> - 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;
> - }
> -
> - 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 (remaining < len)
> - return false;
> -
> - *size = len;
> - if (drop_other_suboptions)
> - *size -= opt_size;
> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> - if (!echo) {
> + if (!mptcp_pm_should_add_signal(msk))
> + goto out;
Hi Yonglong, thanks for revising.
Instead of the goto here, just "return true;".
> +
> + *size = 0;
> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> + if (mptcp_pm_should_add_signal_echo(msk)) {
> + if (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(remote.family, true, !!remote.port);
> + if (remaining < len && mptcp_pm_should_add_signal_addr(msk))
> + goto add_addr;
This goto isn't quite right. It jumps below with opts and remaining
already modified, and may end up modifying 'remaining' again.
Would be better to separate the logic for sending echo-vs-signal, so the
goto isn't necessary.
> + else if (remaining < len)
> + goto out;
> + remaining -= len;
> + *size += len;
> + opts->remote = remote;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> + opts->remote.id, ntohs(opts->remote.port), add_addr);
> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
> +add_addr:
> + if ((local.family == AF_INET6 || 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(local.family, false, !!local.port);
> + if (remaining < len)
> + goto out;
> + *size += len;
> + opts->addr = local;
> opts->ahmac = add_addr_generate_hmac(msk->local_key,
> msk->remote_key,
> &opts->addr);
> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> + pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
> + opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
> }
> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
>
> - return true;
> + if (drop_other_suboptions)
> + *size -= opt_size;
> + spin_lock_bh(&msk->pm.lock);
> + WRITE_ONCE(msk->pm.addr_signal, flags | msk->pm.addr_signal);
> + spin_unlock_bh(&msk->pm.lock);
This would set bits in msk->pm.addr_signal rather than clear them. Did you
intend '&' instead of '|'?
As the kbuild bot noted, 'flags' can be uninitialized. That code path is
not expected and shouldn't happen, but since the pm lock is not held the
whole time the code should handle concurrent changes to
msk->pm.addr_signal. Could initialize flags to 0 and only
lock/write/unlock if flags is nonzero.
> + ret = true;
> +
> +out:
> + return ret;
Since the return is the only thing after the label, better to not use
'goto' and use return statements where needed in the code above.
-Mat
> }
>
> static bool mptcp_established_options_rm_addr(struct sock *sk,
> @@ -1230,21 +1259,18 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> mp_capable_done:
> if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> - u8 echo = MPTCP_ADDR_ECHO;
> + u8 echo = 0;
>
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> if (opts->addr.family == AF_INET6)
> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> #endif
>
> + len += sizeof(opts->ahmac);
> +
> if (opts->addr.port)
> len += TCPOLEN_MPTCP_PORT_LEN;
>
> - if (opts->ahmac) {
> - len += sizeof(opts->ahmac);
> - echo = 0;
> - }
> -
> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> len, echo, opts->addr.id);
> if (opts->addr.family == AF_INET) {
> @@ -1259,30 +1285,55 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> #endif
>
> if (!opts->addr.port) {
> - if (opts->ahmac) {
> - put_unaligned_be64(opts->ahmac, ptr);
> - ptr += 2;
> - }
> + put_unaligned_be64(opts->ahmac, ptr);
> + ptr += 2;
> } else {
> u16 port = ntohs(opts->addr.port);
> + u8 *bptr = (u8 *)ptr;
>
> - if (opts->ahmac) {
> - u8 *bptr = (u8 *)ptr;
> + put_unaligned_be16(port, bptr);
> + bptr += 2;
> + put_unaligned_be64(opts->ahmac, bptr);
> + bptr += 8;
> + put_unaligned_be16(TCPOPT_NOP << 8 |
> + TCPOPT_NOP, bptr);
>
> - put_unaligned_be16(port, bptr);
> - bptr += 2;
> - put_unaligned_be64(opts->ahmac, bptr);
> - bptr += 8;
> - put_unaligned_be16(TCPOPT_NOP << 8 |
> - TCPOPT_NOP, bptr);
> + ptr += 3;
> + }
> + }
>
> - ptr += 3;
> - } else {
> - put_unaligned_be32(port << 16 |
> - TCPOPT_NOP << 8 |
> - TCPOPT_NOP, ptr);
> - ptr += 1;
> - }
> + if (OPTION_MPTCP_ADD_ECHO & opts->suboptions) {
> + u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> + u8 echo = MPTCP_ADDR_ECHO;
> +
> +#if IS_ENABLED(CONFIG_MPTCP_IPV6)
> + if (opts->remote.family == AF_INET6)
> + len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> +#endif
> +
> + if (opts->remote.port)
> + len += TCPOLEN_MPTCP_PORT_LEN;
> +
> + *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> + len, echo, opts->remote.id);
> + if (opts->remote.family == AF_INET) {
> + memcpy((u8 *)ptr, (u8 *)&opts->remote.addr.s_addr, 4);
> + ptr += 1;
> + }
> +#if IS_ENABLED(CONFIG_MPTCP_IPV6)
> + else if (opts->remote.family == AF_INET6) {
> + memcpy((u8 *)ptr, opts->remote.addr6.s6_addr, 16);
> + ptr += 4;
> + }
> +#endif
> +
> + if (opts->remote.port) {
> + u16 port = ntohs(opts->remote.port);
> +
> + put_unaligned_be32(port << 16 |
> + TCPOPT_NOP << 8 |
> + TCPOPT_NOP, ptr);
> + ptr += 1;
> }
> }
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 74be6d7..a62d4a5 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -22,7 +22,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
>
> lockdep_assert_held(&msk->pm.lock);
>
> - if (add_addr) {
> + if (add_addr &
> + (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> pr_warn("addr_signal error, add_addr=%d", add_addr);
> return -EINVAL;
> }
> @@ -252,32 +253,19 @@ 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)
> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> + struct mptcp_addr_info *daddr, 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;
> -
> *saddr = msk->pm.local;
> - add_addr = msk->pm.addr_signal & BIT(MPTCP_RM_ADDR_SIGNAL);
> - WRITE_ONCE(msk->pm.addr_signal, add_addr);
> - ret = true;
> + *daddr = 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);
> }
>
> 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..90fb532 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -22,10 +22,11 @@
> #define OPTION_MPTCP_MPJ_SYNACK BIT(4)
> #define OPTION_MPTCP_MPJ_ACK BIT(5)
> #define OPTION_MPTCP_ADD_ADDR BIT(6)
> -#define OPTION_MPTCP_RM_ADDR BIT(7)
> -#define OPTION_MPTCP_FASTCLOSE BIT(8)
> -#define OPTION_MPTCP_PRIO BIT(9)
> -#define OPTION_MPTCP_RST BIT(10)
> +#define OPTION_MPTCP_ADD_ECHO BIT(7)
> +#define OPTION_MPTCP_RM_ADDR BIT(8)
> +#define OPTION_MPTCP_FASTCLOSE BIT(9)
> +#define OPTION_MPTCP_PRIO BIT(10)
> +#define OPTION_MPTCP_RST BIT(11)
>
> /* MPTCP option subtypes */
> #define MPTCPOPT_MP_CAPABLE 0
> @@ -760,8 +761,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);
> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> + struct mptcp_addr_info *daddr, 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
>
>
>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v3 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-18 0:25 ` Mat Martineau
@ 2021-06-18 1:24 ` Yonglong Li
0 siblings, 0 replies; 13+ messages in thread
From: Yonglong Li @ 2021-06-18 1:24 UTC (permalink / raw)
To: Mat Martineau; +Cc: mptcp
On 2021/6/18 8:25, Mat Martineau wrote:
>
> This goto isn't quite right. It jumps below with opts and remaining already modified, and may end up modifying 'remaining' again.
>
> Would be better to separate the logic for sending echo-vs-signal, so the goto isn't necessary.
Thanks for your review. The goto logic is not right indeed. I will separate the logic for sending echo-vs-signal
>
>> + else if (remaining < len)
>> + goto out;
>> + remaining -= len;
>> + *size += len;
>> + opts->remote = remote;
>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
>> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
>> + opts->remote.id, ntohs(opts->remote.port), add_addr);
>> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
>> +add_addr:
>> + if ((local.family == AF_INET6 || 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(local.family, false, !!local.port);
>> + if (remaining < len)
>> + goto out;
>> + *size += len;
>> + opts->addr = local;
>> opts->ahmac = add_addr_generate_hmac(msk->local_key,
>> msk->remote_key,
>> &opts->addr);
>> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>> + pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
>> + opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
>> }
>> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
>> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
>>
>> - return true;
>> + if (drop_other_suboptions)
>> + *size -= opt_size;
>> + spin_lock_bh(&msk->pm.lock);
>> + WRITE_ONCE(msk->pm.addr_signal, flags | msk->pm.addr_signal);
>> + spin_unlock_bh(&msk->pm.lock);
>
> This would set bits in msk->pm.addr_signal rather than clear them. Did you intend '&' instead of '|'?
Sorry for this mistake. :(
^ permalink raw reply [flat|nested] 13+ messages in thread