* [PATCH v6 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process
@ 2021-06-29 1:41 Yonglong Li
2021-06-29 1:41 ` [PATCH v6 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-29 1:41 UTC (permalink / raw)
To: mptcp; +Cc: mathew.j.martineau, geliangtang, Yonglong Li
fix issue: ADD_ADDR and RM_ADDR use pm.add_signal to mark event, so
in some case pm.add_signal will be flush when ADD_ADDR/RM_ADDR in
process.
fix issue: if ADD_ADDR and ADD_ADDR-echo process at the same time,
only one event can write pm.add_signal. so ADD_ADDR will process
after add_timer timeout or ADD_ADDR-echo will not be process.
Patch 1 fix ADD_ADDR and RM_ADDR maybe clear addr_signal each other.
Patch 2 and 3 deal ADD_ADDR and ADD_ADDR-echo with separately to fix
conflicts in using pm.addr_signal porcess.
Patch 4 MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT is not necessary.
v1->v2:
- remove READ_ONCE under the pm spin lock.
v2->v3:
- Patch 2: mptcp_pm_should_add_addr => mptcp_pm_should_add_signal_addr
- Patch 3: avoid read-modify-write of msk->pm.addr_signal and change
mptcp_pm_add_addr_signal to return void.
v3->v4:
- Patch 1: use ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO))
instead of BIT(MPTCP_RM_ADDR_SIGNAL)
- Patch 3: simple the code; init flags; fix wrong goto logic code;
v4->v5:
- Patch 3: simple the code of 'mptcp_established_options_add_addr'
v5->v6:
- Patch2: fix fails of 'mptcp_join.sh -t'. In mptcp_pm_add_addr_send_ack
without MPTCP_ADD_ADDR_SIGNAL check so pure ack can not be sent for
ADD_ADDR. That cause ADD_ADDR can not be sent in time.
- Patch3: refactor some code according Geliang's suggestions.
- Patch4: modify commit comment
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 v6 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
2021-06-29 1:41 [PATCH v6 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
@ 2021-06-29 1:41 ` Yonglong Li
2021-06-29 5:43 ` Geliang Tang
2021-06-29 1:41 ` [PATCH v6 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Yonglong Li @ 2021-06-29 1:41 UTC (permalink / raw)
To: mptcp; +Cc: mathew.j.martineau, geliangtang, Yonglong Li
ADD_ADDR share pm.addr_signal with RM_ADDR, so after RM_ADDR/ADD_ADDR
done we should not clean ADD_ADDR/RM_ADDR's addr_signal.
Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
net/mptcp/pm.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 9d00fa6..6c427c8 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -252,6 +252,7 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
struct mptcp_addr_info *saddr, bool *echo, bool *port)
{
+ u8 add_addr;
int ret = false;
spin_lock_bh(&msk->pm.lock);
@@ -267,7 +268,8 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
goto out_unlock;
*saddr = msk->pm.local;
- WRITE_ONCE(msk->pm.addr_signal, 0);
+ add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
+ WRITE_ONCE(msk->pm.addr_signal, add_addr);
ret = true;
out_unlock:
@@ -278,6 +280,7 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
struct mptcp_rm_list *rm_list)
{
+ u8 rm_addr;
int ret = false, len;
spin_lock_bh(&msk->pm.lock);
@@ -286,16 +289,17 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
if (!mptcp_pm_should_rm_signal(msk))
goto out_unlock;
+ rm_addr = msk->pm.addr_signal & ~BIT(MPTCP_RM_ADDR_SIGNAL);
len = mptcp_rm_addr_len(&msk->pm.rm_list_tx);
if (len < 0) {
- WRITE_ONCE(msk->pm.addr_signal, 0);
+ WRITE_ONCE(msk->pm.addr_signal, rm_addr);
goto out_unlock;
}
if (remaining < len)
goto out_unlock;
*rm_list = msk->pm.rm_list_tx;
- WRITE_ONCE(msk->pm.addr_signal, 0);
+ WRITE_ONCE(msk->pm.addr_signal, rm_addr);
ret = true;
out_unlock:
--
1.8.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v6 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
2021-06-29 1:41 [PATCH v6 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
2021-06-29 1:41 ` [PATCH v6 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
@ 2021-06-29 1:41 ` Yonglong Li
2021-06-29 1:41 ` [PATCH v6 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
2021-06-29 1:41 ` [PATCH v6 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-29 1:41 UTC (permalink / raw)
To: mptcp; +Cc: mathew.j.martineau, geliangtang, Yonglong Li
MPTCP_ADD_ADDR_SIGNAL only for action of sending ADD_ADDR
MPTCP_ADD_ADDR_ECHO only for action of sending echo ADD_ADDR
add a mptcp_addr_info in struct mptcp_out_options for echo ADD_ADDR
to prepare for the next patch.
Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
net/mptcp/pm.c | 16 ++++++++++------
net/mptcp/pm_netlink.c | 4 ++--
net/mptcp/protocol.h | 6 ++++++
3 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 6c427c8..cf873e9 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -18,19 +18,23 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
{
u8 add_addr = READ_ONCE(msk->pm.addr_signal);
- pr_debug("msk=%p, local_id=%d", msk, addr->id);
+ pr_debug("msk=%p, local_id=%d, echo:%d", msk, addr->id, echo);
lockdep_assert_held(&msk->pm.lock);
- if (add_addr) {
- pr_warn("addr_signal error, add_addr=%d", add_addr);
+ if (add_addr &
+ (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
+ pr_warn("addr_signal error, add_addr=%d, echo=%d", add_addr, echo);
return -EINVAL;
}
- msk->pm.local = *addr;
- add_addr |= BIT(MPTCP_ADD_ADDR_SIGNAL);
- if (echo)
+ if (echo) {
+ msk->pm.remote = *addr;
add_addr |= BIT(MPTCP_ADD_ADDR_ECHO);
+ } else {
+ msk->pm.local = *addr;
+ add_addr |= BIT(MPTCP_ADD_ADDR_SIGNAL);
+ }
if (addr->family == AF_INET6)
add_addr |= BIT(MPTCP_ADD_ADDR_IPV6);
if (addr->port)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 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 v6 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-29 1:41 [PATCH v6 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
2021-06-29 1:41 ` [PATCH v6 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
2021-06-29 1:41 ` [PATCH v6 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
@ 2021-06-29 1:41 ` Yonglong Li
2021-06-29 5:58 ` Geliang Tang
2021-06-29 1:41 ` [PATCH v6 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT Yonglong Li
3 siblings, 1 reply; 15+ messages in thread
From: Yonglong Li @ 2021-06-29 1:41 UTC (permalink / raw)
To: mptcp; +Cc: mathew.j.martineau, geliangtang, Yonglong Li
according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
ADD_ADDR/echo-ADD_ADDR option
add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
include/net/mptcp.h | 3 ++-
net/mptcp/options.c | 65 +++++++++++++++++++++++++++++++---------------------
net/mptcp/pm.c | 33 +++++++++++---------------
net/mptcp/protocol.h | 23 ++++++++++++-------
4 files changed, 69 insertions(+), 55 deletions(-)
diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index d61bbbf..d2c6ebe 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -61,7 +61,8 @@ struct mptcp_out_options {
u64 sndr_key;
u64 rcvr_key;
u64 ahmac;
- struct mptcp_addr_info addr;
+ struct mptcp_addr_info local;
+ struct mptcp_addr_info remote;
struct mptcp_rm_list rm_list;
u8 join_id;
u8 backup;
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 1aec016..1707bec 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
struct mptcp_sock *msk = mptcp_sk(subflow->conn);
bool drop_other_suboptions = false;
unsigned int opt_size = *size;
- bool echo;
- bool port;
- int len;
+ u8 add_addr, flags = 0xff;
+ int len = 0;
- if ((mptcp_pm_should_add_signal_ipv6(msk) ||
- mptcp_pm_should_add_signal_port(msk) ||
- mptcp_pm_should_add_signal_echo(msk)) &&
+ if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
+ return false;
+
+ if ((mptcp_pm_should_add_signal_echo(msk) ||
+ (mptcp_pm_should_add_signal_addr(msk) &&
+ (opts->local.family == AF_INET6 || opts->local.port))) &&
skb && skb_is_tcp_pure_ack(skb)) {
pr_debug("drop other suboptions");
opts->suboptions = 0;
@@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
drop_other_suboptions = true;
}
- if (!mptcp_pm_should_add_signal(msk) ||
- !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
- return false;
- len = mptcp_add_addr_len(opts->addr.family, echo, port);
+ if (mptcp_pm_should_add_signal_echo(msk)) {
+ flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
+ } else {
+ opts->ahmac = add_addr_generate_hmac(msk->local_key,
+ msk->remote_key,
+ &opts->local);
+ flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
+ }
+
+ len = mptcp_add_addr_len(opts);
if (remaining < len)
return false;
@@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
if (drop_other_suboptions)
*size -= opt_size;
opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
- if (!echo) {
- opts->ahmac = add_addr_generate_hmac(msk->local_key,
- msk->remote_key,
- &opts->addr);
- }
- pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
- opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
+
+ spin_lock_bh(&msk->pm.lock);
+ WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
+ spin_unlock_bh(&msk->pm.lock);
+
+ pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
+ add_addr, (opts->ahmac == 0), opts->local.id,
+ opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
return true;
}
@@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
mp_capable_done:
if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
+ struct mptcp_addr_info *addr = &opts->remote;
u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
u8 echo = MPTCP_ADDR_ECHO;
+ if (opts->ahmac)
+ addr = &opts->local;
+
#if IS_ENABLED(CONFIG_MPTCP_IPV6)
- if (opts->addr.family == AF_INET6)
+ if (addr->family == AF_INET6)
len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
#endif
- if (opts->addr.port)
+ if (addr->port)
len += TCPOLEN_MPTCP_PORT_LEN;
if (opts->ahmac) {
@@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
}
*ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
- len, echo, opts->addr.id);
- if (opts->addr.family == AF_INET) {
- memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
+ len, echo, addr->id);
+ if (addr->family == AF_INET) {
+ memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
ptr += 1;
}
#if IS_ENABLED(CONFIG_MPTCP_IPV6)
- else if (opts->addr.family == AF_INET6) {
- memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
+ else if (addr->family == AF_INET6) {
+ memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
ptr += 4;
}
#endif
- if (!opts->addr.port) {
+ if (!addr->port) {
if (opts->ahmac) {
put_unaligned_be64(opts->ahmac, ptr);
ptr += 2;
}
} else {
- u16 port = ntohs(opts->addr.port);
+ u16 port = ntohs(addr->port);
if (opts->ahmac) {
u8 *bptr = (u8 *)ptr;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index cf873e9..9c621293 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
/* path manager helpers */
-bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
- struct mptcp_addr_info *saddr, bool *echo, bool *port)
+bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
+ u8 *add_addr)
{
- 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;
+ if (!mptcp_pm_should_add_signal(msk)) {
+ spin_unlock_bh(&msk->pm.lock);
+ return false;
+ }
- *saddr = msk->pm.local;
- add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
- WRITE_ONCE(msk->pm.addr_signal, add_addr);
- ret = true;
+ opts->local = msk->pm.local;
+ opts->remote = msk->pm.remote;
+ *add_addr = msk->pm.addr_signal;
-out_unlock:
spin_unlock_bh(&msk->pm.lock);
- return ret;
+
+ 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);
+ return true;
}
bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index a0b0ec0..0bfbbdef 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
}
-static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
+static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
{
- u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
+ u8 len = 0;
+ struct mptcp_addr_info *addr = &opts->remote;
- if (family == AF_INET6)
- len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
- if (!echo)
+ if (opts->ahmac) {
+ addr = &opts->local;
len += MPTCPOPT_THMAC_LEN;
+ }
+
+ if (addr->family == AF_INET6)
+ len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
+ else
+ len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
+
/* account for 2 trailing 'nop' options */
- if (port)
+ if (addr->port)
len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
return len;
@@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
}
-bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
- struct mptcp_addr_info *saddr, bool *echo, bool *port);
+bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
+ u8 *add_addr);
bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
struct mptcp_rm_list *rm_list);
int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v6 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
2021-06-29 1:41 [PATCH v6 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-29 1:41 ` [PATCH v6 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
@ 2021-06-29 1:41 ` Yonglong Li
3 siblings, 0 replies; 15+ messages in thread
From: Yonglong Li @ 2021-06-29 1:41 UTC (permalink / raw)
To: mptcp; +Cc: mathew.j.martineau, geliangtang, Yonglong Li
MPTCP_ADD_ADDR_PORT and MPTCP_ADD_ADDR_PORT are not necessary, we
can get these info from pm.local 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 9c621293..4110f3b 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 0bfbbdef..43304ef 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -175,8 +175,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,
};
@@ -722,16 +720,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 v6 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
2021-06-29 1:41 ` [PATCH v6 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
@ 2021-06-29 5:43 ` Geliang Tang
0 siblings, 0 replies; 15+ messages in thread
From: Geliang Tang @ 2021-06-29 5:43 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau
Hi Yonglong,
Thank you for this new patch set!
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 上午9:42写道：
>
> 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;
Here we should use the reverse xmas tree order for variables definition:
int ret = false;
u8 add_addr;
>
> 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));
I think we could use mptcp_pm_should_add_signal_echo to choose ECHO or
SIGNAL to set here:
if (mptcp_pm_should_add_signal_echo(msk))
add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
else
add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
> + WRITE_ONCE(msk->pm.addr_signal, add_addr);
> ret = true;
>
> out_unlock:
> @@ -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 [flat|nested] 15+ messages in thread
* Re: [PATCH v6 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-29 1:41 ` [PATCH v6 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
@ 2021-06-29 5:58 ` Geliang Tang
2021-06-29 6:05 ` Geliang Tang
2021-06-29 7:01 ` Yonglong Li
0 siblings, 2 replies; 15+ messages in thread
From: Geliang Tang @ 2021-06-29 5:58 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 上午9:42写道：
>
> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> ADD_ADDR/echo-ADD_ADDR option
>
> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>
> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> ---
> include/net/mptcp.h | 3 ++-
> net/mptcp/options.c | 65 +++++++++++++++++++++++++++++++---------------------
> net/mptcp/pm.c | 33 +++++++++++---------------
> net/mptcp/protocol.h | 23 ++++++++++++-------
> 4 files changed, 69 insertions(+), 55 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index d61bbbf..d2c6ebe 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -61,7 +61,8 @@ struct mptcp_out_options {
> u64 sndr_key;
> u64 rcvr_key;
> u64 ahmac;
> - struct mptcp_addr_info addr;
> + struct mptcp_addr_info local;
> + struct mptcp_addr_info remote;
> struct mptcp_rm_list rm_list;
> u8 join_id;
> u8 backup;
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..1707bec 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> bool drop_other_suboptions = false;
> unsigned int opt_size = *size;
> - bool echo;
> - bool port;
> - int len;
> + u8 add_addr, flags = 0xff;
> + int len = 0;
>
> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> - mptcp_pm_should_add_signal_port(msk) ||
> - mptcp_pm_should_add_signal_echo(msk)) &&
> + if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
> + return false;
This add_addr argument is useless, let's drop it.
And here add back mptcp_pm_should_add_signal check here. The original code
called mptcp_pm_should_add_signal twice for double check, once out of pm
lock, once under pm lock. We should keep it.
> +
> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> + (mptcp_pm_should_add_signal_addr(msk) &&
> + (opts->local.family == AF_INET6 || opts->local.port))) &&
> skb && skb_is_tcp_pure_ack(skb)) {
> pr_debug("drop other suboptions");
> opts->suboptions = 0;
> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> drop_other_suboptions = true;
> }
>
> - if (!mptcp_pm_should_add_signal(msk) ||
> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> - return false;
>
> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> + if (mptcp_pm_should_add_signal_echo(msk)) {
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> + } else {
> + opts->ahmac = add_addr_generate_hmac(msk->local_key,
> + msk->remote_key,
> + &opts->local);
Keep this ahmac generating code after opts->suboptions set just like the
original code, since ahmac is the more expensive to populate. If remaining
length isn't enough, no need to set ahmac.
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> + }
> +
> + len = mptcp_add_addr_len(opts);
> if (remaining < len)
> return false;
>
> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> if (drop_other_suboptions)
> *size -= opt_size;
> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> - if (!echo) {
> - opts->ahmac = add_addr_generate_hmac(msk->local_key,
> - msk->remote_key,
> - &opts->addr);
> - }
> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> +
> + spin_lock_bh(&msk->pm.lock);
> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> + spin_unlock_bh(&msk->pm.lock);
addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
set it again. I thinks this trunk and all the flags set above should be
dropped.
> +
> + pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> + add_addr, (opts->ahmac == 0), opts->local.id,
> + opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>
> return true;
> }
The whole function is something like this:
'''
struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
struct mptcp_sock *msk = mptcp_sk(subflow->conn);
bool drop_other_suboptions = false;
unsigned int opt_size = *size;
int len;
if (!mptcp_pm_should_add_signal(msk) ||
!mptcp_pm_add_addr_signal(msk, remaining, opts))
return false;
if ((mptcp_pm_should_add_signal_echo(msk) ||
(mptcp_pm_should_add_signal_addr(msk) &&
(opts->local.family == AF_INET6 || opts->local.port))) &&
skb && skb_is_tcp_pure_ack(skb)) {
pr_debug("drop other suboptions");
opts->suboptions = 0;
opts->ext_copy.use_ack = 0;
opts->ext_copy.use_map = 0;
remaining += opt_size;
drop_other_suboptions = true;
}
len = mptcp_add_addr_len(opts);
if (remaining < len)
return false;
*size = len;
if (drop_other_suboptions)
*size -= opt_size;
opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
if (mptcp_pm_should_add_signal_addr(msk)) {
opts->ahmac = add_addr_generate_hmac(msk->local_key,
msk->remote_key,
&opts->local);
}
pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
opts->ahmac, ntohs(opts->local.port),
opts->remote.id, ntohs(opts->remote.port));
return true;
'''
> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>
> mp_capable_done:
> if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> + struct mptcp_addr_info *addr = &opts->remote;
We can simplify it like this:
struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
&opts->remote;
> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> u8 echo = MPTCP_ADDR_ECHO;
>
> + if (opts->ahmac)
> + addr = &opts->local;
And this trunk can be dropped.
> +
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> - if (opts->addr.family == AF_INET6)
> + if (addr->family == AF_INET6)
> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> #endif
>
> - if (opts->addr.port)
> + if (addr->port)
> len += TCPOLEN_MPTCP_PORT_LEN;
>
> if (opts->ahmac) {
> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> }
>
> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> - len, echo, opts->addr.id);
> - if (opts->addr.family == AF_INET) {
> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> + len, echo, addr->id);
> + if (addr->family == AF_INET) {
> + memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
> ptr += 1;
> }
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> - else if (opts->addr.family == AF_INET6) {
> - memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
> + else if (addr->family == AF_INET6) {
> + memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
> ptr += 4;
> }
> #endif
>
> - if (!opts->addr.port) {
> + if (!addr->port) {
> if (opts->ahmac) {
> put_unaligned_be64(opts->ahmac, ptr);
> ptr += 2;
> }
> } else {
> - u16 port = ntohs(opts->addr.port);
> + u16 port = ntohs(addr->port);
>
> if (opts->ahmac) {
> u8 *bptr = (u8 *)ptr;
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index cf873e9..9c621293 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>
> /* path manager helpers */
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> - struct mptcp_addr_info *saddr, bool *echo, bool *port)
> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> + u8 *add_addr)
Drop this add_addr argument.
> {
> - u8 add_addr;
> - int ret = false;
> -
> spin_lock_bh(&msk->pm.lock);
>
> - /* double check after the lock is acquired */
> - if (!mptcp_pm_should_add_signal(msk))
> - goto out_unlock;
Keep this double check code.
> -
> - *echo = mptcp_pm_should_add_signal_echo(msk);
> - *port = mptcp_pm_should_add_signal_port(msk);
> -
> - if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> - goto out_unlock;
Keep this length double check code too.
> + if (!mptcp_pm_should_add_signal(msk)) {
> + spin_unlock_bh(&msk->pm.lock);
> + return false;
> + }
>
> - *saddr = msk->pm.local;
> - add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
- WRITE_ONCE(msk->pm.addr_signal, add_addr);
This code is just added in patch 1, I think we should keep it. And no need
to write addr_signal again in mptcp_established_options_add_addr.
> - ret = true;
> + opts->local = msk->pm.local;
> + opts->remote = msk->pm.remote;
> + *add_addr = msk->pm.addr_signal;
>
> -out_unlock:
> spin_unlock_bh(&msk->pm.lock);
> - return ret;
Keep this out_unlock code.
> +
> + if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> + mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
I'm no sure why we need this two lines, and why you use '&&' here. Do you
mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?
> + return true;
> }
The whole function is something like this:
'''
int ret = false;
u8 add_addr;
spin_lock_bh(&msk->pm.lock);
/* double check after the lock is acquired */
if (!mptcp_pm_should_add_signal(msk))
goto out_unlock;
if (remaining < mptcp_add_addr_len(opts))
goto out_unlock;
opts->local = msk->pm.local;
opts->remote = msk->pm.remote;
if (mptcp_pm_should_add_signal_echo(msk))
add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
else
add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
WRITE_ONCE(msk->pm.addr_signal, add_addr);
ret = true;
out_unlock:
spin_unlock_bh(&msk->pm.lock);
if (ret && mptcp_pm_should_add_signal_echo(msk) &&
mptcp_pm_should_add_signal_addr(msk))
mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
return ret;
'''
>
> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index a0b0ec0..0bfbbdef 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
> return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
> }
>
> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
> {
> - u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> + u8 len = 0;
> + struct mptcp_addr_info *addr = &opts->remote;
We can simplify it like this:
struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
&opts->remote;
And keep the orignal code unchanged.
>
> - if (family == AF_INET6)
> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> - if (!echo)
> + if (opts->ahmac) {
> + addr = &opts->local;
> len += MPTCPOPT_THMAC_LEN;
> + }
> +
> + if (addr->family == AF_INET6)
> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + else
> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
> +
> /* account for 2 trailing 'nop' options */
> - if (port)
> + if (addr->port)
> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>
> return len;
The whole function is something like this:
'''
struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
&opts->remote;
u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
if (addr->family == AF_INET6)
len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
if (opts->ahmac)
len += MPTCPOPT_THMAC_LEN;
/* account for 2 trailing 'nop' options */
if (addr->port)
len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
return len;
'''
Thanks.
-Geliang
> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
> return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
> }
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> - struct mptcp_addr_info *saddr, bool *echo, bool *port);
> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> + u8 *add_addr);
> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> struct mptcp_rm_list *rm_list);
> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> --
> 1.8.3.1
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-29 5:58 ` Geliang Tang
@ 2021-06-29 6:05 ` Geliang Tang
2021-06-29 7:01 ` Yonglong Li
1 sibling, 0 replies; 15+ messages in thread
From: Geliang Tang @ 2021-06-29 6:05 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau
Geliang Tang <geliangtang@gmail.com> 于2021年6月29日周二 下午1:58写道：
>
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 上午9:42写道：
> >
> > according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> > ADD_ADDR/echo-ADD_ADDR option
> >
> > add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >
> > Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> > ---
> > include/net/mptcp.h | 3 ++-
> > net/mptcp/options.c | 65 +++++++++++++++++++++++++++++++---------------------
> > net/mptcp/pm.c | 33 +++++++++++---------------
> > net/mptcp/protocol.h | 23 ++++++++++++-------
> > 4 files changed, 69 insertions(+), 55 deletions(-)
> >
> > diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> > index d61bbbf..d2c6ebe 100644
> > --- a/include/net/mptcp.h
> > +++ b/include/net/mptcp.h
> > @@ -61,7 +61,8 @@ struct mptcp_out_options {
> > u64 sndr_key;
> > u64 rcvr_key;
> > u64 ahmac;
> > - struct mptcp_addr_info addr;
> > + struct mptcp_addr_info local;
> > + struct mptcp_addr_info remote;
> > struct mptcp_rm_list rm_list;
> > u8 join_id;
> > u8 backup;
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index 1aec016..1707bec 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> > struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> > bool drop_other_suboptions = false;
> > unsigned int opt_size = *size;
> > - bool echo;
> > - bool port;
> > - int len;
> > + u8 add_addr, flags = 0xff;
> > + int len = 0;
> >
> > - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> > - mptcp_pm_should_add_signal_port(msk) ||
> > - mptcp_pm_should_add_signal_echo(msk)) &&
> > + if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
> > + return false;
>
> This add_addr argument is useless, let's drop it.
>
> And here add back mptcp_pm_should_add_signal check here. The original code
> called mptcp_pm_should_add_signal twice for double check, once out of pm
> lock, once under pm lock. We should keep it.
>
> > +
> > + if ((mptcp_pm_should_add_signal_echo(msk) ||
> > + (mptcp_pm_should_add_signal_addr(msk) &&
> > + (opts->local.family == AF_INET6 || opts->local.port))) &&
> > skb && skb_is_tcp_pure_ack(skb)) {
> > pr_debug("drop other suboptions");
> > opts->suboptions = 0;
> > @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> > drop_other_suboptions = true;
> > }
> >
> > - if (!mptcp_pm_should_add_signal(msk) ||
> > - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> > - return false;
> >
> > - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> > + if (mptcp_pm_should_add_signal_echo(msk)) {
> > + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> > + } else {
> > + opts->ahmac = add_addr_generate_hmac(msk->local_key,
> > + msk->remote_key,
> > + &opts->local);
>
> Keep this ahmac generating code after opts->suboptions set just like the
> original code, since ahmac is the more expensive to populate. If remaining
> length isn't enough, no need to set ahmac.
>
> > + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> > + }
> > +
> > + len = mptcp_add_addr_len(opts);
> > if (remaining < len)
> > return false;
> >
> > @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> > if (drop_other_suboptions)
> > *size -= opt_size;
> > opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> > - if (!echo) {
> > - opts->ahmac = add_addr_generate_hmac(msk->local_key,
> > - msk->remote_key,
> > - &opts->addr);
> > - }
> > - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> > - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> > +
> > + spin_lock_bh(&msk->pm.lock);
> > + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> > + spin_unlock_bh(&msk->pm.lock);
>
> addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
> set it again. I thinks this trunk and all the flags set above should be
> dropped.
>
> > +
> > + pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> > + add_addr, (opts->ahmac == 0), opts->local.id,
> > + opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
> >
> > return true;
> > }
>
> The whole function is something like this:
> '''
> struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> bool drop_other_suboptions = false;
> unsigned int opt_size = *size;
> int len;
>
> if (!mptcp_pm_should_add_signal(msk) ||
> !mptcp_pm_add_addr_signal(msk, remaining, opts))
> return false;
>
> if ((mptcp_pm_should_add_signal_echo(msk) ||
> (mptcp_pm_should_add_signal_addr(msk) &&
> (opts->local.family == AF_INET6 || opts->local.port))) &&
> skb && skb_is_tcp_pure_ack(skb)) {
> pr_debug("drop other suboptions");
> opts->suboptions = 0;
> opts->ext_copy.use_ack = 0;
> opts->ext_copy.use_map = 0;
> remaining += opt_size;
> drop_other_suboptions = true;
> }
>
> len = mptcp_add_addr_len(opts);
> if (remaining < len)
> return false;
>
> *size = len;
> if (drop_other_suboptions)
> *size -= opt_size;
> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> if (mptcp_pm_should_add_signal_addr(msk)) {
> opts->ahmac = add_addr_generate_hmac(msk->local_key,
> msk->remote_key,
> &opts->local);
> }
>
Sorry, no need to add this blank line here.
> pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
> ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
> opts->ahmac, ntohs(opts->local.port),
> opts->remote.id, ntohs(opts->remote.port));
>
> return true;
> '''
>
> > @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >
> > mp_capable_done:
> > if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> > + struct mptcp_addr_info *addr = &opts->remote;
>
> We can simplify it like this:
> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> &opts->remote;
>
> > u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> > u8 echo = MPTCP_ADDR_ECHO;
> >
> > + if (opts->ahmac)
> > + addr = &opts->local;
>
> And this trunk can be dropped.
>
> > +
> > #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> > - if (opts->addr.family == AF_INET6)
> > + if (addr->family == AF_INET6)
> > len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> > #endif
> >
> > - if (opts->addr.port)
> > + if (addr->port)
> > len += TCPOLEN_MPTCP_PORT_LEN;
> >
> > if (opts->ahmac) {
> > @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> > }
> >
> > *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> > - len, echo, opts->addr.id);
> > - if (opts->addr.family == AF_INET) {
> > - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> > + len, echo, addr->id);
> > + if (addr->family == AF_INET) {
> > + memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
> > ptr += 1;
> > }
> > #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> > - else if (opts->addr.family == AF_INET6) {
> > - memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
> > + else if (addr->family == AF_INET6) {
> > + memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
> > ptr += 4;
> > }
> > #endif
> >
> > - if (!opts->addr.port) {
> > + if (!addr->port) {
> > if (opts->ahmac) {
> > put_unaligned_be64(opts->ahmac, ptr);
> > ptr += 2;
> > }
> > } else {
> > - u16 port = ntohs(opts->addr.port);
> > + u16 port = ntohs(addr->port);
> >
> > if (opts->ahmac) {
> > u8 *bptr = (u8 *)ptr;
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index cf873e9..9c621293 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >
> > /* path manager helpers */
> >
> > -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > - struct mptcp_addr_info *saddr, bool *echo, bool *port)
> > +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> > + u8 *add_addr)
>
> Drop this add_addr argument.
>
> > {
> > - u8 add_addr;
> > - int ret = false;
> > -
> > spin_lock_bh(&msk->pm.lock);
> >
> > - /* double check after the lock is acquired */
> > - if (!mptcp_pm_should_add_signal(msk))
> > - goto out_unlock;
>
> Keep this double check code.
>
> > -
> > - *echo = mptcp_pm_should_add_signal_echo(msk);
> > - *port = mptcp_pm_should_add_signal_port(msk);
> > -
> > - if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> > - goto out_unlock;
>
> Keep this length double check code too.
>
> > + if (!mptcp_pm_should_add_signal(msk)) {
> > + spin_unlock_bh(&msk->pm.lock);
> > + return false;
> > + }
> >
> > - *saddr = msk->pm.local;
> > - add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> - WRITE_ONCE(msk->pm.addr_signal, add_addr);
>
> This code is just added in patch 1, I think we should keep it. And no need
> to write addr_signal again in mptcp_established_options_add_addr.
>
> > - ret = true;
> > + opts->local = msk->pm.local;
> > + opts->remote = msk->pm.remote;
> > + *add_addr = msk->pm.addr_signal;
> >
> > -out_unlock:
> > spin_unlock_bh(&msk->pm.lock);
> > - return ret;
>
> Keep this out_unlock code.
>
> > +
> > + if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> > + mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>
> Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
>
> I'm no sure why we need this two lines, and why you use '&&' here. Do you
> mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?
>
> > + return true;
> > }
>
> The whole function is something like this:
> '''
> int ret = false;
> u8 add_addr;
>
> spin_lock_bh(&msk->pm.lock);
>
> /* double check after the lock is acquired */
> if (!mptcp_pm_should_add_signal(msk))
> goto out_unlock;
>
> if (remaining < mptcp_add_addr_len(opts))
> goto out_unlock;
>
> opts->local = msk->pm.local;
> opts->remote = msk->pm.remote;
> if (mptcp_pm_should_add_signal_echo(msk))
> add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
> else
> add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
> WRITE_ONCE(msk->pm.addr_signal, add_addr);
> ret = true;
>
> out_unlock:
> spin_unlock_bh(&msk->pm.lock);
> if (ret && mptcp_pm_should_add_signal_echo(msk) &&
> mptcp_pm_should_add_signal_addr(msk))
> mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> return ret;
> '''
>
> >
> > bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index a0b0ec0..0bfbbdef 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
> > return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
> > }
> >
> > -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> > +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
> > {
> > - u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> > + u8 len = 0;
> > + struct mptcp_addr_info *addr = &opts->remote;
>
> We can simplify it like this:
> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> &opts->remote;
>
> And keep the orignal code unchanged.
>
> >
> > - if (family == AF_INET6)
> > - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> > - if (!echo)
> > + if (opts->ahmac) {
> > + addr = &opts->local;
> > len += MPTCPOPT_THMAC_LEN;
> > + }
> > +
> > + if (addr->family == AF_INET6)
> > + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> > + else
> > + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
> > +
> > /* account for 2 trailing 'nop' options */
> > - if (port)
> > + if (addr->port)
> > len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >
> > return len;
>
> The whole function is something like this:
> '''
> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> &opts->remote;
> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>
> if (addr->family == AF_INET6)
> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> if (opts->ahmac)
> len += MPTCPOPT_THMAC_LEN;
> /* account for 2 trailing 'nop' options */
> if (addr->port)
> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>
> return len;
> '''
>
> Thanks.
> -Geliang
>
> > @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
> > return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
> > }
> >
> > -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > - struct mptcp_addr_info *saddr, bool *echo, bool *port);
> > +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> > + u8 *add_addr);
> > bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > struct mptcp_rm_list *rm_list);
> > int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> > --
> > 1.8.3.1
> >
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-29 5:58 ` Geliang Tang
2021-06-29 6:05 ` Geliang Tang
@ 2021-06-29 7:01 ` Yonglong Li
2021-06-29 7:35 ` Geliang Tang
1 sibling, 1 reply; 15+ messages in thread
From: Yonglong Li @ 2021-06-29 7:01 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp, Mat Martineau
Hi Geiliang, Thanks for your reviews.
On 2021/6/29 13:58, Geliang Tang wrote:
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 上午9:42写道：
>>
>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>> ADD_ADDR/echo-ADD_ADDR option
>>
>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>
>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
>> ---
>> include/net/mptcp.h | 3 ++-
>> net/mptcp/options.c | 65 +++++++++++++++++++++++++++++++---------------------
>> net/mptcp/pm.c | 33 +++++++++++---------------
>> net/mptcp/protocol.h | 23 ++++++++++++-------
>> 4 files changed, 69 insertions(+), 55 deletions(-)
>>
>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
>> index d61bbbf..d2c6ebe 100644
>> --- a/include/net/mptcp.h
>> +++ b/include/net/mptcp.h
>> @@ -61,7 +61,8 @@ struct mptcp_out_options {
>> u64 sndr_key;
>> u64 rcvr_key;
>> u64 ahmac;
>> - struct mptcp_addr_info addr;
>> + struct mptcp_addr_info local;
>> + struct mptcp_addr_info remote;
>> struct mptcp_rm_list rm_list;
>> u8 join_id;
>> u8 backup;
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index 1aec016..1707bec 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>> bool drop_other_suboptions = false;
>> unsigned int opt_size = *size;
>> - bool echo;
>> - bool port;
>> - int len;
>> + u8 add_addr, flags = 0xff;
>> + int len = 0;
>>
>> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>> - mptcp_pm_should_add_signal_port(msk) ||
>> - mptcp_pm_should_add_signal_echo(msk)) &&
>> + if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
>> + return false;
>
> This add_addr argument is useless, let's drop it.
>
we can use add_addr use in debug log.
> And here add back mptcp_pm_should_add_signal check here. The original code
> called mptcp_pm_should_add_signal twice for double check, once out of pm
> lock, once under pm lock. We should keep it.
Sorry, I think double check is not necessary. does we need double check?
>
>> +
>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
>> + (mptcp_pm_should_add_signal_addr(msk) &&
>> + (opts->local.family == AF_INET6 || opts->local.port))) &&
>> skb && skb_is_tcp_pure_ack(skb)) {
>> pr_debug("drop other suboptions");
>> opts->suboptions = 0;
>> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>> drop_other_suboptions = true;
>> }
>>
>> - if (!mptcp_pm_should_add_signal(msk) ||
>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>> - return false;
>>
>> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
>> + if (mptcp_pm_should_add_signal_echo(msk)) {
>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>> + } else {
>> + opts->ahmac = add_addr_generate_hmac(msk->local_key,
>> + msk->remote_key,
>> + &opts->local);
>
> Keep this ahmac generating code after opts->suboptions set just like the
> original code, since ahmac is the more expensive to populate. If remaining
> length isn't enough, no need to set ahmac.
because mptcp_add_addr_len(opts) will use ahmac to calculate len of opts, so I think Keep this ahmac
generating code after opts->suboptions set is not ok.
>
>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>> + }
>> +
>> + len = mptcp_add_addr_len(opts);
>> if (remaining < len)
>> return false;
>>
>> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>> if (drop_other_suboptions)
>> *size -= opt_size;
>> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>> - if (!echo) {
>> - opts->ahmac = add_addr_generate_hmac(msk->local_key,
>> - msk->remote_key,
>> - &opts->addr);
>> - }
>> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
>> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
>> +
>> + spin_lock_bh(&msk->pm.lock);
>> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
>> + spin_unlock_bh(&msk->pm.lock);
>
> addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
> set it again. I thinks this trunk and all the flags set above should be
> dropped.
Because MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO maybe set at the same time.
So i think we should only unset one flag.
>
>> +
>> + pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
>> + add_addr, (opts->ahmac == 0), opts->local.id,
>> + opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>>
>> return true;
>> }
>
> The whole function is something like this:
> '''
> struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> bool drop_other_suboptions = false;
> unsigned int opt_size = *size;
> int len;
>
> if (!mptcp_pm_should_add_signal(msk) ||
> !mptcp_pm_add_addr_signal(msk, remaining, opts))
> return false;
>
> if ((mptcp_pm_should_add_signal_echo(msk) ||
> (mptcp_pm_should_add_signal_addr(msk) &&
> (opts->local.family == AF_INET6 || opts->local.port))) &&
> skb && skb_is_tcp_pure_ack(skb)) {
> pr_debug("drop other suboptions");
> opts->suboptions = 0;
> opts->ext_copy.use_ack = 0;
> opts->ext_copy.use_map = 0;
> remaining += opt_size;
> drop_other_suboptions = true;
> }
>
> len = mptcp_add_addr_len(opts);
> if (remaining < len)
> return false;
>
> *size = len;
> if (drop_other_suboptions)
> *size -= opt_size;
> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> if (mptcp_pm_should_add_signal_addr(msk)) {
> opts->ahmac = add_addr_generate_hmac(msk->local_key,
> msk->remote_key,
> &opts->local);
> }
>
> pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
> ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
> opts->ahmac, ntohs(opts->local.port),
> opts->remote.id, ntohs(opts->remote.port));
>
> return true;
> '''
>
>> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>
>> mp_capable_done:
>> if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
>> + struct mptcp_addr_info *addr = &opts->remote;
>
> We can simplify it like this:
> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> &opts->remote;
>
>> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>> u8 echo = MPTCP_ADDR_ECHO;
>>
>> + if (opts->ahmac)
>> + addr = &opts->local;
>
> And this trunk can be dropped.
>
>> +
>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>> - if (opts->addr.family == AF_INET6)
>> + if (addr->family == AF_INET6)
>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>> #endif
>>
>> - if (opts->addr.port)
>> + if (addr->port)
>> len += TCPOLEN_MPTCP_PORT_LEN;
>>
>> if (opts->ahmac) {
>> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>> }
>>
>> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
>> - len, echo, opts->addr.id);
>> - if (opts->addr.family == AF_INET) {
>> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
>> + len, echo, addr->id);
>> + if (addr->family == AF_INET) {
>> + memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
>> ptr += 1;
>> }
>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>> - else if (opts->addr.family == AF_INET6) {
>> - memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
>> + else if (addr->family == AF_INET6) {
>> + memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
>> ptr += 4;
>> }
>> #endif
>>
>> - if (!opts->addr.port) {
>> + if (!addr->port) {
>> if (opts->ahmac) {
>> put_unaligned_be64(opts->ahmac, ptr);
>> ptr += 2;
>> }
>> } else {
>> - u16 port = ntohs(opts->addr.port);
>> + u16 port = ntohs(addr->port);
>>
>> if (opts->ahmac) {
>> u8 *bptr = (u8 *)ptr;
>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>> index cf873e9..9c621293 100644
>> --- a/net/mptcp/pm.c
>> +++ b/net/mptcp/pm.c
>> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>
>> /* path manager helpers */
>>
>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>> - struct mptcp_addr_info *saddr, bool *echo, bool *port)
>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
>> + u8 *add_addr)
>
> Drop this add_addr argument.
>
>> {
>> - u8 add_addr;
>> - int ret = false;
>> -
>> spin_lock_bh(&msk->pm.lock);
>>
>> - /* double check after the lock is acquired */
>> - if (!mptcp_pm_should_add_signal(msk))
>> - goto out_unlock;
>
> Keep this double check code.
>
>> -
>> - *echo = mptcp_pm_should_add_signal_echo(msk);
>> - *port = mptcp_pm_should_add_signal_port(msk);
>> -
>> - if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
>> - goto out_unlock;
>
> Keep this length double check code too.
>
>> + if (!mptcp_pm_should_add_signal(msk)) {
>> + spin_unlock_bh(&msk->pm.lock);
>> + return false;
>> + }
>>
>> - *saddr = msk->pm.local;
>> - add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> - WRITE_ONCE(msk->pm.addr_signal, add_addr);
>
> This code is just added in patch 1, I think we should keep it. And no need
> to write addr_signal again in mptcp_established_options_add_addr.
>
>> - ret = true;
>> + opts->local = msk->pm.local;
>> + opts->remote = msk->pm.remote;
>> + *add_addr = msk->pm.addr_signal;
>>
>> -out_unlock:
>> spin_unlock_bh(&msk->pm.lock);
>> - return ret;
>
> Keep this out_unlock code.
>
>> +
>> + if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
>> + mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>
> Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
>
> I'm no sure why we need this two lines, and why you use '&&' here. Do you
> mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?
>
>> + return true;
>> }
>
> The whole function is something like this:
> '''
> int ret = false;
> u8 add_addr;
>
> spin_lock_bh(&msk->pm.lock);
>
> /* double check after the lock is acquired */
> if (!mptcp_pm_should_add_signal(msk))
> goto out_unlock;
>
> if (remaining < mptcp_add_addr_len(opts))
> goto out_unlock;
>
> opts->local = msk->pm.local;
> opts->remote = msk->pm.remote;
> if (mptcp_pm_should_add_signal_echo(msk))
> add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
> else
> add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
> WRITE_ONCE(msk->pm.addr_signal, add_addr);
> ret = true;
>
> out_unlock:
> spin_unlock_bh(&msk->pm.lock);
> if (ret && mptcp_pm_should_add_signal_echo(msk) &&
> mptcp_pm_should_add_signal_addr(msk))
> mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> return ret;
> '''
>
>>
>> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index a0b0ec0..0bfbbdef 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
>> return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
>> }
>>
>> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
>> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
>> {
>> - u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>> + u8 len = 0;
>> + struct mptcp_addr_info *addr = &opts->remote;
>
> We can simplify it like this:
> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> &opts->remote;
>
> And keep the orignal code unchanged.
>
>>
>> - if (family == AF_INET6)
>> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>> - if (!echo)
>> + if (opts->ahmac) {
>> + addr = &opts->local;
>> len += MPTCPOPT_THMAC_LEN;
>> + }
>> +
>> + if (addr->family == AF_INET6)
>> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>> + else
>> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>> +
>> /* account for 2 trailing 'nop' options */
>> - if (port)
>> + if (addr->port)
>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>
>> return len;
>
> The whole function is something like this:
> '''
> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> &opts->remote;
> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>
> if (addr->family == AF_INET6)
> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> if (opts->ahmac)
> len += MPTCPOPT_THMAC_LEN;
> /* account for 2 trailing 'nop' options */
> if (addr->port)
> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>
> return len;
> '''
>
> Thanks.
> -Geliang
>
>> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
>> return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
>> }
>>
>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>> - struct mptcp_addr_info *saddr, bool *echo, bool *port);
>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
>> + u8 *add_addr);
>> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>> struct mptcp_rm_list *rm_list);
>> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
>> --
>> 1.8.3.1
>>
>>
>
>
--
Li YongLong
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-29 7:01 ` Yonglong Li
@ 2021-06-29 7:35 ` Geliang Tang
2021-06-29 7:54 ` Yonglong Li
0 siblings, 1 reply; 15+ messages in thread
From: Geliang Tang @ 2021-06-29 7:35 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 下午3:02写道：
>
>
> Hi Geiliang, Thanks for your reviews.
>
> On 2021/6/29 13:58, Geliang Tang wrote:
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 上午9:42写道：
> >>
> >> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> >> ADD_ADDR/echo-ADD_ADDR option
> >>
> >> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >>
> >> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> >> ---
> >> include/net/mptcp.h | 3 ++-
> >> net/mptcp/options.c | 65 +++++++++++++++++++++++++++++++---------------------
> >> net/mptcp/pm.c | 33 +++++++++++---------------
> >> net/mptcp/protocol.h | 23 ++++++++++++-------
> >> 4 files changed, 69 insertions(+), 55 deletions(-)
> >>
> >> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> >> index d61bbbf..d2c6ebe 100644
> >> --- a/include/net/mptcp.h
> >> +++ b/include/net/mptcp.h
> >> @@ -61,7 +61,8 @@ struct mptcp_out_options {
> >> u64 sndr_key;
> >> u64 rcvr_key;
> >> u64 ahmac;
> >> - struct mptcp_addr_info addr;
> >> + struct mptcp_addr_info local;
> >> + struct mptcp_addr_info remote;
> >> struct mptcp_rm_list rm_list;
> >> u8 join_id;
> >> u8 backup;
> >> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >> index 1aec016..1707bec 100644
> >> --- a/net/mptcp/options.c
> >> +++ b/net/mptcp/options.c
> >> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >> bool drop_other_suboptions = false;
> >> unsigned int opt_size = *size;
> >> - bool echo;
> >> - bool port;
> >> - int len;
> >> + u8 add_addr, flags = 0xff;
> >> + int len = 0;
> >>
> >> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> >> - mptcp_pm_should_add_signal_port(msk) ||
> >> - mptcp_pm_should_add_signal_echo(msk)) &&
> >> + if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
> >> + return false;
> >
> > This add_addr argument is useless, let's drop it.
> >
> we can use add_addr use in debug log.
I think it's not worth adding a new argument just for debugging.
>
> > And here add back mptcp_pm_should_add_signal check here. The original code
> > called mptcp_pm_should_add_signal twice for double check, once out of pm
> > lock, once under pm lock. We should keep it.
> Sorry, I think double check is not necessary. does we need double check?
I think we should keep the original logic here. If we want to drop this
double check or something, we should do it in another patch, don't mix too
much things in one patch.
>
> >
> >> +
> >> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> >> + (mptcp_pm_should_add_signal_addr(msk) &&
> >> + (opts->local.family == AF_INET6 || opts->local.port))) &&
> >> skb && skb_is_tcp_pure_ack(skb)) {
> >> pr_debug("drop other suboptions");
> >> opts->suboptions = 0;
> >> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >> drop_other_suboptions = true;
> >> }
> >>
> >> - if (!mptcp_pm_should_add_signal(msk) ||
> >> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> >> - return false;
> >>
> >> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> >> + if (mptcp_pm_should_add_signal_echo(msk)) {
> >> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> >> + } else {
> >> + opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >> + msk->remote_key,
> >> + &opts->local);
> >
> > Keep this ahmac generating code after opts->suboptions set just like the
> > original code, since ahmac is the more expensive to populate. If remaining
> > length isn't enough, no need to set ahmac.
>
> because mptcp_add_addr_len(opts) will use ahmac to calculate len of opts, so I think Keep this ahmac
> generating code after opts->suboptions set is not ok.
So we should use mptcp_pm_should_add_signal_addr instead of opts->ahmac in
mptcp_add_addr_len.
>
> >
> >> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> >> + }
> >> +
> >> + len = mptcp_add_addr_len(opts);
> >> if (remaining < len)
> >> return false;
> >>
> >> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >> if (drop_other_suboptions)
> >> *size -= opt_size;
> >> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >> - if (!echo) {
> >> - opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >> - msk->remote_key,
> >> - &opts->addr);
> >> - }
> >> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> >> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> >> +
> >> + spin_lock_bh(&msk->pm.lock);
> >> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> >> + spin_unlock_bh(&msk->pm.lock);
> >
> > addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
> > set it again. I thinks this trunk and all the flags set above should be
> > dropped.
>
> Because MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO maybe set at the same time.
> So i think we should only unset one flag.
We can only unset one flag in mptcp_pm_add_addr_signal, see my comment in
patch 1.
-Geliang
>
> >
> >> +
> >> + pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> >> + add_addr, (opts->ahmac == 0), opts->local.id,
> >> + opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
> >>
> >> return true;
> >> }
> >
> > The whole function is something like this:
> > '''
> > struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> > struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> > bool drop_other_suboptions = false;
> > unsigned int opt_size = *size;
> > int len;
> >
> > if (!mptcp_pm_should_add_signal(msk) ||
> > !mptcp_pm_add_addr_signal(msk, remaining, opts))
> > return false;
> >
> > if ((mptcp_pm_should_add_signal_echo(msk) ||
> > (mptcp_pm_should_add_signal_addr(msk) &&
> > (opts->local.family == AF_INET6 || opts->local.port))) &&
> > skb && skb_is_tcp_pure_ack(skb)) {
> > pr_debug("drop other suboptions");
> > opts->suboptions = 0;
> > opts->ext_copy.use_ack = 0;
> > opts->ext_copy.use_map = 0;
> > remaining += opt_size;
> > drop_other_suboptions = true;
> > }
> >
> > len = mptcp_add_addr_len(opts);
> > if (remaining < len)
> > return false;
> >
> > *size = len;
> > if (drop_other_suboptions)
> > *size -= opt_size;
> > opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> > if (mptcp_pm_should_add_signal_addr(msk)) {
> > opts->ahmac = add_addr_generate_hmac(msk->local_key,
> > msk->remote_key,
> > &opts->local);
> > }
> >
> > pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
> > ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> > msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
> > opts->ahmac, ntohs(opts->local.port),
> > opts->remote.id, ntohs(opts->remote.port));
> >
> > return true;
> > '''
> >
> >> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>
> >> mp_capable_done:
> >> if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> >> + struct mptcp_addr_info *addr = &opts->remote;
> >
> > We can simplify it like this:
> > struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> > &opts->remote;
> >
> >> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >> u8 echo = MPTCP_ADDR_ECHO;
> >>
> >> + if (opts->ahmac)
> >> + addr = &opts->local;
> >
> > And this trunk can be dropped.
> >
> >> +
> >> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >> - if (opts->addr.family == AF_INET6)
> >> + if (addr->family == AF_INET6)
> >> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >> #endif
> >>
> >> - if (opts->addr.port)
> >> + if (addr->port)
> >> len += TCPOLEN_MPTCP_PORT_LEN;
> >>
> >> if (opts->ahmac) {
> >> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >> }
> >>
> >> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> >> - len, echo, opts->addr.id);
> >> - if (opts->addr.family == AF_INET) {
> >> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> >> + len, echo, addr->id);
> >> + if (addr->family == AF_INET) {
> >> + memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
> >> ptr += 1;
> >> }
> >> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >> - else if (opts->addr.family == AF_INET6) {
> >> - memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
> >> + else if (addr->family == AF_INET6) {
> >> + memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
> >> ptr += 4;
> >> }
> >> #endif
> >>
> >> - if (!opts->addr.port) {
> >> + if (!addr->port) {
> >> if (opts->ahmac) {
> >> put_unaligned_be64(opts->ahmac, ptr);
> >> ptr += 2;
> >> }
> >> } else {
> >> - u16 port = ntohs(opts->addr.port);
> >> + u16 port = ntohs(addr->port);
> >>
> >> if (opts->ahmac) {
> >> u8 *bptr = (u8 *)ptr;
> >> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> >> index cf873e9..9c621293 100644
> >> --- a/net/mptcp/pm.c
> >> +++ b/net/mptcp/pm.c
> >> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >>
> >> /* path manager helpers */
> >>
> >> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> - struct mptcp_addr_info *saddr, bool *echo, bool *port)
> >> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> >> + u8 *add_addr)
> >
> > Drop this add_addr argument.
> >
> >> {
> >> - u8 add_addr;
> >> - int ret = false;
> >> -
> >> spin_lock_bh(&msk->pm.lock);
> >>
> >> - /* double check after the lock is acquired */
> >> - if (!mptcp_pm_should_add_signal(msk))
> >> - goto out_unlock;
> >
> > Keep this double check code.
> >
> >> -
> >> - *echo = mptcp_pm_should_add_signal_echo(msk);
> >> - *port = mptcp_pm_should_add_signal_port(msk);
> >> -
> >> - if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> >> - goto out_unlock;
> >
> > Keep this length double check code too.
> >
> >> + if (!mptcp_pm_should_add_signal(msk)) {
> >> + spin_unlock_bh(&msk->pm.lock);
> >> + return false;
> >> + }
> >>
> >> - *saddr = msk->pm.local;
> >> - add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> > - WRITE_ONCE(msk->pm.addr_signal, add_addr);
> >
> > This code is just added in patch 1, I think we should keep it. And no need
> > to write addr_signal again in mptcp_established_options_add_addr.
> >
> >> - ret = true;
> >> + opts->local = msk->pm.local;
> >> + opts->remote = msk->pm.remote;
> >> + *add_addr = msk->pm.addr_signal;
> >>
> >> -out_unlock:
> >> spin_unlock_bh(&msk->pm.lock);
> >> - return ret;
> >
> > Keep this out_unlock code.
> >
> >> +
> >> + if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> >> + mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> >
> > Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
> >
> > I'm no sure why we need this two lines, and why you use '&&' here. Do you
> > mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?
> >
> >> + return true;
> >> }
> >
> > The whole function is something like this:
> > '''
> > int ret = false;
> > u8 add_addr;
> >
> > spin_lock_bh(&msk->pm.lock);
> >
> > /* double check after the lock is acquired */
> > if (!mptcp_pm_should_add_signal(msk))
> > goto out_unlock;
> >
> > if (remaining < mptcp_add_addr_len(opts))
> > goto out_unlock;
> >
> > opts->local = msk->pm.local;
> > opts->remote = msk->pm.remote;
> > if (mptcp_pm_should_add_signal_echo(msk))
> > add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
> > else
> > add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
> > WRITE_ONCE(msk->pm.addr_signal, add_addr);
> > ret = true;
> >
> > out_unlock:
> > spin_unlock_bh(&msk->pm.lock);
> > if (ret && mptcp_pm_should_add_signal_echo(msk) &&
> > mptcp_pm_should_add_signal_addr(msk))
> > mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> > return ret;
> > '''
> >
> >>
> >> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> >> index a0b0ec0..0bfbbdef 100644
> >> --- a/net/mptcp/protocol.h
> >> +++ b/net/mptcp/protocol.h
> >> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
> >> return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
> >> }
> >>
> >> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> >> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
> >> {
> >> - u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >> + u8 len = 0;
> >> + struct mptcp_addr_info *addr = &opts->remote;
> >
> > We can simplify it like this:
> > struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> > &opts->remote;
> >
> > And keep the orignal code unchanged.
> >
> >>
> >> - if (family == AF_INET6)
> >> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >> - if (!echo)
> >> + if (opts->ahmac) {
> >> + addr = &opts->local;
> >> len += MPTCPOPT_THMAC_LEN;
> >> + }
> >> +
> >> + if (addr->family == AF_INET6)
> >> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >> + else
> >> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >> +
> >> /* account for 2 trailing 'nop' options */
> >> - if (port)
> >> + if (addr->port)
> >> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >>
> >> return len;
> >
> > The whole function is something like this:
> > '''
> > struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> > &opts->remote;
> > u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >
> > if (addr->family == AF_INET6)
> > len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> > if (opts->ahmac)
> > len += MPTCPOPT_THMAC_LEN;
> > /* account for 2 trailing 'nop' options */
> > if (addr->port)
> > len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >
> > return len;
> > '''
> >
> > Thanks.
> > -Geliang
> >
> >> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
> >> return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
> >> }
> >>
> >> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> - struct mptcp_addr_info *saddr, bool *echo, bool *port);
> >> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> >> + u8 *add_addr);
> >> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> struct mptcp_rm_list *rm_list);
> >> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> >> --
> >> 1.8.3.1
> >>
> >>
> >
> >
>
> --
> Li YongLong
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-29 7:35 ` Geliang Tang
@ 2021-06-29 7:54 ` Yonglong Li
2021-06-29 8:25 ` Geliang Tang
0 siblings, 1 reply; 15+ messages in thread
From: Yonglong Li @ 2021-06-29 7:54 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp, Mat Martineau
On 2021/6/29 15:35, Geliang Tang wrote:
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 下午3:02写道：
>>
>>
>> Hi Geiliang, Thanks for your reviews.
>>
>> On 2021/6/29 13:58, Geliang Tang wrote:
>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 上午9:42写道：
>>>>
>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>>>> ADD_ADDR/echo-ADD_ADDR option
>>>>
>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>>>
>>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
>>>> ---
>>>> include/net/mptcp.h | 3 ++-
>>>> net/mptcp/options.c | 65 +++++++++++++++++++++++++++++++---------------------
>>>> net/mptcp/pm.c | 33 +++++++++++---------------
>>>> net/mptcp/protocol.h | 23 ++++++++++++-------
>>>> 4 files changed, 69 insertions(+), 55 deletions(-)
>>>>
>>>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
>>>> index d61bbbf..d2c6ebe 100644
>>>> --- a/include/net/mptcp.h
>>>> +++ b/include/net/mptcp.h
>>>> @@ -61,7 +61,8 @@ struct mptcp_out_options {
>>>> u64 sndr_key;
>>>> u64 rcvr_key;
>>>> u64 ahmac;
>>>> - struct mptcp_addr_info addr;
>>>> + struct mptcp_addr_info local;
>>>> + struct mptcp_addr_info remote;
>>>> struct mptcp_rm_list rm_list;
>>>> u8 join_id;
>>>> u8 backup;
>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>> index 1aec016..1707bec 100644
>>>> --- a/net/mptcp/options.c
>>>> +++ b/net/mptcp/options.c
>>>> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>> bool drop_other_suboptions = false;
>>>> unsigned int opt_size = *size;
>>>> - bool echo;
>>>> - bool port;
>>>> - int len;
>>>> + u8 add_addr, flags = 0xff;
>>>> + int len = 0;
>>>>
>>>> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>>>> - mptcp_pm_should_add_signal_port(msk) ||
>>>> - mptcp_pm_should_add_signal_echo(msk)) &&
>>>> + if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
>>>> + return false;
>>>
>>> This add_addr argument is useless, let's drop it.
>>>
>> we can use add_addr use in debug log.
>
> I think it's not worth adding a new argument just for debugging.
agree.
>
>>
>>> And here add back mptcp_pm_should_add_signal check here. The original code
>>> called mptcp_pm_should_add_signal twice for double check, once out of pm
>>> lock, once under pm lock. We should keep it.
>> Sorry, I think double check is not necessary. does we need double check?
>
> I think we should keep the original logic here. If we want to drop this
> double check or something, we should do it in another patch, don't mix too
> much things in one patch.
agree.
>
>>
>>>
>>>> +
>>>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>> + (mptcp_pm_should_add_signal_addr(msk) &&
>>>> + (opts->local.family == AF_INET6 || opts->local.port))) &&
>>>> skb && skb_is_tcp_pure_ack(skb)) {
>>>> pr_debug("drop other suboptions");
>>>> opts->suboptions = 0;
>>>> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>> drop_other_suboptions = true;
>>>> }
>>>>
>>>> - if (!mptcp_pm_should_add_signal(msk) ||
>>>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>>>> - return false;
>>>>
>>>> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
>>>> + if (mptcp_pm_should_add_signal_echo(msk)) {
>>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>>>> + } else {
>>>> + opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>> + msk->remote_key,
>>>> + &opts->local);
>>>
>>> Keep this ahmac generating code after opts->suboptions set just like the
>>> original code, since ahmac is the more expensive to populate. If remaining
>>> length isn't enough, no need to set ahmac.
>>
>> because mptcp_add_addr_len(opts) will use ahmac to calculate len of opts, so I think Keep this ahmac
>> generating code after opts->suboptions set is not ok.
>
> So we should use mptcp_pm_should_add_signal_addr instead of opts->ahmac in
> mptcp_add_addr_len.
agree.
>
>>
>>>
>>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>>>> + }
>>>> +
>>>> + len = mptcp_add_addr_len(opts);
>>>> if (remaining < len)
>>>> return false;
>>>>
>>>> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>> if (drop_other_suboptions)
>>>> *size -= opt_size;
>>>> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>> - if (!echo) {
>>>> - opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>> - msk->remote_key,
>>>> - &opts->addr);
>>>> - }
>>>> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
>>>> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
>>>> +
>>>> + spin_lock_bh(&msk->pm.lock);
>>>> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
>>>> + spin_unlock_bh(&msk->pm.lock);
>>>
>>> addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
>>> set it again. I thinks this trunk and all the flags set above should be
>>> dropped.
>>
>> Because MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO maybe set at the same time.
>> So i think we should only unset one flag.
>
> We can only unset one flag in mptcp_pm_add_addr_signal, see my comment in
> patch 1.
if change like this. there is a issue: if remaining len checking is not ok and return false, The ADD_ADDR/ECHO event will
be clear. So I think we should make sure ADD_ADDR/ECHO option will add in packet before clean flags. WDYT?
>
> -Geliang
>
>>
>>>
>>>> +
>>>> + pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
>>>> + add_addr, (opts->ahmac == 0), opts->local.id,
>>>> + opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>>>>
>>>> return true;
>>>> }
>>>
>>> The whole function is something like this:
>>> '''
>>> struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>> bool drop_other_suboptions = false;
>>> unsigned int opt_size = *size;
>>> int len;
>>>
>>> if (!mptcp_pm_should_add_signal(msk) ||
>>> !mptcp_pm_add_addr_signal(msk, remaining, opts))
>>> return false;
>>>
>>> if ((mptcp_pm_should_add_signal_echo(msk) ||
>>> (mptcp_pm_should_add_signal_addr(msk) &&
>>> (opts->local.family == AF_INET6 || opts->local.port))) &&
>>> skb && skb_is_tcp_pure_ack(skb)) {
>>> pr_debug("drop other suboptions");
>>> opts->suboptions = 0;
>>> opts->ext_copy.use_ack = 0;
>>> opts->ext_copy.use_map = 0;
>>> remaining += opt_size;
>>> drop_other_suboptions = true;
>>> }
>>>
>>> len = mptcp_add_addr_len(opts);
>>> if (remaining < len)
>>> return false;
>>>
>>> *size = len;
>>> if (drop_other_suboptions)
>>> *size -= opt_size;
>>> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>> if (mptcp_pm_should_add_signal_addr(msk)) {
>>> opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>> msk->remote_key,
>>> &opts->local);
>>> }
>>>
>>> pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
>>> ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
>>> msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
>>> opts->ahmac, ntohs(opts->local.port),
>>> opts->remote.id, ntohs(opts->remote.port));
>>>
>>> return true;
>>> '''
>>>
>>>> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>>>
>>>> mp_capable_done:
>>>> if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
>>>> + struct mptcp_addr_info *addr = &opts->remote;
>>>
>>> We can simplify it like this:
>>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>> &opts->remote;
>>>
>>>> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>> u8 echo = MPTCP_ADDR_ECHO;
>>>>
>>>> + if (opts->ahmac)
>>>> + addr = &opts->local;
>>>
>>> And this trunk can be dropped.
>>>
>>>> +
>>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>>> - if (opts->addr.family == AF_INET6)
>>>> + if (addr->family == AF_INET6)
>>>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>> #endif
>>>>
>>>> - if (opts->addr.port)
>>>> + if (addr->port)
>>>> len += TCPOLEN_MPTCP_PORT_LEN;
>>>>
>>>> if (opts->ahmac) {
>>>> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>>> }
>>>>
>>>> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
>>>> - len, echo, opts->addr.id);
>>>> - if (opts->addr.family == AF_INET) {
>>>> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
>>>> + len, echo, addr->id);
>>>> + if (addr->family == AF_INET) {
>>>> + memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
>>>> ptr += 1;
>>>> }
>>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>>> - else if (opts->addr.family == AF_INET6) {
>>>> - memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
>>>> + else if (addr->family == AF_INET6) {
>>>> + memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
>>>> ptr += 4;
>>>> }
>>>> #endif
>>>>
>>>> - if (!opts->addr.port) {
>>>> + if (!addr->port) {
>>>> if (opts->ahmac) {
>>>> put_unaligned_be64(opts->ahmac, ptr);
>>>> ptr += 2;
>>>> }
>>>> } else {
>>>> - u16 port = ntohs(opts->addr.port);
>>>> + u16 port = ntohs(addr->port);
>>>>
>>>> if (opts->ahmac) {
>>>> u8 *bptr = (u8 *)ptr;
>>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>>> index cf873e9..9c621293 100644
>>>> --- a/net/mptcp/pm.c
>>>> +++ b/net/mptcp/pm.c
>>>> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>>>
>>>> /* path manager helpers */
>>>>
>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>> - struct mptcp_addr_info *saddr, bool *echo, bool *port)
>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
>>>> + u8 *add_addr)
>>>
>>> Drop this add_addr argument.
>>>
>>>> {
>>>> - u8 add_addr;
>>>> - int ret = false;
>>>> -
>>>> spin_lock_bh(&msk->pm.lock);
>>>>
>>>> - /* double check after the lock is acquired */
>>>> - if (!mptcp_pm_should_add_signal(msk))
>>>> - goto out_unlock;
>>>
>>> Keep this double check code.
>>>
>>>> -
>>>> - *echo = mptcp_pm_should_add_signal_echo(msk);
>>>> - *port = mptcp_pm_should_add_signal_port(msk);
>>>> -
>>>> - if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
>>>> - goto out_unlock;
>>>
>>> Keep this length double check code too.
>>>
>>>> + if (!mptcp_pm_should_add_signal(msk)) {
>>>> + spin_unlock_bh(&msk->pm.lock);
>>>> + return false;
>>>> + }
>>>>
>>>> - *saddr = msk->pm.local;
>>>> - add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
>>> - WRITE_ONCE(msk->pm.addr_signal, add_addr);
>>>
>>> This code is just added in patch 1, I think we should keep it. And no need
>>> to write addr_signal again in mptcp_established_options_add_addr.
>>>
>>>> - ret = true;
>>>> + opts->local = msk->pm.local;
>>>> + opts->remote = msk->pm.remote;
>>>> + *add_addr = msk->pm.addr_signal;
>>>>
>>>> -out_unlock:
>>>> spin_unlock_bh(&msk->pm.lock);
>>>> - return ret;
>>>
>>> Keep this out_unlock code.
>>>
>>>> +
>>>> + if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
>>>> + mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>>>
>>> Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
>>>
>>> I'm no sure why we need this two lines, and why you use '&&' here. Do you
>>> mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?
>>>
>>>> + return true;
>>>> }
>>>
>>> The whole function is something like this:
>>> '''
>>> int ret = false;
>>> u8 add_addr;
>>>
>>> spin_lock_bh(&msk->pm.lock);
>>>
>>> /* double check after the lock is acquired */
>>> if (!mptcp_pm_should_add_signal(msk))
>>> goto out_unlock;
>>>
>>> if (remaining < mptcp_add_addr_len(opts))
>>> goto out_unlock;
>>>
>>> opts->local = msk->pm.local;
>>> opts->remote = msk->pm.remote;
>>> if (mptcp_pm_should_add_signal_echo(msk))
>>> add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
>>> else
>>> add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
>>> WRITE_ONCE(msk->pm.addr_signal, add_addr);
>>> ret = true;
>>>
>>> out_unlock:
>>> spin_unlock_bh(&msk->pm.lock);
>>> if (ret && mptcp_pm_should_add_signal_echo(msk) &&
>>> mptcp_pm_should_add_signal_addr(msk))
>>> mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>>> return ret;
>>> '''
>>>
>>>>
>>>> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>>> index a0b0ec0..0bfbbdef 100644
>>>> --- a/net/mptcp/protocol.h
>>>> +++ b/net/mptcp/protocol.h
>>>> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
>>>> return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
>>>> }
>>>>
>>>> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
>>>> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
>>>> {
>>>> - u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>> + u8 len = 0;
>>>> + struct mptcp_addr_info *addr = &opts->remote;
>>>
>>> We can simplify it like this:
>>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>> &opts->remote;
>>>
>>> And keep the orignal code unchanged.
>>>
>>>>
>>>> - if (family == AF_INET6)
>>>> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>> - if (!echo)
>>>> + if (opts->ahmac) {
>>>> + addr = &opts->local;
>>>> len += MPTCPOPT_THMAC_LEN;
>>>> + }
>>>> +
>>>> + if (addr->family == AF_INET6)
>>>> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>> + else
>>>> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>> +
>>>> /* account for 2 trailing 'nop' options */
>>>> - if (port)
>>>> + if (addr->port)
>>>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>>>
>>>> return len;
>>>
>>> The whole function is something like this:
>>> '''
>>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>> &opts->remote;
>>> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>
>>> if (addr->family == AF_INET6)
>>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>> if (opts->ahmac)
>>> len += MPTCPOPT_THMAC_LEN;
>>> /* account for 2 trailing 'nop' options */
>>> if (addr->port)
>>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>>
>>> return len;
>>> '''
>>>
>>> Thanks.
>>> -Geliang
>>>
>>>> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
>>>> return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
>>>> }
>>>>
>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>> - struct mptcp_addr_info *saddr, bool *echo, bool *port);
>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
>>>> + u8 *add_addr);
>>>> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>> struct mptcp_rm_list *rm_list);
>>>> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
>>>> --
>>>> 1.8.3.1
>>>>
>>>>
>>>
>>>
>>
>> --
>> Li YongLong
>
--
Li YongLong
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-29 7:54 ` Yonglong Li
@ 2021-06-29 8:25 ` Geliang Tang
2021-06-30 1:30 ` Yonglong Li
0 siblings, 1 reply; 15+ messages in thread
From: Geliang Tang @ 2021-06-29 8:25 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 下午3:54写道：
>
>
>
> On 2021/6/29 15:35, Geliang Tang wrote:
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 下午3:02写道：
> >>
> >>
> >> Hi Geiliang, Thanks for your reviews.
> >>
> >> On 2021/6/29 13:58, Geliang Tang wrote:
> >>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 上午9:42写道：
> >>>>
> >>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> >>>> ADD_ADDR/echo-ADD_ADDR option
> >>>>
> >>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >>>>
> >>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> >>>> ---
> >>>> include/net/mptcp.h | 3 ++-
> >>>> net/mptcp/options.c | 65 +++++++++++++++++++++++++++++++---------------------
> >>>> net/mptcp/pm.c | 33 +++++++++++---------------
> >>>> net/mptcp/protocol.h | 23 ++++++++++++-------
> >>>> 4 files changed, 69 insertions(+), 55 deletions(-)
> >>>>
> >>>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> >>>> index d61bbbf..d2c6ebe 100644
> >>>> --- a/include/net/mptcp.h
> >>>> +++ b/include/net/mptcp.h
> >>>> @@ -61,7 +61,8 @@ struct mptcp_out_options {
> >>>> u64 sndr_key;
> >>>> u64 rcvr_key;
> >>>> u64 ahmac;
> >>>> - struct mptcp_addr_info addr;
> >>>> + struct mptcp_addr_info local;
> >>>> + struct mptcp_addr_info remote;
> >>>> struct mptcp_rm_list rm_list;
> >>>> u8 join_id;
> >>>> u8 backup;
> >>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>>> index 1aec016..1707bec 100644
> >>>> --- a/net/mptcp/options.c
> >>>> +++ b/net/mptcp/options.c
> >>>> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >>>> bool drop_other_suboptions = false;
> >>>> unsigned int opt_size = *size;
> >>>> - bool echo;
> >>>> - bool port;
> >>>> - int len;
> >>>> + u8 add_addr, flags = 0xff;
> >>>> + int len = 0;
> >>>>
> >>>> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> >>>> - mptcp_pm_should_add_signal_port(msk) ||
> >>>> - mptcp_pm_should_add_signal_echo(msk)) &&
> >>>> + if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
> >>>> + return false;
> >>>
> >>> This add_addr argument is useless, let's drop it.
> >>>
> >> we can use add_addr use in debug log.
> >
> > I think it's not worth adding a new argument just for debugging.
> agree.
>
> >
> >>
> >>> And here add back mptcp_pm_should_add_signal check here. The original code
> >>> called mptcp_pm_should_add_signal twice for double check, once out of pm
> >>> lock, once under pm lock. We should keep it.
> >> Sorry, I think double check is not necessary. does we need double check?
> >
> > I think we should keep the original logic here. If we want to drop this
> > double check or something, we should do it in another patch, don't mix too
> > much things in one patch.
> agree.
>
> >
> >>
> >>>
> >>>> +
> >>>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> >>>> + (mptcp_pm_should_add_signal_addr(msk) &&
> >>>> + (opts->local.family == AF_INET6 || opts->local.port))) &&
> >>>> skb && skb_is_tcp_pure_ack(skb)) {
> >>>> pr_debug("drop other suboptions");
> >>>> opts->suboptions = 0;
> >>>> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>> drop_other_suboptions = true;
> >>>> }
> >>>>
> >>>> - if (!mptcp_pm_should_add_signal(msk) ||
> >>>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> >>>> - return false;
> >>>>
> >>>> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> >>>> + if (mptcp_pm_should_add_signal_echo(msk)) {
> >>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>> + } else {
> >>>> + opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >>>> + msk->remote_key,
> >>>> + &opts->local);
> >>>
> >>> Keep this ahmac generating code after opts->suboptions set just like the
> >>> original code, since ahmac is the more expensive to populate. If remaining
> >>> length isn't enough, no need to set ahmac.
> >>
> >> because mptcp_add_addr_len(opts) will use ahmac to calculate len of opts, so I think Keep this ahmac
> >> generating code after opts->suboptions set is not ok.
> >
> > So we should use mptcp_pm_should_add_signal_addr instead of opts->ahmac in
> > mptcp_add_addr_len.
> agree.
>
> >
> >>
> >>>
> >>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> >>>> + }
> >>>> +
> >>>> + len = mptcp_add_addr_len(opts);
> >>>> if (remaining < len)
> >>>> return false;
> >>>>
> >>>> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>> if (drop_other_suboptions)
> >>>> *size -= opt_size;
> >>>> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >>>> - if (!echo) {
> >>>> - opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >>>> - msk->remote_key,
> >>>> - &opts->addr);
> >>>> - }
> >>>> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> >>>> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> >>>> +
> >>>> + spin_lock_bh(&msk->pm.lock);
> >>>> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> >>>> + spin_unlock_bh(&msk->pm.lock);
> >>>
> >>> addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
> >>> set it again. I thinks this trunk and all the flags set above should be
> >>> dropped.
> >>
> >> Because MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO maybe set at the same time.
> >> So i think we should only unset one flag.
> >
> > We can only unset one flag in mptcp_pm_add_addr_signal, see my comment in
> > patch 1.
>
> if change like this. there is a issue: if remaining len checking is not ok and return false, The ADD_ADDR/ECHO event will
> be clear. So I think we should make sure ADD_ADDR/ECHO option will add in packet before clean flags. WDYT?
>
You're right, let's clear it in mptcp_established_options_add_addr.
Furthermore, we should do the same thing for RM_ADDR, clear rm_addr in
mptcp_established_options_rm_addr too.
If so, patch 1 will become useless. Let's drop it.
-Geliang
> >
> > -Geliang
> >
> >>
> >>>
> >>>> +
> >>>> + pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> >>>> + add_addr, (opts->ahmac == 0), opts->local.id,
> >>>> + opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
> >>>>
> >>>> return true;
> >>>> }
> >>>
> >>> The whole function is something like this:
> >>> '''
> >>> struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> >>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >>> bool drop_other_suboptions = false;
> >>> unsigned int opt_size = *size;
> >>> int len;
> >>>
> >>> if (!mptcp_pm_should_add_signal(msk) ||
> >>> !mptcp_pm_add_addr_signal(msk, remaining, opts))
> >>> return false;
> >>>
> >>> if ((mptcp_pm_should_add_signal_echo(msk) ||
> >>> (mptcp_pm_should_add_signal_addr(msk) &&
> >>> (opts->local.family == AF_INET6 || opts->local.port))) &&
> >>> skb && skb_is_tcp_pure_ack(skb)) {
> >>> pr_debug("drop other suboptions");
> >>> opts->suboptions = 0;
> >>> opts->ext_copy.use_ack = 0;
> >>> opts->ext_copy.use_map = 0;
> >>> remaining += opt_size;
> >>> drop_other_suboptions = true;
> >>> }
> >>>
> >>> len = mptcp_add_addr_len(opts);
> >>> if (remaining < len)
> >>> return false;
> >>>
> >>> *size = len;
> >>> if (drop_other_suboptions)
> >>> *size -= opt_size;
> >>> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >>> if (mptcp_pm_should_add_signal_addr(msk)) {
> >>> opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >>> msk->remote_key,
> >>> &opts->local);
> >>> }
> >>>
> >>> pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
> >>> ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> >>> msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
> >>> opts->ahmac, ntohs(opts->local.port),
> >>> opts->remote.id, ntohs(opts->remote.port));
> >>>
> >>> return true;
> >>> '''
> >>>
> >>>> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>>>
> >>>> mp_capable_done:
> >>>> if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> >>>> + struct mptcp_addr_info *addr = &opts->remote;
> >>>
> >>> We can simplify it like this:
> >>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> >>> &opts->remote;
> >>>
> >>>> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>> u8 echo = MPTCP_ADDR_ECHO;
> >>>>
> >>>> + if (opts->ahmac)
> >>>> + addr = &opts->local;
> >>>
> >>> And this trunk can be dropped.
> >>>
> >>>> +
> >>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >>>> - if (opts->addr.family == AF_INET6)
> >>>> + if (addr->family == AF_INET6)
> >>>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>> #endif
> >>>>
> >>>> - if (opts->addr.port)
> >>>> + if (addr->port)
> >>>> len += TCPOLEN_MPTCP_PORT_LEN;
> >>>>
> >>>> if (opts->ahmac) {
> >>>> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>>> }
> >>>>
> >>>> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> >>>> - len, echo, opts->addr.id);
> >>>> - if (opts->addr.family == AF_INET) {
> >>>> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> >>>> + len, echo, addr->id);
> >>>> + if (addr->family == AF_INET) {
> >>>> + memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
> >>>> ptr += 1;
> >>>> }
> >>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >>>> - else if (opts->addr.family == AF_INET6) {
> >>>> - memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
> >>>> + else if (addr->family == AF_INET6) {
> >>>> + memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
> >>>> ptr += 4;
> >>>> }
> >>>> #endif
> >>>>
> >>>> - if (!opts->addr.port) {
> >>>> + if (!addr->port) {
> >>>> if (opts->ahmac) {
> >>>> put_unaligned_be64(opts->ahmac, ptr);
> >>>> ptr += 2;
> >>>> }
> >>>> } else {
> >>>> - u16 port = ntohs(opts->addr.port);
> >>>> + u16 port = ntohs(addr->port);
> >>>>
> >>>> if (opts->ahmac) {
> >>>> u8 *bptr = (u8 *)ptr;
> >>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> >>>> index cf873e9..9c621293 100644
> >>>> --- a/net/mptcp/pm.c
> >>>> +++ b/net/mptcp/pm.c
> >>>> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >>>>
> >>>> /* path manager helpers */
> >>>>
> >>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>> - struct mptcp_addr_info *saddr, bool *echo, bool *port)
> >>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> >>>> + u8 *add_addr)
> >>>
> >>> Drop this add_addr argument.
> >>>
> >>>> {
> >>>> - u8 add_addr;
> >>>> - int ret = false;
> >>>> -
> >>>> spin_lock_bh(&msk->pm.lock);
> >>>>
> >>>> - /* double check after the lock is acquired */
> >>>> - if (!mptcp_pm_should_add_signal(msk))
> >>>> - goto out_unlock;
> >>>
> >>> Keep this double check code.
> >>>
> >>>> -
> >>>> - *echo = mptcp_pm_should_add_signal_echo(msk);
> >>>> - *port = mptcp_pm_should_add_signal_port(msk);
> >>>> -
> >>>> - if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> >>>> - goto out_unlock;
> >>>
> >>> Keep this length double check code too.
> >>>
> >>>> + if (!mptcp_pm_should_add_signal(msk)) {
> >>>> + spin_unlock_bh(&msk->pm.lock);
> >>>> + return false;
> >>>> + }
> >>>>
> >>>> - *saddr = msk->pm.local;
> >>>> - add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> >>> - WRITE_ONCE(msk->pm.addr_signal, add_addr);
> >>>
> >>> This code is just added in patch 1, I think we should keep it. And no need
> >>> to write addr_signal again in mptcp_established_options_add_addr.
> >>>
> >>>> - ret = true;
> >>>> + opts->local = msk->pm.local;
> >>>> + opts->remote = msk->pm.remote;
> >>>> + *add_addr = msk->pm.addr_signal;
> >>>>
> >>>> -out_unlock:
> >>>> spin_unlock_bh(&msk->pm.lock);
> >>>> - return ret;
> >>>
> >>> Keep this out_unlock code.
> >>>
> >>>> +
> >>>> + if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> >>>> + mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> >>>
> >>> Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
> >>>
> >>> I'm no sure why we need this two lines, and why you use '&&' here. Do you
> >>> mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?
> >>>
> >>>> + return true;
> >>>> }
> >>>
> >>> The whole function is something like this:
> >>> '''
> >>> int ret = false;
> >>> u8 add_addr;
> >>>
> >>> spin_lock_bh(&msk->pm.lock);
> >>>
> >>> /* double check after the lock is acquired */
> >>> if (!mptcp_pm_should_add_signal(msk))
> >>> goto out_unlock;
> >>>
> >>> if (remaining < mptcp_add_addr_len(opts))
> >>> goto out_unlock;
> >>>
> >>> opts->local = msk->pm.local;
> >>> opts->remote = msk->pm.remote;
> >>> if (mptcp_pm_should_add_signal_echo(msk))
> >>> add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
> >>> else
> >>> add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
> >>> WRITE_ONCE(msk->pm.addr_signal, add_addr);
> >>> ret = true;
> >>>
> >>> out_unlock:
> >>> spin_unlock_bh(&msk->pm.lock);
> >>> if (ret && mptcp_pm_should_add_signal_echo(msk) &&
> >>> mptcp_pm_should_add_signal_addr(msk))
> >>> mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> >>> return ret;
> >>> '''
> >>>
> >>>>
> >>>> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> >>>> index a0b0ec0..0bfbbdef 100644
> >>>> --- a/net/mptcp/protocol.h
> >>>> +++ b/net/mptcp/protocol.h
> >>>> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
> >>>> return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
> >>>> }
> >>>>
> >>>> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> >>>> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
> >>>> {
> >>>> - u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>> + u8 len = 0;
> >>>> + struct mptcp_addr_info *addr = &opts->remote;
> >>>
> >>> We can simplify it like this:
> >>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> >>> &opts->remote;
> >>>
> >>> And keep the orignal code unchanged.
> >>>
> >>>>
> >>>> - if (family == AF_INET6)
> >>>> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>> - if (!echo)
> >>>> + if (opts->ahmac) {
> >>>> + addr = &opts->local;
> >>>> len += MPTCPOPT_THMAC_LEN;
> >>>> + }
> >>>> +
> >>>> + if (addr->family == AF_INET6)
> >>>> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>> + else
> >>>> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>> +
> >>>> /* account for 2 trailing 'nop' options */
> >>>> - if (port)
> >>>> + if (addr->port)
> >>>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >>>>
> >>>> return len;
> >>>
> >>> The whole function is something like this:
> >>> '''
> >>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> >>> &opts->remote;
> >>> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>
> >>> if (addr->family == AF_INET6)
> >>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>> if (opts->ahmac)
> >>> len += MPTCPOPT_THMAC_LEN;
> >>> /* account for 2 trailing 'nop' options */
> >>> if (addr->port)
> >>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >>>
> >>> return len;
> >>> '''
> >>>
> >>> Thanks.
> >>> -Geliang
> >>>
> >>>> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
> >>>> return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
> >>>> }
> >>>>
> >>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>> - struct mptcp_addr_info *saddr, bool *echo, bool *port);
> >>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> >>>> + u8 *add_addr);
> >>>> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>> struct mptcp_rm_list *rm_list);
> >>>> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> >>>> --
> >>>> 1.8.3.1
> >>>>
> >>>>
> >>>
> >>>
> >>
> >> --
> >> Li YongLong
> >
>
> --
> Li YongLong
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-29 8:25 ` Geliang Tang
@ 2021-06-30 1:30 ` Yonglong Li
2021-06-30 2:05 ` Geliang Tang
0 siblings, 1 reply; 15+ messages in thread
From: Yonglong Li @ 2021-06-30 1:30 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp, Mat Martineau
On 2021/6/29 16:25, Geliang Tang wrote:
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 下午3:54写道：
>>
>>
>>
>> On 2021/6/29 15:35, Geliang Tang wrote:
>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 下午3:02写道：
>>>>
>>>>
>>>> Hi Geiliang, Thanks for your reviews.
>>>>
>>>> On 2021/6/29 13:58, Geliang Tang wrote:
>>>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 上午9:42写道：
>>>>>>
>>>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>>>>>> ADD_ADDR/echo-ADD_ADDR option
>>>>>>
>>>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>>>>>
>>>>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
>>>>>> ---
>>>>>> include/net/mptcp.h | 3 ++-
>>>>>> net/mptcp/options.c | 65 +++++++++++++++++++++++++++++++---------------------
>>>>>> net/mptcp/pm.c | 33 +++++++++++---------------
>>>>>> net/mptcp/protocol.h | 23 ++++++++++++-------
>>>>>> 4 files changed, 69 insertions(+), 55 deletions(-)
>>>>>>
>>>>>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
>>>>>> index d61bbbf..d2c6ebe 100644
>>>>>> --- a/include/net/mptcp.h
>>>>>> +++ b/include/net/mptcp.h
>>>>>> @@ -61,7 +61,8 @@ struct mptcp_out_options {
>>>>>> u64 sndr_key;
>>>>>> u64 rcvr_key;
>>>>>> u64 ahmac;
>>>>>> - struct mptcp_addr_info addr;
>>>>>> + struct mptcp_addr_info local;
>>>>>> + struct mptcp_addr_info remote;
>>>>>> struct mptcp_rm_list rm_list;
>>>>>> u8 join_id;
>>>>>> u8 backup;
>>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>>>> index 1aec016..1707bec 100644
>>>>>> --- a/net/mptcp/options.c
>>>>>> +++ b/net/mptcp/options.c
>>>>>> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>>>> bool drop_other_suboptions = false;
>>>>>> unsigned int opt_size = *size;
>>>>>> - bool echo;
>>>>>> - bool port;
>>>>>> - int len;
>>>>>> + u8 add_addr, flags = 0xff;
>>>>>> + int len = 0;
>>>>>>
>>>>>> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>>>>>> - mptcp_pm_should_add_signal_port(msk) ||
>>>>>> - mptcp_pm_should_add_signal_echo(msk)) &&
>>>>>> + if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
>>>>>> + return false;
>>>>>
>>>>> This add_addr argument is useless, let's drop it.
>>>>>
>>>> we can use add_addr use in debug log.
>>>
>>> I think it's not worth adding a new argument just for debugging.
>> agree.
>>
>>>
>>>>
>>>>> And here add back mptcp_pm_should_add_signal check here. The original code
>>>>> called mptcp_pm_should_add_signal twice for double check, once out of pm
>>>>> lock, once under pm lock. We should keep it.
>>>> Sorry, I think double check is not necessary. does we need double check?
>>>
>>> I think we should keep the original logic here. If we want to drop this
>>> double check or something, we should do it in another patch, don't mix too
>>> much things in one patch.
>> agree.
>>
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>>>> + (mptcp_pm_should_add_signal_addr(msk) &&
>>>>>> + (opts->local.family == AF_INET6 || opts->local.port))) &&
>>>>>> skb && skb_is_tcp_pure_ack(skb)) {
>>>>>> pr_debug("drop other suboptions");
>>>>>> opts->suboptions = 0;
>>>>>> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>> drop_other_suboptions = true;
>>>>>> }
>>>>>>
>>>>>> - if (!mptcp_pm_should_add_signal(msk) ||
>>>>>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>>>>>> - return false;
>>>>>>
>>>>>> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
>>>>>> + if (mptcp_pm_should_add_signal_echo(msk)) {
>>>>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>>> + } else {
>>>>>> + opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>>>> + msk->remote_key,
>>>>>> + &opts->local);
>>>>>
>>>>> Keep this ahmac generating code after opts->suboptions set just like the
>>>>> original code, since ahmac is the more expensive to populate. If remaining
>>>>> length isn't enough, no need to set ahmac.
>>>>
>>>> because mptcp_add_addr_len(opts) will use ahmac to calculate len of opts, so I think Keep this ahmac
>>>> generating code after opts->suboptions set is not ok.
>>>
>>> So we should use mptcp_pm_should_add_signal_addr instead of opts->ahmac in
>>> mptcp_add_addr_len.
>> agree.
>>
>>>
>>>>
>>>>>
>>>>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>>>>>> + }
>>>>>> +
>>>>>> + len = mptcp_add_addr_len(opts);
>>>>>> if (remaining < len)
>>>>>> return false;
>>>>>>
>>>>>> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>> if (drop_other_suboptions)
>>>>>> *size -= opt_size;
>>>>>> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>>>> - if (!echo) {
>>>>>> - opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>>>> - msk->remote_key,
>>>>>> - &opts->addr);
>>>>>> - }
>>>>>> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
>>>>>> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
>>>>>> +
>>>>>> + spin_lock_bh(&msk->pm.lock);
>>>>>> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
>>>>>> + spin_unlock_bh(&msk->pm.lock);
>>>>>
>>>>> addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
>>>>> set it again. I thinks this trunk and all the flags set above should be
>>>>> dropped.
>>>>
>>>> Because MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO maybe set at the same time.
>>>> So i think we should only unset one flag.
>>>
>>> We can only unset one flag in mptcp_pm_add_addr_signal, see my comment in
>>> patch 1.
>>
>> if change like this. there is a issue: if remaining len checking is not ok and return false, The ADD_ADDR/ECHO event will
>> be clear. So I think we should make sure ADD_ADDR/ECHO option will add in packet before clean flags. WDYT?
>>
>
> You're right, let's clear it in mptcp_established_options_add_addr.
> Furthermore, we should do the same thing for RM_ADDR, clear rm_addr in
> mptcp_established_options_rm_addr too.
>
> If so, patch 1 will become useless. Let's drop it.
>
> -Geliang
> I think RM_ADDR doesn't have this issue. Because mptcp_pm_rm_addr_signal() check the failed case.
>
>
>>>
>>> -Geliang
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> + pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
>>>>>> + add_addr, (opts->ahmac == 0), opts->local.id,
>>>>>> + opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>>>>>>
>>>>>> return true;
>>>>>> }
>>>>>
>>>>> The whole function is something like this:
>>>>> '''
>>>>> struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>>>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>>> bool drop_other_suboptions = false;
>>>>> unsigned int opt_size = *size;
>>>>> int len;
>>>>>
>>>>> if (!mptcp_pm_should_add_signal(msk) ||
>>>>> !mptcp_pm_add_addr_signal(msk, remaining, opts))
>>>>> return false;
>>>>>
>>>>> if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>>> (mptcp_pm_should_add_signal_addr(msk) &&
>>>>> (opts->local.family == AF_INET6 || opts->local.port))) &&
>>>>> skb && skb_is_tcp_pure_ack(skb)) {
>>>>> pr_debug("drop other suboptions");
>>>>> opts->suboptions = 0;
>>>>> opts->ext_copy.use_ack = 0;
>>>>> opts->ext_copy.use_map = 0;
>>>>> remaining += opt_size;
>>>>> drop_other_suboptions = true;
>>>>> }
>>>>>
>>>>> len = mptcp_add_addr_len(opts);
>>>>> if (remaining < len)
>>>>> return false;
>>>>>
>>>>> *size = len;
>>>>> if (drop_other_suboptions)
>>>>> *size -= opt_size;
>>>>> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>>> if (mptcp_pm_should_add_signal_addr(msk)) {
>>>>> opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>>> msk->remote_key,
>>>>> &opts->local);
>>>>> }
>>>>>
>>>>> pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
>>>>> ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
>>>>> msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
>>>>> opts->ahmac, ntohs(opts->local.port),
>>>>> opts->remote.id, ntohs(opts->remote.port));
>>>>>
>>>>> return true;
>>>>> '''
>>>>>
>>>>>> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>>>>>
>>>>>> mp_capable_done:
>>>>>> if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
>>>>>> + struct mptcp_addr_info *addr = &opts->remote;
>>>>>
>>>>> We can simplify it like this:
>>>>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>>>> &opts->remote;
>>>>>
>>>>>> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>> u8 echo = MPTCP_ADDR_ECHO;
>>>>>>
>>>>>> + if (opts->ahmac)
>>>>>> + addr = &opts->local;
>>>>>
>>>>> And this trunk can be dropped.
>>>>>
>>>>>> +
>>>>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>>>>> - if (opts->addr.family == AF_INET6)
>>>>>> + if (addr->family == AF_INET6)
>>>>>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>> #endif
>>>>>>
>>>>>> - if (opts->addr.port)
>>>>>> + if (addr->port)
>>>>>> len += TCPOLEN_MPTCP_PORT_LEN;
>>>>>>
>>>>>> if (opts->ahmac) {
>>>>>> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>>>>> }
>>>>>>
>>>>>> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
>>>>>> - len, echo, opts->addr.id);
>>>>>> - if (opts->addr.family == AF_INET) {
>>>>>> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
>>>>>> + len, echo, addr->id);
>>>>>> + if (addr->family == AF_INET) {
>>>>>> + memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
>>>>>> ptr += 1;
>>>>>> }
>>>>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>>>>> - else if (opts->addr.family == AF_INET6) {
>>>>>> - memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
>>>>>> + else if (addr->family == AF_INET6) {
>>>>>> + memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
>>>>>> ptr += 4;
>>>>>> }
>>>>>> #endif
>>>>>>
>>>>>> - if (!opts->addr.port) {
>>>>>> + if (!addr->port) {
>>>>>> if (opts->ahmac) {
>>>>>> put_unaligned_be64(opts->ahmac, ptr);
>>>>>> ptr += 2;
>>>>>> }
>>>>>> } else {
>>>>>> - u16 port = ntohs(opts->addr.port);
>>>>>> + u16 port = ntohs(addr->port);
>>>>>>
>>>>>> if (opts->ahmac) {
>>>>>> u8 *bptr = (u8 *)ptr;
>>>>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>>>>> index cf873e9..9c621293 100644
>>>>>> --- a/net/mptcp/pm.c
>>>>>> +++ b/net/mptcp/pm.c
>>>>>> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>>>>>
>>>>>> /* path manager helpers */
>>>>>>
>>>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>> - struct mptcp_addr_info *saddr, bool *echo, bool *port)
>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
>>>>>> + u8 *add_addr)
>>>>>
>>>>> Drop this add_addr argument.
>>>>>
>>>>>> {
>>>>>> - u8 add_addr;
>>>>>> - int ret = false;
>>>>>> -
>>>>>> spin_lock_bh(&msk->pm.lock);
>>>>>>
>>>>>> - /* double check after the lock is acquired */
>>>>>> - if (!mptcp_pm_should_add_signal(msk))
>>>>>> - goto out_unlock;
>>>>>
>>>>> Keep this double check code.
>>>>>
>>>>>> -
>>>>>> - *echo = mptcp_pm_should_add_signal_echo(msk);
>>>>>> - *port = mptcp_pm_should_add_signal_port(msk);
>>>>>> -
>>>>>> - if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
>>>>>> - goto out_unlock;
>>>>>
>>>>> Keep this length double check code too.
>>>>>
>>>>>> + if (!mptcp_pm_should_add_signal(msk)) {
>>>>>> + spin_unlock_bh(&msk->pm.lock);
>>>>>> + return false;
>>>>>> + }
>>>>>>
>>>>>> - *saddr = msk->pm.local;
>>>>>> - add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
>>>>> - WRITE_ONCE(msk->pm.addr_signal, add_addr);
>>>>>
>>>>> This code is just added in patch 1, I think we should keep it. And no need
>>>>> to write addr_signal again in mptcp_established_options_add_addr.
>>>>>
>>>>>> - ret = true;
>>>>>> + opts->local = msk->pm.local;
>>>>>> + opts->remote = msk->pm.remote;
>>>>>> + *add_addr = msk->pm.addr_signal;
>>>>>>
>>>>>> -out_unlock:
>>>>>> spin_unlock_bh(&msk->pm.lock);
>>>>>> - return ret;
>>>>>
>>>>> Keep this out_unlock code.
>>>>>
>>>>>> +
>>>>>> + if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
>>>>>> + mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>>>>>
>>>>> Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
>>>>>
>>>>> I'm no sure why we need this two lines, and why you use '&&' here. Do you
>>>>> mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?
>>>>>
>>>>>> + return true;
>>>>>> }
>>>>>
>>>>> The whole function is something like this:
>>>>> '''
>>>>> int ret = false;
>>>>> u8 add_addr;
>>>>>
>>>>> spin_lock_bh(&msk->pm.lock);
>>>>>
>>>>> /* double check after the lock is acquired */
>>>>> if (!mptcp_pm_should_add_signal(msk))
>>>>> goto out_unlock;
>>>>>
>>>>> if (remaining < mptcp_add_addr_len(opts))
>>>>> goto out_unlock;
>>>>>
>>>>> opts->local = msk->pm.local;
>>>>> opts->remote = msk->pm.remote;
>>>>> if (mptcp_pm_should_add_signal_echo(msk))
>>>>> add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>> else
>>>>> add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
>>>>> WRITE_ONCE(msk->pm.addr_signal, add_addr);
>>>>> ret = true;
>>>>>
>>>>> out_unlock:
>>>>> spin_unlock_bh(&msk->pm.lock);
>>>>> if (ret && mptcp_pm_should_add_signal_echo(msk) &&
>>>>> mptcp_pm_should_add_signal_addr(msk))
>>>>> mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>>>>> return ret;
>>>>> '''
>>>>>
>>>>>>
>>>>>> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>>>>> index a0b0ec0..0bfbbdef 100644
>>>>>> --- a/net/mptcp/protocol.h
>>>>>> +++ b/net/mptcp/protocol.h
>>>>>> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
>>>>>> return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
>>>>>> }
>>>>>>
>>>>>> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
>>>>>> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
>>>>>> {
>>>>>> - u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>> + u8 len = 0;
>>>>>> + struct mptcp_addr_info *addr = &opts->remote;
>>>>>
>>>>> We can simplify it like this:
>>>>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>>>> &opts->remote;
>>>>>
>>>>> And keep the orignal code unchanged.
>>>>>
>>>>>>
>>>>>> - if (family == AF_INET6)
>>>>>> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>> - if (!echo)
>>>>>> + if (opts->ahmac) {
>>>>>> + addr = &opts->local;
>>>>>> len += MPTCPOPT_THMAC_LEN;
>>>>>> + }
>>>>>> +
>>>>>> + if (addr->family == AF_INET6)
>>>>>> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>> + else
>>>>>> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>> +
>>>>>> /* account for 2 trailing 'nop' options */
>>>>>> - if (port)
>>>>>> + if (addr->port)
>>>>>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>>>>>
>>>>>> return len;
>>>>>
>>>>> The whole function is something like this:
>>>>> '''
>>>>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>>>> &opts->remote;
>>>>> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>
>>>>> if (addr->family == AF_INET6)
>>>>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>> if (opts->ahmac)
>>>>> len += MPTCPOPT_THMAC_LEN;
>>>>> /* account for 2 trailing 'nop' options */
>>>>> if (addr->port)
>>>>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>>>>
>>>>> return len;
>>>>> '''
>>>>>
>>>>> Thanks.
>>>>> -Geliang
>>>>>
>>>>>> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
>>>>>> return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
>>>>>> }
>>>>>>
>>>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>> - struct mptcp_addr_info *saddr, bool *echo, bool *port);
>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
>>>>>> + u8 *add_addr);
>>>>>> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>> struct mptcp_rm_list *rm_list);
>>>>>> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
>>>>>> --
>>>>>> 1.8.3.1
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>> --
>>>> Li YongLong
>>>
>>
>> --
>> Li YongLong
>>
>
--
Li YongLong
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-30 1:30 ` Yonglong Li
@ 2021-06-30 2:05 ` Geliang Tang
2021-06-30 6:50 ` Yonglong Li
0 siblings, 1 reply; 15+ messages in thread
From: Geliang Tang @ 2021-06-30 2:05 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月30日周三 上午9:30写道：
>
>
>
> On 2021/6/29 16:25, Geliang Tang wrote:
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 下午3:54写道：
> >>
> >>
> >>
> >> On 2021/6/29 15:35, Geliang Tang wrote:
> >>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 下午3:02写道：
> >>>>
> >>>>
> >>>> Hi Geiliang, Thanks for your reviews.
> >>>>
> >>>> On 2021/6/29 13:58, Geliang Tang wrote:
> >>>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 上午9:42写道：
> >>>>>>
> >>>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> >>>>>> ADD_ADDR/echo-ADD_ADDR option
> >>>>>>
> >>>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >>>>>>
> >>>>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> >>>>>> ---
> >>>>>> include/net/mptcp.h | 3 ++-
> >>>>>> net/mptcp/options.c | 65 +++++++++++++++++++++++++++++++---------------------
> >>>>>> net/mptcp/pm.c | 33 +++++++++++---------------
> >>>>>> net/mptcp/protocol.h | 23 ++++++++++++-------
> >>>>>> 4 files changed, 69 insertions(+), 55 deletions(-)
> >>>>>>
> >>>>>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> >>>>>> index d61bbbf..d2c6ebe 100644
> >>>>>> --- a/include/net/mptcp.h
> >>>>>> +++ b/include/net/mptcp.h
> >>>>>> @@ -61,7 +61,8 @@ struct mptcp_out_options {
> >>>>>> u64 sndr_key;
> >>>>>> u64 rcvr_key;
> >>>>>> u64 ahmac;
> >>>>>> - struct mptcp_addr_info addr;
> >>>>>> + struct mptcp_addr_info local;
> >>>>>> + struct mptcp_addr_info remote;
> >>>>>> struct mptcp_rm_list rm_list;
> >>>>>> u8 join_id;
> >>>>>> u8 backup;
> >>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>>>>> index 1aec016..1707bec 100644
> >>>>>> --- a/net/mptcp/options.c
> >>>>>> +++ b/net/mptcp/options.c
> >>>>>> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >>>>>> bool drop_other_suboptions = false;
> >>>>>> unsigned int opt_size = *size;
> >>>>>> - bool echo;
> >>>>>> - bool port;
> >>>>>> - int len;
> >>>>>> + u8 add_addr, flags = 0xff;
> >>>>>> + int len = 0;
> >>>>>>
> >>>>>> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> >>>>>> - mptcp_pm_should_add_signal_port(msk) ||
> >>>>>> - mptcp_pm_should_add_signal_echo(msk)) &&
> >>>>>> + if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
> >>>>>> + return false;
> >>>>>
> >>>>> This add_addr argument is useless, let's drop it.
> >>>>>
> >>>> we can use add_addr use in debug log.
> >>>
> >>> I think it's not worth adding a new argument just for debugging.
> >> agree.
> >>
> >>>
> >>>>
> >>>>> And here add back mptcp_pm_should_add_signal check here. The original code
> >>>>> called mptcp_pm_should_add_signal twice for double check, once out of pm
> >>>>> lock, once under pm lock. We should keep it.
> >>>> Sorry, I think double check is not necessary. does we need double check?
> >>>
> >>> I think we should keep the original logic here. If we want to drop this
> >>> double check or something, we should do it in another patch, don't mix too
> >>> much things in one patch.
> >> agree.
> >>
> >>>
> >>>>
> >>>>>
> >>>>>> +
> >>>>>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> >>>>>> + (mptcp_pm_should_add_signal_addr(msk) &&
> >>>>>> + (opts->local.family == AF_INET6 || opts->local.port))) &&
> >>>>>> skb && skb_is_tcp_pure_ack(skb)) {
> >>>>>> pr_debug("drop other suboptions");
> >>>>>> opts->suboptions = 0;
> >>>>>> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>>> drop_other_suboptions = true;
> >>>>>> }
> >>>>>>
> >>>>>> - if (!mptcp_pm_should_add_signal(msk) ||
> >>>>>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> >>>>>> - return false;
> >>>>>>
> >>>>>> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> >>>>>> + if (mptcp_pm_should_add_signal_echo(msk)) {
> >>>>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>>>> + } else {
> >>>>>> + opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >>>>>> + msk->remote_key,
> >>>>>> + &opts->local);
> >>>>>
> >>>>> Keep this ahmac generating code after opts->suboptions set just like the
> >>>>> original code, since ahmac is the more expensive to populate. If remaining
> >>>>> length isn't enough, no need to set ahmac.
> >>>>
> >>>> because mptcp_add_addr_len(opts) will use ahmac to calculate len of opts, so I think Keep this ahmac
> >>>> generating code after opts->suboptions set is not ok.
> >>>
> >>> So we should use mptcp_pm_should_add_signal_addr instead of opts->ahmac in
> >>> mptcp_add_addr_len.
> >> agree.
> >>
> >>>
> >>>>
> >>>>>
> >>>>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> >>>>>> + }
> >>>>>> +
> >>>>>> + len = mptcp_add_addr_len(opts);
> >>>>>> if (remaining < len)
> >>>>>> return false;
> >>>>>>
> >>>>>> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>>> if (drop_other_suboptions)
> >>>>>> *size -= opt_size;
> >>>>>> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >>>>>> - if (!echo) {
> >>>>>> - opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >>>>>> - msk->remote_key,
> >>>>>> - &opts->addr);
> >>>>>> - }
> >>>>>> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> >>>>>> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> >>>>>> +
> >>>>>> + spin_lock_bh(&msk->pm.lock);
> >>>>>> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> >>>>>> + spin_unlock_bh(&msk->pm.lock);
> >>>>>
> >>>>> addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
> >>>>> set it again. I thinks this trunk and all the flags set above should be
> >>>>> dropped.
> >>>>
> >>>> Because MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO maybe set at the same time.
> >>>> So i think we should only unset one flag.
> >>>
> >>> We can only unset one flag in mptcp_pm_add_addr_signal, see my comment in
> >>> patch 1.
> >>
> >> if change like this. there is a issue: if remaining len checking is not ok and return false, The ADD_ADDR/ECHO event will
> >> be clear. So I think we should make sure ADD_ADDR/ECHO option will add in packet before clean flags. WDYT?
> >>
> >
> > You're right, let's clear it in mptcp_established_options_add_addr.
> > Furthermore, we should do the same thing for RM_ADDR, clear rm_addr in
> > mptcp_established_options_rm_addr too.
> >
> > If so, patch 1 will become useless. Let's drop it.
> >
> > -Geliang
> > I think RM_ADDR doesn't have this issue. Because mptcp_pm_rm_addr_signal() check the failed case.
If so, how about doing the same thing as RM_ADDR to check the failed case
in mptcp_pm_add_addr_signal too.
I think we should use the same logic for ADD_ADDR and RM_ADDR.
>
> >
> >
> >>>
> >>> -Geliang
> >>>
> >>>>
> >>>>>
> >>>>>> +
> >>>>>> + pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> >>>>>> + add_addr, (opts->ahmac == 0), opts->local.id,
> >>>>>> + opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
> >>>>>>
> >>>>>> return true;
> >>>>>> }
> >>>>>
> >>>>> The whole function is something like this:
> >>>>> '''
> >>>>> struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> >>>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >>>>> bool drop_other_suboptions = false;
> >>>>> unsigned int opt_size = *size;
> >>>>> int len;
> >>>>>
> >>>>> if (!mptcp_pm_should_add_signal(msk) ||
> >>>>> !mptcp_pm_add_addr_signal(msk, remaining, opts))
> >>>>> return false;
> >>>>>
> >>>>> if ((mptcp_pm_should_add_signal_echo(msk) ||
> >>>>> (mptcp_pm_should_add_signal_addr(msk) &&
> >>>>> (opts->local.family == AF_INET6 || opts->local.port))) &&
> >>>>> skb && skb_is_tcp_pure_ack(skb)) {
> >>>>> pr_debug("drop other suboptions");
> >>>>> opts->suboptions = 0;
> >>>>> opts->ext_copy.use_ack = 0;
> >>>>> opts->ext_copy.use_map = 0;
> >>>>> remaining += opt_size;
> >>>>> drop_other_suboptions = true;
> >>>>> }
> >>>>>
> >>>>> len = mptcp_add_addr_len(opts);
> >>>>> if (remaining < len)
> >>>>> return false;
> >>>>>
> >>>>> *size = len;
> >>>>> if (drop_other_suboptions)
> >>>>> *size -= opt_size;
> >>>>> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >>>>> if (mptcp_pm_should_add_signal_addr(msk)) {
> >>>>> opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >>>>> msk->remote_key,
> >>>>> &opts->local);
> >>>>> }
> >>>>>
> >>>>> pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
> >>>>> ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
> >>>>> msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
> >>>>> opts->ahmac, ntohs(opts->local.port),
> >>>>> opts->remote.id, ntohs(opts->remote.port));
> >>>>>
> >>>>> return true;
> >>>>> '''
> >>>>>
> >>>>>> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>>>>>
> >>>>>> mp_capable_done:
> >>>>>> if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> >>>>>> + struct mptcp_addr_info *addr = &opts->remote;
> >>>>>
> >>>>> We can simplify it like this:
> >>>>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> >>>>> &opts->remote;
> >>>>>
> >>>>>> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>>>> u8 echo = MPTCP_ADDR_ECHO;
> >>>>>>
> >>>>>> + if (opts->ahmac)
> >>>>>> + addr = &opts->local;
> >>>>>
> >>>>> And this trunk can be dropped.
> >>>>>
> >>>>>> +
> >>>>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >>>>>> - if (opts->addr.family == AF_INET6)
> >>>>>> + if (addr->family == AF_INET6)
> >>>>>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>>>> #endif
> >>>>>>
> >>>>>> - if (opts->addr.port)
> >>>>>> + if (addr->port)
> >>>>>> len += TCPOLEN_MPTCP_PORT_LEN;
> >>>>>>
> >>>>>> if (opts->ahmac) {
> >>>>>> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>>>>> }
> >>>>>>
> >>>>>> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> >>>>>> - len, echo, opts->addr.id);
> >>>>>> - if (opts->addr.family == AF_INET) {
> >>>>>> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> >>>>>> + len, echo, addr->id);
> >>>>>> + if (addr->family == AF_INET) {
> >>>>>> + memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
> >>>>>> ptr += 1;
> >>>>>> }
> >>>>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >>>>>> - else if (opts->addr.family == AF_INET6) {
> >>>>>> - memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
> >>>>>> + else if (addr->family == AF_INET6) {
> >>>>>> + memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
> >>>>>> ptr += 4;
> >>>>>> }
> >>>>>> #endif
> >>>>>>
> >>>>>> - if (!opts->addr.port) {
> >>>>>> + if (!addr->port) {
> >>>>>> if (opts->ahmac) {
> >>>>>> put_unaligned_be64(opts->ahmac, ptr);
> >>>>>> ptr += 2;
> >>>>>> }
> >>>>>> } else {
> >>>>>> - u16 port = ntohs(opts->addr.port);
> >>>>>> + u16 port = ntohs(addr->port);
> >>>>>>
> >>>>>> if (opts->ahmac) {
> >>>>>> u8 *bptr = (u8 *)ptr;
> >>>>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> >>>>>> index cf873e9..9c621293 100644
> >>>>>> --- a/net/mptcp/pm.c
> >>>>>> +++ b/net/mptcp/pm.c
> >>>>>> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >>>>>>
> >>>>>> /* path manager helpers */
> >>>>>>
> >>>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>>> - struct mptcp_addr_info *saddr, bool *echo, bool *port)
> >>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> >>>>>> + u8 *add_addr)
> >>>>>
> >>>>> Drop this add_addr argument.
> >>>>>
> >>>>>> {
> >>>>>> - u8 add_addr;
> >>>>>> - int ret = false;
> >>>>>> -
> >>>>>> spin_lock_bh(&msk->pm.lock);
> >>>>>>
> >>>>>> - /* double check after the lock is acquired */
> >>>>>> - if (!mptcp_pm_should_add_signal(msk))
> >>>>>> - goto out_unlock;
> >>>>>
> >>>>> Keep this double check code.
> >>>>>
> >>>>>> -
> >>>>>> - *echo = mptcp_pm_should_add_signal_echo(msk);
> >>>>>> - *port = mptcp_pm_should_add_signal_port(msk);
> >>>>>> -
> >>>>>> - if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> >>>>>> - goto out_unlock;
> >>>>>
> >>>>> Keep this length double check code too.
> >>>>>
> >>>>>> + if (!mptcp_pm_should_add_signal(msk)) {
> >>>>>> + spin_unlock_bh(&msk->pm.lock);
> >>>>>> + return false;
> >>>>>> + }
> >>>>>>
> >>>>>> - *saddr = msk->pm.local;
> >>>>>> - add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> >>>>> - WRITE_ONCE(msk->pm.addr_signal, add_addr);
> >>>>>
> >>>>> This code is just added in patch 1, I think we should keep it. And no need
> >>>>> to write addr_signal again in mptcp_established_options_add_addr.
> >>>>>
> >>>>>> - ret = true;
> >>>>>> + opts->local = msk->pm.local;
> >>>>>> + opts->remote = msk->pm.remote;
> >>>>>> + *add_addr = msk->pm.addr_signal;
> >>>>>>
> >>>>>> -out_unlock:
> >>>>>> spin_unlock_bh(&msk->pm.lock);
> >>>>>> - return ret;
> >>>>>
> >>>>> Keep this out_unlock code.
> >>>>>
> >>>>>> +
> >>>>>> + if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> >>>>>> + mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> >>>>>
> >>>>> Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
> >>>>>
> >>>>> I'm no sure why we need this two lines, and why you use '&&' here. Do you
> >>>>> mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?
Please move these two lines into a new patch, and describe why we need it
in the commit log.
Thanks.
-Geliang
> >>>>>
> >>>>>> + return true;
> >>>>>> }
> >>>>>
> >>>>> The whole function is something like this:
> >>>>> '''
> >>>>> int ret = false;
> >>>>> u8 add_addr;
> >>>>>
> >>>>> spin_lock_bh(&msk->pm.lock);
> >>>>>
> >>>>> /* double check after the lock is acquired */
> >>>>> if (!mptcp_pm_should_add_signal(msk))
> >>>>> goto out_unlock;
> >>>>>
> >>>>> if (remaining < mptcp_add_addr_len(opts))
> >>>>> goto out_unlock;
> >>>>>
> >>>>> opts->local = msk->pm.local;
> >>>>> opts->remote = msk->pm.remote;
> >>>>> if (mptcp_pm_should_add_signal_echo(msk))
> >>>>> add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>>> else
> >>>>> add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
> >>>>> WRITE_ONCE(msk->pm.addr_signal, add_addr);
> >>>>> ret = true;
> >>>>>
> >>>>> out_unlock:
> >>>>> spin_unlock_bh(&msk->pm.lock);
> >>>>> if (ret && mptcp_pm_should_add_signal_echo(msk) &&
> >>>>> mptcp_pm_should_add_signal_addr(msk))
> >>>>> mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> >>>>> return ret;
> >>>>> '''
> >>>>>
> >>>>>>
> >>>>>> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> >>>>>> index a0b0ec0..0bfbbdef 100644
> >>>>>> --- a/net/mptcp/protocol.h
> >>>>>> +++ b/net/mptcp/protocol.h
> >>>>>> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
> >>>>>> return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
> >>>>>> }
> >>>>>>
> >>>>>> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> >>>>>> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
> >>>>>> {
> >>>>>> - u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>>>> + u8 len = 0;
> >>>>>> + struct mptcp_addr_info *addr = &opts->remote;
> >>>>>
> >>>>> We can simplify it like this:
> >>>>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> >>>>> &opts->remote;
> >>>>>
> >>>>> And keep the orignal code unchanged.
> >>>>>
> >>>>>>
> >>>>>> - if (family == AF_INET6)
> >>>>>> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>>>> - if (!echo)
> >>>>>> + if (opts->ahmac) {
> >>>>>> + addr = &opts->local;
> >>>>>> len += MPTCPOPT_THMAC_LEN;
> >>>>>> + }
> >>>>>> +
> >>>>>> + if (addr->family == AF_INET6)
> >>>>>> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>>>> + else
> >>>>>> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>>>> +
> >>>>>> /* account for 2 trailing 'nop' options */
> >>>>>> - if (port)
> >>>>>> + if (addr->port)
> >>>>>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >>>>>>
> >>>>>> return len;
> >>>>>
> >>>>> The whole function is something like this:
> >>>>> '''
> >>>>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
> >>>>> &opts->remote;
> >>>>> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>>>
> >>>>> if (addr->family == AF_INET6)
> >>>>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>>> if (opts->ahmac)
> >>>>> len += MPTCPOPT_THMAC_LEN;
> >>>>> /* account for 2 trailing 'nop' options */
> >>>>> if (addr->port)
> >>>>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >>>>>
> >>>>> return len;
> >>>>> '''
> >>>>>
> >>>>> Thanks.
> >>>>> -Geliang
> >>>>>
> >>>>>> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
> >>>>>> return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
> >>>>>> }
> >>>>>>
> >>>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>>> - struct mptcp_addr_info *saddr, bool *echo, bool *port);
> >>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
> >>>>>> + u8 *add_addr);
> >>>>>> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>>> struct mptcp_rm_list *rm_list);
> >>>>>> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> >>>>>> --
> >>>>>> 1.8.3.1
> >>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>> --
> >>>> Li YongLong
> >>>
> >>
> >> --
> >> Li YongLong
> >>
> >
>
> --
> Li YongLong
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-30 2:05 ` Geliang Tang
@ 2021-06-30 6:50 ` Yonglong Li
0 siblings, 0 replies; 15+ messages in thread
From: Yonglong Li @ 2021-06-30 6:50 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp, Mat Martineau
On 2021/6/30 10:05, Geliang Tang wrote:
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月30日周三 上午9:30写道：
>>
>>
>>
>> On 2021/6/29 16:25, Geliang Tang wrote:
>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 下午3:54写道：
>>>>
>>>>
>>>>
>>>> On 2021/6/29 15:35, Geliang Tang wrote:
>>>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 下午3:02写道：
>>>>>>
>>>>>>
>>>>>> Hi Geiliang, Thanks for your reviews.
>>>>>>
>>>>>> On 2021/6/29 13:58, Geliang Tang wrote:
>>>>>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月29日周二 上午9:42写道：
>>>>>>>>
>>>>>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>>>>>>>> ADD_ADDR/echo-ADD_ADDR option
>>>>>>>>
>>>>>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>>>>>>>
>>>>>>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
>>>>>>>> ---
>>>>>>>> include/net/mptcp.h | 3 ++-
>>>>>>>> net/mptcp/options.c | 65 +++++++++++++++++++++++++++++++---------------------
>>>>>>>> net/mptcp/pm.c | 33 +++++++++++---------------
>>>>>>>> net/mptcp/protocol.h | 23 ++++++++++++-------
>>>>>>>> 4 files changed, 69 insertions(+), 55 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
>>>>>>>> index d61bbbf..d2c6ebe 100644
>>>>>>>> --- a/include/net/mptcp.h
>>>>>>>> +++ b/include/net/mptcp.h
>>>>>>>> @@ -61,7 +61,8 @@ struct mptcp_out_options {
>>>>>>>> u64 sndr_key;
>>>>>>>> u64 rcvr_key;
>>>>>>>> u64 ahmac;
>>>>>>>> - struct mptcp_addr_info addr;
>>>>>>>> + struct mptcp_addr_info local;
>>>>>>>> + struct mptcp_addr_info remote;
>>>>>>>> struct mptcp_rm_list rm_list;
>>>>>>>> u8 join_id;
>>>>>>>> u8 backup;
>>>>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>>>>>> index 1aec016..1707bec 100644
>>>>>>>> --- a/net/mptcp/options.c
>>>>>>>> +++ b/net/mptcp/options.c
>>>>>>>> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>>>>>> bool drop_other_suboptions = false;
>>>>>>>> unsigned int opt_size = *size;
>>>>>>>> - bool echo;
>>>>>>>> - bool port;
>>>>>>>> - int len;
>>>>>>>> + u8 add_addr, flags = 0xff;
>>>>>>>> + int len = 0;
>>>>>>>>
>>>>>>>> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>>>>>>>> - mptcp_pm_should_add_signal_port(msk) ||
>>>>>>>> - mptcp_pm_should_add_signal_echo(msk)) &&
>>>>>>>> + if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr))
>>>>>>>> + return false;
>>>>>>>
>>>>>>> This add_addr argument is useless, let's drop it.
>>>>>>>
>>>>>> we can use add_addr use in debug log.
>>>>>
>>>>> I think it's not worth adding a new argument just for debugging.
>>>> agree.
>>>>
>>>>>
>>>>>>
>>>>>>> And here add back mptcp_pm_should_add_signal check here. The original code
>>>>>>> called mptcp_pm_should_add_signal twice for double check, once out of pm
>>>>>>> lock, once under pm lock. We should keep it.
>>>>>> Sorry, I think double check is not necessary. does we need double check?
>>>>>
>>>>> I think we should keep the original logic here. If we want to drop this
>>>>> double check or something, we should do it in another patch, don't mix too
>>>>> much things in one patch.
>>>> agree.
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>>>>>> + (mptcp_pm_should_add_signal_addr(msk) &&
>>>>>>>> + (opts->local.family == AF_INET6 || opts->local.port))) &&
>>>>>>>> skb && skb_is_tcp_pure_ack(skb)) {
>>>>>>>> pr_debug("drop other suboptions");
>>>>>>>> opts->suboptions = 0;
>>>>>>>> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>>>> drop_other_suboptions = true;
>>>>>>>> }
>>>>>>>>
>>>>>>>> - if (!mptcp_pm_should_add_signal(msk) ||
>>>>>>>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>>>>>>>> - return false;
>>>>>>>>
>>>>>>>> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
>>>>>>>> + if (mptcp_pm_should_add_signal_echo(msk)) {
>>>>>>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>>>>> + } else {
>>>>>>>> + opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>>>>>> + msk->remote_key,
>>>>>>>> + &opts->local);
>>>>>>>
>>>>>>> Keep this ahmac generating code after opts->suboptions set just like the
>>>>>>> original code, since ahmac is the more expensive to populate. If remaining
>>>>>>> length isn't enough, no need to set ahmac.
>>>>>>
>>>>>> because mptcp_add_addr_len(opts) will use ahmac to calculate len of opts, so I think Keep this ahmac
>>>>>> generating code after opts->suboptions set is not ok.
>>>>>
>>>>> So we should use mptcp_pm_should_add_signal_addr instead of opts->ahmac in
>>>>> mptcp_add_addr_len.
>>>> agree.
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + len = mptcp_add_addr_len(opts);
>>>>>>>> if (remaining < len)
>>>>>>>> return false;
>>>>>>>>
>>>>>>>> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>>>> if (drop_other_suboptions)
>>>>>>>> *size -= opt_size;
>>>>>>>> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>>>>>> - if (!echo) {
>>>>>>>> - opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>>>>>> - msk->remote_key,
>>>>>>>> - &opts->addr);
>>>>>>>> - }
>>>>>>>> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
>>>>>>>> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
>>>>>>>> +
>>>>>>>> + spin_lock_bh(&msk->pm.lock);
>>>>>>>> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
>>>>>>>> + spin_unlock_bh(&msk->pm.lock);
>>>>>>>
>>>>>>> addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to
>>>>>>> set it again. I thinks this trunk and all the flags set above should be
>>>>>>> dropped.
>>>>>>
>>>>>> Because MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO maybe set at the same time.
>>>>>> So i think we should only unset one flag.
>>>>>
>>>>> We can only unset one flag in mptcp_pm_add_addr_signal, see my comment in
>>>>> patch 1.
>>>>
>>>> if change like this. there is a issue: if remaining len checking is not ok and return false, The ADD_ADDR/ECHO event will
>>>> be clear. So I think we should make sure ADD_ADDR/ECHO option will add in packet before clean flags. WDYT?
>>>>
>>>
>>> You're right, let's clear it in mptcp_established_options_add_addr.
>>> Furthermore, we should do the same thing for RM_ADDR, clear rm_addr in
>>> mptcp_established_options_rm_addr too.
>>>
>>> If so, patch 1 will become useless. Let's drop it.
>>>
>>> -Geliang
>>> I think RM_ADDR doesn't have this issue. Because mptcp_pm_rm_addr_signal() check the failed case.
>
> If so, how about doing the same thing as RM_ADDR to check the failed case
> in mptcp_pm_add_addr_signal too.
>
> I think we should use the same logic for ADD_ADDR and RM_ADDR.
Agree. I will prepare next patch.
>
>>
>>>
>>>
>>>>>
>>>>> -Geliang
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> + pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
>>>>>>>> + add_addr, (opts->ahmac == 0), opts->local.id,
>>>>>>>> + opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>>>>>>>>
>>>>>>>> return true;
>>>>>>>> }
>>>>>>>
>>>>>>> The whole function is something like this:
>>>>>>> '''
>>>>>>> struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>>>>>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>>>>> bool drop_other_suboptions = false;
>>>>>>> unsigned int opt_size = *size;
>>>>>>> int len;
>>>>>>>
>>>>>>> if (!mptcp_pm_should_add_signal(msk) ||
>>>>>>> !mptcp_pm_add_addr_signal(msk, remaining, opts))
>>>>>>> return false;
>>>>>>>
>>>>>>> if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>>>>> (mptcp_pm_should_add_signal_addr(msk) &&
>>>>>>> (opts->local.family == AF_INET6 || opts->local.port))) &&
>>>>>>> skb && skb_is_tcp_pure_ack(skb)) {
>>>>>>> pr_debug("drop other suboptions");
>>>>>>> opts->suboptions = 0;
>>>>>>> opts->ext_copy.use_ack = 0;
>>>>>>> opts->ext_copy.use_map = 0;
>>>>>>> remaining += opt_size;
>>>>>>> drop_other_suboptions = true;
>>>>>>> }
>>>>>>>
>>>>>>> len = mptcp_add_addr_len(opts);
>>>>>>> if (remaining < len)
>>>>>>> return false;
>>>>>>>
>>>>>>> *size = len;
>>>>>>> if (drop_other_suboptions)
>>>>>>> *size -= opt_size;
>>>>>>> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>>>>> if (mptcp_pm_should_add_signal_addr(msk)) {
>>>>>>> opts->ahmac = add_addr_generate_hmac(msk->local_key,
>>>>>>> msk->remote_key,
>>>>>>> &opts->local);
>>>>>>> }
>>>>>>>
>>>>>>> pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d,
>>>>>>> ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d",
>>>>>>> msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id,
>>>>>>> opts->ahmac, ntohs(opts->local.port),
>>>>>>> opts->remote.id, ntohs(opts->remote.port));
>>>>>>>
>>>>>>> return true;
>>>>>>> '''
>>>>>>>
>>>>>>>> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>>>>>>>
>>>>>>>> mp_capable_done:
>>>>>>>> if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
>>>>>>>> + struct mptcp_addr_info *addr = &opts->remote;
>>>>>>>
>>>>>>> We can simplify it like this:
>>>>>>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>>>>>> &opts->remote;
>>>>>>>
>>>>>>>> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>>>> u8 echo = MPTCP_ADDR_ECHO;
>>>>>>>>
>>>>>>>> + if (opts->ahmac)
>>>>>>>> + addr = &opts->local;
>>>>>>>
>>>>>>> And this trunk can be dropped.
>>>>>>>
>>>>>>>> +
>>>>>>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>>>>>>> - if (opts->addr.family == AF_INET6)
>>>>>>>> + if (addr->family == AF_INET6)
>>>>>>>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>>>> #endif
>>>>>>>>
>>>>>>>> - if (opts->addr.port)
>>>>>>>> + if (addr->port)
>>>>>>>> len += TCPOLEN_MPTCP_PORT_LEN;
>>>>>>>>
>>>>>>>> if (opts->ahmac) {
>>>>>>>> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>>>>>>> }
>>>>>>>>
>>>>>>>> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
>>>>>>>> - len, echo, opts->addr.id);
>>>>>>>> - if (opts->addr.family == AF_INET) {
>>>>>>>> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
>>>>>>>> + len, echo, addr->id);
>>>>>>>> + if (addr->family == AF_INET) {
>>>>>>>> + memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4);
>>>>>>>> ptr += 1;
>>>>>>>> }
>>>>>>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>>>>>>> - else if (opts->addr.family == AF_INET6) {
>>>>>>>> - memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16);
>>>>>>>> + else if (addr->family == AF_INET6) {
>>>>>>>> + memcpy((u8 *)ptr, addr->addr6.s6_addr, 16);
>>>>>>>> ptr += 4;
>>>>>>>> }
>>>>>>>> #endif
>>>>>>>>
>>>>>>>> - if (!opts->addr.port) {
>>>>>>>> + if (!addr->port) {
>>>>>>>> if (opts->ahmac) {
>>>>>>>> put_unaligned_be64(opts->ahmac, ptr);
>>>>>>>> ptr += 2;
>>>>>>>> }
>>>>>>>> } else {
>>>>>>>> - u16 port = ntohs(opts->addr.port);
>>>>>>>> + u16 port = ntohs(addr->port);
>>>>>>>>
>>>>>>>> if (opts->ahmac) {
>>>>>>>> u8 *bptr = (u8 *)ptr;
>>>>>>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>>>>>>> index cf873e9..9c621293 100644
>>>>>>>> --- a/net/mptcp/pm.c
>>>>>>>> +++ b/net/mptcp/pm.c
>>>>>>>> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>>>>>>>
>>>>>>>> /* path manager helpers */
>>>>>>>>
>>>>>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>>>> - struct mptcp_addr_info *saddr, bool *echo, bool *port)
>>>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
>>>>>>>> + u8 *add_addr)
>>>>>>>
>>>>>>> Drop this add_addr argument.
>>>>>>>
>>>>>>>> {
>>>>>>>> - u8 add_addr;
>>>>>>>> - int ret = false;
>>>>>>>> -
>>>>>>>> spin_lock_bh(&msk->pm.lock);
>>>>>>>>
>>>>>>>> - /* double check after the lock is acquired */
>>>>>>>> - if (!mptcp_pm_should_add_signal(msk))
>>>>>>>> - goto out_unlock;
>>>>>>>
>>>>>>> Keep this double check code.
>>>>>>>
>>>>>>>> -
>>>>>>>> - *echo = mptcp_pm_should_add_signal_echo(msk);
>>>>>>>> - *port = mptcp_pm_should_add_signal_port(msk);
>>>>>>>> -
>>>>>>>> - if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
>>>>>>>> - goto out_unlock;
>>>>>>>
>>>>>>> Keep this length double check code too.
>>>>>>>
>>>>>>>> + if (!mptcp_pm_should_add_signal(msk)) {
>>>>>>>> + spin_unlock_bh(&msk->pm.lock);
>>>>>>>> + return false;
>>>>>>>> + }
>>>>>>>>
>>>>>>>> - *saddr = msk->pm.local;
>>>>>>>> - add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
>>>>>>> - WRITE_ONCE(msk->pm.addr_signal, add_addr);
>>>>>>>
>>>>>>> This code is just added in patch 1, I think we should keep it. And no need
>>>>>>> to write addr_signal again in mptcp_established_options_add_addr.
>>>>>>>
>>>>>>>> - ret = true;
>>>>>>>> + opts->local = msk->pm.local;
>>>>>>>> + opts->remote = msk->pm.remote;
>>>>>>>> + *add_addr = msk->pm.addr_signal;
>>>>>>>>
>>>>>>>> -out_unlock:
>>>>>>>> spin_unlock_bh(&msk->pm.lock);
>>>>>>>> - return ret;
>>>>>>>
>>>>>>> Keep this out_unlock code.
>>>>>>>
>>>>>>>> +
>>>>>>>> + if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
>>>>>>>> + mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>>>>>>>
>>>>>>> Could we use mptcp_pm_add_addr_send_ack here instead of open coding?
>>>>>>>
>>>>>>> I'm no sure why we need this two lines, and why you use '&&' here. Do you
>>>>>>> mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time?
>
> Please move these two lines into a new patch, and describe why we need it
> in the commit log.
>
> Thanks.
> -Geliang
>
>>>>>>>
>>>>>>>> + return true;
>>>>>>>> }
>>>>>>>
>>>>>>> The whole function is something like this:
>>>>>>> '''
>>>>>>> int ret = false;
>>>>>>> u8 add_addr;
>>>>>>>
>>>>>>> spin_lock_bh(&msk->pm.lock);
>>>>>>>
>>>>>>> /* double check after the lock is acquired */
>>>>>>> if (!mptcp_pm_should_add_signal(msk))
>>>>>>> goto out_unlock;
>>>>>>>
>>>>>>> if (remaining < mptcp_add_addr_len(opts))
>>>>>>> goto out_unlock;
>>>>>>>
>>>>>>> opts->local = msk->pm.local;
>>>>>>> opts->remote = msk->pm.remote;
>>>>>>> if (mptcp_pm_should_add_signal_echo(msk))
>>>>>>> add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>>>> else
>>>>>>> add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
>>>>>>> WRITE_ONCE(msk->pm.addr_signal, add_addr);
>>>>>>> ret = true;
>>>>>>>
>>>>>>> out_unlock:
>>>>>>> spin_unlock_bh(&msk->pm.lock);
>>>>>>> if (ret && mptcp_pm_should_add_signal_echo(msk) &&
>>>>>>> mptcp_pm_should_add_signal_addr(msk))
>>>>>>> mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>>>>>>> return ret;
>>>>>>> '''
>>>>>>>
>>>>>>>>
>>>>>>>> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>>>>>>> index a0b0ec0..0bfbbdef 100644
>>>>>>>> --- a/net/mptcp/protocol.h
>>>>>>>> +++ b/net/mptcp/protocol.h
>>>>>>>> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
>>>>>>>> return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
>>>>>>>> }
>>>>>>>>
>>>>>>>> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
>>>>>>>> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts)
>>>>>>>> {
>>>>>>>> - u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>>>> + u8 len = 0;
>>>>>>>> + struct mptcp_addr_info *addr = &opts->remote;
>>>>>>>
>>>>>>> We can simplify it like this:
>>>>>>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>>>>>> &opts->remote;
>>>>>>>
>>>>>>> And keep the orignal code unchanged.
>>>>>>>
>>>>>>>>
>>>>>>>> - if (family == AF_INET6)
>>>>>>>> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>>>> - if (!echo)
>>>>>>>> + if (opts->ahmac) {
>>>>>>>> + addr = &opts->local;
>>>>>>>> len += MPTCPOPT_THMAC_LEN;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + if (addr->family == AF_INET6)
>>>>>>>> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>>>> + else
>>>>>>>> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>>>> +
>>>>>>>> /* account for 2 trailing 'nop' options */
>>>>>>>> - if (port)
>>>>>>>> + if (addr->port)
>>>>>>>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>>>>>>>
>>>>>>>> return len;
>>>>>>>
>>>>>>> The whole function is something like this:
>>>>>>> '''
>>>>>>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local :
>>>>>>> &opts->remote;
>>>>>>> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>>>
>>>>>>> if (addr->family == AF_INET6)
>>>>>>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>>> if (opts->ahmac)
>>>>>>> len += MPTCPOPT_THMAC_LEN;
>>>>>>> /* account for 2 trailing 'nop' options */
>>>>>>> if (addr->port)
>>>>>>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>>>>>>
>>>>>>> return len;
>>>>>>> '''
>>>>>>>
>>>>>>> Thanks.
>>>>>>> -Geliang
>>>>>>>
>>>>>>>> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
>>>>>>>> return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
>>>>>>>> }
>>>>>>>>
>>>>>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>>>> - struct mptcp_addr_info *saddr, bool *echo, bool *port);
>>>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts,
>>>>>>>> + u8 *add_addr);
>>>>>>>> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>>>> struct mptcp_rm_list *rm_list);
>>>>>>>> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
>>>>>>>> --
>>>>>>>> 1.8.3.1
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Li YongLong
>>>>>
>>>>
>>>> --
>>>> Li YongLong
>>>>
>>>
>>
>> --
>> Li YongLong
>
--
Li YongLong
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-06-30 6:50 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-29 1:41 [PATCH v6 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
2021-06-29 1:41 ` [PATCH v6 1/4] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
2021-06-29 5:43 ` Geliang Tang
2021-06-29 1:41 ` [PATCH v6 2/4] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
2021-06-29 1:41 ` [PATCH v6 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
2021-06-29 5:58 ` Geliang Tang
2021-06-29 6:05 ` Geliang Tang
2021-06-29 7:01 ` Yonglong Li
2021-06-29 7:35 ` Geliang Tang
2021-06-29 7:54 ` Yonglong Li
2021-06-29 8:25 ` Geliang Tang
2021-06-30 1:30 ` Yonglong Li
2021-06-30 2:05 ` Geliang Tang
2021-06-30 6:50 ` Yonglong Li
2021-06-29 1:41 ` [PATCH v6 4/4] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT Yonglong Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).