From 1f5e9e2f5fd55fbf9b58ae6fefb021ad1c91b66a Mon Sep 17 00:00:00 2001 From: Yonglong Li Date: Mon, 23 Aug 2021 18:05:39 -0700 Subject: [PATCH 1/6] mptcp: move drop_other_suboptions check under pm lock This patch moved the drop_other_suboptions check from mptcp_established_options_add_addr() into mptcp_pm_add_addr_signal(), do it under the PM lock to avoid the race between this check and mptcp_pm_add_addr_signal(). For this, added a new parameter for mptcp_pm_add_addr_signal() to get the drop_other_suboptions value. And drop the other suboptions after the option length check if drop_other_suboptions is true. Additionally, always drop the other suboption for TCP pure ack: that makes both the code simpler and the MPTCP behaviour more consistent. Co-developed-by: Geliang Tang Signed-off-by: Geliang Tang Co-developed-by: Paolo Abeni Signed-off-by: Paolo Abeni Signed-off-by: Yonglong Li Signed-off-by: Mat Martineau Signed-off-by: David S. Miller --- net/mptcp/options.c | 28 ++++++++++++++-------------- net/mptcp/pm.c | 15 +++++++++++++-- net/mptcp/protocol.h | 6 ++++-- 3 files changed, 31 insertions(+), 18 deletions(-) diff --git a/net/mptcp/options.c b/net/mptcp/options.c index bebb759f470e..4c37f4b215ee 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -667,29 +667,29 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff * bool port; 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; - } - + /* add addr will strip the existing options, be sure to avoid breaking + * MPC/MPJ handshakes + */ if (!mptcp_pm_should_add_signal(msk) || - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port))) + (opts->suboptions & (OPTION_MPTCP_MPJ_ACK | OPTION_MPTCP_MPC_ACK)) || + !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, &opts->addr, + &echo, &port, &drop_other_suboptions)) return false; + if (drop_other_suboptions) + remaining += opt_size; len = mptcp_add_addr_len(opts->addr.family, echo, port); if (remaining < len) return false; *size = len; - if (drop_other_suboptions) + if (drop_other_suboptions) { + pr_debug("drop other suboptions"); + opts->suboptions = 0; + opts->ext_copy.use_ack = 0; + opts->ext_copy.use_map = 0; *size -= opt_size; + } opts->suboptions |= OPTION_MPTCP_ADD_ADDR; if (!echo) { opts->ahmac = add_addr_generate_hmac(msk->local_key, diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c index 0ed3e565f8f8..24e2f6f6178b 100644 --- a/net/mptcp/pm.c +++ b/net/mptcp/pm.c @@ -251,8 +251,10 @@ 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 sk_buff *skb, + unsigned int opt_size, unsigned int remaining, + struct mptcp_addr_info *saddr, bool *echo, + bool *port, bool *drop_other_suboptions) { int ret = false; @@ -262,6 +264,15 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining, if (!mptcp_pm_should_add_signal(msk)) goto out_unlock; + /* always drop every other options for pure ack ADD_ADDR; this is a + * plain dup-ack from TCP perspective. The other MPTCP-relevant info, + * if any, will be carried by the 'original' TCP ack + */ + if (skb && skb_is_tcp_pure_ack(skb)) { + remaining += opt_size; + *drop_other_suboptions = true; + } + *echo = mptcp_pm_should_add_signal_echo(msk); *port = mptcp_pm_should_add_signal_port(msk); diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index bc1bfd7ac9c1..40bc9d31e1fa 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -794,8 +794,10 @@ 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 sk_buff *skb, + unsigned int opt_size, unsigned int remaining, + struct mptcp_addr_info *saddr, bool *echo, + bool *port, bool *drop_other_suboptions); 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); From 18fc1a922e2416998c5d37c26c69aab940c07ffb Mon Sep 17 00:00:00 2001 From: Yonglong Li Date: Mon, 23 Aug 2021 18:05:40 -0700 Subject: [PATCH 2/6] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Use MPTCP_ADD_ADDR_SIGNAL only for the action of sending ADD_ADDR, and use MPTCP_ADD_ADDR_ECHO only for the action of sending ADD_ADDR echo. Use msk->pm.local to save the announced ADD_ADDR address only, and reuse msk->pm.remote to save the announced ADD_ADDR_ECHO address. To prepare for the next patch. Co-developed-by: Geliang Tang Signed-off-by: Geliang Tang Signed-off-by: Yonglong Li Signed-off-by: Mat Martineau Signed-off-by: David S. Miller --- net/mptcp/pm.c | 16 ++++++++++------ net/mptcp/pm_netlink.c | 4 ++-- net/mptcp/protocol.h | 6 ++++++ 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c index 24e2f6f6178b..b1727cef1cfd 100644 --- a/net/mptcp/pm.c +++ b/net/mptcp/pm.c @@ -20,19 +20,23 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk, { u8 add_addr = READ_ONCE(msk->pm.addr_signal); - pr_debug("msk=%p, local_id=%d", msk, addr->id); + pr_debug("msk=%p, local_id=%d, echo=%d", msk, addr->id, echo); lockdep_assert_held(&msk->pm.lock); - if (add_addr) { - pr_warn("addr_signal error, add_addr=%d", add_addr); + if (add_addr & + (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) { + pr_warn("addr_signal error, add_addr=%d, echo=%d", add_addr, echo); return -EINVAL; } - msk->pm.local = *addr; - add_addr |= BIT(MPTCP_ADD_ADDR_SIGNAL); - if (echo) + if (echo) { + msk->pm.remote = *addr; add_addr |= BIT(MPTCP_ADD_ADDR_ECHO); + } else { + msk->pm.local = *addr; + add_addr |= BIT(MPTCP_ADD_ADDR_SIGNAL); + } if (addr->family == AF_INET6) add_addr |= BIT(MPTCP_ADD_ADDR_IPV6); if (addr->port) diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c index 480f43ec1bfb..d8dfd872a6dd 100644 --- a/net/mptcp/pm_netlink.c +++ b/net/mptcp/pm_netlink.c @@ -317,14 +317,14 @@ static void mptcp_pm_add_timer(struct timer_list *timer) if (!entry->addr.id) return; - if (mptcp_pm_should_add_signal(msk)) { + if (mptcp_pm_should_add_signal_addr(msk)) { sk_reset_timer(sk, timer, jiffies + TCP_RTO_MAX / 8); goto out; } spin_lock_bh(&msk->pm.lock); - if (!mptcp_pm_should_add_signal(msk)) { + if (!mptcp_pm_should_add_signal_addr(msk)) { pr_debug("retransmit ADD_ADDR id=%d", entry->addr.id); mptcp_pm_announce_addr(msk, &entry->addr, false); mptcp_pm_add_addr_send_ack(msk); diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index 40bc9d31e1fa..3c388c1a9de4 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -747,6 +747,12 @@ void mptcp_event_addr_announced(const struct mptcp_sock *msk, const struct mptcp void mptcp_event_addr_removed(const struct mptcp_sock *msk, u8 id); static inline bool mptcp_pm_should_add_signal(struct mptcp_sock *msk) +{ + return READ_ONCE(msk->pm.addr_signal) & + (BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO)); +} + +static inline bool mptcp_pm_should_add_signal_addr(struct mptcp_sock *msk) { return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_ADD_ADDR_SIGNAL); } From 119c022096f5805680c79dfa74e15044c289856d Mon Sep 17 00:00:00 2001 From: Yonglong Li Date: Mon, 23 Aug 2021 18:05:41 -0700 Subject: [PATCH 3/6] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other ADD_ADDR shares pm.addr_signal with RM_ADDR, so after RM_ADDR/ADD_ADDR has done, we should not clean ADD_ADDR/RM_ADDR's addr_signal. Co-developed-by: Geliang Tang Signed-off-by: Geliang Tang Signed-off-by: Yonglong Li Signed-off-by: Mat Martineau Signed-off-by: David S. Miller --- net/mptcp/pm.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c index b1727cef1cfd..bc03c08eeee5 100644 --- a/net/mptcp/pm.c +++ b/net/mptcp/pm.c @@ -261,6 +261,7 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb, bool *port, bool *drop_other_suboptions) { int ret = false; + u8 add_addr; spin_lock_bh(&msk->pm.lock); @@ -284,7 +285,11 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb, goto out_unlock; *saddr = msk->pm.local; - WRITE_ONCE(msk->pm.addr_signal, 0); + if (*echo) + 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: @@ -296,6 +301,7 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining, struct mptcp_rm_list *rm_list) { int ret = false, len; + u8 rm_addr; spin_lock_bh(&msk->pm.lock); @@ -303,16 +309,17 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining, if (!mptcp_pm_should_rm_signal(msk)) goto out_unlock; + rm_addr = msk->pm.addr_signal & ~BIT(MPTCP_RM_ADDR_SIGNAL); len = mptcp_rm_addr_len(&msk->pm.rm_list_tx); if (len < 0) { - WRITE_ONCE(msk->pm.addr_signal, 0); + WRITE_ONCE(msk->pm.addr_signal, rm_addr); goto out_unlock; } if (remaining < len) goto out_unlock; *rm_list = msk->pm.rm_list_tx; - WRITE_ONCE(msk->pm.addr_signal, 0); + WRITE_ONCE(msk->pm.addr_signal, rm_addr); ret = true; out_unlock: From f462a446384d0c00c6e457f7e8eb2053b095a2f1 Mon Sep 17 00:00:00 2001 From: Yonglong Li Date: Mon, 23 Aug 2021 18:05:42 -0700 Subject: [PATCH 4/6] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal According to the MPTCP_ADD_ADDR_SIGNAL or MPTCP_ADD_ADDR_ECHO flag, build the ADD_ADDR/ADD_ADDR_ECHO option. In mptcp_pm_add_addr_signal(), use opts->addr to save the announced ADD_ADDR or ADD_ADDR_ECHO address. Co-developed-by: Geliang Tang Signed-off-by: Geliang Tang Co-developed-by: Paolo Abeni Signed-off-by: Paolo Abeni Signed-off-by: Yonglong Li Signed-off-by: Mat Martineau Signed-off-by: David S. Miller --- net/mptcp/pm.c | 14 +++++++++----- net/mptcp/protocol.h | 2 +- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c index bc03c08eeee5..f1b520df228a 100644 --- a/net/mptcp/pm.c +++ b/net/mptcp/pm.c @@ -257,11 +257,12 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup) bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb, unsigned int opt_size, unsigned int remaining, - struct mptcp_addr_info *saddr, bool *echo, + struct mptcp_addr_info *addr, bool *echo, bool *port, bool *drop_other_suboptions) { int ret = false; u8 add_addr; + u8 family; spin_lock_bh(&msk->pm.lock); @@ -281,14 +282,17 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb, *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)) + family = *echo ? msk->pm.remote.family : msk->pm.local.family; + if (remaining < mptcp_add_addr_len(family, *echo, *port)) goto out_unlock; - *saddr = msk->pm.local; - if (*echo) + if (*echo) { + *addr = msk->pm.remote; add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO); - else + } else { + *addr = msk->pm.local; add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL); + } WRITE_ONCE(msk->pm.addr_signal, add_addr); ret = true; diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index 3c388c1a9de4..27afacb6fde2 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -802,7 +802,7 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list) bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb, unsigned int opt_size, unsigned int remaining, - struct mptcp_addr_info *saddr, bool *echo, + struct mptcp_addr_info *addr, bool *echo, bool *port, bool *drop_other_suboptions); bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining, struct mptcp_rm_list *rm_list); From c233ef13907038239303a73ca0565bcc3f3373bc Mon Sep 17 00:00:00 2001 From: Yonglong Li Date: Mon, 23 Aug 2021 18:05:43 -0700 Subject: [PATCH 5/6] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT are not necessary, we can get these info from pm.local or pm.remote. Drop mptcp_pm_should_add_signal_ipv6 and mptcp_pm_should_add_signal_port too. Co-developed-by: Geliang Tang Signed-off-by: Geliang Tang Signed-off-by: Yonglong Li Signed-off-by: Mat Martineau Signed-off-by: David S. Miller --- net/mptcp/pm.c | 6 +----- net/mptcp/pm_netlink.c | 6 ++---- net/mptcp/protocol.h | 12 ------------ 3 files changed, 3 insertions(+), 21 deletions(-) diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c index f1b520df228a..da0c4c925350 100644 --- a/net/mptcp/pm.c +++ b/net/mptcp/pm.c @@ -37,10 +37,6 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk, msk->pm.local = *addr; add_addr |= BIT(MPTCP_ADD_ADDR_SIGNAL); } - if (addr->family == AF_INET6) - add_addr |= BIT(MPTCP_ADD_ADDR_IPV6); - if (addr->port) - add_addr |= BIT(MPTCP_ADD_ADDR_PORT); WRITE_ONCE(msk->pm.addr_signal, add_addr); return 0; } @@ -280,7 +276,7 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb, } *echo = mptcp_pm_should_add_signal_echo(msk); - *port = mptcp_pm_should_add_signal_port(msk); + *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port); family = *echo ? msk->pm.remote.family : msk->pm.local.family; if (remaining < mptcp_add_addr_len(family, *echo, *port)) diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c index d8dfd872a6dd..1e4289c507ff 100644 --- a/net/mptcp/pm_netlink.c +++ b/net/mptcp/pm_netlink.c @@ -647,10 +647,8 @@ void mptcp_pm_nl_addr_send_ack(struct mptcp_sock *msk) bool slow; spin_unlock_bh(&msk->pm.lock); - pr_debug("send ack for %s%s%s", - mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr", - mptcp_pm_should_add_signal_ipv6(msk) ? " [ipv6]" : "", - mptcp_pm_should_add_signal_port(msk) ? " [port]" : ""); + pr_debug("send ack for %s", + mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr"); slow = lock_sock_fast(ssk); tcp_send_ack(ssk); diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index 27afacb6fde2..7cd3d5979bcd 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -178,8 +178,6 @@ enum mptcp_pm_status { enum mptcp_addr_signal_status { MPTCP_ADD_ADDR_SIGNAL, MPTCP_ADD_ADDR_ECHO, - MPTCP_ADD_ADDR_IPV6, - MPTCP_ADD_ADDR_PORT, MPTCP_RM_ADDR_SIGNAL, }; @@ -762,16 +760,6 @@ static inline bool mptcp_pm_should_add_signal_echo(struct mptcp_sock *msk) return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_ADD_ADDR_ECHO); } -static inline bool mptcp_pm_should_add_signal_ipv6(struct mptcp_sock *msk) -{ - return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_ADD_ADDR_IPV6); -} - -static inline bool mptcp_pm_should_add_signal_port(struct mptcp_sock *msk) -{ - return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_ADD_ADDR_PORT); -} - static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk) { return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL); From 33c563ad28e3bf614c82450fbf83a7c3c203db87 Mon Sep 17 00:00:00 2001 From: Yonglong Li Date: Mon, 23 Aug 2021 18:05:44 -0700 Subject: [PATCH 6/6] selftests: mptcp: add_addr and echo race test This patch added an extra test for the singal_address_tests() to do the ADD_ADDR and ADD_ADDR_ECHO race test. Co-developed-by: Geliang Tang Signed-off-by: Geliang Tang Signed-off-by: Yonglong Li Signed-off-by: Mat Martineau Signed-off-by: David S. Miller --- tools/testing/selftests/net/mptcp/mptcp_join.sh | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh index 8c7117e2c337..7b3e6cc56935 100755 --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh @@ -1016,6 +1016,21 @@ signal_address_tests() run_tests $ns1 $ns2 10.0.1.1 chk_join_nr "signal invalid addresses" 1 1 1 chk_add_nr 3 3 + + # signal addresses race test + reset + ip netns exec $ns1 ./pm_nl_ctl limits 4 4 + ip netns exec $ns2 ./pm_nl_ctl limits 4 4 + ip netns exec $ns1 ./pm_nl_ctl add 10.0.1.1 flags signal + ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal + ip netns exec $ns1 ./pm_nl_ctl add 10.0.3.1 flags signal + ip netns exec $ns1 ./pm_nl_ctl add 10.0.4.1 flags signal + ip netns exec $ns2 ./pm_nl_ctl add 10.0.1.2 flags signal + ip netns exec $ns2 ./pm_nl_ctl add 10.0.2.2 flags signal + ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags signal + ip netns exec $ns2 ./pm_nl_ctl add 10.0.4.2 flags signal + run_tests $ns1 $ns2 10.0.1.1 + chk_add_nr 4 4 } link_failure_tests()