* [PATCH v4 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process
@ 2021-06-18 8:18 Yonglong Li
2021-06-18 8:18 ` [PATCH v4 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Yonglong Li @ 2021-06-18 8:18 UTC (permalink / raw)
To: mptcp; +Cc: mathew.j.martineau, geliangtang, qitiepeng, 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;
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] 15+ messages in thread
* [PATCH v4 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
2021-06-18 8:18 [PATCH v4 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
@ 2021-06-18 8:18 ` Yonglong Li
2021-06-18 8:18 ` [PATCH v4 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Yonglong Li @ 2021-06-18 8:18 UTC (permalink / raw)
To: mptcp; +Cc: mathew.j.martineau, geliangtang, qitiepeng, 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] 15+ messages in thread
* [PATCH v4 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
2021-06-18 8:18 [PATCH v4 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
2021-06-18 8:18 ` [PATCH v4 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
@ 2021-06-18 8:18 ` Yonglong Li
2021-06-18 8:18 ` [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
2021-06-18 8:18 ` [PATCH v4 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT Yonglong Li
3 siblings, 0 replies; 15+ messages in thread
From: Yonglong Li @ 2021-06-18 8:18 UTC (permalink / raw)
To: mptcp; +Cc: mathew.j.martineau, geliangtang, qitiepeng, 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] 15+ messages in thread
* [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-18 8:18 [PATCH v4 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
2021-06-18 8:18 ` [PATCH v4 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
2021-06-18 8:18 ` [PATCH v4 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
@ 2021-06-18 8:18 ` Yonglong Li
2021-06-18 11:20 ` Geliang Tang
` (2 more replies)
2021-06-18 8:18 ` [PATCH v4 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT Yonglong Li
3 siblings, 3 replies; 15+ messages in thread
From: Yonglong Li @ 2021-06-18 8:18 UTC (permalink / raw)
To: mptcp; +Cc: mathew.j.martineau, geliangtang, qitiepeng, 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>
---
net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
net/mptcp/pm.c | 30 ++++---------
net/mptcp/protocol.h | 13 +++---
3 files changed, 92 insertions(+), 75 deletions(-)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 1aec016..43e3241 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
struct mptcp_sock *msk = mptcp_sk(subflow->conn);
bool drop_other_suboptions = false;
unsigned int opt_size = *size;
- bool echo;
- bool port;
+ struct mptcp_addr_info remote;
+ struct mptcp_addr_info local;
+ u8 add_addr, flags = 0xff;
int len;
- if ((mptcp_pm_should_add_signal_ipv6(msk) ||
- mptcp_pm_should_add_signal_port(msk) ||
- mptcp_pm_should_add_signal_echo(msk)) &&
- skb && skb_is_tcp_pure_ack(skb)) {
- pr_debug("drop other suboptions");
- opts->suboptions = 0;
- opts->ext_copy.use_ack = 0;
- opts->ext_copy.use_map = 0;
- remaining += opt_size;
- drop_other_suboptions = true;
- }
-
- if (!mptcp_pm_should_add_signal(msk) ||
- !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
- return false;
-
- len = mptcp_add_addr_len(opts->addr.family, echo, port);
- if (remaining < len)
+ if (!mptcp_pm_should_add_signal(msk))
return false;
- *size = len;
- if (drop_other_suboptions)
- *size -= opt_size;
- opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
- if (!echo) {
+ *size = 0;
+ mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
+ if (mptcp_pm_should_add_signal_echo(msk)) {
+ if (skb && skb_is_tcp_pure_ack(skb)) {
+ pr_debug("drop other suboptions");
+ opts->suboptions = 0;
+ opts->ext_copy.use_ack = 0;
+ opts->ext_copy.use_map = 0;
+ remaining += opt_size;
+ drop_other_suboptions = true;
+ }
+ len = mptcp_add_addr_len(remote.family, true, !!remote.port);
+ if (remaining < len)
+ return false;
+ remaining -= len;
+ *size += len;
+ opts->remote = remote;
+ flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
+ opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
+ pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
+ opts->remote.id, ntohs(opts->remote.port), add_addr);
+ } else if (mptcp_pm_should_add_signal_addr(msk)) {
+ if ((local.family == AF_INET6 || local.port) && skb &&
+ skb_is_tcp_pure_ack(skb)) {
+ pr_debug("drop other suboptions");
+ opts->suboptions = 0;
+ opts->ext_copy.use_ack = 0;
+ opts->ext_copy.use_map = 0;
+ remaining += opt_size;
+ drop_other_suboptions = true;
+ }
+ len = mptcp_add_addr_len(local.family, false, !!local.port);
+ if (remaining < len)
+ return false;
+ *size += len;
+ opts->addr = local;
opts->ahmac = add_addr_generate_hmac(msk->local_key,
msk->remote_key,
&opts->addr);
+ opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
+ flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
+ pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
+ opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
}
- pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
- opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
+
+ if (drop_other_suboptions)
+ *size -= opt_size;
+ spin_lock_bh(&msk->pm.lock);
+ WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
+ spin_unlock_bh(&msk->pm.lock);
return true;
}
@@ -1228,45 +1251,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->addr;
+ } 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 +1304,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] 15+ messages in thread
* [PATCH v4 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
2021-06-18 8:18 [PATCH v4 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-18 8:18 ` [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
@ 2021-06-18 8:18 ` Yonglong Li
3 siblings, 0 replies; 15+ messages in thread
From: Yonglong Li @ 2021-06-18 8:18 UTC (permalink / raw)
To: mptcp; +Cc: mathew.j.martineau, geliangtang, qitiepeng, 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] 15+ messages in thread
* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-18 8:18 ` [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
@ 2021-06-18 11:20 ` Geliang Tang
2021-06-21 3:51 ` Yonglong Li
2021-06-21 7:42 ` Geliang Tang
2021-06-21 8:29 ` Geliang Tang
2 siblings, 1 reply; 15+ messages in thread
From: Geliang Tang @ 2021-06-18 11:20 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng
Hi Yonglong,
Thanks for v4!
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
>
> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> ADD_ADDR/echo-ADD_ADDR option
>
> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>
> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> ---
> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
> net/mptcp/pm.c | 30 ++++---------
> net/mptcp/protocol.h | 13 +++---
> 3 files changed, 92 insertions(+), 75 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..43e3241 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> bool drop_other_suboptions = false;
> unsigned int opt_size = *size;
> - bool echo;
> - bool port;
> + struct mptcp_addr_info remote;
> + struct mptcp_addr_info local;
> + u8 add_addr, flags = 0xff;
> int len;
>
> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> - mptcp_pm_should_add_signal_port(msk) ||
> - mptcp_pm_should_add_signal_echo(msk)) &&
> - skb && skb_is_tcp_pure_ack(skb)) {
> - pr_debug("drop other suboptions");
> - opts->suboptions = 0;
> - opts->ext_copy.use_ack = 0;
> - opts->ext_copy.use_map = 0;
> - remaining += opt_size;
> - drop_other_suboptions = true;
> - }
> -
> - if (!mptcp_pm_should_add_signal(msk) ||
> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> - return false;
> -
> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> - if (remaining < len)
> + if (!mptcp_pm_should_add_signal(msk))
> return false;
>
> - *size = len;
> - if (drop_other_suboptions)
> - *size -= opt_size;
> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> - if (!echo) {
> + *size = 0;
> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> + if (mptcp_pm_should_add_signal_echo(msk)) {
> + if (skb && skb_is_tcp_pure_ack(skb)) {
'''
> + pr_debug("drop other suboptions");
> + opts->suboptions = 0;
> + opts->ext_copy.use_ack = 0;
> + opts->ext_copy.use_map = 0;
> + remaining += opt_size;
> + drop_other_suboptions = true;
'''
> + }
> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> + if (remaining < len)
> + return false;
> + remaining -= len;
> + *size += len;
> + opts->remote = remote;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> + opts->remote.id, ntohs(opts->remote.port), add_addr);
> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
> + if ((local.family == AF_INET6 || local.port) && skb &&
> + skb_is_tcp_pure_ack(skb)) {
'''
> + pr_debug("drop other suboptions");
> + opts->suboptions = 0;
> + opts->ext_copy.use_ack = 0;
> + opts->ext_copy.use_map = 0;
> + remaining += opt_size;
> + drop_other_suboptions = true;
'''
I think this "drop other suboptions" trunk here is still duplicated. Can
we just use one "drop other suboptions" trunk only?
Thanks.
-Geliang
> + }
> + len = mptcp_add_addr_len(local.family, false, !!local.port);
> + if (remaining < len)
> + return false;
> + *size += len;
> + opts->addr = local;
> opts->ahmac = add_addr_generate_hmac(msk->local_key,
> msk->remote_key,
> &opts->addr);
> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> + pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
> + opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
> }
> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> +
> + if (drop_other_suboptions)
> + *size -= opt_size;
> + spin_lock_bh(&msk->pm.lock);
> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> + spin_unlock_bh(&msk->pm.lock);
>
> return true;
> }
> @@ -1228,45 +1251,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->addr;
> + } 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 +1304,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] 15+ messages in thread
* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-18 11:20 ` Geliang Tang
@ 2021-06-21 3:51 ` Yonglong Li
2021-06-21 6:42 ` Geliang Tang
0 siblings, 1 reply; 15+ messages in thread
From: Yonglong Li @ 2021-06-21 3:51 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp, Mat Martineau, qitiepeng
On 2021/6/18 19:20, Geliang Tang wrote:
> Hi Yonglong,
>
> Thanks for v4!
>
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
>>
>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>> ADD_ADDR/echo-ADD_ADDR option
>>
>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>
>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
>> ---
>> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
>> net/mptcp/pm.c | 30 ++++---------
>> net/mptcp/protocol.h | 13 +++---
>> 3 files changed, 92 insertions(+), 75 deletions(-)
>>
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index 1aec016..43e3241 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>> bool drop_other_suboptions = false;
>> unsigned int opt_size = *size;
>> - bool echo;
>> - bool port;
>> + struct mptcp_addr_info remote;
>> + struct mptcp_addr_info local;
>> + u8 add_addr, flags = 0xff;
>> int len;
>>
>> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>> - mptcp_pm_should_add_signal_port(msk) ||
>> - mptcp_pm_should_add_signal_echo(msk)) &&
>> - skb && skb_is_tcp_pure_ack(skb)) {
>> - pr_debug("drop other suboptions");
>> - opts->suboptions = 0;
>> - opts->ext_copy.use_ack = 0;
>> - opts->ext_copy.use_map = 0;
>> - remaining += opt_size;
>> - drop_other_suboptions = true;
>> - }
>> -
>> - if (!mptcp_pm_should_add_signal(msk) ||
>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>> - return false;
>> -
>> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
>> - if (remaining < len)
>> + if (!mptcp_pm_should_add_signal(msk))
>> return false;
>>
>> - *size = len;
>> - if (drop_other_suboptions)
>> - *size -= opt_size;
>> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>> - if (!echo) {
>> + *size = 0;
>> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
>> + if (mptcp_pm_should_add_signal_echo(msk)) {
>> + if (skb && skb_is_tcp_pure_ack(skb)) {
>
> '''
>> + pr_debug("drop other suboptions");
>> + opts->suboptions = 0;
>> + opts->ext_copy.use_ack = 0;
>> + opts->ext_copy.use_map = 0;
>> + remaining += opt_size;
>> + drop_other_suboptions = true;
> '''
>
>> + }
>> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
>> + if (remaining < len)
>> + return false;
>> + remaining -= len;
>> + *size += len;
>> + opts->remote = remote;
>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
>> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
>> + opts->remote.id, ntohs(opts->remote.port), add_addr);
>> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
>> + if ((local.family == AF_INET6 || local.port) && skb &&
>> + skb_is_tcp_pure_ack(skb)) {
>
> '''
>> + pr_debug("drop other suboptions");
>> + opts->suboptions = 0;
>> + opts->ext_copy.use_ack = 0;
>> + opts->ext_copy.use_map = 0;
>> + remaining += opt_size;
>> + drop_other_suboptions = true;
> '''
>
> I think this "drop other suboptions" trunk here is still duplicated. Can
> we just use one "drop other suboptions" trunk only?
>
> Thanks.
> -Geliang
>
Hi Geliang, Thanks for you replay.
The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
IP version."
ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
to decide "drop other suboptions" in two trunk.
>
>
>> + }
>> + len = mptcp_add_addr_len(local.family, false, !!local.port);
>> + if (remaining < len)
>> + return false;
>> + *size += len;
>> + opts->addr = local;
>> opts->ahmac = add_addr_generate_hmac(msk->local_key,
>> msk->remote_key,
>> &opts->addr);
>> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>> + pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
>> + opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
>> }
>> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
>> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
>> +
>> + if (drop_other_suboptions)
>> + *size -= opt_size;
>> + spin_lock_bh(&msk->pm.lock);
>> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
>> + spin_unlock_bh(&msk->pm.lock);
>>
>> return true;
>> }
>> @@ -1228,45 +1251,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->addr;
>> + } 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 +1304,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] 15+ messages in thread
* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-21 3:51 ` Yonglong Li
@ 2021-06-21 6:42 ` Geliang Tang
2021-06-21 7:15 ` Yonglong Li
0 siblings, 1 reply; 15+ messages in thread
From: Geliang Tang @ 2021-06-21 6:42 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng
Hi Yonglong,
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 上午11:52写道：
>
>
>
> On 2021/6/18 19:20, Geliang Tang wrote:
> > Hi Yonglong,
> >
> > Thanks for v4!
> >
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
> >>
> >> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> >> ADD_ADDR/echo-ADD_ADDR option
> >>
> >> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >>
> >> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> >> ---
> >> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
> >> net/mptcp/pm.c | 30 ++++---------
> >> net/mptcp/protocol.h | 13 +++---
> >> 3 files changed, 92 insertions(+), 75 deletions(-)
> >>
> >> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >> index 1aec016..43e3241 100644
> >> --- a/net/mptcp/options.c
> >> +++ b/net/mptcp/options.c
> >> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >> bool drop_other_suboptions = false;
> >> unsigned int opt_size = *size;
> >> - bool echo;
> >> - bool port;
> >> + struct mptcp_addr_info remote;
> >> + struct mptcp_addr_info local;
> >> + u8 add_addr, flags = 0xff;
> >> int len;
> >>
> >> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> >> - mptcp_pm_should_add_signal_port(msk) ||
> >> - mptcp_pm_should_add_signal_echo(msk)) &&
> >> - skb && skb_is_tcp_pure_ack(skb)) {
> >> - pr_debug("drop other suboptions");
> >> - opts->suboptions = 0;
> >> - opts->ext_copy.use_ack = 0;
> >> - opts->ext_copy.use_map = 0;
> >> - remaining += opt_size;
> >> - drop_other_suboptions = true;
> >> - }
> >> -
> >> - if (!mptcp_pm_should_add_signal(msk) ||
> >> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> >> - return false;
> >> -
> >> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> >> - if (remaining < len)
> >> + if (!mptcp_pm_should_add_signal(msk))
> >> return false;
> >>
> >> - *size = len;
> >> - if (drop_other_suboptions)
> >> - *size -= opt_size;
> >> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >> - if (!echo) {
> >> + *size = 0;
> >> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> >> + if (mptcp_pm_should_add_signal_echo(msk)) {
> >> + if (skb && skb_is_tcp_pure_ack(skb)) {
> >
> > '''
> >> + pr_debug("drop other suboptions");
> >> + opts->suboptions = 0;
> >> + opts->ext_copy.use_ack = 0;
> >> + opts->ext_copy.use_map = 0;
> >> + remaining += opt_size;
> >> + drop_other_suboptions = true;
> > '''
> >
> >> + }
> >> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> >> + if (remaining < len)
> >> + return false;
> >> + remaining -= len;
> >> + *size += len;
> >> + opts->remote = remote;
> >> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> >> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> >> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> >> + opts->remote.id, ntohs(opts->remote.port), add_addr);
> >> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
> >> + if ((local.family == AF_INET6 || local.port) && skb &&
> >> + skb_is_tcp_pure_ack(skb)) {
> >
> > '''
> >> + pr_debug("drop other suboptions");
> >> + opts->suboptions = 0;
> >> + opts->ext_copy.use_ack = 0;
> >> + opts->ext_copy.use_map = 0;
> >> + remaining += opt_size;
> >> + drop_other_suboptions = true;
> > '''
> >
> > I think this "drop other suboptions" trunk here is still duplicated. Can
> > we just use one "drop other suboptions" trunk only?
> >
> > Thanks.
> > -Geliang
> >
> Hi Geliang, Thanks for you replay.
>
> The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
> carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
> only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
> IP version."
> ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
> to decide "drop other suboptions" in two trunk.
Could we change it like this:
'''
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index e77b5d532fb8..8b4cb0581a49 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -673,15 +673,20 @@ static bool
mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
*size = 0;
mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
+
+ if ((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;
+ opts->ext_copy.use_ack = 0;
+ opts->ext_copy.use_map = 0;
+ remaining += opt_size;
+ drop_other_suboptions = true;
+ }
+
if (mptcp_pm_should_add_signal_echo(msk)) {
- if (skb && skb_is_tcp_pure_ack(skb)) {
- pr_debug("drop other suboptions");
- opts->suboptions = 0;
- opts->ext_copy.use_ack = 0;
- opts->ext_copy.use_map = 0;
- remaining += opt_size;
- drop_other_suboptions = true;
- }
len = mptcp_add_addr_len(remote.family, true, !!remote.port);
if (remaining < len)
return false;
@@ -693,15 +698,6 @@ static bool
mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
opts->remote.id, ntohs(opts->remote.port), add_addr);
} else if (mptcp_pm_should_add_signal_addr(msk)) {
- if ((local.family == AF_INET6 || local.port) && skb &&
- skb_is_tcp_pure_ack(skb)) {
- pr_debug("drop other suboptions");
- opts->suboptions = 0;
- opts->ext_copy.use_ack = 0;
- opts->ext_copy.use_map = 0;
- remaining += opt_size;
- drop_other_suboptions = true;
- }
len = mptcp_add_addr_len(local.family, false, !!local.port);
if (remaining < len)
return false;
'''
WDYT?
>
> >
> >
> >> + }
> >> + len = mptcp_add_addr_len(local.family, false, !!local.port);
> >> + if (remaining < len)
> >> + return false;
And here, I think "remaining -= len;" is missing.
Thanks,
-Geliang
> >> + *size += len;
> >> + opts->addr = local;
> >> opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >> msk->remote_key,
> >> &opts->addr);
> >> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> >> + pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
> >> + opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
> >> }
> >> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> >> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> >> +
> >> + if (drop_other_suboptions)
> >> + *size -= opt_size;
> >> + spin_lock_bh(&msk->pm.lock);
> >> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> >> + spin_unlock_bh(&msk->pm.lock);
> >>
> >> return true;
> >> }
> >> @@ -1228,45 +1251,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->addr;
> >> + } 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 +1304,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] 15+ messages in thread
* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-21 6:42 ` Geliang Tang
@ 2021-06-21 7:15 ` Yonglong Li
2021-06-21 7:39 ` Geliang Tang
0 siblings, 1 reply; 15+ messages in thread
From: Yonglong Li @ 2021-06-21 7:15 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp, Mat Martineau, qitiepeng
On 2021/6/21 14:42, Geliang Tang wrote:
> Hi Yonglong,
>
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 上午11:52写道：
>>
>>
>> On 2021/6/18 19:20, Geliang Tang wrote:
>>> Hi Yonglong,
>>>
>>> Thanks for v4!
>>>
>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>>>> ADD_ADDR/echo-ADD_ADDR option
>>>>
>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>>>
>>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
>>>> ---
>>>> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
>>>> net/mptcp/pm.c | 30 ++++---------
>>>> net/mptcp/protocol.h | 13 +++---
>>>> 3 files changed, 92 insertions(+), 75 deletions(-)
>>>>
>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>> index 1aec016..43e3241 100644
>>>> --- a/net/mptcp/options.c
>>>> +++ b/net/mptcp/options.c
>>>> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>> bool drop_other_suboptions = false;
>>>> unsigned int opt_size = *size;
>>>> - bool echo;
>>>> - bool port;
>>>> + struct mptcp_addr_info remote;
>>>> + struct mptcp_addr_info local;
>>>> + u8 add_addr, flags = 0xff;
>>>> int len;
>>>>
>>>> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>>>> - mptcp_pm_should_add_signal_port(msk) ||
>>>> - mptcp_pm_should_add_signal_echo(msk)) &&
>>>> - skb && skb_is_tcp_pure_ack(skb)) {
>>>> - pr_debug("drop other suboptions");
>>>> - opts->suboptions = 0;
>>>> - opts->ext_copy.use_ack = 0;
>>>> - opts->ext_copy.use_map = 0;
>>>> - remaining += opt_size;
>>>> - drop_other_suboptions = true;
>>>> - }
>>>> -
>>>> - if (!mptcp_pm_should_add_signal(msk) ||
>>>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>>>> - return false;
>>>> -
>>>> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
>>>> - if (remaining < len)
>>>> + if (!mptcp_pm_should_add_signal(msk))
>>>> return false;
>>>>
>>>> - *size = len;
>>>> - if (drop_other_suboptions)
>>>> - *size -= opt_size;
>>>> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>> - if (!echo) {
>>>> + *size = 0;
>>>> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
>>>> + if (mptcp_pm_should_add_signal_echo(msk)) {
>>>> + if (skb && skb_is_tcp_pure_ack(skb)) {
>>> '''
>>>> + pr_debug("drop other suboptions");
>>>> + opts->suboptions = 0;
>>>> + opts->ext_copy.use_ack = 0;
>>>> + opts->ext_copy.use_map = 0;
>>>> + remaining += opt_size;
>>>> + drop_other_suboptions = true;
>>> '''
>>>
>>>> + }
>>>> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
>>>> + if (remaining < len)
>>>> + return false;
>>>> + remaining -= len;
>>>> + *size += len;
>>>> + opts->remote = remote;
>>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>>>> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
>>>> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
>>>> + opts->remote.id, ntohs(opts->remote.port), add_addr);
>>>> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
>>>> + if ((local.family == AF_INET6 || local.port) && skb &&
>>>> + skb_is_tcp_pure_ack(skb)) {
>>> '''
>>>> + pr_debug("drop other suboptions");
>>>> + opts->suboptions = 0;
>>>> + opts->ext_copy.use_ack = 0;
>>>> + opts->ext_copy.use_map = 0;
>>>> + remaining += opt_size;
>>>> + drop_other_suboptions = true;
>>> '''
>>>
>>> I think this "drop other suboptions" trunk here is still duplicated. Can
>>> we just use one "drop other suboptions" trunk only?
>>>
>>> Thanks.
>>> -Geliang
>>>
>> Hi Geliang, Thanks for you replay.
>>
>> The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
>> carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
>> only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
>> IP version."
>> ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
>> to decide "drop other suboptions" in two trunk.
> Could we change it like this:
>
> '''
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index e77b5d532fb8..8b4cb0581a49 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -673,15 +673,20 @@ static bool
> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>
> *size = 0;
> mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> +
> + if ((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;
> + opts->ext_copy.use_ack = 0;
> + opts->ext_copy.use_map = 0;
> + remaining += opt_size;
> + drop_other_suboptions = true;
> + }
> +
> if (mptcp_pm_should_add_signal_echo(msk)) {
> - if (skb && skb_is_tcp_pure_ack(skb)) {
> - pr_debug("drop other suboptions");
> - opts->suboptions = 0;
> - opts->ext_copy.use_ack = 0;
> - opts->ext_copy.use_map = 0;
> - remaining += opt_size;
> - drop_other_suboptions = true;
> - }
> len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> if (remaining < len)
> return false;
> @@ -693,15 +698,6 @@ static bool
> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> opts->remote.id, ntohs(opts->remote.port), add_addr);
> } else if (mptcp_pm_should_add_signal_addr(msk)) {
> - if ((local.family == AF_INET6 || local.port) && skb &&
> - skb_is_tcp_pure_ack(skb)) {
> - pr_debug("drop other suboptions");
> - opts->suboptions = 0;
> - opts->ext_copy.use_ack = 0;
> - opts->ext_copy.use_map = 0;
> - remaining += opt_size;
> - drop_other_suboptions = true;
> - }
> len = mptcp_add_addr_len(local.family, false, !!local.port);
> if (remaining < len)
> return false;
> '''
> WDYT?
Thanks for your advice.
Because MPTCP_ADD_ADDR_ECHO and MPTCP_ADD_ADDR_SIGNAL can be set at the same time. So as your advice we should
change like this(still I think it not clear than before):
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;
+ opts->ext_copy.use_ack = 0;
+ opts->ext_copy.use_map = 0;
+ remaining += opt_size;
+ drop_other_suboptions = true;
+ }
+
if (mptcp_pm_should_add_signal_echo(msk)) {
- if (skb && skb_is_tcp_pure_ack(skb)) {
>
>>>
>>>> + }
>>>> + len = mptcp_add_addr_len(local.family, false, !!local.port);
>>>> + if (remaining < len)
>>>> + return false;
> And here, I think "remaining -= len;" is missing.
>
> Thanks,
> -Geliang
>
"remaining" is not being used in the flowing code. So "remaining -=len;" is not necessary. But you remindme that the "remaining -= len;" can be removed in the first trunk.
I will send v5 as your advice.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-21 7:15 ` Yonglong Li
@ 2021-06-21 7:39 ` Geliang Tang
2021-06-21 7:49 ` Yonglong Li
0 siblings, 1 reply; 15+ messages in thread
From: Geliang Tang @ 2021-06-21 7:39 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 下午3:16写道：
>
>
>
> On 2021/6/21 14:42, Geliang Tang wrote:
> > Hi Yonglong,
> >
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 上午11:52写道：
> >>
> >>
> >> On 2021/6/18 19:20, Geliang Tang wrote:
> >>> Hi Yonglong,
> >>>
> >>> Thanks for v4!
> >>>
> >>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
> >>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> >>>> ADD_ADDR/echo-ADD_ADDR option
> >>>>
> >>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >>>>
> >>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> >>>> ---
> >>>> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
> >>>> net/mptcp/pm.c | 30 ++++---------
> >>>> net/mptcp/protocol.h | 13 +++---
> >>>> 3 files changed, 92 insertions(+), 75 deletions(-)
> >>>>
> >>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>>> index 1aec016..43e3241 100644
> >>>> --- a/net/mptcp/options.c
> >>>> +++ b/net/mptcp/options.c
> >>>> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >>>> bool drop_other_suboptions = false;
> >>>> unsigned int opt_size = *size;
> >>>> - bool echo;
> >>>> - bool port;
> >>>> + struct mptcp_addr_info remote;
> >>>> + struct mptcp_addr_info local;
> >>>> + u8 add_addr, flags = 0xff;
> >>>> int len;
> >>>>
> >>>> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> >>>> - mptcp_pm_should_add_signal_port(msk) ||
> >>>> - mptcp_pm_should_add_signal_echo(msk)) &&
> >>>> - skb && skb_is_tcp_pure_ack(skb)) {
> >>>> - pr_debug("drop other suboptions");
> >>>> - opts->suboptions = 0;
> >>>> - opts->ext_copy.use_ack = 0;
> >>>> - opts->ext_copy.use_map = 0;
> >>>> - remaining += opt_size;
> >>>> - drop_other_suboptions = true;
> >>>> - }
> >>>> -
> >>>> - if (!mptcp_pm_should_add_signal(msk) ||
> >>>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> >>>> - return false;
> >>>> -
> >>>> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> >>>> - if (remaining < len)
> >>>> + if (!mptcp_pm_should_add_signal(msk))
> >>>> return false;
> >>>>
> >>>> - *size = len;
> >>>> - if (drop_other_suboptions)
> >>>> - *size -= opt_size;
> >>>> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >>>> - if (!echo) {
> >>>> + *size = 0;
> >>>> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> >>>> + if (mptcp_pm_should_add_signal_echo(msk)) {
> >>>> + if (skb && skb_is_tcp_pure_ack(skb)) {
> >>> '''
> >>>> + pr_debug("drop other suboptions");
> >>>> + opts->suboptions = 0;
> >>>> + opts->ext_copy.use_ack = 0;
> >>>> + opts->ext_copy.use_map = 0;
> >>>> + remaining += opt_size;
> >>>> + drop_other_suboptions = true;
> >>> '''
> >>>
> >>>> + }
> >>>> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> >>>> + if (remaining < len)
> >>>> + return false;
> >>>> + remaining -= len;
> >>>> + *size += len;
> >>>> + opts->remote = remote;
> >>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> >>>> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> >>>> + opts->remote.id, ntohs(opts->remote.port), add_addr);
> >>>> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
> >>>> + if ((local.family == AF_INET6 || local.port) && skb &&
> >>>> + skb_is_tcp_pure_ack(skb)) {
> >>> '''
> >>>> + pr_debug("drop other suboptions");
> >>>> + opts->suboptions = 0;
> >>>> + opts->ext_copy.use_ack = 0;
> >>>> + opts->ext_copy.use_map = 0;
> >>>> + remaining += opt_size;
> >>>> + drop_other_suboptions = true;
> >>> '''
> >>>
> >>> I think this "drop other suboptions" trunk here is still duplicated. Can
> >>> we just use one "drop other suboptions" trunk only?
> >>>
> >>> Thanks.
> >>> -Geliang
> >>>
> >> Hi Geliang, Thanks for you replay.
> >>
> >> The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
> >> carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
> >> only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
> >> IP version."
> >> ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
> >> to decide "drop other suboptions" in two trunk.
> > Could we change it like this:
> >
> > '''
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index e77b5d532fb8..8b4cb0581a49 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -673,15 +673,20 @@ static bool
> > mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >
> > *size = 0;
> > mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> > +
> > + if ((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;
> > + opts->ext_copy.use_ack = 0;
> > + opts->ext_copy.use_map = 0;
> > + remaining += opt_size;
> > + drop_other_suboptions = true;
> > + }
> > +
> > if (mptcp_pm_should_add_signal_echo(msk)) {
> > - if (skb && skb_is_tcp_pure_ack(skb)) {
> > - pr_debug("drop other suboptions");
> > - opts->suboptions = 0;
> > - opts->ext_copy.use_ack = 0;
> > - opts->ext_copy.use_map = 0;
> > - remaining += opt_size;
> > - drop_other_suboptions = true;
> > - }
> > len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> > if (remaining < len)
> > return false;
> > @@ -693,15 +698,6 @@ static bool
> > mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> > pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> > opts->remote.id, ntohs(opts->remote.port), add_addr);
> > } else if (mptcp_pm_should_add_signal_addr(msk)) {
> > - if ((local.family == AF_INET6 || local.port) && skb &&
> > - skb_is_tcp_pure_ack(skb)) {
> > - pr_debug("drop other suboptions");
> > - opts->suboptions = 0;
> > - opts->ext_copy.use_ack = 0;
> > - opts->ext_copy.use_map = 0;
> > - remaining += opt_size;
> > - drop_other_suboptions = true;
> > - }
> > len = mptcp_add_addr_len(local.family, false, !!local.port);
> > if (remaining < len)
> > return false;
> > '''
> > WDYT?
> Thanks for your advice.
>
> Because MPTCP_ADD_ADDR_ECHO and MPTCP_ADD_ADDR_SIGNAL can be set at the same time. So as your advice we should
> change like this(still I think it not clear than before):
>
> 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;
> + opts->ext_copy.use_ack = 0;
> + opts->ext_copy.use_map = 0;
> + remaining += opt_size;
> + drop_other_suboptions = true;
> + }
> +
> if (mptcp_pm_should_add_signal_echo(msk)) {
> - if (skb && skb_is_tcp_pure_ack(skb)) {
>
>
> >
> >>>
> >>>> + }
> >>>> + len = mptcp_add_addr_len(local.family, false, !!local.port);
> >>>> + if (remaining < len)
> >>>> + return false;
> > And here, I think "remaining -= len;" is missing.
> >
> > Thanks,
> > -Geliang
> >
> "remaining" is not being used in the flowing code. So "remaining -=len;" is not necessary. But you remindme that the "remaining -= len;" can be removed in the first trunk.
I think we should keep this 'remaining -= len;', remaining can be used
in tcp_established_options.
>
> I will send v5 as your advice.
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-18 8:18 ` [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
2021-06-18 11:20 ` Geliang Tang
@ 2021-06-21 7:42 ` Geliang Tang
2021-06-21 7:51 ` Yonglong Li
2021-06-21 8:29 ` Geliang Tang
2 siblings, 1 reply; 15+ messages in thread
From: Geliang Tang @ 2021-06-21 7:42 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
>
> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> ADD_ADDR/echo-ADD_ADDR option
>
> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>
> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> ---
> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
> net/mptcp/pm.c | 30 ++++---------
> net/mptcp/protocol.h | 13 +++---
> 3 files changed, 92 insertions(+), 75 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..43e3241 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> bool drop_other_suboptions = false;
> unsigned int opt_size = *size;
> - bool echo;
> - bool port;
> + struct mptcp_addr_info remote;
> + struct mptcp_addr_info local;
> + u8 add_addr, flags = 0xff;
> int len;
>
> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> - mptcp_pm_should_add_signal_port(msk) ||
> - mptcp_pm_should_add_signal_echo(msk)) &&
> - skb && skb_is_tcp_pure_ack(skb)) {
> - pr_debug("drop other suboptions");
> - opts->suboptions = 0;
> - opts->ext_copy.use_ack = 0;
> - opts->ext_copy.use_map = 0;
> - remaining += opt_size;
> - drop_other_suboptions = true;
> - }
> -
> - if (!mptcp_pm_should_add_signal(msk) ||
> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> - return false;
> -
> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> - if (remaining < len)
> + if (!mptcp_pm_should_add_signal(msk))
> return false;
>
> - *size = len;
> - if (drop_other_suboptions)
> - *size -= opt_size;
> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> - if (!echo) {
> + *size = 0;
> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> + if (mptcp_pm_should_add_signal_echo(msk)) {
> + if (skb && skb_is_tcp_pure_ack(skb)) {
> + pr_debug("drop other suboptions");
> + opts->suboptions = 0;
> + opts->ext_copy.use_ack = 0;
> + opts->ext_copy.use_map = 0;
> + remaining += opt_size;
> + drop_other_suboptions = true;
> + }
> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> + if (remaining < len)
> + return false;
> + remaining -= len;
> + *size += len;
> + opts->remote = remote;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> + opts->remote.id, ntohs(opts->remote.port), add_addr);
> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
> + if ((local.family == AF_INET6 || local.port) && skb &&
> + skb_is_tcp_pure_ack(skb)) {
> + pr_debug("drop other suboptions");
> + opts->suboptions = 0;
> + opts->ext_copy.use_ack = 0;
> + opts->ext_copy.use_map = 0;
> + remaining += opt_size;
> + drop_other_suboptions = true;
> + }
> + len = mptcp_add_addr_len(local.family, false, !!local.port);
> + if (remaining < len)
> + return false;
> + *size += len;
> + opts->addr = local;
Could we rename this struct member addr in struct mptcp_out_options to
local?
> opts->ahmac = add_addr_generate_hmac(msk->local_key,
> msk->remote_key,
> &opts->addr);
> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> + pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
> + opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
Could we merge these two debug logs into one and move it at the the end
of this function, before 'return true'?
-Geliang
> }
> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> +
> + if (drop_other_suboptions)
> + *size -= opt_size;
> + spin_lock_bh(&msk->pm.lock);
> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> + spin_unlock_bh(&msk->pm.lock);
>
> return true;
> }
> @@ -1228,45 +1251,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->addr;
> + } 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 +1304,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] 15+ messages in thread
* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-21 7:39 ` Geliang Tang
@ 2021-06-21 7:49 ` Yonglong Li
2021-06-21 8:06 ` Geliang Tang
0 siblings, 1 reply; 15+ messages in thread
From: Yonglong Li @ 2021-06-21 7:49 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp, Mat Martineau, qitiepeng
On 2021/6/21 15:39, Geliang Tang wrote:
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 下午3:16写道：
>>
>>
>>
>> On 2021/6/21 14:42, Geliang Tang wrote:
>>> Hi Yonglong,
>>>
>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 上午11:52写道：
>>>>
>>>>
>>>> On 2021/6/18 19:20, Geliang Tang wrote:
>>>>> Hi Yonglong,
>>>>>
>>>>> Thanks for v4!
>>>>>
>>>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
>>>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>>>>>> ADD_ADDR/echo-ADD_ADDR option
>>>>>>
>>>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>>>>>
>>>>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
>>>>>> ---
>>>>>> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
>>>>>> net/mptcp/pm.c | 30 ++++---------
>>>>>> net/mptcp/protocol.h | 13 +++---
>>>>>> 3 files changed, 92 insertions(+), 75 deletions(-)
>>>>>>
>>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>>>> index 1aec016..43e3241 100644
>>>>>> --- a/net/mptcp/options.c
>>>>>> +++ b/net/mptcp/options.c
>>>>>> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>>>> bool drop_other_suboptions = false;
>>>>>> unsigned int opt_size = *size;
>>>>>> - bool echo;
>>>>>> - bool port;
>>>>>> + struct mptcp_addr_info remote;
>>>>>> + struct mptcp_addr_info local;
>>>>>> + u8 add_addr, flags = 0xff;
>>>>>> int len;
>>>>>>
>>>>>> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>>>>>> - mptcp_pm_should_add_signal_port(msk) ||
>>>>>> - mptcp_pm_should_add_signal_echo(msk)) &&
>>>>>> - skb && skb_is_tcp_pure_ack(skb)) {
>>>>>> - pr_debug("drop other suboptions");
>>>>>> - opts->suboptions = 0;
>>>>>> - opts->ext_copy.use_ack = 0;
>>>>>> - opts->ext_copy.use_map = 0;
>>>>>> - remaining += opt_size;
>>>>>> - drop_other_suboptions = true;
>>>>>> - }
>>>>>> -
>>>>>> - if (!mptcp_pm_should_add_signal(msk) ||
>>>>>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>>>>>> - return false;
>>>>>> -
>>>>>> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
>>>>>> - if (remaining < len)
>>>>>> + if (!mptcp_pm_should_add_signal(msk))
>>>>>> return false;
>>>>>>
>>>>>> - *size = len;
>>>>>> - if (drop_other_suboptions)
>>>>>> - *size -= opt_size;
>>>>>> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>>>> - if (!echo) {
>>>>>> + *size = 0;
>>>>>> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
>>>>>> + if (mptcp_pm_should_add_signal_echo(msk)) {
>>>>>> + if (skb && skb_is_tcp_pure_ack(skb)) {
>>>>> '''
>>>>>> + pr_debug("drop other suboptions");
>>>>>> + opts->suboptions = 0;
>>>>>> + opts->ext_copy.use_ack = 0;
>>>>>> + opts->ext_copy.use_map = 0;
>>>>>> + remaining += opt_size;
>>>>>> + drop_other_suboptions = true;
>>>>> '''
>>>>>
>>>>>> + }
>>>>>> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
>>>>>> + if (remaining < len)
>>>>>> + return false;
>>>>>> + remaining -= len;
>>>>>> + *size += len;
>>>>>> + opts->remote = remote;
>>>>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>>> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
>>>>>> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
>>>>>> + opts->remote.id, ntohs(opts->remote.port), add_addr);
>>>>>> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
>>>>>> + if ((local.family == AF_INET6 || local.port) && skb &&
>>>>>> + skb_is_tcp_pure_ack(skb)) {
>>>>> '''
>>>>>> + pr_debug("drop other suboptions");
>>>>>> + opts->suboptions = 0;
>>>>>> + opts->ext_copy.use_ack = 0;
>>>>>> + opts->ext_copy.use_map = 0;
>>>>>> + remaining += opt_size;
>>>>>> + drop_other_suboptions = true;
>>>>> '''
>>>>>
>>>>> I think this "drop other suboptions" trunk here is still duplicated. Can
>>>>> we just use one "drop other suboptions" trunk only?
>>>>>
>>>>> Thanks.
>>>>> -Geliang
>>>>>
>>>> Hi Geliang, Thanks for you replay.
>>>>
>>>> The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
>>>> carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
>>>> only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
>>>> IP version."
>>>> ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
>>>> to decide "drop other suboptions" in two trunk.
>>> Could we change it like this:
>>>
>>> '''
>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>> index e77b5d532fb8..8b4cb0581a49 100644
>>> --- a/net/mptcp/options.c
>>> +++ b/net/mptcp/options.c
>>> @@ -673,15 +673,20 @@ static bool
>>> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>
>>> *size = 0;
>>> mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
>>> +
>>> + if ((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;
>>> + opts->ext_copy.use_ack = 0;
>>> + opts->ext_copy.use_map = 0;
>>> + remaining += opt_size;
>>> + drop_other_suboptions = true;
>>> + }
>>> +
>>> if (mptcp_pm_should_add_signal_echo(msk)) {
>>> - if (skb && skb_is_tcp_pure_ack(skb)) {
>>> - pr_debug("drop other suboptions");
>>> - opts->suboptions = 0;
>>> - opts->ext_copy.use_ack = 0;
>>> - opts->ext_copy.use_map = 0;
>>> - remaining += opt_size;
>>> - drop_other_suboptions = true;
>>> - }
>>> len = mptcp_add_addr_len(remote.family, true, !!remote.port);
>>> if (remaining < len)
>>> return false;
>>> @@ -693,15 +698,6 @@ static bool
>>> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>> pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
>>> opts->remote.id, ntohs(opts->remote.port), add_addr);
>>> } else if (mptcp_pm_should_add_signal_addr(msk)) {
>>> - if ((local.family == AF_INET6 || local.port) && skb &&
>>> - skb_is_tcp_pure_ack(skb)) {
>>> - pr_debug("drop other suboptions");
>>> - opts->suboptions = 0;
>>> - opts->ext_copy.use_ack = 0;
>>> - opts->ext_copy.use_map = 0;
>>> - remaining += opt_size;
>>> - drop_other_suboptions = true;
>>> - }
>>> len = mptcp_add_addr_len(local.family, false, !!local.port);
>>> if (remaining < len)
>>> return false;
>>> '''
>>> WDYT?
>> Thanks for your advice.
>>
>> Because MPTCP_ADD_ADDR_ECHO and MPTCP_ADD_ADDR_SIGNAL can be set at the same time. So as your advice we should
>> change like this(still I think it not clear than before):
>>
>> 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;
>> + opts->ext_copy.use_ack = 0;
>> + opts->ext_copy.use_map = 0;
>> + remaining += opt_size;
>> + drop_other_suboptions = true;
>> + }
>> +
>> if (mptcp_pm_should_add_signal_echo(msk)) {
>> - if (skb && skb_is_tcp_pure_ack(skb)) {
>>
>>
>>>
>>>>>
>>>>>> + }
>>>>>> + len = mptcp_add_addr_len(local.family, false, !!local.port);
>>>>>> + if (remaining < len)
>>>>>> + return false;
>>> And here, I think "remaining -= len;" is missing.
>>>
>>> Thanks,
>>> -Geliang
>>>
>> "remaining" is not being used in the flowing code. So "remaining -=len;" is not necessary. But you remindme that the "remaining -= len;" can be removed in the first trunk.
>
> I think we should keep this 'remaining -= len;', remaining can be used
> in tcp_established_options.
>
Thanks for your review.
I think "remaining" will not use in tcp_established_options. "size" is used by tcp_established_options.
>>
>> I will send v5 as your advice.
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-21 7:42 ` Geliang Tang
@ 2021-06-21 7:51 ` Yonglong Li
0 siblings, 0 replies; 15+ messages in thread
From: Yonglong Li @ 2021-06-21 7:51 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp, Mat Martineau
On 2021/6/21 15:42, Geliang Tang wrote:
>> + }
>> + len = mptcp_add_addr_len(local.family, false, !!local.port);
>> + if (remaining < len)
>> + return false;
>> + *size += len;
>> + opts->addr = local;
> Could we rename this struct member addr in struct mptcp_out_options to
> local?
>
>> opts->ahmac = add_addr_generate_hmac(msk->local_key,
>> msk->remote_key,
>> &opts->addr);
>> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>> + pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
>> + opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
> Could we merge these two debug logs into one and move it at the the end
> of this function, before 'return true'?
>
> -Geliang
>
Thanks for your review.
I will change them in v5 as your advice.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-21 7:49 ` Yonglong Li
@ 2021-06-21 8:06 ` Geliang Tang
0 siblings, 0 replies; 15+ messages in thread
From: Geliang Tang @ 2021-06-21 8:06 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 下午3:50写道：
>
>
>
> On 2021/6/21 15:39, Geliang Tang wrote:
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 下午3:16写道：
> >>
> >>
> >>
> >> On 2021/6/21 14:42, Geliang Tang wrote:
> >>> Hi Yonglong,
> >>>
> >>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 上午11:52写道：
> >>>>
> >>>>
> >>>> On 2021/6/18 19:20, Geliang Tang wrote:
> >>>>> Hi Yonglong,
> >>>>>
> >>>>> Thanks for v4!
> >>>>>
> >>>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
> >>>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> >>>>>> ADD_ADDR/echo-ADD_ADDR option
> >>>>>>
> >>>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >>>>>>
> >>>>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> >>>>>> ---
> >>>>>> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
> >>>>>> net/mptcp/pm.c | 30 ++++---------
> >>>>>> net/mptcp/protocol.h | 13 +++---
> >>>>>> 3 files changed, 92 insertions(+), 75 deletions(-)
> >>>>>>
> >>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>>>>> index 1aec016..43e3241 100644
> >>>>>> --- a/net/mptcp/options.c
> >>>>>> +++ b/net/mptcp/options.c
> >>>>>> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >>>>>> bool drop_other_suboptions = false;
> >>>>>> unsigned int opt_size = *size;
> >>>>>> - bool echo;
> >>>>>> - bool port;
> >>>>>> + struct mptcp_addr_info remote;
> >>>>>> + struct mptcp_addr_info local;
> >>>>>> + u8 add_addr, flags = 0xff;
> >>>>>> int len;
> >>>>>>
> >>>>>> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> >>>>>> - mptcp_pm_should_add_signal_port(msk) ||
> >>>>>> - mptcp_pm_should_add_signal_echo(msk)) &&
> >>>>>> - skb && skb_is_tcp_pure_ack(skb)) {
> >>>>>> - pr_debug("drop other suboptions");
> >>>>>> - opts->suboptions = 0;
> >>>>>> - opts->ext_copy.use_ack = 0;
> >>>>>> - opts->ext_copy.use_map = 0;
> >>>>>> - remaining += opt_size;
> >>>>>> - drop_other_suboptions = true;
> >>>>>> - }
> >>>>>> -
> >>>>>> - if (!mptcp_pm_should_add_signal(msk) ||
> >>>>>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> >>>>>> - return false;
> >>>>>> -
> >>>>>> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> >>>>>> - if (remaining < len)
> >>>>>> + if (!mptcp_pm_should_add_signal(msk))
> >>>>>> return false;
> >>>>>>
> >>>>>> - *size = len;
> >>>>>> - if (drop_other_suboptions)
> >>>>>> - *size -= opt_size;
> >>>>>> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >>>>>> - if (!echo) {
> >>>>>> + *size = 0;
> >>>>>> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> >>>>>> + if (mptcp_pm_should_add_signal_echo(msk)) {
> >>>>>> + if (skb && skb_is_tcp_pure_ack(skb)) {
> >>>>> '''
> >>>>>> + pr_debug("drop other suboptions");
> >>>>>> + opts->suboptions = 0;
> >>>>>> + opts->ext_copy.use_ack = 0;
> >>>>>> + opts->ext_copy.use_map = 0;
> >>>>>> + remaining += opt_size;
> >>>>>> + drop_other_suboptions = true;
> >>>>> '''
> >>>>>
> >>>>>> + }
> >>>>>> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> >>>>>> + if (remaining < len)
> >>>>>> + return false;
> >>>>>> + remaining -= len;
> >>>>>> + *size += len;
> >>>>>> + opts->remote = remote;
> >>>>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>>>> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> >>>>>> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> >>>>>> + opts->remote.id, ntohs(opts->remote.port), add_addr);
> >>>>>> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
> >>>>>> + if ((local.family == AF_INET6 || local.port) && skb &&
> >>>>>> + skb_is_tcp_pure_ack(skb)) {
> >>>>> '''
> >>>>>> + pr_debug("drop other suboptions");
> >>>>>> + opts->suboptions = 0;
> >>>>>> + opts->ext_copy.use_ack = 0;
> >>>>>> + opts->ext_copy.use_map = 0;
> >>>>>> + remaining += opt_size;
> >>>>>> + drop_other_suboptions = true;
> >>>>> '''
> >>>>>
> >>>>> I think this "drop other suboptions" trunk here is still duplicated. Can
> >>>>> we just use one "drop other suboptions" trunk only?
> >>>>>
> >>>>> Thanks.
> >>>>> -Geliang
> >>>>>
> >>>> Hi Geliang, Thanks for you replay.
> >>>>
> >>>> The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
> >>>> carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
> >>>> only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
> >>>> IP version."
> >>>> ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
> >>>> to decide "drop other suboptions" in two trunk.
> >>> Could we change it like this:
> >>>
> >>> '''
> >>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>> index e77b5d532fb8..8b4cb0581a49 100644
> >>> --- a/net/mptcp/options.c
> >>> +++ b/net/mptcp/options.c
> >>> @@ -673,15 +673,20 @@ static bool
> >>> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>
> >>> *size = 0;
> >>> mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> >>> +
> >>> + if ((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;
> >>> + opts->ext_copy.use_ack = 0;
> >>> + opts->ext_copy.use_map = 0;
> >>> + remaining += opt_size;
> >>> + drop_other_suboptions = true;
> >>> + }
> >>> +
> >>> if (mptcp_pm_should_add_signal_echo(msk)) {
> >>> - if (skb && skb_is_tcp_pure_ack(skb)) {
> >>> - pr_debug("drop other suboptions");
> >>> - opts->suboptions = 0;
> >>> - opts->ext_copy.use_ack = 0;
> >>> - opts->ext_copy.use_map = 0;
> >>> - remaining += opt_size;
> >>> - drop_other_suboptions = true;
> >>> - }
> >>> len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> >>> if (remaining < len)
> >>> return false;
> >>> @@ -693,15 +698,6 @@ static bool
> >>> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>> pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> >>> opts->remote.id, ntohs(opts->remote.port), add_addr);
> >>> } else if (mptcp_pm_should_add_signal_addr(msk)) {
> >>> - if ((local.family == AF_INET6 || local.port) && skb &&
> >>> - skb_is_tcp_pure_ack(skb)) {
> >>> - pr_debug("drop other suboptions");
> >>> - opts->suboptions = 0;
> >>> - opts->ext_copy.use_ack = 0;
> >>> - opts->ext_copy.use_map = 0;
> >>> - remaining += opt_size;
> >>> - drop_other_suboptions = true;
> >>> - }
> >>> len = mptcp_add_addr_len(local.family, false, !!local.port);
> >>> if (remaining < len)
> >>> return false;
> >>> '''
> >>> WDYT?
> >> Thanks for your advice.
> >>
> >> Because MPTCP_ADD_ADDR_ECHO and MPTCP_ADD_ADDR_SIGNAL can be set at the same time. So as your advice we should
> >> change like this(still I think it not clear than before):
> >>
> >> 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;
> >> + opts->ext_copy.use_ack = 0;
> >> + opts->ext_copy.use_map = 0;
> >> + remaining += opt_size;
> >> + drop_other_suboptions = true;
> >> + }
> >> +
> >> if (mptcp_pm_should_add_signal_echo(msk)) {
> >> - if (skb && skb_is_tcp_pure_ack(skb)) {
> >>
> >>
> >>>
> >>>>>
> >>>>>> + }
> >>>>>> + len = mptcp_add_addr_len(local.family, false, !!local.port);
> >>>>>> + if (remaining < len)
> >>>>>> + return false;
> >>> And here, I think "remaining -= len;" is missing.
> >>>
> >>> Thanks,
> >>> -Geliang
> >>>
> >> "remaining" is not being used in the flowing code. So "remaining -=len;" is not necessary. But you remindme that the "remaining -= len;" can be removed in the first trunk.
> >
> > I think we should keep this 'remaining -= len;', remaining can be used
> > in tcp_established_options.
> >
> Thanks for your review.
> I think "remaining" will not use in tcp_established_options. "size" is used by tcp_established_options.
You're right, we should drop this 'remaining -= len;' in this function.
>
> >>
> >> I will send v5 as your advice.
> >>
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-18 8:18 ` [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
2021-06-18 11:20 ` Geliang Tang
2021-06-21 7:42 ` Geliang Tang
@ 2021-06-21 8:29 ` Geliang Tang
2 siblings, 0 replies; 15+ messages in thread
From: Geliang Tang @ 2021-06-21 8:29 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
>
> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> ADD_ADDR/echo-ADD_ADDR option
>
> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>
> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> ---
> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
> net/mptcp/pm.c | 30 ++++---------
> net/mptcp/protocol.h | 13 +++---
> 3 files changed, 92 insertions(+), 75 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..43e3241 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> bool drop_other_suboptions = false;
> unsigned int opt_size = *size;
> - bool echo;
> - bool port;
> + struct mptcp_addr_info remote;
> + struct mptcp_addr_info local;
> + u8 add_addr, flags = 0xff;
> int len;
>
> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> - mptcp_pm_should_add_signal_port(msk) ||
> - mptcp_pm_should_add_signal_echo(msk)) &&
> - skb && skb_is_tcp_pure_ack(skb)) {
> - pr_debug("drop other suboptions");
> - opts->suboptions = 0;
> - opts->ext_copy.use_ack = 0;
> - opts->ext_copy.use_map = 0;
> - remaining += opt_size;
> - drop_other_suboptions = true;
> - }
> -
> - if (!mptcp_pm_should_add_signal(msk) ||
> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> - return false;
> -
> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> - if (remaining < len)
> + if (!mptcp_pm_should_add_signal(msk))
> return false;
>
> - *size = len;
> - if (drop_other_suboptions)
> - *size -= opt_size;
> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> - if (!echo) {
> + *size = 0;
> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> + if (mptcp_pm_should_add_signal_echo(msk)) {
> + if (skb && skb_is_tcp_pure_ack(skb)) {
> + pr_debug("drop other suboptions");
> + opts->suboptions = 0;
> + opts->ext_copy.use_ack = 0;
> + opts->ext_copy.use_map = 0;
> + remaining += opt_size;
> + drop_other_suboptions = true;
> + }
> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> + if (remaining < len)
> + return false;
> + remaining -= len;
> + *size += len;
Could we drop the above '*size = 0', change this line to "*size = len;",
and move it out of the if... else... trunk, just like the original code:
*size = len;
if (drop_other_suboptions)
*size -= opt_size;
> + opts->remote = remote;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> + opts->remote.id, ntohs(opts->remote.port), add_addr);
> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
Since we called mptcp_pm_should_add_signal before, could we just use
'else' here?
-Geliang
> + if ((local.family == AF_INET6 || local.port) && skb &&
> + skb_is_tcp_pure_ack(skb)) {
> + pr_debug("drop other suboptions");
> + opts->suboptions = 0;
> + opts->ext_copy.use_ack = 0;
> + opts->ext_copy.use_map = 0;
> + remaining += opt_size;
> + drop_other_suboptions = true;
> + }
> + len = mptcp_add_addr_len(local.family, false, !!local.port);
> + if (remaining < len)
> + return false;
> + *size += len;
> + opts->addr = local;
> opts->ahmac = add_addr_generate_hmac(msk->local_key,
> msk->remote_key,
> &opts->addr);
> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> + pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
> + opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
> }
> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> +
> + if (drop_other_suboptions)
> + *size -= opt_size;
> + spin_lock_bh(&msk->pm.lock);
> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> + spin_unlock_bh(&msk->pm.lock);
>
> return true;
> }
> @@ -1228,45 +1251,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->addr;
> + } 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 +1304,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] 15+ messages in thread
end of thread, other threads:[~2021-06-21 8:29 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-18 8:18 [PATCH v4 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
2021-06-18 8:18 ` [PATCH v4 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
2021-06-18 8:18 ` [PATCH v4 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
2021-06-18 8:18 ` [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
2021-06-18 11:20 ` Geliang Tang
2021-06-21 3:51 ` Yonglong Li
2021-06-21 6:42 ` Geliang Tang
2021-06-21 7:15 ` Yonglong Li
2021-06-21 7:39 ` Geliang Tang
2021-06-21 7:49 ` Yonglong Li
2021-06-21 8:06 ` Geliang Tang
2021-06-21 7:42 ` Geliang Tang
2021-06-21 7:51 ` Yonglong Li
2021-06-21 8:29 ` Geliang Tang
2021-06-18 8:18 ` [PATCH v4 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT 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.