* [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
* 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: 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: 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
* [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 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 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: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: 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: 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
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).