From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from chinatelecom.cn (prt-mail.chinatelecom.cn [42.123.76.228]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 6239317F for ; Tue, 29 Jun 2021 07:54:43 +0000 (UTC) HMM_SOURCE_IP:172.18.0.48:45060.387473755 HMM_ATTACHE_NUM:0000 HMM_SOURCE_TYPE:SMTP Received: from clientip-36.111.140.26?logid-f304ceb951004f4f8b28607134136f76 (unknown [172.18.0.48]) by chinatelecom.cn (HERMES) with SMTP id D6CF428010B; Tue, 29 Jun 2021 15:54:39 +0800 (CST) X-189-SAVE-TO-SEND: liyonglong@chinatelecom.cn Received: from ([172.18.0.48]) by app0024 with ESMTP id f304ceb951004f4f8b28607134136f76 for mathew.j.martineau@linux.intel.com; Tue Jun 29 15:54:40 2021 X-Transaction-ID: f304ceb951004f4f8b28607134136f76 X-filter-score: X-Real-From: liyonglong@chinatelecom.cn X-Receive-IP: 172.18.0.48 X-MEDUSA-Status: 0 Sender: liyonglong@chinatelecom.cn Subject: Re: [PATCH v6 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal To: Geliang Tang Cc: mptcp@lists.linux.dev, Mat Martineau References: <1624930899-99623-1-git-send-email-liyonglong@chinatelecom.cn> <1624930899-99623-4-git-send-email-liyonglong@chinatelecom.cn> <3ab57409-8d5b-981a-7656-fc2f1f6167ad@chinatelecom.cn> From: Yonglong Li Message-ID: <2f36e070-496f-c7a7-cb5a-26787db05dbd@chinatelecom.cn> Date: Tue, 29 Jun 2021 15:54:35 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 Precedence: bulk X-Mailing-List: mptcp@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 2021/6/29 15:35, Geliang Tang wrote: > Yonglong Li 于2021年6月29日周二 下午3:02写道： >> >> >> Hi Geiliang, Thanks for your reviews. >> >> On 2021/6/29 13:58, Geliang Tang wrote: >>> Yonglong Li 于2021年6月29日周二 上午9:42写道： >>>> >>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build >>>> ADD_ADDR/echo-ADD_ADDR option >>>> >>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option >>>> >>>> Signed-off-by: Yonglong Li >>>> --- >>>> include/net/mptcp.h | 3 ++- >>>> net/mptcp/options.c | 65 +++++++++++++++++++++++++++++++--------------------- >>>> net/mptcp/pm.c | 33 +++++++++++--------------- >>>> net/mptcp/protocol.h | 23 ++++++++++++------- >>>> 4 files changed, 69 insertions(+), 55 deletions(-) >>>> >>>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h >>>> index d61bbbf..d2c6ebe 100644 >>>> --- a/include/net/mptcp.h >>>> +++ b/include/net/mptcp.h >>>> @@ -61,7 +61,8 @@ struct mptcp_out_options { >>>> u64 sndr_key; >>>> u64 rcvr_key; >>>> u64 ahmac; >>>> - struct mptcp_addr_info addr; >>>> + struct mptcp_addr_info local; >>>> + struct mptcp_addr_info remote; >>>> struct mptcp_rm_list rm_list; >>>> u8 join_id; >>>> u8 backup; >>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c >>>> index 1aec016..1707bec 100644 >>>> --- a/net/mptcp/options.c >>>> +++ b/net/mptcp/options.c >>>> @@ -655,13 +655,15 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff * >>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn); >>>> bool drop_other_suboptions = false; >>>> unsigned int opt_size = *size; >>>> - bool echo; >>>> - bool port; >>>> - int len; >>>> + u8 add_addr, flags = 0xff; >>>> + int len = 0; >>>> >>>> - if ((mptcp_pm_should_add_signal_ipv6(msk) || >>>> - mptcp_pm_should_add_signal_port(msk) || >>>> - mptcp_pm_should_add_signal_echo(msk)) && >>>> + if (!mptcp_pm_add_addr_signal(msk, opts, &add_addr)) >>>> + return false; >>> >>> This add_addr argument is useless, let's drop it. >>> >> we can use add_addr use in debug log. > > I think it's not worth adding a new argument just for debugging. agree. > >> >>> And here add back mptcp_pm_should_add_signal check here. The original code >>> called mptcp_pm_should_add_signal twice for double check, once out of pm >>> lock, once under pm lock. We should keep it. >> Sorry, I think double check is not necessary. does we need double check? > > I think we should keep the original logic here. If we want to drop this > double check or something, we should do it in another patch, don't mix too > much things in one patch. agree. > >> >>> >>>> + >>>> + if ((mptcp_pm_should_add_signal_echo(msk) || >>>> + (mptcp_pm_should_add_signal_addr(msk) && >>>> + (opts->local.family == AF_INET6 || opts->local.port))) && >>>> skb && skb_is_tcp_pure_ack(skb)) { >>>> pr_debug("drop other suboptions"); >>>> opts->suboptions = 0; >>>> @@ -671,11 +673,17 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff * >>>> drop_other_suboptions = true; >>>> } >>>> >>>> - if (!mptcp_pm_should_add_signal(msk) || >>>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port))) >>>> - return false; >>>> >>>> - len = mptcp_add_addr_len(opts->addr.family, echo, port); >>>> + if (mptcp_pm_should_add_signal_echo(msk)) { >>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO); >>>> + } else { >>>> + opts->ahmac = add_addr_generate_hmac(msk->local_key, >>>> + msk->remote_key, >>>> + &opts->local); >>> >>> Keep this ahmac generating code after opts->suboptions set just like the >>> original code, since ahmac is the more expensive to populate. If remaining >>> length isn't enough, no need to set ahmac. >> >> because mptcp_add_addr_len(opts) will use ahmac to calculate len of opts, so I think Keep this ahmac >> generating code after opts->suboptions set is not ok. > > So we should use mptcp_pm_should_add_signal_addr instead of opts->ahmac in > mptcp_add_addr_len. agree. > >> >>> >>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL); >>>> + } >>>> + >>>> + len = mptcp_add_addr_len(opts); >>>> if (remaining < len) >>>> return false; >>>> >>>> @@ -683,13 +691,14 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff * >>>> if (drop_other_suboptions) >>>> *size -= opt_size; >>>> opts->suboptions |= OPTION_MPTCP_ADD_ADDR; >>>> - if (!echo) { >>>> - opts->ahmac = add_addr_generate_hmac(msk->local_key, >>>> - msk->remote_key, >>>> - &opts->addr); >>>> - } >>>> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d", >>>> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port)); >>>> + >>>> + spin_lock_bh(&msk->pm.lock); >>>> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal); >>>> + spin_unlock_bh(&msk->pm.lock); >>> >>> addr_signal has been set in mptcp_pm_add_addr_signal in patch 1, no need to >>> set it again. I thinks this trunk and all the flags set above should be >>> dropped. >> >> Because MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO maybe set at the same time. >> So i think we should only unset one flag. > > We can only unset one flag in mptcp_pm_add_addr_signal, see my comment in > patch 1. if change like this. there is a issue: if remaining len checking is not ok and return false, The ADD_ADDR/ECHO event will be clear. So I think we should make sure ADD_ADDR/ECHO option will add in packet before clean flags. WDYT? > > -Geliang > >> >>> >>>> + >>>> + pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d", >>>> + add_addr, (opts->ahmac == 0), opts->local.id, >>>> + opts->ahmac, ntohs(opts->local.port), opts->remote.id, ntohs(opts->remote.port)); >>>> >>>> return true; >>>> } >>> >>> The whole function is something like this: >>> ''' >>> struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk); >>> struct mptcp_sock *msk = mptcp_sk(subflow->conn); >>> bool drop_other_suboptions = false; >>> unsigned int opt_size = *size; >>> int len; >>> >>> if (!mptcp_pm_should_add_signal(msk) || >>> !mptcp_pm_add_addr_signal(msk, remaining, opts)) >>> return false; >>> >>> if ((mptcp_pm_should_add_signal_echo(msk) || >>> (mptcp_pm_should_add_signal_addr(msk) && >>> (opts->local.family == AF_INET6 || opts->local.port))) && >>> skb && skb_is_tcp_pure_ack(skb)) { >>> pr_debug("drop other suboptions"); >>> opts->suboptions = 0; >>> opts->ext_copy.use_ack = 0; >>> opts->ext_copy.use_map = 0; >>> remaining += opt_size; >>> drop_other_suboptions = true; >>> } >>> >>> len = mptcp_add_addr_len(opts); >>> if (remaining < len) >>> return false; >>> >>> *size = len; >>> if (drop_other_suboptions) >>> *size -= opt_size; >>> opts->suboptions |= OPTION_MPTCP_ADD_ADDR; >>> if (mptcp_pm_should_add_signal_addr(msk)) { >>> opts->ahmac = add_addr_generate_hmac(msk->local_key, >>> msk->remote_key, >>> &opts->local); >>> } >>> >>> pr_debug("addr_signal:%x, echo=%d, local_addr_id=%d, >>> ahmac=%llu, local_port=%d, remote_addr_id=%d, remote_port=%d", >>> msk->pm.addr_signal, (opts->ahmac == 0), opts->local.id, >>> opts->ahmac, ntohs(opts->local.port), >>> opts->remote.id, ntohs(opts->remote.port)); >>> >>> return true; >>> ''' >>> >>>> @@ -1229,15 +1238,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, >>>> >>>> mp_capable_done: >>>> if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) { >>>> + struct mptcp_addr_info *addr = &opts->remote; >>> >>> We can simplify it like this: >>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local : >>> &opts->remote; >>> >>>> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE; >>>> u8 echo = MPTCP_ADDR_ECHO; >>>> >>>> + if (opts->ahmac) >>>> + addr = &opts->local; >>> >>> And this trunk can be dropped. >>> >>>> + >>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6) >>>> - if (opts->addr.family == AF_INET6) >>>> + if (addr->family == AF_INET6) >>>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE; >>>> #endif >>>> >>>> - if (opts->addr.port) >>>> + if (addr->port) >>>> len += TCPOLEN_MPTCP_PORT_LEN; >>>> >>>> if (opts->ahmac) { >>>> @@ -1246,25 +1259,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, >>>> } >>>> >>>> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR, >>>> - len, echo, opts->addr.id); >>>> - if (opts->addr.family == AF_INET) { >>>> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4); >>>> + len, echo, addr->id); >>>> + if (addr->family == AF_INET) { >>>> + memcpy((u8 *)ptr, (u8 *)&addr->addr.s_addr, 4); >>>> ptr += 1; >>>> } >>>> #if IS_ENABLED(CONFIG_MPTCP_IPV6) >>>> - else if (opts->addr.family == AF_INET6) { >>>> - memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16); >>>> + else if (addr->family == AF_INET6) { >>>> + memcpy((u8 *)ptr, addr->addr6.s6_addr, 16); >>>> ptr += 4; >>>> } >>>> #endif >>>> >>>> - if (!opts->addr.port) { >>>> + if (!addr->port) { >>>> if (opts->ahmac) { >>>> put_unaligned_be64(opts->ahmac, ptr); >>>> ptr += 2; >>>> } >>>> } else { >>>> - u16 port = ntohs(opts->addr.port); >>>> + u16 port = ntohs(addr->port); >>>> >>>> if (opts->ahmac) { >>>> u8 *bptr = (u8 *)ptr; >>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c >>>> index cf873e9..9c621293 100644 >>>> --- a/net/mptcp/pm.c >>>> +++ b/net/mptcp/pm.c >>>> @@ -253,32 +253,25 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup) >>>> >>>> /* path manager helpers */ >>>> >>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining, >>>> - struct mptcp_addr_info *saddr, bool *echo, bool *port) >>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts, >>>> + u8 *add_addr) >>> >>> Drop this add_addr argument. >>> >>>> { >>>> - u8 add_addr; >>>> - int ret = false; >>>> - >>>> spin_lock_bh(&msk->pm.lock); >>>> >>>> - /* double check after the lock is acquired */ >>>> - if (!mptcp_pm_should_add_signal(msk)) >>>> - goto out_unlock; >>> >>> Keep this double check code. >>> >>>> - >>>> - *echo = mptcp_pm_should_add_signal_echo(msk); >>>> - *port = mptcp_pm_should_add_signal_port(msk); >>>> - >>>> - if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port)) >>>> - goto out_unlock; >>> >>> Keep this length double check code too. >>> >>>> + if (!mptcp_pm_should_add_signal(msk)) { >>>> + spin_unlock_bh(&msk->pm.lock); >>>> + return false; >>>> + } >>>> >>>> - *saddr = msk->pm.local; >>>> - add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO)); >>> - WRITE_ONCE(msk->pm.addr_signal, add_addr); >>> >>> This code is just added in patch 1, I think we should keep it. And no need >>> to write addr_signal again in mptcp_established_options_add_addr. >>> >>>> - ret = true; >>>> + opts->local = msk->pm.local; >>>> + opts->remote = msk->pm.remote; >>>> + *add_addr = msk->pm.addr_signal; >>>> >>>> -out_unlock: >>>> spin_unlock_bh(&msk->pm.lock); >>>> - return ret; >>> >>> Keep this out_unlock code. >>> >>>> + >>>> + if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk))) >>>> + mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK); >>> >>> Could we use mptcp_pm_add_addr_send_ack here instead of open coding? >>> >>> I'm no sure why we need this two lines, and why you use '&&' here. Do you >>> mean set MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO at the same time? >>> >>>> + return true; >>>> } >>> >>> The whole function is something like this: >>> ''' >>> int ret = false; >>> u8 add_addr; >>> >>> spin_lock_bh(&msk->pm.lock); >>> >>> /* double check after the lock is acquired */ >>> if (!mptcp_pm_should_add_signal(msk)) >>> goto out_unlock; >>> >>> if (remaining < mptcp_add_addr_len(opts)) >>> goto out_unlock; >>> >>> opts->local = msk->pm.local; >>> opts->remote = msk->pm.remote; >>> if (mptcp_pm_should_add_signal_echo(msk)) >>> add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO); >>> else >>> add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL); >>> WRITE_ONCE(msk->pm.addr_signal, add_addr); >>> ret = true; >>> >>> out_unlock: >>> spin_unlock_bh(&msk->pm.lock); >>> if (ret && mptcp_pm_should_add_signal_echo(msk) && >>> mptcp_pm_should_add_signal_addr(msk)) >>> mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK); >>> return ret; >>> ''' >>> >>>> >>>> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining, >>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h >>>> index a0b0ec0..0bfbbdef 100644 >>>> --- a/net/mptcp/protocol.h >>>> +++ b/net/mptcp/protocol.h >>>> @@ -737,16 +737,23 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk) >>>> return READ_ONCE(msk->pm.addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL); >>>> } >>>> >>>> -static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port) >>>> +static inline unsigned int mptcp_add_addr_len(struct mptcp_out_options *opts) >>>> { >>>> - u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE; >>>> + u8 len = 0; >>>> + struct mptcp_addr_info *addr = &opts->remote; >>> >>> We can simplify it like this: >>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local : >>> &opts->remote; >>> >>> And keep the orignal code unchanged. >>> >>>> >>>> - if (family == AF_INET6) >>>> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE; >>>> - if (!echo) >>>> + if (opts->ahmac) { >>>> + addr = &opts->local; >>>> len += MPTCPOPT_THMAC_LEN; >>>> + } >>>> + >>>> + if (addr->family == AF_INET6) >>>> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE; >>>> + else >>>> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE; >>>> + >>>> /* account for 2 trailing 'nop' options */ >>>> - if (port) >>>> + if (addr->port) >>>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN; >>>> >>>> return len; >>> >>> The whole function is something like this: >>> ''' >>> struct mptcp_addr_info *addr = opts->ahmac ? &opts->local : >>> &opts->remote; >>> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE; >>> >>> if (addr->family == AF_INET6) >>> len = TCPOLEN_MPTCP_ADD_ADDR6_BASE; >>> if (opts->ahmac) >>> len += MPTCPOPT_THMAC_LEN; >>> /* account for 2 trailing 'nop' options */ >>> if (addr->port) >>> len += TCPOLEN_MPTCP_PORT_LEN + TCPOLEN_MPTCP_PORT_ALIGN; >>> >>> return len; >>> ''' >>> >>> Thanks. >>> -Geliang >>> >>>> @@ -760,8 +767,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list) >>>> return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1; >>>> } >>>> >>>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining, >>>> - struct mptcp_addr_info *saddr, bool *echo, bool *port); >>>> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_out_options *opts, >>>> + u8 *add_addr); >>>> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining, >>>> struct mptcp_rm_list *rm_list); >>>> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc); >>>> -- >>>> 1.8.3.1 >>>> >>>> >>> >>> >> >> -- >> Li YongLong > -- Li YongLong