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