* [MPTCP][PATCH mptcp-next] Squash to "mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate"
@ 2021-07-11 15:15 Geliang Tang
2021-07-11 15:15 ` [MPTCP][PATCH mptcp-next] Squash to "mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other" Geliang Tang
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Geliang Tang @ 2021-07-11 15:15 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang
A small cleanup.
Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
net/mptcp/pm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 30deb76fa5d0..1eeecd68f159 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, echo:%d", msk, addr->id, echo);
+ pr_debug("msk=%p, local_id=%d, echo=%d", msk, addr->id, echo);
lockdep_assert_held(&msk->pm.lock);
--
2.31.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [MPTCP][PATCH mptcp-next] Squash to "mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other"
2021-07-11 15:15 [MPTCP][PATCH mptcp-next] Squash to "mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate" Geliang Tang
@ 2021-07-11 15:15 ` Geliang Tang
2021-07-12 9:55 ` Yonglong Li
2021-07-11 15:15 ` [MPTCP][PATCH mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal" Geliang Tang
2021-07-12 22:10 ` [MPTCP][PATCH mptcp-next] Squash to "mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate" Mat Martineau
2 siblings, 1 reply; 17+ messages in thread
From: Geliang Tang @ 2021-07-11 15:15 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang
Add READ_ONCE() for reading msk->pm.addr_signal.
Use mptcp_pm_should_add_signal_echo instead of open coding.
Use '&=' to clear flag.
Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
net/mptcp/pm.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index c9622696716e..be16da2dcb6b 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -257,6 +257,7 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
struct mptcp_addr_info *saddr, bool *echo, bool *port)
{
int ret = false;
+ u8 add_addr;
spin_lock_bh(&msk->pm.lock);
@@ -271,10 +272,12 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
goto out_unlock;
*saddr = msk->pm.local;
- if ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)))
- WRITE_ONCE(msk->pm.addr_signal, msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO));
+ add_addr = READ_ONCE(msk->pm.addr_signal);
+ if (mptcp_pm_should_add_signal_echo(msk))
+ add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
else
- WRITE_ONCE(msk->pm.addr_signal, msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL));
+ add_addr &= ~BIT(MPTCP_ADD_ADDR_SIGNAL);
+ WRITE_ONCE(msk->pm.addr_signal, add_addr);
ret = true;
out_unlock:
@@ -294,7 +297,8 @@ 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);
+ rm_addr = READ_ONCE(msk->pm.addr_signal);
+ rm_addr &= ~BIT(MPTCP_RM_ADDR_SIGNAL);
len = mptcp_rm_addr_len(&msk->pm.rm_list_tx);
if (len < 0) {
WRITE_ONCE(msk->pm.addr_signal, rm_addr);
--
2.31.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [MPTCP][PATCH mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"
2021-07-11 15:15 [MPTCP][PATCH mptcp-next] Squash to "mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate" Geliang Tang
2021-07-11 15:15 ` [MPTCP][PATCH mptcp-next] Squash to "mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other" Geliang Tang
@ 2021-07-11 15:15 ` Geliang Tang
2021-07-12 1:34 ` Yonglong Li
2021-07-12 22:10 ` [MPTCP][PATCH mptcp-next] Squash to "mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate" Mat Martineau
2 siblings, 1 reply; 17+ messages in thread
From: Geliang Tang @ 2021-07-11 15:15 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang
I think there're still some issues in v8:
The remaining value is incorrect since "remaining += opt_size;" in the
"drop other suboptions" checks has been called twice in
mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
opts->local and opts->remote in mptcp_pm_add_addr_signal need be
populate after the length chech, not before the check.
The squash-to patch keeped the more orignal code unchanged, and just do
the least, necessary modifications.
Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
Change arguments of mptcp_pm_add_addr_signal.
Keep mptcp_add_addr_len unchanged.
Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
net/mptcp/options.c | 35 +++++++++++++++++------------------
net/mptcp/pm.c | 23 +++++++++--------------
net/mptcp/protocol.h | 27 +++++++++------------------
3 files changed, 35 insertions(+), 50 deletions(-)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 5c0ad9b90866..93ad7b134f74 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -663,16 +663,14 @@ 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;
- u8 add_addr;
+ bool echo;
+ bool port;
+ u8 family;
int len;
- if (!mptcp_pm_should_add_signal(msk) ||
- !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts, &add_addr))
- return false;
-
- if (((add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) ||
- ((add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
- (opts->local.family == AF_INET6 || opts->local.port))) &&
+ if ((mptcp_pm_should_add_signal_echo(msk) ||
+ (mptcp_pm_should_add_signal_addr(msk) &&
+ (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
skb && skb_is_tcp_pure_ack(skb)) {
pr_debug("drop other suboptions");
opts->suboptions = 0;
@@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
drop_other_suboptions = true;
}
- len = mptcp_add_addr_len(opts, add_addr);
+ if (!mptcp_pm_should_add_signal(msk) ||
+ !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
+ return false;
+
+ family = echo ? opts->remote.family : opts->local.family;
+ len = mptcp_add_addr_len(family, echo, port);
if (remaining < len)
return false;
@@ -690,15 +693,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 (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
- (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
+ if (!echo) {
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",
- add_addr, (opts->ahmac == 0), opts->local.id, opts->ahmac,
- ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
+ pr_debug("local_id=%d, local_port=%d, remote_id=%d, remote_port=%d, ahmac=%llu, echo=%d",
+ opts->local.id, ntohs(opts->local.port), opts->remote.id,
+ ntohs(opts->remote.port), opts->ahmac, echo);
return true;
}
@@ -1253,13 +1255,10 @@ 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;
+ 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;
-
#if IS_ENABLED(CONFIG_MPTCP_IPV6)
if (addr->family == AF_INET6)
len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 264f522af530..399b59cb7563 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
/* path manager helpers */
-bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
- unsigned int opt_size, unsigned int remaining,
- struct mptcp_out_options *opts, u8 *add_addr)
+bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
+ struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
+ bool *echo, bool *port)
{
int ret = false;
u8 add_addr;
+ u8 family;
spin_lock_bh(&msk->pm.lock);
@@ -266,21 +267,15 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
if (!mptcp_pm_should_add_signal(msk))
goto out_unlock;
- opts->local = msk->pm.local;
- opts->remote = msk->pm.remote;
- *add_addr = msk->pm.addr_signal;
+ *echo = mptcp_pm_should_add_signal_echo(msk);
+ *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
- if (((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)) ||
- ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
- (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
- skb && skb_is_tcp_pure_ack(skb)) {
- remaining += opt_size;
- }
-
- if (remaining < mptcp_add_addr_len(opts, *add_addr))
+ family = *echo ? msk->pm.remote.family : msk->pm.local.family;
+ if (remaining < mptcp_add_addr_len(family, *echo, *port))
goto out_unlock;
*saddr = msk->pm.local;
+ *daddr = msk->pm.remote;
add_addr = READ_ONCE(msk->pm.addr_signal);
if (mptcp_pm_should_add_signal_echo(msk))
add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 937e0309e340..4b63cc6079fa 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -766,25 +766,16 @@ 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(struct mptcp_out_options *opts,
- u8 add_addr)
+static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
{
- struct mptcp_addr_info *addr = &opts->remote;
- u8 len = 0;
+ u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
- if (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
- (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
- addr = &opts->local;
+ if (family == AF_INET6)
+ len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
+ if (!echo)
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 (addr->port)
+ if (port)
len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
return len;
@@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
- unsigned int opt_size, unsigned int remaining,
- struct mptcp_out_options *opts, u8 *add_addr);
+bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
+ struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
+ bool *echo, bool *port);
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);
--
2.31.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"
2021-07-11 15:15 ` [MPTCP][PATCH mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal" Geliang Tang
@ 2021-07-12 1:34 ` Yonglong Li
2021-07-12 7:33 ` Geliang Tang
0 siblings, 1 reply; 17+ messages in thread
From: Yonglong Li @ 2021-07-12 1:34 UTC (permalink / raw)
To: Geliang Tang, mptcp
On 2021/7/11 23:15, Geliang Tang wrote:
> I think there're still some issues in v8:
>
> The remaining value is incorrect since "remaining += opt_size;" in the
> "drop other suboptions" checks has been called twice in
> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
>
I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
mptcp_established_options_add_addr.
> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
> populate after the length chech, not before the check.]
>
> The squash-to patch keeped the more orignal code unchanged, and just do
> the least, necessary modifications.
>
Agree opts->local and opts->remote should be asigned after the length check.
But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
as orignal code, there is a race that:
==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
==> call mptcp_pm_add_addr_signal
==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
==> at this time opts->remote is empty and the length is incorrect.
So I think the orignal code is incorrect. WDYT?
> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
>
> Change arguments of mptcp_pm_add_addr_signal.
>
> Keep mptcp_add_addr_len unchanged.
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> net/mptcp/options.c | 35 +++++++++++++++++------------------
> net/mptcp/pm.c | 23 +++++++++--------------
> net/mptcp/protocol.h | 27 +++++++++------------------
> 3 files changed, 35 insertions(+), 50 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 5c0ad9b90866..93ad7b134f74 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -663,16 +663,14 @@ 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;
> - u8 add_addr;
> + bool echo;
> + bool port;
> + u8 family;
> int len;
>
> - if (!mptcp_pm_should_add_signal(msk) ||
> - !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts, &add_addr))
> - return false;
> -
> - if (((add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> - ((add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
> - (opts->local.family == AF_INET6 || opts->local.port))) &&
> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> + (mptcp_pm_should_add_signal_addr(msk) &&
> + (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> skb && skb_is_tcp_pure_ack(skb)) {
> pr_debug("drop other suboptions");
> opts->suboptions = 0;
> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> drop_other_suboptions = true;
> }
>
> - len = mptcp_add_addr_len(opts, add_addr);
> + if (!mptcp_pm_should_add_signal(msk) ||
> + !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
> + return false;
> +
> + family = echo ? opts->remote.family : opts->local.family;
> + len = mptcp_add_addr_len(family, echo, port);
> if (remaining < len)
> return false;
>
> @@ -690,15 +693,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 (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> + if (!echo) {
> 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",
> - add_addr, (opts->ahmac == 0), opts->local.id, opts->ahmac,
> - ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
> + pr_debug("local_id=%d, local_port=%d, remote_id=%d, remote_port=%d, ahmac=%llu, echo=%d",
> + opts->local.id, ntohs(opts->local.port), opts->remote.id,
> + ntohs(opts->remote.port), opts->ahmac, echo);
>
> return true;
> }
> @@ -1253,13 +1255,10 @@ 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;
> + 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;
> -
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> if (addr->family == AF_INET6)
> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 264f522af530..399b59cb7563 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>
> /* path manager helpers */
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> - unsigned int opt_size, unsigned int remaining,
> - struct mptcp_out_options *opts, u8 *add_addr)
> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> + bool *echo, bool *port)
> {
> int ret = false;
> u8 add_addr;
> + u8 family;
>
> spin_lock_bh(&msk->pm.lock);
>
> @@ -266,21 +267,15 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> if (!mptcp_pm_should_add_signal(msk))
> goto out_unlock;
>
> - opts->local = msk->pm.local;
> - opts->remote = msk->pm.remote;
> - *add_addr = msk->pm.addr_signal;
> + *echo = mptcp_pm_should_add_signal_echo(msk);
> + *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
>
> - if (((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> - ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
> - (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> - skb && skb_is_tcp_pure_ack(skb)) {
> - remaining += opt_size;
> - }
> -
> - if (remaining < mptcp_add_addr_len(opts, *add_addr))
> + family = *echo ? msk->pm.remote.family : msk->pm.local.family;
> + if (remaining < mptcp_add_addr_len(family, *echo, *port))
> goto out_unlock;
>
> *saddr = msk->pm.local;
> + *daddr = msk->pm.remote;
> add_addr = READ_ONCE(msk->pm.addr_signal);
> if (mptcp_pm_should_add_signal_echo(msk))
> add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 937e0309e340..4b63cc6079fa 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -766,25 +766,16 @@ 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(struct mptcp_out_options *opts,
> - u8 add_addr)
> +static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> {
> - struct mptcp_addr_info *addr = &opts->remote;
> - u8 len = 0;
> + u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>
> - if (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> - addr = &opts->local;
> + if (family == AF_INET6)
> + len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + if (!echo)
> 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 (addr->port)
> + if (port)
> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>
> return len;
> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
> - unsigned int opt_size, unsigned int remaining,
> - struct mptcp_out_options *opts, u8 *add_addr);
> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> + bool *echo, bool *port);
> 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);
>
--
Li YongLong
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"
2021-07-12 1:34 ` Yonglong Li
@ 2021-07-12 7:33 ` Geliang Tang
2021-07-12 8:06 ` Yonglong Li
0 siblings, 1 reply; 17+ messages in thread
From: Geliang Tang @ 2021-07-12 7:33 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp
Hi Yonglong,
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
>
>
>
> On 2021/7/11 23:15, Geliang Tang wrote:
> > I think there're still some issues in v8:
> >
> > The remaining value is incorrect since "remaining += opt_size;" in the
> > "drop other suboptions" checks has been called twice in
> > mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
> >
> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
> mptcp_established_options_add_addr.
>
> > opts->local and opts->remote in mptcp_pm_add_addr_signal need be
> > populate after the length chech, not before the check.]
> >
> > The squash-to patch keeped the more orignal code unchanged, and just do
> > the least, necessary modifications.
> >
> Agree opts->local and opts->remote should be asigned after the length check.
> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
> as orignal code, there is a race that:
>
> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> ==> call mptcp_pm_add_addr_signal
> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
> ==> at this time opts->remote is empty and the length is incorrect.
>
What will happen in v8 when this race occurs? How dose v8 deal with the
race?
> So I think the orignal code is incorrect. WDYT?
>
> > Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
> >
> > Change arguments of mptcp_pm_add_addr_signal.
> >
> > Keep mptcp_add_addr_len unchanged.
> >
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > ---
> > net/mptcp/options.c | 35 +++++++++++++++++------------------
> > net/mptcp/pm.c | 23 +++++++++--------------
> > net/mptcp/protocol.h | 27 +++++++++------------------
> > 3 files changed, 35 insertions(+), 50 deletions(-)
> >
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index 5c0ad9b90866..93ad7b134f74 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -663,16 +663,14 @@ 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;
> > - u8 add_addr;
> > + bool echo;
> > + bool port;
> > + u8 family;
> > int len;
> >
> > - if (!mptcp_pm_should_add_signal(msk) ||
> > - !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts, &add_addr))
> > - return false;
> > -
> > - if (((add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> > - ((add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
> > - (opts->local.family == AF_INET6 || opts->local.port))) &&
> > + if ((mptcp_pm_should_add_signal_echo(msk) ||
> > + (mptcp_pm_should_add_signal_addr(msk) &&
> > + (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> > skb && skb_is_tcp_pure_ack(skb)) {
> > pr_debug("drop other suboptions");
> > opts->suboptions = 0;
> > @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> > drop_other_suboptions = true;
> > }
> >
> > - len = mptcp_add_addr_len(opts, add_addr);
> > + if (!mptcp_pm_should_add_signal(msk) ||
> > + !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
> > + return false;
> > +
> > + family = echo ? opts->remote.family : opts->local.family;
> > + len = mptcp_add_addr_len(family, echo, port);
> > if (remaining < len)
> > return false;
> >
> > @@ -690,15 +693,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 (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
> > - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> > + if (!echo) {
> > 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",
> > - add_addr, (opts->ahmac == 0), opts->local.id, opts->ahmac,
> > - ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
> > + pr_debug("local_id=%d, local_port=%d, remote_id=%d, remote_port=%d, ahmac=%llu, echo=%d",
> > + opts->local.id, ntohs(opts->local.port), opts->remote.id,
> > + ntohs(opts->remote.port), opts->ahmac, echo);
> >
> > return true;
> > }
> > @@ -1253,13 +1255,10 @@ 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;
> > + 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;
> > -
> > #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> > if (addr->family == AF_INET6)
> > len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index 264f522af530..399b59cb7563 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >
> > /* path manager helpers */
> >
> > -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> > - unsigned int opt_size, unsigned int remaining,
> > - struct mptcp_out_options *opts, u8 *add_addr)
> > +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> > + bool *echo, bool *port)
> > {
> > int ret = false;
> > u8 add_addr;
> > + u8 family;
> >
> > spin_lock_bh(&msk->pm.lock);
> >
> > @@ -266,21 +267,15 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> > if (!mptcp_pm_should_add_signal(msk))
> > goto out_unlock;
> >
> > - opts->local = msk->pm.local;
> > - opts->remote = msk->pm.remote;
> > - *add_addr = msk->pm.addr_signal;
> > + *echo = mptcp_pm_should_add_signal_echo(msk);
> > + *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
> >
> > - if (((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> > - ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
> > - (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> > - skb && skb_is_tcp_pure_ack(skb)) {
> > - remaining += opt_size;
> > - }
> > -
> > - if (remaining < mptcp_add_addr_len(opts, *add_addr))
> > + family = *echo ? msk->pm.remote.family : msk->pm.local.family;
> > + if (remaining < mptcp_add_addr_len(family, *echo, *port))
> > goto out_unlock;
> >
> > *saddr = msk->pm.local;
> > + *daddr = msk->pm.remote;
> > add_addr = READ_ONCE(msk->pm.addr_signal);
> > if (mptcp_pm_should_add_signal_echo(msk))
> > add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index 937e0309e340..4b63cc6079fa 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -766,25 +766,16 @@ 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(struct mptcp_out_options *opts,
> > - u8 add_addr)
> > +static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> > {
> > - struct mptcp_addr_info *addr = &opts->remote;
> > - u8 len = 0;
> > + u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >
> > - if (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
> > - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> > - addr = &opts->local;
> > + if (family == AF_INET6)
> > + len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> > + if (!echo)
> > 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 (addr->port)
> > + if (port)
> > len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >
> > return len;
> > @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
> > - unsigned int opt_size, unsigned int remaining,
> > - struct mptcp_out_options *opts, u8 *add_addr);
> > +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> > + bool *echo, bool *port);
> > 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);
> >
>
> --
> Li YongLong
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"
2021-07-12 7:33 ` Geliang Tang
@ 2021-07-12 8:06 ` Yonglong Li
2021-07-12 8:44 ` Geliang Tang
0 siblings, 1 reply; 17+ messages in thread
From: Yonglong Li @ 2021-07-12 8:06 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp
On 2021/7/12 15:33, Geliang Tang wrote:
> Hi Yonglong,
>
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
>>
>>
>>
>> On 2021/7/11 23:15, Geliang Tang wrote:
>>> I think there're still some issues in v8:
>>>
>>> The remaining value is incorrect since "remaining += opt_size;" in the
>>> "drop other suboptions" checks has been called twice in
>>> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
>>>
>> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
>> mptcp_established_options_add_addr.
>>
>>> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
>>> populate after the length chech, not before the check.]
>>>
>>> The squash-to patch keeped the more orignal code unchanged, and just do
>>> the least, necessary modifications.
>>>
>> Agree opts->local and opts->remote should be asigned after the length check.
>> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
>> as orignal code, there is a race that:
>>
>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>> ==> call mptcp_pm_add_addr_signal
>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
>> ==> at this time opts->remote is empty and the length is incorrect.
>>
>
> What will happen in v8 when this race occurs? How dose v8 deal with the
> race?
Hi Geliang, thinks for your patience.
I think v8 doesn't have this issue:
==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
==> call mptcp_pm_add_addr_signal, save pm.addr_signal to add_addr and save addr in opts under pm.lock
==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO), but add_addr doesn't changed.
==> use add_addr and opts to check length.
==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
>
>> So I think the orignal code is incorrect. WDYT?
>>
>>> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
>>>
>>> Change arguments of mptcp_pm_add_addr_signal.
>>>
>>> Keep mptcp_add_addr_len unchanged.
>>>
>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>> ---
>>> net/mptcp/options.c | 35 +++++++++++++++++------------------
>>> net/mptcp/pm.c | 23 +++++++++--------------
>>> net/mptcp/protocol.h | 27 +++++++++------------------
>>> 3 files changed, 35 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>> index 5c0ad9b90866..93ad7b134f74 100644
>>> --- a/net/mptcp/options.c
>>> +++ b/net/mptcp/options.c
>>> @@ -663,16 +663,14 @@ 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;
>>> - u8 add_addr;
>>> + bool echo;
>>> + bool port;
>>> + u8 family;
>>> int len;
>>>
>>> - if (!mptcp_pm_should_add_signal(msk) ||
>>> - !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts, &add_addr))
>>> - return false;
>>> -
>>> - if (((add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) ||
>>> - ((add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
>>> - (opts->local.family == AF_INET6 || opts->local.port))) &&
>>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
>>> + (mptcp_pm_should_add_signal_addr(msk) &&
>>> + (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
>>> skb && skb_is_tcp_pure_ack(skb)) {
>>> pr_debug("drop other suboptions");
>>> opts->suboptions = 0;
>>> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>> drop_other_suboptions = true;
>>> }
>>>
>>> - len = mptcp_add_addr_len(opts, add_addr);
>>> + if (!mptcp_pm_should_add_signal(msk) ||
>>> + !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
>>> + return false;
>>> +
>>> + family = echo ? opts->remote.family : opts->local.family;
>>> + len = mptcp_add_addr_len(family, echo, port);
>>> if (remaining < len)
>>> return false;
>>>
>>> @@ -690,15 +693,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 (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
>>> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
>>> + if (!echo) {
>>> 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",
>>> - add_addr, (opts->ahmac == 0), opts->local.id, opts->ahmac,
>>> - ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>>> + pr_debug("local_id=%d, local_port=%d, remote_id=%d, remote_port=%d, ahmac=%llu, echo=%d",
>>> + opts->local.id, ntohs(opts->local.port), opts->remote.id,
>>> + ntohs(opts->remote.port), opts->ahmac, echo);
>>>
>>> return true;
>>> }
>>> @@ -1253,13 +1255,10 @@ 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;
>>> + 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;
>>> -
>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>> if (addr->family == AF_INET6)
>>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>> index 264f522af530..399b59cb7563 100644
>>> --- a/net/mptcp/pm.c
>>> +++ b/net/mptcp/pm.c
>>> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>>
>>> /* path manager helpers */
>>>
>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
>>> - unsigned int opt_size, unsigned int remaining,
>>> - struct mptcp_out_options *opts, u8 *add_addr)
>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
>>> + bool *echo, bool *port)
>>> {
>>> int ret = false;
>>> u8 add_addr;
>>> + u8 family;
>>>
>>> spin_lock_bh(&msk->pm.lock);
>>>
>>> @@ -266,21 +267,15 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
>>> if (!mptcp_pm_should_add_signal(msk))
>>> goto out_unlock;
>>>
>>> - opts->local = msk->pm.local;
>>> - opts->remote = msk->pm.remote;
>>> - *add_addr = msk->pm.addr_signal;
>>> + *echo = mptcp_pm_should_add_signal_echo(msk);
>>> + *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
>>>
>>> - if (((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)) ||
>>> - ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
>>> - (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
>>> - skb && skb_is_tcp_pure_ack(skb)) {
>>> - remaining += opt_size;
>>> - }
>>> -
>>> - if (remaining < mptcp_add_addr_len(opts, *add_addr))
>>> + family = *echo ? msk->pm.remote.family : msk->pm.local.family;
>>> + if (remaining < mptcp_add_addr_len(family, *echo, *port))
>>> goto out_unlock;
>>>
>>> *saddr = msk->pm.local;
>>> + *daddr = msk->pm.remote;
>>> add_addr = READ_ONCE(msk->pm.addr_signal);
>>> if (mptcp_pm_should_add_signal_echo(msk))
>>> add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>> index 937e0309e340..4b63cc6079fa 100644
>>> --- a/net/mptcp/protocol.h
>>> +++ b/net/mptcp/protocol.h
>>> @@ -766,25 +766,16 @@ 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(struct mptcp_out_options *opts,
>>> - u8 add_addr)
>>> +static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
>>> {
>>> - struct mptcp_addr_info *addr = &opts->remote;
>>> - u8 len = 0;
>>> + u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>
>>> - if (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
>>> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
>>> - addr = &opts->local;
>>> + if (family == AF_INET6)
>>> + len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>> + if (!echo)
>>> 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 (addr->port)
>>> + if (port)
>>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>>
>>> return len;
>>> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
>>> - unsigned int opt_size, unsigned int remaining,
>>> - struct mptcp_out_options *opts, u8 *add_addr);
>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
>>> + bool *echo, bool *port);
>>> 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);
>>>
>>
>> --
>> Li YongLong
>
--
Li YongLong
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"
2021-07-12 8:06 ` Yonglong Li
@ 2021-07-12 8:44 ` Geliang Tang
2021-07-12 9:07 ` Geliang Tang
2021-07-12 9:14 ` Yonglong Li
0 siblings, 2 replies; 17+ messages in thread
From: Geliang Tang @ 2021-07-12 8:44 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午4:07写道：
>
>
>
> On 2021/7/12 15:33, Geliang Tang wrote:
> > Hi Yonglong,
> >
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
> >>
> >>
> >>
> >> On 2021/7/11 23:15, Geliang Tang wrote:
> >>> I think there're still some issues in v8:
> >>>
> >>> The remaining value is incorrect since "remaining += opt_size;" in the
> >>> "drop other suboptions" checks has been called twice in
> >>> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
> >>>
> >> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
> >> mptcp_established_options_add_addr.
> >>
> >>> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
> >>> populate after the length chech, not before the check.]
> >>>
> >>> The squash-to patch keeped the more orignal code unchanged, and just do
> >>> the least, necessary modifications.
> >>>
> >> Agree opts->local and opts->remote should be asigned after the length check.
> >> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
> >> as orignal code, there is a race that:
> >>
> >> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> >> ==> call mptcp_pm_add_addr_signal
> >> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
> >> ==> at this time opts->remote is empty and the length is incorrect.
> >>
> >
> > What will happen in v8 when this race occurs? How dose v8 deal with the
> > race?
> Hi Geliang, thinks for your patience.
>
> I think v8 doesn't have this issue:
> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> ==> call mptcp_pm_add_addr_signal, save pm.addr_signal to add_addr and save addr in opts under pm.lock
> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO), but add_addr doesn't changed.
> ==> use add_addr and opts to check length.
> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
Thanks for your explanation.
I think this squash-to patch did the same thing:
==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
==> call mptcp_pm_add_addr_signal, save echo bit of pm.addr_signal to
'echo' (echo = false), save the port number to 'port', and save addr
in opts under pm.lock
==> an echo add addr event trigger (pm.addr_signal ==
MPTCP_ADD_ADDR_ECHO), but 'echo' and 'port' don't changed.
==> use 'echo' to get the address family, use 'family', 'echo' and
'port' to check length.
==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
Do you think so?
>
> >
> >> So I think the orignal code is incorrect. WDYT?
> >>
> >>> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
> >>>
> >>> Change arguments of mptcp_pm_add_addr_signal.
> >>>
> >>> Keep mptcp_add_addr_len unchanged.
> >>>
> >>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> >>> ---
> >>> net/mptcp/options.c | 35 +++++++++++++++++------------------
> >>> net/mptcp/pm.c | 23 +++++++++--------------
> >>> net/mptcp/protocol.h | 27 +++++++++------------------
> >>> 3 files changed, 35 insertions(+), 50 deletions(-)
> >>>
> >>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>> index 5c0ad9b90866..93ad7b134f74 100644
> >>> --- a/net/mptcp/options.c
> >>> +++ b/net/mptcp/options.c
> >>> @@ -663,16 +663,14 @@ 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;
> >>> - u8 add_addr;
> >>> + bool echo;
> >>> + bool port;
> >>> + u8 family;
> >>> int len;
> >>>
> >>> - if (!mptcp_pm_should_add_signal(msk) ||
> >>> - !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts, &add_addr))
> >>> - return false;
> >>> -
> >>> - if (((add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> >>> - ((add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
> >>> - (opts->local.family == AF_INET6 || opts->local.port))) &&
> >>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> >>> + (mptcp_pm_should_add_signal_addr(msk) &&
> >>> + (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> >>> skb && skb_is_tcp_pure_ack(skb)) {
> >>> pr_debug("drop other suboptions");
> >>> opts->suboptions = 0;
> >>> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>> drop_other_suboptions = true;
> >>> }
> >>>
> >>> - len = mptcp_add_addr_len(opts, add_addr);
> >>> + if (!mptcp_pm_should_add_signal(msk) ||
> >>> + !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
> >>> + return false;
> >>> +
> >>> + family = echo ? opts->remote.family : opts->local.family;
> >>> + len = mptcp_add_addr_len(family, echo, port);
> >>> if (remaining < len)
> >>> return false;
> >>>
> >>> @@ -690,15 +693,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 (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
> >>> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> >>> + if (!echo) {
> >>> 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",
> >>> - add_addr, (opts->ahmac == 0), opts->local.id, opts->ahmac,
> >>> - ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
> >>> + pr_debug("local_id=%d, local_port=%d, remote_id=%d, remote_port=%d, ahmac=%llu, echo=%d",
> >>> + opts->local.id, ntohs(opts->local.port), opts->remote.id,
> >>> + ntohs(opts->remote.port), opts->ahmac, echo);
> >>>
> >>> return true;
> >>> }
> >>> @@ -1253,13 +1255,10 @@ 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;
> >>> + 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;
> >>> -
> >>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >>> if (addr->family == AF_INET6)
> >>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> >>> index 264f522af530..399b59cb7563 100644
> >>> --- a/net/mptcp/pm.c
> >>> +++ b/net/mptcp/pm.c
> >>> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >>>
> >>> /* path manager helpers */
> >>>
> >>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> >>> - unsigned int opt_size, unsigned int remaining,
> >>> - struct mptcp_out_options *opts, u8 *add_addr)
> >>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> >>> + bool *echo, bool *port)
> >>> {
> >>> int ret = false;
> >>> u8 add_addr;
> >>> + u8 family;
> >>>
> >>> spin_lock_bh(&msk->pm.lock);
> >>>
> >>> @@ -266,21 +267,15 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> >>> if (!mptcp_pm_should_add_signal(msk))
> >>> goto out_unlock;
> >>>
> >>> - opts->local = msk->pm.local;
> >>> - opts->remote = msk->pm.remote;
> >>> - *add_addr = msk->pm.addr_signal;
> >>> + *echo = mptcp_pm_should_add_signal_echo(msk);
> >>> + *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
> >>>
> >>> - if (((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> >>> - ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
> >>> - (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> >>> - skb && skb_is_tcp_pure_ack(skb)) {
> >>> - remaining += opt_size;
> >>> - }
> >>> -
> >>> - if (remaining < mptcp_add_addr_len(opts, *add_addr))
> >>> + family = *echo ? msk->pm.remote.family : msk->pm.local.family;
> >>> + if (remaining < mptcp_add_addr_len(family, *echo, *port))
> >>> goto out_unlock;
> >>>
> >>> *saddr = msk->pm.local;
> >>> + *daddr = msk->pm.remote;
> >>> add_addr = READ_ONCE(msk->pm.addr_signal);
> >>> if (mptcp_pm_should_add_signal_echo(msk))
> >>> add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
> >>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> >>> index 937e0309e340..4b63cc6079fa 100644
> >>> --- a/net/mptcp/protocol.h
> >>> +++ b/net/mptcp/protocol.h
> >>> @@ -766,25 +766,16 @@ 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(struct mptcp_out_options *opts,
> >>> - u8 add_addr)
> >>> +static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> >>> {
> >>> - struct mptcp_addr_info *addr = &opts->remote;
> >>> - u8 len = 0;
> >>> + u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>
> >>> - if (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
> >>> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> >>> - addr = &opts->local;
> >>> + if (family == AF_INET6)
> >>> + len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>> + if (!echo)
> >>> 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 (addr->port)
> >>> + if (port)
> >>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >>>
> >>> return len;
> >>> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
> >>> - unsigned int opt_size, unsigned int remaining,
> >>> - struct mptcp_out_options *opts, u8 *add_addr);
> >>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> >>> + bool *echo, bool *port);
> >>> 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);
> >>>
> >>
> >> --
> >> Li YongLong
> >
>
> --
> Li YongLong
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"
2021-07-12 8:44 ` Geliang Tang
@ 2021-07-12 9:07 ` Geliang Tang
2021-07-12 9:21 ` Yonglong Li
2021-07-12 9:14 ` Yonglong Li
1 sibling, 1 reply; 17+ messages in thread
From: Geliang Tang @ 2021-07-12 9:07 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp
Geliang Tang <geliangtang@gmail.com> 于2021年7月12日周一 下午4:44写道：
>
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午4:07写道：
> >
> >
> >
> > On 2021/7/12 15:33, Geliang Tang wrote:
> > > Hi Yonglong,
> > >
> > > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
> > >>
> > >>
> > >>
> > >> On 2021/7/11 23:15, Geliang Tang wrote:
> > >>> I think there're still some issues in v8:
> > >>>
> > >>> The remaining value is incorrect since "remaining += opt_size;" in the
> > >>> "drop other suboptions" checks has been called twice in
> > >>> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
> > >>>
> > >> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
> > >> mptcp_established_options_add_addr.
> > >>
> > >>> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
> > >>> populate after the length chech, not before the check.]
> > >>>
> > >>> The squash-to patch keeped the more orignal code unchanged, and just do
> > >>> the least, necessary modifications.
> > >>>
> > >> Agree opts->local and opts->remote should be asigned after the length check.
> > >> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
> > >> as orignal code, there is a race that:
> > >>
> > >> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> > >> ==> call mptcp_pm_add_addr_signal
> > >> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
> > >> ==> at this time opts->remote is empty and the length is incorrect.
> > >>
> > >
> > > What will happen in v8 when this race occurs? How dose v8 deal with the
> > > race?
> > Hi Geliang, thinks for your patience.
> >
> > I think v8 doesn't have this issue:
> > ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> > ==> call mptcp_pm_add_addr_signal, save pm.addr_signal to add_addr and save addr in opts under pm.lock
> > ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO), but add_addr doesn't changed.
> > ==> use add_addr and opts to check length.
> > ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
>
> Thanks for your explanation.
>
> I think this squash-to patch did the same thing:
>
> ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> ==> call mptcp_pm_add_addr_signal, save echo bit of pm.addr_signal to
> 'echo' (echo = false), save the port number to 'port', and save addr
> in opts under pm.lock
> ==> an echo add addr event trigger (pm.addr_signal ==
> MPTCP_ADD_ADDR_ECHO), but 'echo' and 'port' don't changed.
> ==> use 'echo' to get the address family, use 'family', 'echo' and
> 'port' to check length.
> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
>
> Do you think so?
And one more thing. How do you test this "mptcp: fix conflicts when using
pm.add_signal in ADD_ADDR/echo and RM_ADDR process" series? Do you use our
mptcp_join.sh to test it, or you did some special tests? Does this race
scenario mentioned above easy to reproduce?
I just did the mptcp_join.sh tests for my squash-to patches, and everything
looks fine. If you have some special tests, could you please help me to test
these squash-to patches too? Hope it works in the race scenario.
Thanks.
-Geliang
>
> >
> > >
> > >> So I think the orignal code is incorrect. WDYT?
> > >>
> > >>> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
> > >>>
> > >>> Change arguments of mptcp_pm_add_addr_signal.
> > >>>
> > >>> Keep mptcp_add_addr_len unchanged.
> > >>>
> > >>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > >>> ---
> > >>> net/mptcp/options.c | 35 +++++++++++++++++------------------
> > >>> net/mptcp/pm.c | 23 +++++++++--------------
> > >>> net/mptcp/protocol.h | 27 +++++++++------------------
> > >>> 3 files changed, 35 insertions(+), 50 deletions(-)
> > >>>
> > >>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > >>> index 5c0ad9b90866..93ad7b134f74 100644
> > >>> --- a/net/mptcp/options.c
> > >>> +++ b/net/mptcp/options.c
> > >>> @@ -663,16 +663,14 @@ 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;
> > >>> - u8 add_addr;
> > >>> + bool echo;
> > >>> + bool port;
> > >>> + u8 family;
> > >>> int len;
> > >>>
> > >>> - if (!mptcp_pm_should_add_signal(msk) ||
> > >>> - !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts, &add_addr))
> > >>> - return false;
> > >>> -
> > >>> - if (((add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> > >>> - ((add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
> > >>> - (opts->local.family == AF_INET6 || opts->local.port))) &&
> > >>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> > >>> + (mptcp_pm_should_add_signal_addr(msk) &&
> > >>> + (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> > >>> skb && skb_is_tcp_pure_ack(skb)) {
> > >>> pr_debug("drop other suboptions");
> > >>> opts->suboptions = 0;
> > >>> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> > >>> drop_other_suboptions = true;
> > >>> }
> > >>>
> > >>> - len = mptcp_add_addr_len(opts, add_addr);
> > >>> + if (!mptcp_pm_should_add_signal(msk) ||
> > >>> + !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
> > >>> + return false;
> > >>> +
> > >>> + family = echo ? opts->remote.family : opts->local.family;
> > >>> + len = mptcp_add_addr_len(family, echo, port);
> > >>> if (remaining < len)
> > >>> return false;
> > >>>
> > >>> @@ -690,15 +693,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 (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
> > >>> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> > >>> + if (!echo) {
> > >>> 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",
> > >>> - add_addr, (opts->ahmac == 0), opts->local.id, opts->ahmac,
> > >>> - ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
> > >>> + pr_debug("local_id=%d, local_port=%d, remote_id=%d, remote_port=%d, ahmac=%llu, echo=%d",
> > >>> + opts->local.id, ntohs(opts->local.port), opts->remote.id,
> > >>> + ntohs(opts->remote.port), opts->ahmac, echo);
> > >>>
> > >>> return true;
> > >>> }
> > >>> @@ -1253,13 +1255,10 @@ 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;
> > >>> + 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;
> > >>> -
> > >>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> > >>> if (addr->family == AF_INET6)
> > >>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> > >>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > >>> index 264f522af530..399b59cb7563 100644
> > >>> --- a/net/mptcp/pm.c
> > >>> +++ b/net/mptcp/pm.c
> > >>> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> > >>>
> > >>> /* path manager helpers */
> > >>>
> > >>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> > >>> - unsigned int opt_size, unsigned int remaining,
> > >>> - struct mptcp_out_options *opts, u8 *add_addr)
> > >>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > >>> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> > >>> + bool *echo, bool *port)
> > >>> {
> > >>> int ret = false;
> > >>> u8 add_addr;
> > >>> + u8 family;
> > >>>
> > >>> spin_lock_bh(&msk->pm.lock);
> > >>>
> > >>> @@ -266,21 +267,15 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> > >>> if (!mptcp_pm_should_add_signal(msk))
> > >>> goto out_unlock;
> > >>>
> > >>> - opts->local = msk->pm.local;
> > >>> - opts->remote = msk->pm.remote;
> > >>> - *add_addr = msk->pm.addr_signal;
> > >>> + *echo = mptcp_pm_should_add_signal_echo(msk);
> > >>> + *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
> > >>>
> > >>> - if (((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> > >>> - ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
> > >>> - (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> > >>> - skb && skb_is_tcp_pure_ack(skb)) {
> > >>> - remaining += opt_size;
> > >>> - }
> > >>> -
> > >>> - if (remaining < mptcp_add_addr_len(opts, *add_addr))
> > >>> + family = *echo ? msk->pm.remote.family : msk->pm.local.family;
> > >>> + if (remaining < mptcp_add_addr_len(family, *echo, *port))
> > >>> goto out_unlock;
> > >>>
> > >>> *saddr = msk->pm.local;
> > >>> + *daddr = msk->pm.remote;
> > >>> add_addr = READ_ONCE(msk->pm.addr_signal);
> > >>> if (mptcp_pm_should_add_signal_echo(msk))
> > >>> add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
> > >>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > >>> index 937e0309e340..4b63cc6079fa 100644
> > >>> --- a/net/mptcp/protocol.h
> > >>> +++ b/net/mptcp/protocol.h
> > >>> @@ -766,25 +766,16 @@ 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(struct mptcp_out_options *opts,
> > >>> - u8 add_addr)
> > >>> +static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> > >>> {
> > >>> - struct mptcp_addr_info *addr = &opts->remote;
> > >>> - u8 len = 0;
> > >>> + u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> > >>>
> > >>> - if (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
> > >>> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> > >>> - addr = &opts->local;
> > >>> + if (family == AF_INET6)
> > >>> + len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> > >>> + if (!echo)
> > >>> 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 (addr->port)
> > >>> + if (port)
> > >>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> > >>>
> > >>> return len;
> > >>> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
> > >>> - unsigned int opt_size, unsigned int remaining,
> > >>> - struct mptcp_out_options *opts, u8 *add_addr);
> > >>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > >>> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> > >>> + bool *echo, bool *port);
> > >>> 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);
> > >>>
> > >>
> > >> --
> > >> Li YongLong
> > >
> >
> > --
> > Li YongLong
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"
2021-07-12 8:44 ` Geliang Tang
2021-07-12 9:07 ` Geliang Tang
@ 2021-07-12 9:14 ` Yonglong Li
2021-07-12 9:29 ` Geliang Tang
1 sibling, 1 reply; 17+ messages in thread
From: Yonglong Li @ 2021-07-12 9:14 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp
On 2021/7/12 16:44, Geliang Tang wrote:
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午4:07写道：
>>
>>
>>
>> On 2021/7/12 15:33, Geliang Tang wrote:
>>> Hi Yonglong,
>>>
>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
>>>>
>>>>
>>>>
>>>> On 2021/7/11 23:15, Geliang Tang wrote:
>>>>> I think there're still some issues in v8:
>>>>>
>>>>> The remaining value is incorrect since "remaining += opt_size;" in the
>>>>> "drop other suboptions" checks has been called twice in
>>>>> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
>>>>>
>>>> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
>>>> mptcp_established_options_add_addr.
>>>>
>>>>> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
>>>>> populate after the length chech, not before the check.]
>>>>>
>>>>> The squash-to patch keeped the more orignal code unchanged, and just do
>>>>> the least, necessary modifications.
>>>>>
>>>> Agree opts->local and opts->remote should be asigned after the length check.
>>>> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
>>>> as orignal code, there is a race that:
>>>>
>>>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>>>> ==> call mptcp_pm_add_addr_signal
>>>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
>>>> ==> at this time opts->remote is empty and the length is incorrect.
>>>>
>>>
>>> What will happen in v8 when this race occurs? How dose v8 deal with the
>>> race?
>> Hi Geliang, thinks for your patience.
>>
>> I think v8 doesn't have this issue:
>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>> ==> call mptcp_pm_add_addr_signal, save pm.addr_signal to add_addr and save addr in opts under pm.lock
>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO), but add_addr doesn't changed.
>> ==> use add_addr and opts to check length.
>> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
>
> Thanks for your explanation.
>
> I think this squash-to patch did the same thing:
>
> ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> ==> call mptcp_pm_add_addr_signal, save echo bit of pm.addr_signal to
> 'echo' (echo = false), save the port number to 'port', and save addr
> in opts under pm.lock
> ==> an echo add addr event trigger (pm.addr_signal ==
> MPTCP_ADD_ADDR_ECHO), but 'echo' and 'port' don't changed.
> ==> use 'echo' to get the address family, use 'family', 'echo' and
> 'port' to check length.
> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
>
> Do you think so?
yep. In this case the squash-to patch is ok. But I think between "drop other suboptions" checks and
mptcp_pm_add_addr_signal the race still exist.
==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
==> "drop other suboptions" checks will use MPTCP_ADD_ADDR_SIGNAL to check
==> an echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL | MPTCP_ADD_ADDR_ECHO )
==> call mptcp_pm_add_addr_signal, MPTCP_ADD_ADDR_ECHO will be clear in pm.addr_signal
==> process MPTCP_ADD_ADDR_ECHO event.
WDYT?
>
>>
>>>
>>>> So I think the orignal code is incorrect. WDYT?
>>>>
>>>>> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
>>>>>
>>>>> Change arguments of mptcp_pm_add_addr_signal.
>>>>>
>>>>> Keep mptcp_add_addr_len unchanged.
>>>>>
>>>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>>>> ---
>>>>> net/mptcp/options.c | 35 +++++++++++++++++------------------
>>>>> net/mptcp/pm.c | 23 +++++++++--------------
>>>>> net/mptcp/protocol.h | 27 +++++++++------------------
>>>>> 3 files changed, 35 insertions(+), 50 deletions(-)
>>>>>
>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>>> index 5c0ad9b90866..93ad7b134f74 100644
>>>>> --- a/net/mptcp/options.c
>>>>> +++ b/net/mptcp/options.c
>>>>> @@ -663,16 +663,14 @@ 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;
>>>>> - u8 add_addr;
>>>>> + bool echo;
>>>>> + bool port;
>>>>> + u8 family;
>>>>> int len;
>>>>>
>>>>> - if (!mptcp_pm_should_add_signal(msk) ||
>>>>> - !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts, &add_addr))
>>>>> - return false;
>>>>> -
>>>>> - if (((add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) ||
>>>>> - ((add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
>>>>> - (opts->local.family == AF_INET6 || opts->local.port))) &&
>>>>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>>> + (mptcp_pm_should_add_signal_addr(msk) &&
>>>>> + (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
>>>>> skb && skb_is_tcp_pure_ack(skb)) {
>>>>> pr_debug("drop other suboptions");
>>>>> opts->suboptions = 0;
>>>>> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>> drop_other_suboptions = true;
>>>>> }
>>>>>
>>>>> - len = mptcp_add_addr_len(opts, add_addr);
>>>>> + if (!mptcp_pm_should_add_signal(msk) ||
>>>>> + !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
>>>>> + return false;
>>>>> +
>>>>> + family = echo ? opts->remote.family : opts->local.family;
>>>>> + len = mptcp_add_addr_len(family, echo, port);
>>>>> if (remaining < len)
>>>>> return false;
>>>>>
>>>>> @@ -690,15 +693,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 (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
>>>>> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
>>>>> + if (!echo) {
>>>>> 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",
>>>>> - add_addr, (opts->ahmac == 0), opts->local.id, opts->ahmac,
>>>>> - ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>>>>> + pr_debug("local_id=%d, local_port=%d, remote_id=%d, remote_port=%d, ahmac=%llu, echo=%d",
>>>>> + opts->local.id, ntohs(opts->local.port), opts->remote.id,
>>>>> + ntohs(opts->remote.port), opts->ahmac, echo);
>>>>>
>>>>> return true;
>>>>> }
>>>>> @@ -1253,13 +1255,10 @@ 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;
>>>>> + 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;
>>>>> -
>>>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>>>> if (addr->family == AF_INET6)
>>>>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>>>> index 264f522af530..399b59cb7563 100644
>>>>> --- a/net/mptcp/pm.c
>>>>> +++ b/net/mptcp/pm.c
>>>>> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>>>>
>>>>> /* path manager helpers */
>>>>>
>>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
>>>>> - unsigned int opt_size, unsigned int remaining,
>>>>> - struct mptcp_out_options *opts, u8 *add_addr)
>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
>>>>> + bool *echo, bool *port)
>>>>> {
>>>>> int ret = false;
>>>>> u8 add_addr;
>>>>> + u8 family;
>>>>>
>>>>> spin_lock_bh(&msk->pm.lock);
>>>>>
>>>>> @@ -266,21 +267,15 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
>>>>> if (!mptcp_pm_should_add_signal(msk))
>>>>> goto out_unlock;
>>>>>
>>>>> - opts->local = msk->pm.local;
>>>>> - opts->remote = msk->pm.remote;
>>>>> - *add_addr = msk->pm.addr_signal;
>>>>> + *echo = mptcp_pm_should_add_signal_echo(msk);
>>>>> + *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
>>>>>
>>>>> - if (((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)) ||
>>>>> - ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
>>>>> - (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
>>>>> - skb && skb_is_tcp_pure_ack(skb)) {
>>>>> - remaining += opt_size;
>>>>> - }
>>>>> -
>>>>> - if (remaining < mptcp_add_addr_len(opts, *add_addr))
>>>>> + family = *echo ? msk->pm.remote.family : msk->pm.local.family;
>>>>> + if (remaining < mptcp_add_addr_len(family, *echo, *port))
>>>>> goto out_unlock;
>>>>>
>>>>> *saddr = msk->pm.local;
>>>>> + *daddr = msk->pm.remote;
>>>>> add_addr = READ_ONCE(msk->pm.addr_signal);
>>>>> if (mptcp_pm_should_add_signal_echo(msk))
>>>>> add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>>>> index 937e0309e340..4b63cc6079fa 100644
>>>>> --- a/net/mptcp/protocol.h
>>>>> +++ b/net/mptcp/protocol.h
>>>>> @@ -766,25 +766,16 @@ 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(struct mptcp_out_options *opts,
>>>>> - u8 add_addr)
>>>>> +static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
>>>>> {
>>>>> - struct mptcp_addr_info *addr = &opts->remote;
>>>>> - u8 len = 0;
>>>>> + u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>
>>>>> - if (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
>>>>> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
>>>>> - addr = &opts->local;
>>>>> + if (family == AF_INET6)
>>>>> + len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>> + if (!echo)
>>>>> 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 (addr->port)
>>>>> + if (port)
>>>>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>>>>
>>>>> return len;
>>>>> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
>>>>> - unsigned int opt_size, unsigned int remaining,
>>>>> - struct mptcp_out_options *opts, u8 *add_addr);
>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
>>>>> + bool *echo, bool *port);
>>>>> 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);
>>>>>
>>>>
>>>> --
>>>> Li YongLong
>>>
>>
>> --
>> Li YongLong
>
>
--
Li YongLong
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"
2021-07-12 9:07 ` Geliang Tang
@ 2021-07-12 9:21 ` Yonglong Li
0 siblings, 0 replies; 17+ messages in thread
From: Yonglong Li @ 2021-07-12 9:21 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp
On 2021/7/12 17:07, Geliang Tang wrote:
> Geliang Tang <geliangtang@gmail.com> 于2021年7月12日周一 下午4:44写道：
>>
>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午4:07写道：
>>>
>>>
>>>
>>> On 2021/7/12 15:33, Geliang Tang wrote:
>>>> Hi Yonglong,
>>>>
>>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
>>>>>
>>>>>
>>>>>
>>>>> On 2021/7/11 23:15, Geliang Tang wrote:
>>>>>> I think there're still some issues in v8:
>>>>>>
>>>>>> The remaining value is incorrect since "remaining += opt_size;" in the
>>>>>> "drop other suboptions" checks has been called twice in
>>>>>> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
>>>>>>
>>>>> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
>>>>> mptcp_established_options_add_addr.
>>>>>
>>>>>> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
>>>>>> populate after the length chech, not before the check.]
>>>>>>
>>>>>> The squash-to patch keeped the more orignal code unchanged, and just do
>>>>>> the least, necessary modifications.
>>>>>>
>>>>> Agree opts->local and opts->remote should be asigned after the length check.
>>>>> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
>>>>> as orignal code, there is a race that:
>>>>>
>>>>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>>>>> ==> call mptcp_pm_add_addr_signal
>>>>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
>>>>> ==> at this time opts->remote is empty and the length is incorrect.
>>>>>
>>>>
>>>> What will happen in v8 when this race occurs? How dose v8 deal with the
>>>> race?
>>> Hi Geliang, thinks for your patience.
>>>
>>> I think v8 doesn't have this issue:
>>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>>> ==> call mptcp_pm_add_addr_signal, save pm.addr_signal to add_addr and save addr in opts under pm.lock
>>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO), but add_addr doesn't changed.
>>> ==> use add_addr and opts to check length.
>>> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
>>
>> Thanks for your explanation.
>>
>> I think this squash-to patch did the same thing:
>>
>> ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>> ==> call mptcp_pm_add_addr_signal, save echo bit of pm.addr_signal to
>> 'echo' (echo = false), save the port number to 'port', and save addr
>> in opts under pm.lock
>> ==> an echo add addr event trigger (pm.addr_signal ==
>> MPTCP_ADD_ADDR_ECHO), but 'echo' and 'port' don't changed.
>> ==> use 'echo' to get the address family, use 'family', 'echo' and
>> 'port' to check length.
>> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
>>
>> Do you think so?
>
> And one more thing. How do you test this "mptcp: fix conflicts when using
> pm.add_signal in ADD_ADDR/echo and RM_ADDR process" series? Do you use our
> mptcp_join.sh to test it, or you did some special tests? Does this race
> scenario mentioned above easy to reproduce?
>
> I just did the mptcp_join.sh tests for my squash-to patches, and everything
> looks fine. If you have some special tests, could you please help me to test
> these squash-to patches too? Hope it works in the race scenario.
>
> Thanks.
> -Geliang
>
OK. I will try to test the squash-to patches.
The race scenario is not easy to reproduce. :(
A loop script will try many times to reproduce.
>
>
>>
>>>
>>>>
>>>>> So I think the orignal code is incorrect. WDYT?
>>>>>
>>>>>> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
>>>>>>
>>>>>> Change arguments of mptcp_pm_add_addr_signal.
>>>>>>
>>>>>> Keep mptcp_add_addr_len unchanged.
>>>>>>
>>>>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>>>>> ---
>>>>>> net/mptcp/options.c | 35 +++++++++++++++++------------------
>>>>>> net/mptcp/pm.c | 23 +++++++++--------------
>>>>>> net/mptcp/protocol.h | 27 +++++++++------------------
>>>>>> 3 files changed, 35 insertions(+), 50 deletions(-)
>>>>>>
>>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>>>> index 5c0ad9b90866..93ad7b134f74 100644
>>>>>> --- a/net/mptcp/options.c
>>>>>> +++ b/net/mptcp/options.c
>>>>>> @@ -663,16 +663,14 @@ 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;
>>>>>> - u8 add_addr;
>>>>>> + bool echo;
>>>>>> + bool port;
>>>>>> + u8 family;
>>>>>> int len;
>>>>>>
>>>>>> - if (!mptcp_pm_should_add_signal(msk) ||
>>>>>> - !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts, &add_addr))
>>>>>> - return false;
>>>>>> -
>>>>>> - if (((add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) ||
>>>>>> - ((add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
>>>>>> - (opts->local.family == AF_INET6 || opts->local.port))) &&
>>>>>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>>>> + (mptcp_pm_should_add_signal_addr(msk) &&
>>>>>> + (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
>>>>>> skb && skb_is_tcp_pure_ack(skb)) {
>>>>>> pr_debug("drop other suboptions");
>>>>>> opts->suboptions = 0;
>>>>>> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>> drop_other_suboptions = true;
>>>>>> }
>>>>>>
>>>>>> - len = mptcp_add_addr_len(opts, add_addr);
>>>>>> + if (!mptcp_pm_should_add_signal(msk) ||
>>>>>> + !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
>>>>>> + return false;
>>>>>> +
>>>>>> + family = echo ? opts->remote.family : opts->local.family;
>>>>>> + len = mptcp_add_addr_len(family, echo, port);
>>>>>> if (remaining < len)
>>>>>> return false;
>>>>>>
>>>>>> @@ -690,15 +693,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 (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
>>>>>> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
>>>>>> + if (!echo) {
>>>>>> 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",
>>>>>> - add_addr, (opts->ahmac == 0), opts->local.id, opts->ahmac,
>>>>>> - ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>>>>>> + pr_debug("local_id=%d, local_port=%d, remote_id=%d, remote_port=%d, ahmac=%llu, echo=%d",
>>>>>> + opts->local.id, ntohs(opts->local.port), opts->remote.id,
>>>>>> + ntohs(opts->remote.port), opts->ahmac, echo);
>>>>>>
>>>>>> return true;
>>>>>> }
>>>>>> @@ -1253,13 +1255,10 @@ 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;
>>>>>> + 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;
>>>>>> -
>>>>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>>>>> if (addr->family == AF_INET6)
>>>>>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>>>>> index 264f522af530..399b59cb7563 100644
>>>>>> --- a/net/mptcp/pm.c
>>>>>> +++ b/net/mptcp/pm.c
>>>>>> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>>>>>
>>>>>> /* path manager helpers */
>>>>>>
>>>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
>>>>>> - unsigned int opt_size, unsigned int remaining,
>>>>>> - struct mptcp_out_options *opts, u8 *add_addr)
>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
>>>>>> + bool *echo, bool *port)
>>>>>> {
>>>>>> int ret = false;
>>>>>> u8 add_addr;
>>>>>> + u8 family;
>>>>>>
>>>>>> spin_lock_bh(&msk->pm.lock);
>>>>>>
>>>>>> @@ -266,21 +267,15 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
>>>>>> if (!mptcp_pm_should_add_signal(msk))
>>>>>> goto out_unlock;
>>>>>>
>>>>>> - opts->local = msk->pm.local;
>>>>>> - opts->remote = msk->pm.remote;
>>>>>> - *add_addr = msk->pm.addr_signal;
>>>>>> + *echo = mptcp_pm_should_add_signal_echo(msk);
>>>>>> + *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
>>>>>>
>>>>>> - if (((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)) ||
>>>>>> - ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
>>>>>> - (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
>>>>>> - skb && skb_is_tcp_pure_ack(skb)) {
>>>>>> - remaining += opt_size;
>>>>>> - }
>>>>>> -
>>>>>> - if (remaining < mptcp_add_addr_len(opts, *add_addr))
>>>>>> + family = *echo ? msk->pm.remote.family : msk->pm.local.family;
>>>>>> + if (remaining < mptcp_add_addr_len(family, *echo, *port))
>>>>>> goto out_unlock;
>>>>>>
>>>>>> *saddr = msk->pm.local;
>>>>>> + *daddr = msk->pm.remote;
>>>>>> add_addr = READ_ONCE(msk->pm.addr_signal);
>>>>>> if (mptcp_pm_should_add_signal_echo(msk))
>>>>>> add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>>>>> index 937e0309e340..4b63cc6079fa 100644
>>>>>> --- a/net/mptcp/protocol.h
>>>>>> +++ b/net/mptcp/protocol.h
>>>>>> @@ -766,25 +766,16 @@ 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(struct mptcp_out_options *opts,
>>>>>> - u8 add_addr)
>>>>>> +static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
>>>>>> {
>>>>>> - struct mptcp_addr_info *addr = &opts->remote;
>>>>>> - u8 len = 0;
>>>>>> + u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>>
>>>>>> - if (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
>>>>>> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
>>>>>> - addr = &opts->local;
>>>>>> + if (family == AF_INET6)
>>>>>> + len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>> + if (!echo)
>>>>>> 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 (addr->port)
>>>>>> + if (port)
>>>>>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>>>>>
>>>>>> return len;
>>>>>> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
>>>>>> - unsigned int opt_size, unsigned int remaining,
>>>>>> - struct mptcp_out_options *opts, u8 *add_addr);
>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
>>>>>> + bool *echo, bool *port);
>>>>>> 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);
>>>>>>
>>>>>
>>>>> --
>>>>> Li YongLong
>>>>
>>>
>>> --
>>> Li YongLong
>
--
Li YongLong
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"
2021-07-12 9:14 ` Yonglong Li
@ 2021-07-12 9:29 ` Geliang Tang
2021-07-12 9:44 ` Yonglong Li
0 siblings, 1 reply; 17+ messages in thread
From: Geliang Tang @ 2021-07-12 9:29 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午5:14写道：
>
>
>
> On 2021/7/12 16:44, Geliang Tang wrote:
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午4:07写道：
> >>
> >>
> >>
> >> On 2021/7/12 15:33, Geliang Tang wrote:
> >>> Hi Yonglong,
> >>>
> >>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
> >>>>
> >>>>
> >>>>
> >>>> On 2021/7/11 23:15, Geliang Tang wrote:
> >>>>> I think there're still some issues in v8:
> >>>>>
> >>>>> The remaining value is incorrect since "remaining += opt_size;" in the
> >>>>> "drop other suboptions" checks has been called twice in
> >>>>> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
> >>>>>
> >>>> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
> >>>> mptcp_established_options_add_addr.
> >>>>
> >>>>> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
> >>>>> populate after the length chech, not before the check.]
> >>>>>
> >>>>> The squash-to patch keeped the more orignal code unchanged, and just do
> >>>>> the least, necessary modifications.
> >>>>>
> >>>> Agree opts->local and opts->remote should be asigned after the length check.
> >>>> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
> >>>> as orignal code, there is a race that:
> >>>>
> >>>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> >>>> ==> call mptcp_pm_add_addr_signal
> >>>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
> >>>> ==> at this time opts->remote is empty and the length is incorrect.
> >>>>
> >>>
> >>> What will happen in v8 when this race occurs? How dose v8 deal with the
> >>> race?
> >> Hi Geliang, thinks for your patience.
> >>
> >> I think v8 doesn't have this issue:
> >> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> >> ==> call mptcp_pm_add_addr_signal, save pm.addr_signal to add_addr and save addr in opts under pm.lock
> >> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO), but add_addr doesn't changed.
> >> ==> use add_addr and opts to check length.
> >> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
> >
> > Thanks for your explanation.
> >
> > I think this squash-to patch did the same thing:
> >
> > ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> > ==> call mptcp_pm_add_addr_signal, save echo bit of pm.addr_signal to
> > 'echo' (echo = false), save the port number to 'port', and save addr
> > in opts under pm.lock
> > ==> an echo add addr event trigger (pm.addr_signal ==
> > MPTCP_ADD_ADDR_ECHO), but 'echo' and 'port' don't changed.
> > ==> use 'echo' to get the address family, use 'family', 'echo' and
> > 'port' to check length.
> > ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
> >
> > Do you think so?
> yep. In this case the squash-to patch is ok. But I think between "drop other suboptions" checks and
> mptcp_pm_add_addr_signal the race still exist.
>
I think this is easy to fix:
Add a new argument "drop_other_suboptions" for mptcp_pm_add_addr_signal,
move this "drop other suboptions" check code into mptcp_pm_add_addr_signal,
I'll sent a v2 later.
Thanks,
-Geliang
> ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> ==> "drop other suboptions" checks will use MPTCP_ADD_ADDR_SIGNAL to check
> ==> an echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL | MPTCP_ADD_ADDR_ECHO )
> ==> call mptcp_pm_add_addr_signal, MPTCP_ADD_ADDR_ECHO will be clear in pm.addr_signal
> ==> process MPTCP_ADD_ADDR_ECHO event.
>
> WDYT?
>
> >
> >>
> >>>
> >>>> So I think the orignal code is incorrect. WDYT?
> >>>>
> >>>>> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
> >>>>>
> >>>>> Change arguments of mptcp_pm_add_addr_signal.
> >>>>>
> >>>>> Keep mptcp_add_addr_len unchanged.
> >>>>>
> >>>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> >>>>> ---
> >>>>> net/mptcp/options.c | 35 +++++++++++++++++------------------
> >>>>> net/mptcp/pm.c | 23 +++++++++--------------
> >>>>> net/mptcp/protocol.h | 27 +++++++++------------------
> >>>>> 3 files changed, 35 insertions(+), 50 deletions(-)
> >>>>>
> >>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>>>> index 5c0ad9b90866..93ad7b134f74 100644
> >>>>> --- a/net/mptcp/options.c
> >>>>> +++ b/net/mptcp/options.c
> >>>>> @@ -663,16 +663,14 @@ 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;
> >>>>> - u8 add_addr;
> >>>>> + bool echo;
> >>>>> + bool port;
> >>>>> + u8 family;
> >>>>> int len;
> >>>>>
> >>>>> - if (!mptcp_pm_should_add_signal(msk) ||
> >>>>> - !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts, &add_addr))
> >>>>> - return false;
> >>>>> -
> >>>>> - if (((add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> >>>>> - ((add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
> >>>>> - (opts->local.family == AF_INET6 || opts->local.port))) &&
> >>>>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> >>>>> + (mptcp_pm_should_add_signal_addr(msk) &&
> >>>>> + (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> >>>>> skb && skb_is_tcp_pure_ack(skb)) {
> >>>>> pr_debug("drop other suboptions");
> >>>>> opts->suboptions = 0;
> >>>>> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>> drop_other_suboptions = true;
> >>>>> }
> >>>>>
> >>>>> - len = mptcp_add_addr_len(opts, add_addr);
> >>>>> + if (!mptcp_pm_should_add_signal(msk) ||
> >>>>> + !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
> >>>>> + return false;
> >>>>> +
> >>>>> + family = echo ? opts->remote.family : opts->local.family;
> >>>>> + len = mptcp_add_addr_len(family, echo, port);
> >>>>> if (remaining < len)
> >>>>> return false;
> >>>>>
> >>>>> @@ -690,15 +693,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 (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
> >>>>> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> >>>>> + if (!echo) {
> >>>>> 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",
> >>>>> - add_addr, (opts->ahmac == 0), opts->local.id, opts->ahmac,
> >>>>> - ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
> >>>>> + pr_debug("local_id=%d, local_port=%d, remote_id=%d, remote_port=%d, ahmac=%llu, echo=%d",
> >>>>> + opts->local.id, ntohs(opts->local.port), opts->remote.id,
> >>>>> + ntohs(opts->remote.port), opts->ahmac, echo);
> >>>>>
> >>>>> return true;
> >>>>> }
> >>>>> @@ -1253,13 +1255,10 @@ 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;
> >>>>> + 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;
> >>>>> -
> >>>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >>>>> if (addr->family == AF_INET6)
> >>>>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> >>>>> index 264f522af530..399b59cb7563 100644
> >>>>> --- a/net/mptcp/pm.c
> >>>>> +++ b/net/mptcp/pm.c
> >>>>> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >>>>>
> >>>>> /* path manager helpers */
> >>>>>
> >>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> >>>>> - unsigned int opt_size, unsigned int remaining,
> >>>>> - struct mptcp_out_options *opts, u8 *add_addr)
> >>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> >>>>> + bool *echo, bool *port)
> >>>>> {
> >>>>> int ret = false;
> >>>>> u8 add_addr;
> >>>>> + u8 family;
> >>>>>
> >>>>> spin_lock_bh(&msk->pm.lock);
> >>>>>
> >>>>> @@ -266,21 +267,15 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> >>>>> if (!mptcp_pm_should_add_signal(msk))
> >>>>> goto out_unlock;
> >>>>>
> >>>>> - opts->local = msk->pm.local;
> >>>>> - opts->remote = msk->pm.remote;
> >>>>> - *add_addr = msk->pm.addr_signal;
> >>>>> + *echo = mptcp_pm_should_add_signal_echo(msk);
> >>>>> + *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
> >>>>>
> >>>>> - if (((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> >>>>> - ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
> >>>>> - (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> >>>>> - skb && skb_is_tcp_pure_ack(skb)) {
> >>>>> - remaining += opt_size;
> >>>>> - }
> >>>>> -
> >>>>> - if (remaining < mptcp_add_addr_len(opts, *add_addr))
> >>>>> + family = *echo ? msk->pm.remote.family : msk->pm.local.family;
> >>>>> + if (remaining < mptcp_add_addr_len(family, *echo, *port))
> >>>>> goto out_unlock;
> >>>>>
> >>>>> *saddr = msk->pm.local;
> >>>>> + *daddr = msk->pm.remote;
> >>>>> add_addr = READ_ONCE(msk->pm.addr_signal);
> >>>>> if (mptcp_pm_should_add_signal_echo(msk))
> >>>>> add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> >>>>> index 937e0309e340..4b63cc6079fa 100644
> >>>>> --- a/net/mptcp/protocol.h
> >>>>> +++ b/net/mptcp/protocol.h
> >>>>> @@ -766,25 +766,16 @@ 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(struct mptcp_out_options *opts,
> >>>>> - u8 add_addr)
> >>>>> +static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> >>>>> {
> >>>>> - struct mptcp_addr_info *addr = &opts->remote;
> >>>>> - u8 len = 0;
> >>>>> + u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>>>
> >>>>> - if (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
> >>>>> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> >>>>> - addr = &opts->local;
> >>>>> + if (family == AF_INET6)
> >>>>> + len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>>> + if (!echo)
> >>>>> 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 (addr->port)
> >>>>> + if (port)
> >>>>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >>>>>
> >>>>> return len;
> >>>>> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
> >>>>> - unsigned int opt_size, unsigned int remaining,
> >>>>> - struct mptcp_out_options *opts, u8 *add_addr);
> >>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> >>>>> + bool *echo, bool *port);
> >>>>> 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);
> >>>>>
> >>>>
> >>>> --
> >>>> Li YongLong
> >>>
> >>
> >> --
> >> Li YongLong
> >
> >
>
> --
> Li YongLong
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"
2021-07-12 9:29 ` Geliang Tang
@ 2021-07-12 9:44 ` Yonglong Li
2021-07-12 10:34 ` Geliang Tang
0 siblings, 1 reply; 17+ messages in thread
From: Yonglong Li @ 2021-07-12 9:44 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp
On 2021/7/12 17:29, Geliang Tang wrote:
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午5:14写道：
>>
>>
>>
>> On 2021/7/12 16:44, Geliang Tang wrote:
>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午4:07写道：
>>>>
>>>>
>>>>
>>>> On 2021/7/12 15:33, Geliang Tang wrote:
>>>>> Hi Yonglong,
>>>>>
>>>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2021/7/11 23:15, Geliang Tang wrote:
>>>>>>> I think there're still some issues in v8:
>>>>>>>
>>>>>>> The remaining value is incorrect since "remaining += opt_size;" in the
>>>>>>> "drop other suboptions" checks has been called twice in
>>>>>>> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
>>>>>>>
>>>>>> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
>>>>>> mptcp_established_options_add_addr.
>>>>>>
>>>>>>> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
>>>>>>> populate after the length chech, not before the check.]
>>>>>>>
>>>>>>> The squash-to patch keeped the more orignal code unchanged, and just do
>>>>>>> the least, necessary modifications.
>>>>>>>
>>>>>> Agree opts->local and opts->remote should be asigned after the length check.
>>>>>> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
>>>>>> as orignal code, there is a race that:
>>>>>>
>>>>>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>>>>>> ==> call mptcp_pm_add_addr_signal
>>>>>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
>>>>>> ==> at this time opts->remote is empty and the length is incorrect.
>>>>>>
>>>>>
>>>>> What will happen in v8 when this race occurs? How dose v8 deal with the
>>>>> race?
>>>> Hi Geliang, thinks for your patience.
>>>>
>>>> I think v8 doesn't have this issue:
>>>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>>>> ==> call mptcp_pm_add_addr_signal, save pm.addr_signal to add_addr and save addr in opts under pm.lock
>>>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO), but add_addr doesn't changed.
>>>> ==> use add_addr and opts to check length.
>>>> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
>>>
>>> Thanks for your explanation.
>>>
>>> I think this squash-to patch did the same thing:
>>>
>>> ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>>> ==> call mptcp_pm_add_addr_signal, save echo bit of pm.addr_signal to
>>> 'echo' (echo = false), save the port number to 'port', and save addr
>>> in opts under pm.lock
>>> ==> an echo add addr event trigger (pm.addr_signal ==
>>> MPTCP_ADD_ADDR_ECHO), but 'echo' and 'port' don't changed.
>>> ==> use 'echo' to get the address family, use 'family', 'echo' and
>>> 'port' to check length.
>>> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
>>>
>>> Do you think so?
>> yep. In this case the squash-to patch is ok. But I think between "drop other suboptions" checks and
>> mptcp_pm_add_addr_signal the race still exist.
>>
>
> I think this is easy to fix:
>
> Add a new argument "drop_other_suboptions" for mptcp_pm_add_addr_signal,
> move this "drop other suboptions" check code into mptcp_pm_add_addr_signal,
> I'll sent a v2 later.
Thanks. And the v8 do the same thing. Why not use v8 directly :)
>
> Thanks,
> -Geliang
>
>> ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
>> ==> "drop other suboptions" checks will use MPTCP_ADD_ADDR_SIGNAL to check
>> ==> an echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL | MPTCP_ADD_ADDR_ECHO )
>> ==> call mptcp_pm_add_addr_signal, MPTCP_ADD_ADDR_ECHO will be clear in pm.addr_signal
>> ==> process MPTCP_ADD_ADDR_ECHO event.
>>
>> WDYT?
>>
>>>
>>>>
>>>>>
>>>>>> So I think the orignal code is incorrect. WDYT?
>>>>>>
>>>>>>> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
>>>>>>>
>>>>>>> Change arguments of mptcp_pm_add_addr_signal.
>>>>>>>
>>>>>>> Keep mptcp_add_addr_len unchanged.
>>>>>>>
>>>>>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>>>>>> ---
>>>>>>> net/mptcp/options.c | 35 +++++++++++++++++------------------
>>>>>>> net/mptcp/pm.c | 23 +++++++++--------------
>>>>>>> net/mptcp/protocol.h | 27 +++++++++------------------
>>>>>>> 3 files changed, 35 insertions(+), 50 deletions(-)
>>>>>>>
>>>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>>>>> index 5c0ad9b90866..93ad7b134f74 100644
>>>>>>> --- a/net/mptcp/options.c
>>>>>>> +++ b/net/mptcp/options.c
>>>>>>> @@ -663,16 +663,14 @@ 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;
>>>>>>> - u8 add_addr;
>>>>>>> + bool echo;
>>>>>>> + bool port;
>>>>>>> + u8 family;
>>>>>>> int len;
>>>>>>>
>>>>>>> - if (!mptcp_pm_should_add_signal(msk) ||
>>>>>>> - !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts, &add_addr))
>>>>>>> - return false;
>>>>>>> -
>>>>>>> - if (((add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) ||
>>>>>>> - ((add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
>>>>>>> - (opts->local.family == AF_INET6 || opts->local.port))) &&
>>>>>>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
>>>>>>> + (mptcp_pm_should_add_signal_addr(msk) &&
>>>>>>> + (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
>>>>>>> skb && skb_is_tcp_pure_ack(skb)) {
>>>>>>> pr_debug("drop other suboptions");
>>>>>>> opts->suboptions = 0;
>>>>>>> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>>> drop_other_suboptions = true;
>>>>>>> }
>>>>>>>
>>>>>>> - len = mptcp_add_addr_len(opts, add_addr);
>>>>>>> + if (!mptcp_pm_should_add_signal(msk) ||
>>>>>>> + !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
>>>>>>> + return false;
>>>>>>> +
>>>>>>> + family = echo ? opts->remote.family : opts->local.family;
>>>>>>> + len = mptcp_add_addr_len(family, echo, port);
>>>>>>> if (remaining < len)
>>>>>>> return false;
>>>>>>>
>>>>>>> @@ -690,15 +693,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 (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
>>>>>>> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
>>>>>>> + if (!echo) {
>>>>>>> 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",
>>>>>>> - add_addr, (opts->ahmac == 0), opts->local.id, opts->ahmac,
>>>>>>> - ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
>>>>>>> + pr_debug("local_id=%d, local_port=%d, remote_id=%d, remote_port=%d, ahmac=%llu, echo=%d",
>>>>>>> + opts->local.id, ntohs(opts->local.port), opts->remote.id,
>>>>>>> + ntohs(opts->remote.port), opts->ahmac, echo);
>>>>>>>
>>>>>>> return true;
>>>>>>> }
>>>>>>> @@ -1253,13 +1255,10 @@ 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;
>>>>>>> + 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;
>>>>>>> -
>>>>>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>>>>>>> if (addr->family == AF_INET6)
>>>>>>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>>>>>> index 264f522af530..399b59cb7563 100644
>>>>>>> --- a/net/mptcp/pm.c
>>>>>>> +++ b/net/mptcp/pm.c
>>>>>>> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>>>>>>
>>>>>>> /* path manager helpers */
>>>>>>>
>>>>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
>>>>>>> - unsigned int opt_size, unsigned int remaining,
>>>>>>> - struct mptcp_out_options *opts, u8 *add_addr)
>>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>>> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
>>>>>>> + bool *echo, bool *port)
>>>>>>> {
>>>>>>> int ret = false;
>>>>>>> u8 add_addr;
>>>>>>> + u8 family;
>>>>>>>
>>>>>>> spin_lock_bh(&msk->pm.lock);
>>>>>>>
>>>>>>> @@ -266,21 +267,15 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
>>>>>>> if (!mptcp_pm_should_add_signal(msk))
>>>>>>> goto out_unlock;
>>>>>>>
>>>>>>> - opts->local = msk->pm.local;
>>>>>>> - opts->remote = msk->pm.remote;
>>>>>>> - *add_addr = msk->pm.addr_signal;
>>>>>>> + *echo = mptcp_pm_should_add_signal_echo(msk);
>>>>>>> + *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
>>>>>>>
>>>>>>> - if (((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)) ||
>>>>>>> - ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
>>>>>>> - (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
>>>>>>> - skb && skb_is_tcp_pure_ack(skb)) {
>>>>>>> - remaining += opt_size;
>>>>>>> - }
>>>>>>> -
>>>>>>> - if (remaining < mptcp_add_addr_len(opts, *add_addr))
>>>>>>> + family = *echo ? msk->pm.remote.family : msk->pm.local.family;
>>>>>>> + if (remaining < mptcp_add_addr_len(family, *echo, *port))
>>>>>>> goto out_unlock;
>>>>>>>
>>>>>>> *saddr = msk->pm.local;
>>>>>>> + *daddr = msk->pm.remote;
>>>>>>> add_addr = READ_ONCE(msk->pm.addr_signal);
>>>>>>> if (mptcp_pm_should_add_signal_echo(msk))
>>>>>>> add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>>>>>> index 937e0309e340..4b63cc6079fa 100644
>>>>>>> --- a/net/mptcp/protocol.h
>>>>>>> +++ b/net/mptcp/protocol.h
>>>>>>> @@ -766,25 +766,16 @@ 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(struct mptcp_out_options *opts,
>>>>>>> - u8 add_addr)
>>>>>>> +static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
>>>>>>> {
>>>>>>> - struct mptcp_addr_info *addr = &opts->remote;
>>>>>>> - u8 len = 0;
>>>>>>> + u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>>>>>>
>>>>>>> - if (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
>>>>>>> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
>>>>>>> - addr = &opts->local;
>>>>>>> + if (family == AF_INET6)
>>>>>>> + len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>>>>>>> + if (!echo)
>>>>>>> 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 (addr->port)
>>>>>>> + if (port)
>>>>>>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
>>>>>>>
>>>>>>> return len;
>>>>>>> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
>>>>>>> - unsigned int opt_size, unsigned int remaining,
>>>>>>> - struct mptcp_out_options *opts, u8 *add_addr);
>>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>>>>> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
>>>>>>> + bool *echo, bool *port);
>>>>>>> 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);
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Li YongLong
>>>>>
>>>>
>>>> --
>>>> Li YongLong
>>>
>>>
>>
>> --
>> Li YongLong
>
--
Li YongLong
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other"
2021-07-11 15:15 ` [MPTCP][PATCH mptcp-next] Squash to "mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other" Geliang Tang
@ 2021-07-12 9:55 ` Yonglong Li
2021-07-12 10:34 ` Geliang Tang
0 siblings, 1 reply; 17+ messages in thread
From: Yonglong Li @ 2021-07-12 9:55 UTC (permalink / raw)
To: Geliang Tang, mptcp, Paolo Abeni
On 2021/7/11 23:15, Geliang Tang wrote:
> Add READ_ONCE() for reading msk->pm.addr_signal.
>
> Use mptcp_pm_should_add_signal_echo instead of open coding.
>
> Use '&=' to clear flag.
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> net/mptcp/pm.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index c9622696716e..be16da2dcb6b 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -257,6 +257,7 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> struct mptcp_addr_info *saddr, bool *echo, bool *port)
> {
> int ret = false;
> + u8 add_addr;
>
> spin_lock_bh(&msk->pm.lock);
>
> @@ -271,10 +272,12 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> goto out_unlock;
>
> *saddr = msk->pm.local;
> - if ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)))
> - WRITE_ONCE(msk->pm.addr_signal, msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO));
> + add_addr = READ_ONCE(msk->pm.addr_signal);
> + if (mptcp_pm_should_add_signal_echo(msk))
> + add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
> else
> - WRITE_ONCE(msk->pm.addr_signal, msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL));
> + add_addr &= ~BIT(MPTCP_ADD_ADDR_SIGNAL);
> + WRITE_ONCE(msk->pm.addr_signal, add_addr);
> ret = true;
>
> out_unlock:
> @@ -294,7 +297,8 @@ 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);
> + rm_addr = READ_ONCE(msk->pm.addr_signal);
> + rm_addr &= ~BIT(MPTCP_RM_ADDR_SIGNAL);
> len = mptcp_rm_addr_len(&msk->pm.rm_list_tx);
> if (len < 0) {
> WRITE_ONCE(msk->pm.addr_signal, rm_addr);
>
These chunk of code is under the pm.lock so It is no need to use READ_ONCE() as Paolo saied before.
--
Li YongLong
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal"
2021-07-12 9:44 ` Yonglong Li
@ 2021-07-12 10:34 ` Geliang Tang
0 siblings, 0 replies; 17+ messages in thread
From: Geliang Tang @ 2021-07-12 10:34 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午5:44写道：
>
>
>
> On 2021/7/12 17:29, Geliang Tang wrote:
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午5:14写道：
> >>
> >>
> >>
> >> On 2021/7/12 16:44, Geliang Tang wrote:
> >>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午4:07写道：
> >>>>
> >>>>
> >>>>
> >>>> On 2021/7/12 15:33, Geliang Tang wrote:
> >>>>> Hi Yonglong,
> >>>>>
> >>>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 上午9:34写道：
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 2021/7/11 23:15, Geliang Tang wrote:
> >>>>>>> I think there're still some issues in v8:
> >>>>>>>
> >>>>>>> The remaining value is incorrect since "remaining += opt_size;" in the
> >>>>>>> "drop other suboptions" checks has been called twice in
> >>>>>>> mptcp_pm_add_addr_signal and mptcp_established_options_add_addr.
> >>>>>>>
> >>>>>> I think "remaining" in mptcp_pm_add_addr_signal does not touch "remaining" in
> >>>>>> mptcp_established_options_add_addr.
> >>>>>>
> >>>>>>> opts->local and opts->remote in mptcp_pm_add_addr_signal need be
> >>>>>>> populate after the length chech, not before the check.]
> >>>>>>>
> >>>>>>> The squash-to patch keeped the more orignal code unchanged, and just do
> >>>>>>> the least, necessary modifications.
> >>>>>>>
> >>>>>> Agree opts->local and opts->remote should be asigned after the length check.
> >>>>>> But if keep the length check out of mptcp_pm_add_addr_signal (out of pm lock )
> >>>>>> as orignal code, there is a race that:
> >>>>>>
> >>>>>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> >>>>>> ==> call mptcp_pm_add_addr_signal
> >>>>>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO|MPTCP_ADD_ADDR_SIGNAL)
> >>>>>> ==> at this time opts->remote is empty and the length is incorrect.
> >>>>>>
> >>>>>
> >>>>> What will happen in v8 when this race occurs? How dose v8 deal with the
> >>>>> race?
> >>>> Hi Geliang, thinks for your patience.
> >>>>
> >>>> I think v8 doesn't have this issue:
> >>>> ==> a add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> >>>> ==> call mptcp_pm_add_addr_signal, save pm.addr_signal to add_addr and save addr in opts under pm.lock
> >>>> ==> a echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_ECHO), but add_addr doesn't changed.
> >>>> ==> use add_addr and opts to check length.
> >>>> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
> >>>
> >>> Thanks for your explanation.
> >>>
> >>> I think this squash-to patch did the same thing:
> >>>
> >>> ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> >>> ==> call mptcp_pm_add_addr_signal, save echo bit of pm.addr_signal to
> >>> 'echo' (echo = false), save the port number to 'port', and save addr
> >>> in opts under pm.lock
> >>> ==> an echo add addr event trigger (pm.addr_signal ==
> >>> MPTCP_ADD_ADDR_ECHO), but 'echo' and 'port' don't changed.
> >>> ==> use 'echo' to get the address family, use 'family', 'echo' and
> >>> 'port' to check length.
> >>> ==> next send ack process will deal with MPTCP_ADD_ADDR_ECHO event.
> >>>
> >>> Do you think so?
> >> yep. In this case the squash-to patch is ok. But I think between "drop other suboptions" checks and
> >> mptcp_pm_add_addr_signal the race still exist.
> >>
> >
> > I think this is easy to fix:
> >
> > Add a new argument "drop_other_suboptions" for mptcp_pm_add_addr_signal,
> > move this "drop other suboptions" check code into mptcp_pm_add_addr_signal,
> > I'll sent a v2 later.
>
> Thanks. And the v8 do the same thing. Why not use v8 directly :)
>
You'll see the difference later. :)
> >
> > Thanks,
> > -Geliang
> >
> >> ==> an add addr event (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL)
> >> ==> "drop other suboptions" checks will use MPTCP_ADD_ADDR_SIGNAL to check
> >> ==> an echo add addr event trigger (pm.addr_signal == MPTCP_ADD_ADDR_SIGNAL | MPTCP_ADD_ADDR_ECHO )
> >> ==> call mptcp_pm_add_addr_signal, MPTCP_ADD_ADDR_ECHO will be clear in pm.addr_signal
> >> ==> process MPTCP_ADD_ADDR_ECHO event.
> >>
> >> WDYT?
> >>
> >>>
> >>>>
> >>>>>
> >>>>>> So I think the orignal code is incorrect. WDYT?
> >>>>>>
> >>>>>>> Drop the "drop other suboptions" check in mptcp_pm_add_addr_signal.
> >>>>>>>
> >>>>>>> Change arguments of mptcp_pm_add_addr_signal.
> >>>>>>>
> >>>>>>> Keep mptcp_add_addr_len unchanged.
> >>>>>>>
> >>>>>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> >>>>>>> ---
> >>>>>>> net/mptcp/options.c | 35 +++++++++++++++++------------------
> >>>>>>> net/mptcp/pm.c | 23 +++++++++--------------
> >>>>>>> net/mptcp/protocol.h | 27 +++++++++------------------
> >>>>>>> 3 files changed, 35 insertions(+), 50 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>>>>>> index 5c0ad9b90866..93ad7b134f74 100644
> >>>>>>> --- a/net/mptcp/options.c
> >>>>>>> +++ b/net/mptcp/options.c
> >>>>>>> @@ -663,16 +663,14 @@ 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;
> >>>>>>> - u8 add_addr;
> >>>>>>> + bool echo;
> >>>>>>> + bool port;
> >>>>>>> + u8 family;
> >>>>>>> int len;
> >>>>>>>
> >>>>>>> - if (!mptcp_pm_should_add_signal(msk) ||
> >>>>>>> - !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts, &add_addr))
> >>>>>>> - return false;
> >>>>>>> -
> >>>>>>> - if (((add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> >>>>>>> - ((add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
> >>>>>>> - (opts->local.family == AF_INET6 || opts->local.port))) &&
> >>>>>>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> >>>>>>> + (mptcp_pm_should_add_signal_addr(msk) &&
> >>>>>>> + (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> >>>>>>> skb && skb_is_tcp_pure_ack(skb)) {
> >>>>>>> pr_debug("drop other suboptions");
> >>>>>>> opts->suboptions = 0;
> >>>>>>> @@ -682,7 +680,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>>>> drop_other_suboptions = true;
> >>>>>>> }
> >>>>>>>
> >>>>>>> - len = mptcp_add_addr_len(opts, add_addr);
> >>>>>>> + if (!mptcp_pm_should_add_signal(msk) ||
> >>>>>>> + !mptcp_pm_add_addr_signal(msk, remaining, &opts->local, &opts->remote, &echo, &port))
> >>>>>>> + return false;
> >>>>>>> +
> >>>>>>> + family = echo ? opts->remote.family : opts->local.family;
> >>>>>>> + len = mptcp_add_addr_len(family, echo, port);
> >>>>>>> if (remaining < len)
> >>>>>>> return false;
> >>>>>>>
> >>>>>>> @@ -690,15 +693,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 (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
> >>>>>>> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> >>>>>>> + if (!echo) {
> >>>>>>> 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",
> >>>>>>> - add_addr, (opts->ahmac == 0), opts->local.id, opts->ahmac,
> >>>>>>> - ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port));
> >>>>>>> + pr_debug("local_id=%d, local_port=%d, remote_id=%d, remote_port=%d, ahmac=%llu, echo=%d",
> >>>>>>> + opts->local.id, ntohs(opts->local.port), opts->remote.id,
> >>>>>>> + ntohs(opts->remote.port), opts->ahmac, echo);
> >>>>>>>
> >>>>>>> return true;
> >>>>>>> }
> >>>>>>> @@ -1253,13 +1255,10 @@ 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;
> >>>>>>> + 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;
> >>>>>>> -
> >>>>>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >>>>>>> if (addr->family == AF_INET6)
> >>>>>>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>>>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> >>>>>>> index 264f522af530..399b59cb7563 100644
> >>>>>>> --- a/net/mptcp/pm.c
> >>>>>>> +++ b/net/mptcp/pm.c
> >>>>>>> @@ -253,12 +253,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >>>>>>>
> >>>>>>> /* path manager helpers */
> >>>>>>>
> >>>>>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> >>>>>>> - unsigned int opt_size, unsigned int remaining,
> >>>>>>> - struct mptcp_out_options *opts, u8 *add_addr)
> >>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>>>> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> >>>>>>> + bool *echo, bool *port)
> >>>>>>> {
> >>>>>>> int ret = false;
> >>>>>>> u8 add_addr;
> >>>>>>> + u8 family;
> >>>>>>>
> >>>>>>> spin_lock_bh(&msk->pm.lock);
> >>>>>>>
> >>>>>>> @@ -266,21 +267,15 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> >>>>>>> if (!mptcp_pm_should_add_signal(msk))
> >>>>>>> goto out_unlock;
> >>>>>>>
> >>>>>>> - opts->local = msk->pm.local;
> >>>>>>> - opts->remote = msk->pm.remote;
> >>>>>>> - *add_addr = msk->pm.addr_signal;
> >>>>>>> + *echo = mptcp_pm_should_add_signal_echo(msk);
> >>>>>>> + *port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
> >>>>>>>
> >>>>>>> - if (((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> >>>>>>> - ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
> >>>>>>> - (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> >>>>>>> - skb && skb_is_tcp_pure_ack(skb)) {
> >>>>>>> - remaining += opt_size;
> >>>>>>> - }
> >>>>>>> -
> >>>>>>> - if (remaining < mptcp_add_addr_len(opts, *add_addr))
> >>>>>>> + family = *echo ? msk->pm.remote.family : msk->pm.local.family;
> >>>>>>> + if (remaining < mptcp_add_addr_len(family, *echo, *port))
> >>>>>>> goto out_unlock;
> >>>>>>>
> >>>>>>> *saddr = msk->pm.local;
> >>>>>>> + *daddr = msk->pm.remote;
> >>>>>>> add_addr = READ_ONCE(msk->pm.addr_signal);
> >>>>>>> if (mptcp_pm_should_add_signal_echo(msk))
> >>>>>>> add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> >>>>>>> index 937e0309e340..4b63cc6079fa 100644
> >>>>>>> --- a/net/mptcp/protocol.h
> >>>>>>> +++ b/net/mptcp/protocol.h
> >>>>>>> @@ -766,25 +766,16 @@ 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(struct mptcp_out_options *opts,
> >>>>>>> - u8 add_addr)
> >>>>>>> +static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> >>>>>>> {
> >>>>>>> - struct mptcp_addr_info *addr = &opts->remote;
> >>>>>>> - u8 len = 0;
> >>>>>>> + u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>>>>>>
> >>>>>>> - if (!(add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) &&
> >>>>>>> - (add_addr & BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> >>>>>>> - addr = &opts->local;
> >>>>>>> + if (family == AF_INET6)
> >>>>>>> + len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >>>>>>> + if (!echo)
> >>>>>>> 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 (addr->port)
> >>>>>>> + if (port)
> >>>>>>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN;
> >>>>>>>
> >>>>>>> return len;
> >>>>>>> @@ -798,9 +789,9 @@ 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, struct sk_buff *skb,
> >>>>>>> - unsigned int opt_size, unsigned int remaining,
> >>>>>>> - struct mptcp_out_options *opts, u8 *add_addr);
> >>>>>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >>>>>>> + struct mptcp_addr_info *saddr, struct mptcp_addr_info *daddr,
> >>>>>>> + bool *echo, bool *port);
> >>>>>>> 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);
> >>>>>>>
> >>>>>>
> >>>>>> --
> >>>>>> Li YongLong
> >>>>>
> >>>>
> >>>> --
> >>>> Li YongLong
> >>>
> >>>
> >>
> >> --
> >> Li YongLong
> >
>
> --
> Li YongLong
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other"
2021-07-12 9:55 ` Yonglong Li
@ 2021-07-12 10:34 ` Geliang Tang
2021-07-12 22:27 ` Mat Martineau
0 siblings, 1 reply; 17+ messages in thread
From: Geliang Tang @ 2021-07-12 10:34 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Paolo Abeni
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午5:55写道：
>
>
>
> On 2021/7/11 23:15, Geliang Tang wrote:
> > Add READ_ONCE() for reading msk->pm.addr_signal.
> >
> > Use mptcp_pm_should_add_signal_echo instead of open coding.
> >
> > Use '&=' to clear flag.
> >
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > ---
> > net/mptcp/pm.c | 12 ++++++++----
> > 1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index c9622696716e..be16da2dcb6b 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -257,6 +257,7 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > struct mptcp_addr_info *saddr, bool *echo, bool *port)
> > {
> > int ret = false;
> > + u8 add_addr;
> >
> > spin_lock_bh(&msk->pm.lock);
> >
> > @@ -271,10 +272,12 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> > goto out_unlock;
> >
> > *saddr = msk->pm.local;
> > - if ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)))
> > - WRITE_ONCE(msk->pm.addr_signal, msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO));
> > + add_addr = READ_ONCE(msk->pm.addr_signal);
> > + if (mptcp_pm_should_add_signal_echo(msk))
> > + add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
> > else
> > - WRITE_ONCE(msk->pm.addr_signal, msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL));
> > + add_addr &= ~BIT(MPTCP_ADD_ADDR_SIGNAL);
> > + WRITE_ONCE(msk->pm.addr_signal, add_addr);
> > ret = true;
> >
> > out_unlock:
> > @@ -294,7 +297,8 @@ 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);
> > + rm_addr = READ_ONCE(msk->pm.addr_signal);
> > + rm_addr &= ~BIT(MPTCP_RM_ADDR_SIGNAL);
> > len = mptcp_rm_addr_len(&msk->pm.rm_list_tx);
> > if (len < 0) {
> > WRITE_ONCE(msk->pm.addr_signal, rm_addr);
> >
>
> These chunk of code is under the pm.lock so It is no need to use READ_ONCE() as Paolo saied before.
I'll drop this READ_ONCE() in v2.
>
> --
> Li YongLong
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate"
2021-07-11 15:15 [MPTCP][PATCH mptcp-next] Squash to "mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate" Geliang Tang
2021-07-11 15:15 ` [MPTCP][PATCH mptcp-next] Squash to "mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other" Geliang Tang
2021-07-11 15:15 ` [MPTCP][PATCH mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal" Geliang Tang
@ 2021-07-12 22:10 ` Mat Martineau
2 siblings, 0 replies; 17+ messages in thread
From: Mat Martineau @ 2021-07-12 22:10 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp
On Sun, 11 Jul 2021, Geliang Tang wrote:
> A small cleanup.
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> net/mptcp/pm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 30deb76fa5d0..1eeecd68f159 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, echo:%d", msk, addr->id, echo);
> + pr_debug("msk=%p, local_id=%d, echo=%d", msk, addr->id, echo);
>
> lockdep_assert_held(&msk->pm.lock);
>
> --
> 2.31.1
Thanks for catching this detail!
Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other"
2021-07-12 10:34 ` Geliang Tang
@ 2021-07-12 22:27 ` Mat Martineau
0 siblings, 0 replies; 17+ messages in thread
From: Mat Martineau @ 2021-07-12 22:27 UTC (permalink / raw)
To: Geliang Tang; +Cc: Yonglong Li, mptcp, Paolo Abeni
[-- Attachment #1: Type: text/plain, Size: 2499 bytes --]
On Mon, 12 Jul 2021, Geliang Tang wrote:
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月12日周一 下午5:55写道：
>>
>>
>>
>> On 2021/7/11 23:15, Geliang Tang wrote:
>>> Add READ_ONCE() for reading msk->pm.addr_signal.
>>>
>>> Use mptcp_pm_should_add_signal_echo instead of open coding.
>>>
>>> Use '&=' to clear flag.
>>>
>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>> ---
>>> net/mptcp/pm.c | 12 ++++++++----
>>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>> index c9622696716e..be16da2dcb6b 100644
>>> --- a/net/mptcp/pm.c
>>> +++ b/net/mptcp/pm.c
>>> @@ -257,6 +257,7 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>> struct mptcp_addr_info *saddr, bool *echo, bool *port)
>>> {
>>> int ret = false;
>>> + u8 add_addr;
>>>
>>> spin_lock_bh(&msk->pm.lock);
>>>
>>> @@ -271,10 +272,12 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>> goto out_unlock;
>>>
>>> *saddr = msk->pm.local;
>>> - if ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)))
>>> - WRITE_ONCE(msk->pm.addr_signal, msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO));
>>> + add_addr = READ_ONCE(msk->pm.addr_signal);
Like below, pm.lock is held here so READ_ONCE() isn't needed.
>>> + if (mptcp_pm_should_add_signal_echo(msk))
>>> + add_addr &= ~BIT(MPTCP_ADD_ADDR_ECHO);
>>> else
>>> - WRITE_ONCE(msk->pm.addr_signal, msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL));
>>> + add_addr &= ~BIT(MPTCP_ADD_ADDR_SIGNAL);
>>> + WRITE_ONCE(msk->pm.addr_signal, add_addr);
>>> ret = true;
>>>
>>> out_unlock:
>>> @@ -294,7 +297,8 @@ 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);
>>> + rm_addr = READ_ONCE(msk->pm.addr_signal);
>>> + rm_addr &= ~BIT(MPTCP_RM_ADDR_SIGNAL);
>>> len = mptcp_rm_addr_len(&msk->pm.rm_list_tx);
>>> if (len < 0) {
>>> WRITE_ONCE(msk->pm.addr_signal, rm_addr);
>>>
>>
>> These chunk of code is under the pm.lock so It is no need to use READ_ONCE() as Paolo saied before.
>
> I'll drop this READ_ONCE() in v2.
>
>>
>> --
>> Li YongLong
>
>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-07-12 22:27 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-11 15:15 [MPTCP][PATCH mptcp-next] Squash to "mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate" Geliang Tang
2021-07-11 15:15 ` [MPTCP][PATCH mptcp-next] Squash to "mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other" Geliang Tang
2021-07-12 9:55 ` Yonglong Li
2021-07-12 10:34 ` Geliang Tang
2021-07-12 22:27 ` Mat Martineau
2021-07-11 15:15 ` [MPTCP][PATCH mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal" Geliang Tang
2021-07-12 1:34 ` Yonglong Li
2021-07-12 7:33 ` Geliang Tang
2021-07-12 8:06 ` Yonglong Li
2021-07-12 8:44 ` Geliang Tang
2021-07-12 9:07 ` Geliang Tang
2021-07-12 9:21 ` Yonglong Li
2021-07-12 9:14 ` Yonglong Li
2021-07-12 9:29 ` Geliang Tang
2021-07-12 9:44 ` Yonglong Li
2021-07-12 10:34 ` Geliang Tang
2021-07-12 22:10 ` [MPTCP][PATCH mptcp-next] Squash to "mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate" Mat Martineau
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.