* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).