* [PATCH v5 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process
@ 2021-06-22 4:45 Yonglong Li
2021-06-22 4:45 ` [PATCH v5 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Yonglong Li @ 2021-06-22 4:45 UTC (permalink / raw)
To: mptcp; +Cc: mathew.j.martineau, geliangtang, Yonglong Li
fix issue: ADD_ADDR and RM_ADDR use pm.add_signal to mark event, so
in some case pm.add_signal will be flush when ADD_ADDR/RM_ADDR in
process.
fix issue: if ADD_ADDR and ADD_ADDR-echo process at the same time,
only one event can write pm.add_signal. so ADD_ADDR will process
after add_timer timeout or ADD_ADDR-echo will not be process.
Patch 1 fix ADD_ADDR and RM_ADDR maybe clear addr_signal each other.
Patch 2 and 3 deal ADD_ADDR and ADD_ADDR-echo with separately to fix
conflicts in using pm.addr_signal porcess.
Patch 4 MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT is not necessary.
v1->v2:
- remove READ_ONCE under the pm spin lock.
v2->v3:
- Patch 2: rename mptcp_pm_should_add_addr to mptcp_pm_should_add_signal_addr
- Patch 3: avoid read-modify-write of msk->pm.addr_signal and change
mptcp_pm_add_addr_signal to return void.
v3->v4:
- Patch 1: use ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO)) instead
of BIT(MPTCP_RM_ADDR_SIGNAL)
- Patch 3: simple the code; init flags; fix wrong goto logic code;
v4->v5:
- Patch 3: simple the code of function 'mptcp_established_options_add_addr'
Yonglong Li (4):
mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
include/net/mptcp.h | 1 +
net/mptcp/options.c | 161 ++++++++++++++++++++++++++++++++-----------------
net/mptcp/pm.c | 53 +++++++---------
net/mptcp/pm_netlink.c | 10 ++-
net/mptcp/protocol.h | 31 ++++------
5 files changed, 147 insertions(+), 109 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
2021-06-22 4:45 [PATCH v5 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
@ 2021-06-22 4:45 ` Yonglong Li
2021-06-22 4:45 ` [PATCH v5 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
` (3 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Yonglong Li @ 2021-06-22 4:45 UTC (permalink / raw)
To: mptcp; +Cc: mathew.j.martineau, geliangtang, Yonglong Li
ADD_ADDR share pm.addr_signal with RM_ADDR, so after RM_ADDR/ADD_ADDR
done we should not clean ADD_ADDR/RM_ADDR's addr_signal.
Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
net/mptcp/pm.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 9d00fa6..6c427c8 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -252,6 +252,7 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
struct mptcp_addr_info *saddr, bool *echo, bool *port)
{
+ u8 add_addr;
int ret = false;
spin_lock_bh(&msk->pm.lock);
@@ -267,7 +268,8 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
goto out_unlock;
*saddr = msk->pm.local;
- WRITE_ONCE(msk->pm.addr_signal, 0);
+ 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;
out_unlock:
@@ -278,6 +280,7 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
struct mptcp_rm_list *rm_list)
{
+ u8 rm_addr;
int ret = false, len;
spin_lock_bh(&msk->pm.lock);
@@ -286,16 +289,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:
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v5 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
2021-06-22 4:45 [PATCH v5 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
2021-06-22 4:45 ` [PATCH v5 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
@ 2021-06-22 4:45 ` Yonglong Li
2021-06-25 10:33 ` Geliang Tang
2021-06-22 4:45 ` [PATCH v5 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Yonglong Li @ 2021-06-22 4:45 UTC (permalink / raw)
To: mptcp; +Cc: mathew.j.martineau, geliangtang, Yonglong Li
MPTCP_ADD_ADDR_SIGNAL only for action of sending ADD_ADDR
MPTCP_ADD_ADDR_ECHO only for action of sending echo ADD_ADDR
add a mptcp_addr_info in struct mptcp_out_options for echo ADD_ADDR
to prepare for the next patch.
Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
include/net/mptcp.h | 1 +
net/mptcp/pm.c | 13 ++++++++-----
net/mptcp/pm_netlink.c | 4 ++--
net/mptcp/protocol.h | 6 ++++++
4 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index d61bbbf..637e90b 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -62,6 +62,7 @@ struct mptcp_out_options {
u64 rcvr_key;
u64 ahmac;
struct mptcp_addr_info addr;
+ struct mptcp_addr_info remote;
struct mptcp_rm_list rm_list;
u8 join_id;
u8 backup;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 6c427c8..107a5a2 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -18,7 +18,7 @@ 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);
@@ -27,10 +27,13 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
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)
@@ -214,7 +217,7 @@ void mptcp_pm_add_addr_echoed(struct mptcp_sock *msk,
void mptcp_pm_add_addr_send_ack(struct mptcp_sock *msk)
{
- if (!mptcp_pm_should_add_signal(msk))
+ if (!mptcp_pm_should_add_signal_echo(msk))
return;
mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index d4732a4..0f302d2 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 160c2ab..a0b0ec0 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -708,6 +708,12 @@ void mptcp_event(enum mptcp_event_type type, const struct mptcp_sock *msk,
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);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v5 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-22 4:45 [PATCH v5 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
2021-06-22 4:45 ` [PATCH v5 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
2021-06-22 4:45 ` [PATCH v5 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
@ 2021-06-22 4:45 ` Yonglong Li
2021-06-25 4:44 ` Geliang Tang
` (3 more replies)
2021-06-22 4:45 ` [PATCH v5 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT Yonglong Li
2021-06-25 0:28 ` [PATCH v5 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Mat Martineau
4 siblings, 4 replies; 14+ messages in thread
From: Yonglong Li @ 2021-06-22 4:45 UTC (permalink / raw)
To: mptcp; +Cc: mathew.j.martineau, geliangtang, Yonglong Li
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 | 2 +-
net/mptcp/options.c | 105 ++++++++++++++++++++++++++++++---------------------
net/mptcp/pm.c | 30 +++++----------
net/mptcp/protocol.h | 13 ++++---
4 files changed, 80 insertions(+), 70 deletions(-)
diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 637e90b..d2c6ebe 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -61,7 +61,7 @@ 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;
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 1aec016..a1fafed 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -655,13 +655,19 @@ 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;
+ struct mptcp_addr_info remote;
+ struct mptcp_addr_info local;
+ 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_should_add_signal(msk))
+ return false;
+
+ mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
+ if ((mptcp_pm_should_add_signal_echo(msk) ||
+ (!mptcp_pm_should_add_signal_echo(msk) &&
+ mptcp_pm_should_add_signal_addr(msk) &&
+ (local.family == AF_INET6 || local.port))) &&
skb && skb_is_tcp_pure_ack(skb)) {
pr_debug("drop other suboptions");
opts->suboptions = 0;
@@ -671,25 +677,35 @@ 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 (remaining < len)
- return false;
+ if (mptcp_pm_should_add_signal_echo(msk)) {
+ len = mptcp_add_addr_len(remote.family, true, !!remote.port);
+ if (remaining < len)
+ return false;
+ opts->remote = remote;
+ flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
+ opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
+ } else {
+ len = mptcp_add_addr_len(local.family, false, !!local.port);
+ if (remaining < len)
+ return false;
+ opts->local = local;
+ opts->ahmac = add_addr_generate_hmac(msk->local_key,
+ msk->remote_key,
+ &opts->local);
+ opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
+ flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
+ }
*size = len;
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, mptcp_pm_should_add_signal_echo(msk), opts->local.id,
+ opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
return true;
}
@@ -1228,45 +1244,51 @@ 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;
+ if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
+ struct mptcp_addr_info *addr_info;
+ u8 len = 0;
+ u8 echo = 0;
+
+ if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
+ len += sizeof(opts->ahmac);
+ addr_info = &opts->local;
+ } else {
+ echo = MPTCP_ADDR_ECHO;
+ addr_info = &opts->remote;
+ }
#if IS_ENABLED(CONFIG_MPTCP_IPV6)
- if (opts->addr.family == AF_INET6)
- len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
+ if (addr_info->family == AF_INET6)
+ len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
+ else
#endif
+ len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
- if (opts->addr.port)
+ if (addr_info->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) {
- memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
+ len, echo, addr_info->id);
+ if (addr_info->family == AF_INET) {
+ memcpy((u8 *)ptr, (u8 *)&addr_info->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_info->family == AF_INET6) {
+ memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
ptr += 4;
}
#endif
- if (!opts->addr.port) {
- if (opts->ahmac) {
+ if (!addr_info->port) {
+ if (!echo) {
put_unaligned_be64(opts->ahmac, ptr);
ptr += 2;
}
} else {
- u16 port = ntohs(opts->addr.port);
+ u16 port = ntohs(addr_info->port);
- if (opts->ahmac) {
+ if (!echo) {
u8 *bptr = (u8 *)ptr;
put_unaligned_be16(port, bptr);
@@ -1275,7 +1297,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
bptr += 8;
put_unaligned_be16(TCPOPT_NOP << 8 |
TCPOPT_NOP, bptr);
-
ptr += 3;
} else {
put_unaligned_be32(port << 16 |
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 107a5a2..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_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
- 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 related [flat|nested] 14+ messages in thread
* [PATCH v5 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
2021-06-22 4:45 [PATCH v5 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
` (2 preceding siblings ...)
2021-06-22 4:45 ` [PATCH v5 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
@ 2021-06-22 4:45 ` Yonglong Li
2021-06-25 10:01 ` Geliang Tang
2021-06-25 0:28 ` [PATCH v5 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Mat Martineau
4 siblings, 1 reply; 14+ messages in thread
From: Yonglong Li @ 2021-06-22 4:45 UTC (permalink / raw)
To: mptcp; +Cc: mathew.j.martineau, geliangtang, Yonglong Li
there not need MPTCP_ADD_ADDR_PORT and MPTCP_ADD_ADDR_PORT, we can
get these info from pm.addr or pm.remote
Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
net/mptcp/pm.c | 4 ----
net/mptcp/pm_netlink.c | 6 ++----
net/mptcp/protocol.h | 12 ------------
3 files changed, 2 insertions(+), 20 deletions(-)
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index a62d4a5..f051e48 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -35,10 +35,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;
}
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 0f302d2..bfa9d6d 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -543,10 +543,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 90fb532..71e747c 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -176,8 +176,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,
};
@@ -723,16 +721,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);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v5 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process
2021-06-22 4:45 [PATCH v5 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
` (3 preceding siblings ...)
2021-06-22 4:45 ` [PATCH v5 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT Yonglong Li
@ 2021-06-25 0:28 ` Mat Martineau
2021-06-25 1:47 ` Yonglong Li
4 siblings, 1 reply; 14+ messages in thread
From: Mat Martineau @ 2021-06-25 0:28 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, geliangtang
On Tue, 22 Jun 2021, Yonglong Li wrote:
> fix issue: ADD_ADDR and RM_ADDR use pm.add_signal to mark event, so
> in some case pm.add_signal will be flush when ADD_ADDR/RM_ADDR in
> process.
>
> fix issue: if ADD_ADDR and ADD_ADDR-echo process at the same time,
> only one event can write pm.add_signal. so ADD_ADDR will process
> after add_timer timeout or ADD_ADDR-echo will not be process.
>
> Patch 1 fix ADD_ADDR and RM_ADDR maybe clear addr_signal each other.
>
> Patch 2 and 3 deal ADD_ADDR and ADD_ADDR-echo with separately to fix
> conflicts in using pm.addr_signal porcess.
>
> Patch 4 MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT is not necessary.
>
> v1->v2:
> - remove READ_ONCE under the pm spin lock.
>
> v2->v3:
> - Patch 2: rename mptcp_pm_should_add_addr to mptcp_pm_should_add_signal_addr
> - Patch 3: avoid read-modify-write of msk->pm.addr_signal and change
> mptcp_pm_add_addr_signal to return void.
>
> v3->v4:
> - Patch 1: use ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO)) instead
> of BIT(MPTCP_RM_ADDR_SIGNAL)
> - Patch 3: simple the code; init flags; fix wrong goto logic code;
>
> v4->v5:
> - Patch 3: simple the code of function 'mptcp_established_options_add_addr'
Hi Yonglong -
Thank you for all of your work on this patch series! I think it is close
to being ready, in terms of code review I don't have any changes to
suggest.
When I run the ADD_ADDR timeout test cases (mptcp_join.sh -t), I'm seeing
test case failures on every run because fewer ADD_ADDRs were received than
were expected.
Here's one run:
"""
[mjmartin@mjmartin-nucvm01 mptcp]$ sudo ./mptcp_join.sh -t -c
Created /tmp/tmp.SoSi2pHquT (size 1 KB) containing data sent by client
Created /tmp/tmp.CGf8pIwMGf (size 1 KB) containing data sent by server
Capturing traffic for test 1 into mp_join-01-ns1-0-kHO27s.pcap
tcpdump: data link type LINUX_SLL2
dropped privs to mjmartin
tcpdump: listening on any, link-type LINUX_SLL2 (Linux cooked v2),
snapshot length 65535 bytes
149 packets captured
149 packets received by filter
0 packets dropped by kernel
01 signal address, ADD_ADDR timeout syn[ ok ] - synack[ ok ] - ack[ ok ]
add[ ok ] - echo [ ok ]
Capturing traffic for test 2 into mp_join-02-ns1-0-MXTUAg.pcap
tcpdump: data link type LINUX_SLL2
dropped privs to mjmartin
tcpdump: listening on any, link-type LINUX_SLL2 (Linux cooked v2),
snapshot length 65535 bytes
150 packets captured
150 packets received by filter
0 packets dropped by kernel
02 signal address, ADD_ADDR6 timeout syn[ ok ] - synack[ ok ] - ack[ ok ]
add[fail] got 2 ADD_ADDR[s] expected 4
- echo [ ok ]
Server ns stats
MPTcpExtMPCapableSYNRX 1 0.0
MPTcpExtMPCapableACKRX 1 0.0
MPTcpExtMPJoinSynRx 1 0.0
MPTcpExtMPJoinAckRx 1 0.0
Client ns stats
MPTcpExtMPCapableSYNTX 1 0.0
MPTcpExtMPCapableSYNACKRX 1 0.0
MPTcpExtMPJoinSynAckRx 1 0.0
MPTcpExtAddAddr 2 0.0
Capturing traffic for test 3 into mp_join-03-ns1-0-J4cBbN.pcap
tcpdump: data link type LINUX_SLL2
dropped privs to mjmartin
tcpdump: listening on any, link-type LINUX_SLL2 (Linux cooked v2), snapshot length 65535 bytes
432 packets captured
432 packets received by filter
0 packets dropped by kernel
03 signal addresses, ADD_ADDR timeout syn[ ok ] - synack[ ok ] - ack[ ok ]
add[ ok ] - echo [ ok ]
Capturing traffic for test 4 into mp_join-04-ns1-0-0DJYdc.pcap
tcpdump: data link type LINUX_SLL2
dropped privs to mjmartin
tcpdump: listening on any, link-type LINUX_SLL2 (Linux cooked v2),
snapshot length 65535 bytes
415 packets captured
415 packets received by filter
0 packets dropped by kernel
04 invalid address, ADD_ADDR timeout syn[ ok ] - synack[ ok ] - ack[ ok ]
add[fail] got 7 ADD_ADDR[s] expected 8
- echo [ ok ]
Server ns stats
MPTcpExtMPCapableSYNRX 1 0.0
MPTcpExtMPCapableACKRX 1 0.0
MPTcpExtMPJoinSynRx 1 0.0
MPTcpExtMPJoinAckRx 1 0.0
Client ns stats
MPTcpExtMPCapableSYNTX 1 0.0
MPTcpExtMPCapableSYNACKRX 1 0.0
MPTcpExtMPJoinSynAckRx 1 0.0
MPTcpExtAddAddr 7 0.0
"""
If you run the tests with -c (mptcp_join.sh -t -c) you can review the
.pcap files to see if the new behavior is expected.
Please see if you can replicate the test results above (I've seen
different combinations of these tests succeed and fail). If I'm not the
only person seeing the failures, either the tests or the code need to be
updated.
Best regards,
Mat
>
> Yonglong Li (4):
> mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
> mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
> mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
> mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
>
> include/net/mptcp.h | 1 +
> net/mptcp/options.c | 161 ++++++++++++++++++++++++++++++++-----------------
> net/mptcp/pm.c | 53 +++++++---------
> net/mptcp/pm_netlink.c | 10 ++-
> net/mptcp/protocol.h | 31 ++++------
> 5 files changed, 147 insertions(+), 109 deletions(-)
>
> --
> 1.8.3.1
>
>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process
2021-06-25 0:28 ` [PATCH v5 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Mat Martineau
@ 2021-06-25 1:47 ` Yonglong Li
0 siblings, 0 replies; 14+ messages in thread
From: Yonglong Li @ 2021-06-25 1:47 UTC (permalink / raw)
To: Mat Martineau; +Cc: mptcp, geliangtang
On 2021/6/25 8:28, Mat Martineau wrote:
> On Tue, 22 Jun 2021, Yonglong Li wrote:
>
>> fix issue: ADD_ADDR and RM_ADDR use pm.add_signal to mark event, so
>> in some case pm.add_signal will be flush when ADD_ADDR/RM_ADDR in
>> process.
>>
>> fix issue: if ADD_ADDR and ADD_ADDR-echo process at the same time,
>> only one event can write pm.add_signal. so ADD_ADDR will process
>> after add_timer timeout or ADD_ADDR-echo will not be process.
>>
>> Patch 1 fix ADD_ADDR and RM_ADDR maybe clear addr_signal each other.
>>
>> Patch 2 and 3 deal ADD_ADDR and ADD_ADDR-echo with separately to fix
>> conflicts in using pm.addr_signal porcess.
>>
>> Patch 4 MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT is not necessary.
>>
>> v1->v2:
>> - remove READ_ONCE under the pm spin lock.
>>
>> v2->v3:
>> - Patch 2: rename mptcp_pm_should_add_addr to mptcp_pm_should_add_signal_addr
>> - Patch 3: avoid read-modify-write of msk->pm.addr_signal and change
>> mptcp_pm_add_addr_signal to return void.
>>
>> v3->v4:
>> - Patch 1: use ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO)) instead
>> of BIT(MPTCP_RM_ADDR_SIGNAL)
>> - Patch 3: simple the code; init flags; fix wrong goto logic code;
>>
>> v4->v5:
>> - Patch 3: simple the code of function 'mptcp_established_options_add_addr'
>
> Hi Yonglong -
>
> Thank you for all of your work on this patch series! I think it is close to being ready, in terms of code review I don't have any changes to suggest.
>
> When I run the ADD_ADDR timeout test cases (mptcp_join.sh -t), I'm seeing test case failures on every run because fewer ADD_ADDRs were received than were expected.
>
> Here's one run:
>
> """
> [mjmartin@mjmartin-nucvm01 mptcp]$ sudo ./mptcp_join.sh -t -c
> Created /tmp/tmp.SoSi2pHquT (size 1 KB) containing data sent by client
> Created /tmp/tmp.CGf8pIwMGf (size 1 KB) containing data sent by server
> Capturing traffic for test 1 into mp_join-01-ns1-0-kHO27s.pcap
> tcpdump: data link type LINUX_SLL2
> dropped privs to mjmartin
> tcpdump: listening on any, link-type LINUX_SLL2 (Linux cooked v2), snapshot length 65535 bytes
> 149 packets captured
> 149 packets received by filter
> 0 packets dropped by kernel
> 01 signal address, ADD_ADDR timeout syn[ ok ] - synack[ ok ] - ack[ ok ]
> add[ ok ] - echo [ ok ]
> Capturing traffic for test 2 into mp_join-02-ns1-0-MXTUAg.pcap
> tcpdump: data link type LINUX_SLL2
> dropped privs to mjmartin
> tcpdump: listening on any, link-type LINUX_SLL2 (Linux cooked v2), snapshot length 65535 bytes
> 150 packets captured
> 150 packets received by filter
> 0 packets dropped by kernel
> 02 signal address, ADD_ADDR6 timeout syn[ ok ] - synack[ ok ] - ack[ ok ]
> add[fail] got 2 ADD_ADDR[s] expected 4
> - echo [ ok ]
> Server ns stats
> MPTcpExtMPCapableSYNRX 1 0.0
> MPTcpExtMPCapableACKRX 1 0.0
> MPTcpExtMPJoinSynRx 1 0.0
> MPTcpExtMPJoinAckRx 1 0.0
> Client ns stats
> MPTcpExtMPCapableSYNTX 1 0.0
> MPTcpExtMPCapableSYNACKRX 1 0.0
> MPTcpExtMPJoinSynAckRx 1 0.0
> MPTcpExtAddAddr 2 0.0
> Capturing traffic for test 3 into mp_join-03-ns1-0-J4cBbN.pcap
> tcpdump: data link type LINUX_SLL2
> dropped privs to mjmartin
> tcpdump: listening on any, link-type LINUX_SLL2 (Linux cooked v2), snapshot length 65535 bytes
> 432 packets captured
> 432 packets received by filter
> 0 packets dropped by kernel
> 03 signal addresses, ADD_ADDR timeout syn[ ok ] - synack[ ok ] - ack[ ok ]
> add[ ok ] - echo [ ok ]
> Capturing traffic for test 4 into mp_join-04-ns1-0-0DJYdc.pcap
> tcpdump: data link type LINUX_SLL2
> dropped privs to mjmartin
> tcpdump: listening on any, link-type LINUX_SLL2 (Linux cooked v2), snapshot length 65535 bytes
> 415 packets captured
> 415 packets received by filter
> 0 packets dropped by kernel
> 04 invalid address, ADD_ADDR timeout syn[ ok ] - synack[ ok ] - ack[ ok ]
> add[fail] got 7 ADD_ADDR[s] expected 8
> - echo [ ok ]
> Server ns stats
> MPTcpExtMPCapableSYNRX 1 0.0
> MPTcpExtMPCapableACKRX 1 0.0
> MPTcpExtMPJoinSynRx 1 0.0
> MPTcpExtMPJoinAckRx 1 0.0
> Client ns stats
> MPTcpExtMPCapableSYNTX 1 0.0
> MPTcpExtMPCapableSYNACKRX 1 0.0
> MPTcpExtMPJoinSynAckRx 1 0.0
> MPTcpExtAddAddr 7 0.0
> """
>
> If you run the tests with -c (mptcp_join.sh -t -c) you can review the .pcap files to see if the new behavior is expected.
>
> Please see if you can replicate the test results above (I've seen different combinations of these tests succeed and fail). If I'm not the only person seeing the failures, either the tests or the code need to be updated.
>
>
> Best regards,
>
> Mat
>
Thanks for your patience and suggestions.
I will check the test cases.
>
>
>>
>> Yonglong Li (4):
>> mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
>> mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
>> mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
>> mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
>>
>> include/net/mptcp.h | 1 +
>> net/mptcp/options.c | 161 ++++++++++++++++++++++++++++++++-----------------
>> net/mptcp/pm.c | 53 +++++++---------
>> net/mptcp/pm_netlink.c | 10 ++-
>> net/mptcp/protocol.h | 31 ++++------
>> 5 files changed, 147 insertions(+), 109 deletions(-)
>>
>> --
>> 1.8.3.1
>>
>>
>
> --
> Mat Martineau
> Intel
>
--
Li YongLong
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-22 4:45 ` [PATCH v5 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
@ 2021-06-25 4:44 ` Geliang Tang
2021-06-25 9:43 ` Yonglong Li
2021-06-25 10:39 ` Geliang Tang
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Geliang Tang @ 2021-06-25 4:44 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau
Hi Yonglong,
Thank you for this new patch!
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月22日周二 下午12:46写道：
>
> 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 | 2 +-
> net/mptcp/options.c | 105 ++++++++++++++++++++++++++++++---------------------
> net/mptcp/pm.c | 30 +++++----------
> net/mptcp/protocol.h | 13 ++++---
> 4 files changed, 80 insertions(+), 70 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 637e90b..d2c6ebe 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -61,7 +61,7 @@ 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;
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..a1fafed 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,13 +655,19 @@ 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;
> + struct mptcp_addr_info remote;
> + struct mptcp_addr_info local;
> + 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_should_add_signal(msk))
> + return false;
> +
> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
Could we check the return value of mptcp_pm_add_addr_signal as the original
code:
if (!mptcp_pm_should_add_signal(msk) ||
!(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr,
&echo, &port)))
return false;
> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> + (!mptcp_pm_should_add_signal_echo(msk) &&
> + mptcp_pm_should_add_signal_addr(msk) &&
> + (local.family == AF_INET6 || local.port))) &&
> skb && skb_is_tcp_pure_ack(skb)) {
> pr_debug("drop other suboptions");
> opts->suboptions = 0;
> @@ -671,25 +677,35 @@ 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 (remaining < len)
> - return false;
> + if (mptcp_pm_should_add_signal_echo(msk)) {
> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
Could we pass 'opts' as the only argument to mptcp_add_addr_len, and use
mptcp_pm_should_add_signal_echo in mptcp_add_addr_len to check whether
it's a ADD_ADDR_ECHO?
> + if (remaining < len)
> + return false;
Then we can move these lines out of the if... else... trunk.
> + opts->remote = remote;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
I prefer to change the order of these three lines to:
opts->remote = remote;
opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> + } else {
> + len = mptcp_add_addr_len(local.family, false, !!local.port);
> + if (remaining < len)
> + return false;
> + opts->local = local;
> + opts->ahmac = add_addr_generate_hmac(msk->local_key,
> + msk->remote_key,
> + &opts->local);
> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
And here I prefer to use the same order as the if trunk:
opts->local = local;
opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
opts->ahmac = add_addr_generate_hmac(msk->local_key,
msk->remote_key,
&opts->local);
flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> + }
>
> *size = len;
> 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, mptcp_pm_should_add_signal_echo(msk), opts->local.id,
> + opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>
> return true;
> }
> @@ -1228,45 +1244,51 @@ 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;
> + if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
> + struct mptcp_addr_info *addr_info;
> + u8 len = 0;
> + u8 echo = 0;
> +
> + if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> + len += sizeof(opts->ahmac);
> + addr_info = &opts->local;
> + } else {
> + echo = MPTCP_ADDR_ECHO;
> + addr_info = &opts->remote;
> + }
>
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> - if (opts->addr.family == AF_INET6)
> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + if (addr_info->family == AF_INET6)
> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + else
> #endif
> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>
> - if (opts->addr.port)
> + if (addr_info->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) {
> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> + len, echo, addr_info->id);
> + if (addr_info->family == AF_INET) {
> + memcpy((u8 *)ptr, (u8 *)&addr_info->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_info->family == AF_INET6) {
> + memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
> ptr += 4;
> }
> #endif
>
> - if (!opts->addr.port) {
> - if (opts->ahmac) {
> + if (!addr_info->port) {
> + if (!echo) {
> put_unaligned_be64(opts->ahmac, ptr);
> ptr += 2;
> }
> } else {
> - u16 port = ntohs(opts->addr.port);
> + u16 port = ntohs(addr_info->port);
>
> - if (opts->ahmac) {
> + if (!echo) {
> u8 *bptr = (u8 *)ptr;
>
> put_unaligned_be16(port, bptr);
> @@ -1275,7 +1297,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> bptr += 8;
> put_unaligned_be16(TCPOPT_NOP << 8 |
> TCPOPT_NOP, bptr);
> -
> ptr += 3;
> } else {
> put_unaligned_be32(port << 16 |
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 107a5a2..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)
Could we keep the return value as bool here?
And pass 'opts' as an argument of this function, instead of using two
arguments 'saddr' and 'daddr'.
> {
> - 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;
Could we keep these double check codes here?
> -
> - *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;
Use 'opts->local = msk->pm.local' here...
> - 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;
> + *daddr = msk->pm.remote;
And 'opts->remote = msk->pm.remote' here.
WDYT?
-Geliang
> + *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] 14+ messages in thread
* Re: [PATCH v5 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-25 4:44 ` Geliang Tang
@ 2021-06-25 9:43 ` Yonglong Li
0 siblings, 0 replies; 14+ messages in thread
From: Yonglong Li @ 2021-06-25 9:43 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp, Mat Martineau
Hi Geliang,
Thanks for your review. I will prepare v6 as your suggestion.
On 2021/6/25 12:44, Geliang Tang wrote:
> Hi Yonglong,
>
> Thank you for this new patch!
>
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月22日周二 下午12:46写道：
>>
>> 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 | 2 +-
>> net/mptcp/options.c | 105 ++++++++++++++++++++++++++++++---------------------
>> net/mptcp/pm.c | 30 +++++----------
>> net/mptcp/protocol.h | 13 ++++---
>> 4 files changed, 80 insertions(+), 70 deletions(-)
>>
>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
>> index 637e90b..d2c6ebe 100644
>> --- a/include/net/mptcp.h
>> +++ b/include/net/mptcp.h
>> @@ -61,7 +61,7 @@ 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;
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index 1aec016..a1fafed 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -655,13 +655,19 @@ 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;
>> + struct mptcp_addr_info remote;
>> + struct mptcp_addr_info local;
>> + 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_should_add_signal(msk))
>> + return false;
>> +
>> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
>
> Could we check the return value of mptcp_pm_add_addr_signal as the original
> code:
>
> if (!mptcp_pm_should_add_signal(msk) ||
> !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr,
> &echo, &port)))
> return false;
>
>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
>> + (!mptcp_pm_should_add_signal_echo(msk) &&
>> + mptcp_pm_should_add_signal_addr(msk) &&
>> + (local.family == AF_INET6 || local.port))) &&
>> skb && skb_is_tcp_pure_ack(skb)) {
>> pr_debug("drop other suboptions");
>> opts->suboptions = 0;
>> @@ -671,25 +677,35 @@ 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 (remaining < len)
>> - return false;
>> + if (mptcp_pm_should_add_signal_echo(msk)) {
>> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
>
> Could we pass 'opts' as the only argument to mptcp_add_addr_len, and use
> mptcp_pm_should_add_signal_echo in mptcp_add_addr_len to check whether
> it's a ADD_ADDR_ECHO?
>
>> + if (remaining < len)
>> + return false;
>
> Then we can move these lines out of the if... else... trunk.
>
>> + opts->remote = remote;
>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
>
> I prefer to change the order of these three lines to:
>
> opts->remote = remote;
> opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>
>> + } else {
>> + len = mptcp_add_addr_len(local.family, false, !!local.port);
>> + if (remaining < len)
>> + return false;
>> + opts->local = local;
>> + opts->ahmac = add_addr_generate_hmac(msk->local_key,
>> + msk->remote_key,
>> + &opts->local);
>> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>
> And here I prefer to use the same order as the if trunk:
>
> opts->local = local;
> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> opts->ahmac = add_addr_generate_hmac(msk->local_key,
> msk->remote_key,
> &opts->local);
> flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>
>> + }
>>
>> *size = len;
>> 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, mptcp_pm_should_add_signal_echo(msk), opts->local.id,
>> + opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>>
>> return true;
>> }
>> @@ -1228,45 +1244,51 @@ 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;
>> + if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
>> + struct mptcp_addr_info *addr_info;
>> + u8 len = 0;
>> + u8 echo = 0;
>> +
>> + if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
>> + len += sizeof(opts->ahmac);
>> + addr_info = &opts->local;
>> + } else {
>> + echo = MPTCP_ADDR_ECHO;
>> + addr_info = &opts->remote;
>> + }
>>
>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>> - if (opts->addr.family == AF_INET6)
>> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>> + if (addr_info->family == AF_INET6)
>> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>> + else
>> #endif
>> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>
>> - if (opts->addr.port)
>> + if (addr_info->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) {
>> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
>> + len, echo, addr_info->id);
>> + if (addr_info->family == AF_INET) {
>> + memcpy((u8 *)ptr, (u8 *)&addr_info->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_info->family == AF_INET6) {
>> + memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
>> ptr += 4;
>> }
>> #endif
>>
>> - if (!opts->addr.port) {
>> - if (opts->ahmac) {
>> + if (!addr_info->port) {
>> + if (!echo) {
>> put_unaligned_be64(opts->ahmac, ptr);
>> ptr += 2;
>> }
>> } else {
>> - u16 port = ntohs(opts->addr.port);
>> + u16 port = ntohs(addr_info->port);
>>
>> - if (opts->ahmac) {
>> + if (!echo) {
>> u8 *bptr = (u8 *)ptr;
>>
>> put_unaligned_be16(port, bptr);
>> @@ -1275,7 +1297,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>> bptr += 8;
>> put_unaligned_be16(TCPOPT_NOP << 8 |
>> TCPOPT_NOP, bptr);
>> -
>> ptr += 3;
>> } else {
>> put_unaligned_be32(port << 16 |
>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>> index 107a5a2..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)
>
> Could we keep the return value as bool here?
>
> And pass 'opts' as an argument of this function, instead of using two
> arguments 'saddr' and 'daddr'.
>
>> {
>> - 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;
>
> Could we keep these double check codes here?
>
>> -
>> - *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;
>
> Use 'opts->local = msk->pm.local' here...
>
>> - 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;
>> + *daddr = msk->pm.remote;
>
> And 'opts->remote = msk->pm.remote' here.
>
> WDYT?
>
> -Geliang
>
>> + *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
>>
>
--
Li YongLong
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
2021-06-22 4:45 ` [PATCH v5 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT Yonglong Li
@ 2021-06-25 10:01 ` Geliang Tang
0 siblings, 0 replies; 14+ messages in thread
From: Geliang Tang @ 2021-06-25 10:01 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月22日周二 下午12:46写道：
>
> there not need MPTCP_ADD_ADDR_PORT and MPTCP_ADD_ADDR_PORT, we can
> get these info from pm.addr or pm.remote
There's no pm.addr, do you mean pm.local?
>
> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> ---
> net/mptcp/pm.c | 4 ----
> net/mptcp/pm_netlink.c | 6 ++----
> net/mptcp/protocol.h | 12 ------------
> 3 files changed, 2 insertions(+), 20 deletions(-)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index a62d4a5..f051e48 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -35,10 +35,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;
> }
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 0f302d2..bfa9d6d 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -543,10 +543,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 90fb532..71e747c 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -176,8 +176,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,
> };
>
> @@ -723,16 +721,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);
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
2021-06-22 4:45 ` [PATCH v5 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
@ 2021-06-25 10:33 ` Geliang Tang
0 siblings, 0 replies; 14+ messages in thread
From: Geliang Tang @ 2021-06-25 10:33 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月22日周二 下午12:45写道：
>
> MPTCP_ADD_ADDR_SIGNAL only for action of sending ADD_ADDR
> MPTCP_ADD_ADDR_ECHO only for action of sending echo ADD_ADDR
> add a mptcp_addr_info in struct mptcp_out_options for echo ADD_ADDR
>
> to prepare for the next patch.
>
> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> ---
> include/net/mptcp.h | 1 +
> net/mptcp/pm.c | 13 ++++++++-----
> net/mptcp/pm_netlink.c | 4 ++--
> net/mptcp/protocol.h | 6 ++++++
> 4 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index d61bbbf..637e90b 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -62,6 +62,7 @@ struct mptcp_out_options {
> u64 rcvr_key;
> u64 ahmac;
> struct mptcp_addr_info addr;
> + struct mptcp_addr_info remote;
> struct mptcp_rm_list rm_list;
> u8 join_id;
> u8 backup;
This new struct member 'remote' isn't used in this patch, I think it's better
to add it in the next patch. So move this trunk into patch 3/4 "mptcp: build
ADD_ADDR/echo-ADD_ADDR option according pm.add_signal".
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 6c427c8..107a5a2 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -18,7 +18,7 @@ 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);
>
> @@ -27,10 +27,13 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
> 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)
> @@ -214,7 +217,7 @@ void mptcp_pm_add_addr_echoed(struct mptcp_sock *msk,
>
> void mptcp_pm_add_addr_send_ack(struct mptcp_sock *msk)
> {
> - if (!mptcp_pm_should_add_signal(msk))
> + if (!mptcp_pm_should_add_signal_echo(msk))
> return;
>
> mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index d4732a4..0f302d2 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 160c2ab..a0b0ec0 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -708,6 +708,12 @@ void mptcp_event(enum mptcp_event_type type, const struct mptcp_sock *msk,
>
> 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);
> }
>
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-22 4:45 ` [PATCH v5 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
2021-06-25 4:44 ` Geliang Tang
@ 2021-06-25 10:39 ` Geliang Tang
2021-06-25 11:43 ` Geliang Tang
2021-06-25 12:29 ` Geliang Tang
3 siblings, 0 replies; 14+ messages in thread
From: Geliang Tang @ 2021-06-25 10:39 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月22日周二 下午12:46写道：
>
> 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 | 2 +-
> net/mptcp/options.c | 105 ++++++++++++++++++++++++++++++---------------------
> net/mptcp/pm.c | 30 +++++----------
> net/mptcp/protocol.h | 13 ++++---
> 4 files changed, 80 insertions(+), 70 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 637e90b..d2c6ebe 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -61,7 +61,7 @@ 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;
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..a1fafed 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,13 +655,19 @@ 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;
> + struct mptcp_addr_info remote;
> + struct mptcp_addr_info local;
> + 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_should_add_signal(msk))
> + return false;
> +
> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> + (!mptcp_pm_should_add_signal_echo(msk) &&
> + mptcp_pm_should_add_signal_addr(msk) &&
> + (local.family == AF_INET6 || local.port))) &&
> skb && skb_is_tcp_pure_ack(skb)) {
> pr_debug("drop other suboptions");
> opts->suboptions = 0;
> @@ -671,25 +677,35 @@ 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 (remaining < len)
> - return false;
> + if (mptcp_pm_should_add_signal_echo(msk)) {
> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> + if (remaining < len)
> + return false;
> + opts->remote = remote;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> + } else {
> + len = mptcp_add_addr_len(local.family, false, !!local.port);
> + if (remaining < len)
> + return false;
> + opts->local = local;
> + opts->ahmac = add_addr_generate_hmac(msk->local_key,
> + msk->remote_key,
> + &opts->local);
> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> + }
>
> *size = len;
> 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, mptcp_pm_should_add_signal_echo(msk), opts->local.id,
> + opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>
> return true;
> }
> @@ -1228,45 +1244,51 @@ 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;
> + if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
> + struct mptcp_addr_info *addr_info;
> + u8 len = 0;
> + u8 echo = 0;
> +
> + if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> + len += sizeof(opts->ahmac);
> + addr_info = &opts->local;
> + } else {
> + echo = MPTCP_ADDR_ECHO;
> + addr_info = &opts->remote;
> + }
>
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> - if (opts->addr.family == AF_INET6)
> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + if (addr_info->family == AF_INET6)
> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + else
> #endif
> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>
> - if (opts->addr.port)
> + if (addr_info->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) {
> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> + len, echo, addr_info->id);
> + if (addr_info->family == AF_INET) {
> + memcpy((u8 *)ptr, (u8 *)&addr_info->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_info->family == AF_INET6) {
> + memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
> ptr += 4;
> }
> #endif
>
> - if (!opts->addr.port) {
> - if (opts->ahmac) {
> + if (!addr_info->port) {
> + if (!echo) {
> put_unaligned_be64(opts->ahmac, ptr);
> ptr += 2;
> }
> } else {
> - u16 port = ntohs(opts->addr.port);
> + u16 port = ntohs(addr_info->port);
>
> - if (opts->ahmac) {
> + if (!echo) {
> u8 *bptr = (u8 *)ptr;
>
> put_unaligned_be16(port, bptr);
> @@ -1275,7 +1297,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> bptr += 8;
> put_unaligned_be16(TCPOPT_NOP << 8 |
> TCPOPT_NOP, bptr);
> -
> ptr += 3;
> } else {
> put_unaligned_be32(port << 16 |
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 107a5a2..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;
> }
I think this part in mptcp_pm_announce_addr should be put into the previous
patch 2/4 "mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO
separate".
WDYT?
-Geliang
> @@ -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_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> - 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] 14+ messages in thread
* Re: [PATCH v5 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-22 4:45 ` [PATCH v5 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
2021-06-25 4:44 ` Geliang Tang
2021-06-25 10:39 ` Geliang Tang
@ 2021-06-25 11:43 ` Geliang Tang
2021-06-25 12:29 ` Geliang Tang
3 siblings, 0 replies; 14+ messages in thread
From: Geliang Tang @ 2021-06-25 11:43 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月22日周二 下午12:46写道：
>
> 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 | 2 +-
> net/mptcp/options.c | 105 ++++++++++++++++++++++++++++++---------------------
> net/mptcp/pm.c | 30 +++++----------
> net/mptcp/protocol.h | 13 ++++---
> 4 files changed, 80 insertions(+), 70 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 637e90b..d2c6ebe 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -61,7 +61,7 @@ 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;
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..a1fafed 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,13 +655,19 @@ 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;
> + struct mptcp_addr_info remote;
> + struct mptcp_addr_info local;
> + 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_should_add_signal(msk))
> + return false;
> +
> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> + (!mptcp_pm_should_add_signal_echo(msk) &&
I think this '!mptcp_pm_should_add_signal_echo(msk) &&' could be dropped,
WDYT?
-Geliang
> + mptcp_pm_should_add_signal_addr(msk) &&
> + (local.family == AF_INET6 || local.port))) &&
> skb && skb_is_tcp_pure_ack(skb)) {
> pr_debug("drop other suboptions");
> opts->suboptions = 0;
> @@ -671,25 +677,35 @@ 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 (remaining < len)
> - return false;
> + if (mptcp_pm_should_add_signal_echo(msk)) {
> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> + if (remaining < len)
> + return false;
> + opts->remote = remote;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> + } else {
> + len = mptcp_add_addr_len(local.family, false, !!local.port);
> + if (remaining < len)
> + return false;
> + opts->local = local;
> + opts->ahmac = add_addr_generate_hmac(msk->local_key,
> + msk->remote_key,
> + &opts->local);
> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> + }
>
> *size = len;
> 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, mptcp_pm_should_add_signal_echo(msk), opts->local.id,
> + opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>
> return true;
> }
> @@ -1228,45 +1244,51 @@ 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;
> + if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
> + struct mptcp_addr_info *addr_info;
> + u8 len = 0;
> + u8 echo = 0;
> +
> + if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> + len += sizeof(opts->ahmac);
> + addr_info = &opts->local;
> + } else {
> + echo = MPTCP_ADDR_ECHO;
> + addr_info = &opts->remote;
> + }
>
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> - if (opts->addr.family == AF_INET6)
> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + if (addr_info->family == AF_INET6)
> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + else
> #endif
> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>
> - if (opts->addr.port)
> + if (addr_info->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) {
> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> + len, echo, addr_info->id);
> + if (addr_info->family == AF_INET) {
> + memcpy((u8 *)ptr, (u8 *)&addr_info->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_info->family == AF_INET6) {
> + memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
> ptr += 4;
> }
> #endif
>
> - if (!opts->addr.port) {
> - if (opts->ahmac) {
> + if (!addr_info->port) {
> + if (!echo) {
> put_unaligned_be64(opts->ahmac, ptr);
> ptr += 2;
> }
> } else {
> - u16 port = ntohs(opts->addr.port);
> + u16 port = ntohs(addr_info->port);
>
> - if (opts->ahmac) {
> + if (!echo) {
> u8 *bptr = (u8 *)ptr;
>
> put_unaligned_be16(port, bptr);
> @@ -1275,7 +1297,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> bptr += 8;
> put_unaligned_be16(TCPOPT_NOP << 8 |
> TCPOPT_NOP, bptr);
> -
> ptr += 3;
> } else {
> put_unaligned_be32(port << 16 |
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 107a5a2..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_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> - 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] 14+ messages in thread
* Re: [PATCH v5 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-22 4:45 ` [PATCH v5 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
` (2 preceding siblings ...)
2021-06-25 11:43 ` Geliang Tang
@ 2021-06-25 12:29 ` Geliang Tang
3 siblings, 0 replies; 14+ messages in thread
From: Geliang Tang @ 2021-06-25 12:29 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月22日周二 下午12:46写道：
>
> 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 | 2 +-
> net/mptcp/options.c | 105 ++++++++++++++++++++++++++++++---------------------
> net/mptcp/pm.c | 30 +++++----------
> net/mptcp/protocol.h | 13 ++++---
> 4 files changed, 80 insertions(+), 70 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 637e90b..d2c6ebe 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -61,7 +61,7 @@ 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;
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..a1fafed 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,13 +655,19 @@ 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;
> + struct mptcp_addr_info remote;
> + struct mptcp_addr_info local;
> + 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_should_add_signal(msk))
> + return false;
> +
> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> + (!mptcp_pm_should_add_signal_echo(msk) &&
> + mptcp_pm_should_add_signal_addr(msk) &&
> + (local.family == AF_INET6 || local.port))) &&
> skb && skb_is_tcp_pure_ack(skb)) {
> pr_debug("drop other suboptions");
> opts->suboptions = 0;
> @@ -671,25 +677,35 @@ 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 (remaining < len)
> - return false;
> + if (mptcp_pm_should_add_signal_echo(msk)) {
> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> + if (remaining < len)
> + return false;
> + opts->remote = remote;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> + } else {
> + len = mptcp_add_addr_len(local.family, false, !!local.port);
> + if (remaining < len)
> + return false;
> + opts->local = local;
> + opts->ahmac = add_addr_generate_hmac(msk->local_key,
> + msk->remote_key,
> + &opts->local);
> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> + }
>
> *size = len;
> 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, mptcp_pm_should_add_signal_echo(msk), opts->local.id,
> + opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>
> return true;
> }
> @@ -1228,45 +1244,51 @@ 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;
> + if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
> + struct mptcp_addr_info *addr_info;
I prefer to rename addr_info to addr here, and set a default value for it:
struct mptcp_addr_info *addr = &opts->local;
> + u8 len = 0;
> + u8 echo = 0;
Keep the original default values here:
u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
u8 echo = MPTCP_ADDR_ECHO;
> +
> + if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> + len += sizeof(opts->ahmac);
> + addr_info = &opts->local;
> + } else {
> + echo = MPTCP_ADDR_ECHO;
> + addr_info = &opts->remote;
> + }
Change the default value of addr when ECHO flag is set:
if (OPTION_MPTCP_ADD_ECHO & opts->suboptions)
addr = &opts->remote;
>
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> - if (opts->addr.family == AF_INET6)
> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + if (addr_info->family == AF_INET6)
> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + else
> #endif
> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
Adjust the length for the IPV6 case:
#if IS_ENABLED(CONFIG_MPTCP_IPV6)
if (addr->family == AF_INET6)
len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
#endif
>
> - if (opts->addr.port)
> + if (addr_info->port)
> len += TCPOLEN_MPTCP_PORT_LEN;
>
Adjust the length for the port case:
if (addr->port)
len += TCPOLEN_MPTCP_PORT_LEN;
> - if (opts->ahmac) {
> - len += sizeof(opts->ahmac);
> - echo = 0;
> - }
> -
Keep this opts->ahmac trunk here.
> *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_info->id);
> + if (addr_info->family == AF_INET) {
> + memcpy((u8 *)ptr, (u8 *)&addr_info->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_info->family == AF_INET6) {
> + memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
> ptr += 4;
> }
> #endif
>
> - if (!opts->addr.port) {
> - if (opts->ahmac) {
Keep this opts->ahmac unchanged.
> + if (!addr_info->port) {
> + if (!echo) {
> put_unaligned_be64(opts->ahmac, ptr);
> ptr += 2;
> }
> } else {
> - u16 port = ntohs(opts->addr.port);
> + u16 port = ntohs(addr_info->port);
>
> - if (opts->ahmac) {
Keep this opts->ahmac unchanged too.
> + if (!echo) {
> u8 *bptr = (u8 *)ptr;
>
> put_unaligned_be16(port, bptr);
> @@ -1275,7 +1297,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> bptr += 8;
> put_unaligned_be16(TCPOPT_NOP << 8 |
> TCPOPT_NOP, bptr);
> -
No need to drop this line.
Thanks.
-Geliang
> ptr += 3;
> } else {
> put_unaligned_be32(port << 16 |
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 107a5a2..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_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> - 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] 14+ messages in thread
end of thread, other threads:[~2021-06-25 12:29 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-22 4:45 [PATCH v5 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
2021-06-22 4:45 ` [PATCH v5 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
2021-06-22 4:45 ` [PATCH v5 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
2021-06-25 10:33 ` Geliang Tang
2021-06-22 4:45 ` [PATCH v5 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
2021-06-25 4:44 ` Geliang Tang
2021-06-25 9:43 ` Yonglong Li
2021-06-25 10:39 ` Geliang Tang
2021-06-25 11:43 ` Geliang Tang
2021-06-25 12:29 ` Geliang Tang
2021-06-22 4:45 ` [PATCH v5 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT Yonglong Li
2021-06-25 10:01 ` Geliang Tang
2021-06-25 0:28 ` [PATCH v5 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Mat Martineau
2021-06-25 1:47 ` Yonglong Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.