* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-18 8:18 ` [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
@ 2021-06-18 11:20 ` Geliang Tang
2021-06-21 3:51 ` Yonglong Li
2021-06-21 7:42 ` Geliang Tang
2021-06-21 8:29 ` Geliang Tang
2 siblings, 1 reply; 15+ messages in thread
From: Geliang Tang @ 2021-06-18 11:20 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng
Hi Yonglong,
Thanks for v4!
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
>
> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> ADD_ADDR/echo-ADD_ADDR option
>
> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>
> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> ---
> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
> net/mptcp/pm.c | 30 ++++---------
> net/mptcp/protocol.h | 13 +++---
> 3 files changed, 92 insertions(+), 75 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..43e3241 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> bool drop_other_suboptions = false;
> unsigned int opt_size = *size;
> - bool echo;
> - bool port;
> + struct mptcp_addr_info remote;
> + struct mptcp_addr_info local;
> + u8 add_addr, flags = 0xff;
> int len;
>
> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> - mptcp_pm_should_add_signal_port(msk) ||
> - mptcp_pm_should_add_signal_echo(msk)) &&
> - skb && skb_is_tcp_pure_ack(skb)) {
> - pr_debug("drop other suboptions");
> - opts->suboptions = 0;
> - opts->ext_copy.use_ack = 0;
> - opts->ext_copy.use_map = 0;
> - remaining += opt_size;
> - drop_other_suboptions = true;
> - }
> -
> - if (!mptcp_pm_should_add_signal(msk) ||
> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> - return false;
> -
> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> - if (remaining < len)
> + if (!mptcp_pm_should_add_signal(msk))
> return false;
>
> - *size = len;
> - if (drop_other_suboptions)
> - *size -= opt_size;
> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> - if (!echo) {
> + *size = 0;
> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> + if (mptcp_pm_should_add_signal_echo(msk)) {
> + if (skb && skb_is_tcp_pure_ack(skb)) {
'''
> + pr_debug("drop other suboptions");
> + opts->suboptions = 0;
> + opts->ext_copy.use_ack = 0;
> + opts->ext_copy.use_map = 0;
> + remaining += opt_size;
> + drop_other_suboptions = true;
'''
> + }
> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> + if (remaining < len)
> + return false;
> + remaining -= len;
> + *size += len;
> + opts->remote = remote;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> + opts->remote.id, ntohs(opts->remote.port), add_addr);
> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
> + if ((local.family == AF_INET6 || local.port) && skb &&
> + skb_is_tcp_pure_ack(skb)) {
'''
> + pr_debug("drop other suboptions");
> + opts->suboptions = 0;
> + opts->ext_copy.use_ack = 0;
> + opts->ext_copy.use_map = 0;
> + remaining += opt_size;
> + drop_other_suboptions = true;
'''
I think this "drop other suboptions" trunk here is still duplicated. Can
we just use one "drop other suboptions" trunk only?
Thanks.
-Geliang
> + }
> + len = mptcp_add_addr_len(local.family, false, !!local.port);
> + if (remaining < len)
> + return false;
> + *size += len;
> + opts->addr = local;
> opts->ahmac = add_addr_generate_hmac(msk->local_key,
> msk->remote_key,
> &opts->addr);
> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> + pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
> + opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
> }
> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> +
> + if (drop_other_suboptions)
> + *size -= opt_size;
> + spin_lock_bh(&msk->pm.lock);
> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> + spin_unlock_bh(&msk->pm.lock);
>
> return true;
> }
> @@ -1228,45 +1251,51 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> }
>
> mp_capable_done:
> - if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> - u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> - u8 echo = MPTCP_ADDR_ECHO;
> + if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
> + struct mptcp_addr_info *addr_info;
> + u8 len = 0;
> + u8 echo = 0;
> +
> + if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> + len += sizeof(opts->ahmac);
> + addr_info = &opts->addr;
> + } else {
> + echo = MPTCP_ADDR_ECHO;
> + addr_info = &opts->remote;
> + }
>
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> - if (opts->addr.family == AF_INET6)
> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + if (addr_info->family == AF_INET6)
> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + else
> #endif
> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>
> - if (opts->addr.port)
> + if (addr_info->port)
> len += TCPOLEN_MPTCP_PORT_LEN;
>
> - if (opts->ahmac) {
> - len += sizeof(opts->ahmac);
> - echo = 0;
> - }
> -
> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> - len, echo, opts->addr.id);
> - if (opts->addr.family == AF_INET) {
> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> + len, echo, addr_info->id);
> + if (addr_info->family == AF_INET) {
> + memcpy((u8 *)ptr, (u8 *)&addr_info->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_info->family == AF_INET6) {
> + memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
> ptr += 4;
> }
> #endif
>
> - if (!opts->addr.port) {
> - if (opts->ahmac) {
> + if (!addr_info->port) {
> + if (!echo) {
> put_unaligned_be64(opts->ahmac, ptr);
> ptr += 2;
> }
> } else {
> - u16 port = ntohs(opts->addr.port);
> + u16 port = ntohs(addr_info->port);
>
> - if (opts->ahmac) {
> + if (!echo) {
> u8 *bptr = (u8 *)ptr;
>
> put_unaligned_be16(port, bptr);
> @@ -1275,7 +1304,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> bptr += 8;
> put_unaligned_be16(TCPOPT_NOP << 8 |
> TCPOPT_NOP, bptr);
> -
> ptr += 3;
> } else {
> put_unaligned_be32(port << 16 |
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 107a5a2..a62d4a5 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -22,7 +22,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
>
> lockdep_assert_held(&msk->pm.lock);
>
> - if (add_addr) {
> + if (add_addr &
> + (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> pr_warn("addr_signal error, add_addr=%d", add_addr);
> return -EINVAL;
> }
> @@ -252,32 +253,19 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>
> /* path manager helpers */
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> - struct mptcp_addr_info *saddr, bool *echo, bool *port)
> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> + struct mptcp_addr_info *daddr, u8 *add_addr)
> {
> - u8 add_addr;
> - int ret = false;
> -
> spin_lock_bh(&msk->pm.lock);
>
> - /* double check after the lock is acquired */
> - if (!mptcp_pm_should_add_signal(msk))
> - goto out_unlock;
> -
> - *echo = mptcp_pm_should_add_signal_echo(msk);
> - *port = mptcp_pm_should_add_signal_port(msk);
> -
> - if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> - goto out_unlock;
> -
> *saddr = msk->pm.local;
> - add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> - WRITE_ONCE(msk->pm.addr_signal, add_addr);
> - ret = true;
> + *daddr = msk->pm.remote;
> + *add_addr = msk->pm.addr_signal;
>
> -out_unlock:
> spin_unlock_bh(&msk->pm.lock);
> - return ret;
> +
> + if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> + mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> }
>
> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index a0b0ec0..90fb532 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -22,10 +22,11 @@
> #define OPTION_MPTCP_MPJ_SYNACK BIT(4)
> #define OPTION_MPTCP_MPJ_ACK BIT(5)
> #define OPTION_MPTCP_ADD_ADDR BIT(6)
> -#define OPTION_MPTCP_RM_ADDR BIT(7)
> -#define OPTION_MPTCP_FASTCLOSE BIT(8)
> -#define OPTION_MPTCP_PRIO BIT(9)
> -#define OPTION_MPTCP_RST BIT(10)
> +#define OPTION_MPTCP_ADD_ECHO BIT(7)
> +#define OPTION_MPTCP_RM_ADDR BIT(8)
> +#define OPTION_MPTCP_FASTCLOSE BIT(9)
> +#define OPTION_MPTCP_PRIO BIT(10)
> +#define OPTION_MPTCP_RST BIT(11)
>
> /* MPTCP option subtypes */
> #define MPTCPOPT_MP_CAPABLE 0
> @@ -760,8 +761,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
> return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
> }
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> - struct mptcp_addr_info *saddr, bool *echo, bool *port);
> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> + struct mptcp_addr_info *daddr, u8 *add_addr);
> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> struct mptcp_rm_list *rm_list);
> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-18 11:20 ` Geliang Tang
@ 2021-06-21 3:51 ` Yonglong Li
2021-06-21 6:42 ` Geliang Tang
0 siblings, 1 reply; 15+ messages in thread
From: Yonglong Li @ 2021-06-21 3:51 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp, Mat Martineau, qitiepeng
On 2021/6/18 19:20, Geliang Tang wrote:
> Hi Yonglong,
>
> Thanks for v4!
>
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
>>
>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>> ADD_ADDR/echo-ADD_ADDR option
>>
>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>
>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
>> ---
>> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
>> net/mptcp/pm.c | 30 ++++---------
>> net/mptcp/protocol.h | 13 +++---
>> 3 files changed, 92 insertions(+), 75 deletions(-)
>>
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index 1aec016..43e3241 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>> bool drop_other_suboptions = false;
>> unsigned int opt_size = *size;
>> - bool echo;
>> - bool port;
>> + struct mptcp_addr_info remote;
>> + struct mptcp_addr_info local;
>> + u8 add_addr, flags = 0xff;
>> int len;
>>
>> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>> - mptcp_pm_should_add_signal_port(msk) ||
>> - mptcp_pm_should_add_signal_echo(msk)) &&
>> - skb && skb_is_tcp_pure_ack(skb)) {
>> - pr_debug("drop other suboptions");
>> - opts->suboptions = 0;
>> - opts->ext_copy.use_ack = 0;
>> - opts->ext_copy.use_map = 0;
>> - remaining += opt_size;
>> - drop_other_suboptions = true;
>> - }
>> -
>> - if (!mptcp_pm_should_add_signal(msk) ||
>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>> - return false;
>> -
>> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
>> - if (remaining < len)
>> + if (!mptcp_pm_should_add_signal(msk))
>> return false;
>>
>> - *size = len;
>> - if (drop_other_suboptions)
>> - *size -= opt_size;
>> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>> - if (!echo) {
>> + *size = 0;
>> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
>> + if (mptcp_pm_should_add_signal_echo(msk)) {
>> + if (skb && skb_is_tcp_pure_ack(skb)) {
>
> '''
>> + pr_debug("drop other suboptions");
>> + opts->suboptions = 0;
>> + opts->ext_copy.use_ack = 0;
>> + opts->ext_copy.use_map = 0;
>> + remaining += opt_size;
>> + drop_other_suboptions = true;
> '''
>
>> + }
>> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
>> + if (remaining < len)
>> + return false;
>> + remaining -= len;
>> + *size += len;
>> + opts->remote = remote;
>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
>> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
>> + opts->remote.id, ntohs(opts->remote.port), add_addr);
>> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
>> + if ((local.family == AF_INET6 || local.port) && skb &&
>> + skb_is_tcp_pure_ack(skb)) {
>
> '''
>> + pr_debug("drop other suboptions");
>> + opts->suboptions = 0;
>> + opts->ext_copy.use_ack = 0;
>> + opts->ext_copy.use_map = 0;
>> + remaining += opt_size;
>> + drop_other_suboptions = true;
> '''
>
> I think this "drop other suboptions" trunk here is still duplicated. Can
> we just use one "drop other suboptions" trunk only?
>
> Thanks.
> -Geliang
>
Hi Geliang, Thanks for you replay.
The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
IP version."
ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
to decide "drop other suboptions" in two trunk.
>
>
>> + }
>> + len = mptcp_add_addr_len(local.family, false, !!local.port);
>> + if (remaining < len)
>> + return false;
>> + *size += len;
>> + opts->addr = local;
>> opts->ahmac = add_addr_generate_hmac(msk->local_key,
>> msk->remote_key,
>> &opts->addr);
>> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>> + pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
>> + opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
>> }
>> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
>> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
>> +
>> + if (drop_other_suboptions)
>> + *size -= opt_size;
>> + spin_lock_bh(&msk->pm.lock);
>> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
>> + spin_unlock_bh(&msk->pm.lock);
>>
>> return true;
>> }
>> @@ -1228,45 +1251,51 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>> }
>>
>> mp_capable_done:
>> - if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
>> - u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>> - u8 echo = MPTCP_ADDR_ECHO;
>> + if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
>> + struct mptcp_addr_info *addr_info;
>> + u8 len = 0;
>> + u8 echo = 0;
>> +
>> + if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
>> + len += sizeof(opts->ahmac);
>> + addr_info = &opts->addr;
>> + } else {
>> + echo = MPTCP_ADDR_ECHO;
>> + addr_info = &opts->remote;
>> + }
>>
>> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>> - if (opts->addr.family == AF_INET6)
>> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>> + if (addr_info->family == AF_INET6)
>> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
>> + else
>> #endif
>> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>>
>> - if (opts->addr.port)
>> + if (addr_info->port)
>> len += TCPOLEN_MPTCP_PORT_LEN;
>>
>> - if (opts->ahmac) {
>> - len += sizeof(opts->ahmac);
>> - echo = 0;
>> - }
>> -
>> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
>> - len, echo, opts->addr.id);
>> - if (opts->addr.family == AF_INET) {
>> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
>> + len, echo, addr_info->id);
>> + if (addr_info->family == AF_INET) {
>> + memcpy((u8 *)ptr, (u8 *)&addr_info->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_info->family == AF_INET6) {
>> + memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
>> ptr += 4;
>> }
>> #endif
>>
>> - if (!opts->addr.port) {
>> - if (opts->ahmac) {
>> + if (!addr_info->port) {
>> + if (!echo) {
>> put_unaligned_be64(opts->ahmac, ptr);
>> ptr += 2;
>> }
>> } else {
>> - u16 port = ntohs(opts->addr.port);
>> + u16 port = ntohs(addr_info->port);
>>
>> - if (opts->ahmac) {
>> + if (!echo) {
>> u8 *bptr = (u8 *)ptr;
>>
>> put_unaligned_be16(port, bptr);
>> @@ -1275,7 +1304,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>> bptr += 8;
>> put_unaligned_be16(TCPOPT_NOP << 8 |
>> TCPOPT_NOP, bptr);
>> -
>> ptr += 3;
>> } else {
>> put_unaligned_be32(port << 16 |
>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>> index 107a5a2..a62d4a5 100644
>> --- a/net/mptcp/pm.c
>> +++ b/net/mptcp/pm.c
>> @@ -22,7 +22,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
>>
>> lockdep_assert_held(&msk->pm.lock);
>>
>> - if (add_addr) {
>> + if (add_addr &
>> + (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
>> pr_warn("addr_signal error, add_addr=%d", add_addr);
>> return -EINVAL;
>> }
>> @@ -252,32 +253,19 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>>
>> /* path manager helpers */
>>
>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>> - struct mptcp_addr_info *saddr, bool *echo, bool *port)
>> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
>> + struct mptcp_addr_info *daddr, u8 *add_addr)
>> {
>> - u8 add_addr;
>> - int ret = false;
>> -
>> spin_lock_bh(&msk->pm.lock);
>>
>> - /* double check after the lock is acquired */
>> - if (!mptcp_pm_should_add_signal(msk))
>> - goto out_unlock;
>> -
>> - *echo = mptcp_pm_should_add_signal_echo(msk);
>> - *port = mptcp_pm_should_add_signal_port(msk);
>> -
>> - if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
>> - goto out_unlock;
>> -
>> *saddr = msk->pm.local;
>> - add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
>> - WRITE_ONCE(msk->pm.addr_signal, add_addr);
>> - ret = true;
>> + *daddr = msk->pm.remote;
>> + *add_addr = msk->pm.addr_signal;
>>
>> -out_unlock:
>> spin_unlock_bh(&msk->pm.lock);
>> - return ret;
>> +
>> + if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
>> + mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>> }
>>
>> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index a0b0ec0..90fb532 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -22,10 +22,11 @@
>> #define OPTION_MPTCP_MPJ_SYNACK BIT(4)
>> #define OPTION_MPTCP_MPJ_ACK BIT(5)
>> #define OPTION_MPTCP_ADD_ADDR BIT(6)
>> -#define OPTION_MPTCP_RM_ADDR BIT(7)
>> -#define OPTION_MPTCP_FASTCLOSE BIT(8)
>> -#define OPTION_MPTCP_PRIO BIT(9)
>> -#define OPTION_MPTCP_RST BIT(10)
>> +#define OPTION_MPTCP_ADD_ECHO BIT(7)
>> +#define OPTION_MPTCP_RM_ADDR BIT(8)
>> +#define OPTION_MPTCP_FASTCLOSE BIT(9)
>> +#define OPTION_MPTCP_PRIO BIT(10)
>> +#define OPTION_MPTCP_RST BIT(11)
>>
>> /* MPTCP option subtypes */
>> #define MPTCPOPT_MP_CAPABLE 0
>> @@ -760,8 +761,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
>> return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
>> }
>>
>> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>> - struct mptcp_addr_info *saddr, bool *echo, bool *port);
>> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
>> + struct mptcp_addr_info *daddr, u8 *add_addr);
>> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>> struct mptcp_rm_list *rm_list);
>> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
>> --
>> 1.8.3.1
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-21 3:51 ` Yonglong Li
@ 2021-06-21 6:42 ` Geliang Tang
2021-06-21 7:15 ` Yonglong Li
0 siblings, 1 reply; 15+ messages in thread
From: Geliang Tang @ 2021-06-21 6:42 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng
Hi Yonglong,
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 上午11:52写道：
>
>
>
> On 2021/6/18 19:20, Geliang Tang wrote:
> > Hi Yonglong,
> >
> > Thanks for v4!
> >
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
> >>
> >> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> >> ADD_ADDR/echo-ADD_ADDR option
> >>
> >> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >>
> >> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> >> ---
> >> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
> >> net/mptcp/pm.c | 30 ++++---------
> >> net/mptcp/protocol.h | 13 +++---
> >> 3 files changed, 92 insertions(+), 75 deletions(-)
> >>
> >> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >> index 1aec016..43e3241 100644
> >> --- a/net/mptcp/options.c
> >> +++ b/net/mptcp/options.c
> >> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >> bool drop_other_suboptions = false;
> >> unsigned int opt_size = *size;
> >> - bool echo;
> >> - bool port;
> >> + struct mptcp_addr_info remote;
> >> + struct mptcp_addr_info local;
> >> + u8 add_addr, flags = 0xff;
> >> int len;
> >>
> >> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> >> - mptcp_pm_should_add_signal_port(msk) ||
> >> - mptcp_pm_should_add_signal_echo(msk)) &&
> >> - skb && skb_is_tcp_pure_ack(skb)) {
> >> - pr_debug("drop other suboptions");
> >> - opts->suboptions = 0;
> >> - opts->ext_copy.use_ack = 0;
> >> - opts->ext_copy.use_map = 0;
> >> - remaining += opt_size;
> >> - drop_other_suboptions = true;
> >> - }
> >> -
> >> - if (!mptcp_pm_should_add_signal(msk) ||
> >> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> >> - return false;
> >> -
> >> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> >> - if (remaining < len)
> >> + if (!mptcp_pm_should_add_signal(msk))
> >> return false;
> >>
> >> - *size = len;
> >> - if (drop_other_suboptions)
> >> - *size -= opt_size;
> >> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >> - if (!echo) {
> >> + *size = 0;
> >> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> >> + if (mptcp_pm_should_add_signal_echo(msk)) {
> >> + if (skb && skb_is_tcp_pure_ack(skb)) {
> >
> > '''
> >> + pr_debug("drop other suboptions");
> >> + opts->suboptions = 0;
> >> + opts->ext_copy.use_ack = 0;
> >> + opts->ext_copy.use_map = 0;
> >> + remaining += opt_size;
> >> + drop_other_suboptions = true;
> > '''
> >
> >> + }
> >> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> >> + if (remaining < len)
> >> + return false;
> >> + remaining -= len;
> >> + *size += len;
> >> + opts->remote = remote;
> >> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> >> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> >> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> >> + opts->remote.id, ntohs(opts->remote.port), add_addr);
> >> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
> >> + if ((local.family == AF_INET6 || local.port) && skb &&
> >> + skb_is_tcp_pure_ack(skb)) {
> >
> > '''
> >> + pr_debug("drop other suboptions");
> >> + opts->suboptions = 0;
> >> + opts->ext_copy.use_ack = 0;
> >> + opts->ext_copy.use_map = 0;
> >> + remaining += opt_size;
> >> + drop_other_suboptions = true;
> > '''
> >
> > I think this "drop other suboptions" trunk here is still duplicated. Can
> > we just use one "drop other suboptions" trunk only?
> >
> > Thanks.
> > -Geliang
> >
> Hi Geliang, Thanks for you replay.
>
> The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
> carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
> only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
> IP version."
> ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
> to decide "drop other suboptions" in two trunk.
Could we change it like this:
'''
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index e77b5d532fb8..8b4cb0581a49 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -673,15 +673,20 @@ static bool
mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
*size = 0;
mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
+
+ if ((mptcp_pm_should_add_signal_echo(msk) ||
+ (mptcp_pm_should_add_signal_addr(msk) &&
+ (local.family == AF_INET6 || local.port))) &&
+ skb && skb_is_tcp_pure_ack(skb)) {
+ pr_debug("drop other suboptions");
+ opts->suboptions = 0;
+ opts->ext_copy.use_ack = 0;
+ opts->ext_copy.use_map = 0;
+ remaining += opt_size;
+ drop_other_suboptions = true;
+ }
+
if (mptcp_pm_should_add_signal_echo(msk)) {
- if (skb && skb_is_tcp_pure_ack(skb)) {
- pr_debug("drop other suboptions");
- opts->suboptions = 0;
- opts->ext_copy.use_ack = 0;
- opts->ext_copy.use_map = 0;
- remaining += opt_size;
- drop_other_suboptions = true;
- }
len = mptcp_add_addr_len(remote.family, true, !!remote.port);
if (remaining < len)
return false;
@@ -693,15 +698,6 @@ static bool
mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
opts->remote.id, ntohs(opts->remote.port), add_addr);
} else if (mptcp_pm_should_add_signal_addr(msk)) {
- if ((local.family == AF_INET6 || local.port) && skb &&
- skb_is_tcp_pure_ack(skb)) {
- pr_debug("drop other suboptions");
- opts->suboptions = 0;
- opts->ext_copy.use_ack = 0;
- opts->ext_copy.use_map = 0;
- remaining += opt_size;
- drop_other_suboptions = true;
- }
len = mptcp_add_addr_len(local.family, false, !!local.port);
if (remaining < len)
return false;
'''
WDYT?
>
> >
> >
> >> + }
> >> + len = mptcp_add_addr_len(local.family, false, !!local.port);
> >> + if (remaining < len)
> >> + return false;
And here, I think "remaining -= len;" is missing.
Thanks,
-Geliang
> >> + *size += len;
> >> + opts->addr = local;
> >> opts->ahmac = add_addr_generate_hmac(msk->local_key,
> >> msk->remote_key,
> >> &opts->addr);
> >> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> >> + pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
> >> + opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
> >> }
> >> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> >> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> >> +
> >> + if (drop_other_suboptions)
> >> + *size -= opt_size;
> >> + spin_lock_bh(&msk->pm.lock);
> >> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> >> + spin_unlock_bh(&msk->pm.lock);
> >>
> >> return true;
> >> }
> >> @@ -1228,45 +1251,51 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >> }
> >>
> >> mp_capable_done:
> >> - if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> >> - u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >> - u8 echo = MPTCP_ADDR_ECHO;
> >> + if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
> >> + struct mptcp_addr_info *addr_info;
> >> + u8 len = 0;
> >> + u8 echo = 0;
> >> +
> >> + if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> >> + len += sizeof(opts->ahmac);
> >> + addr_info = &opts->addr;
> >> + } else {
> >> + echo = MPTCP_ADDR_ECHO;
> >> + addr_info = &opts->remote;
> >> + }
> >>
> >> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> >> - if (opts->addr.family == AF_INET6)
> >> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >> + if (addr_info->family == AF_INET6)
> >> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> >> + else
> >> #endif
> >> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
> >>
> >> - if (opts->addr.port)
> >> + if (addr_info->port)
> >> len += TCPOLEN_MPTCP_PORT_LEN;
> >>
> >> - if (opts->ahmac) {
> >> - len += sizeof(opts->ahmac);
> >> - echo = 0;
> >> - }
> >> -
> >> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> >> - len, echo, opts->addr.id);
> >> - if (opts->addr.family == AF_INET) {
> >> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> >> + len, echo, addr_info->id);
> >> + if (addr_info->family == AF_INET) {
> >> + memcpy((u8 *)ptr, (u8 *)&addr_info->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_info->family == AF_INET6) {
> >> + memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
> >> ptr += 4;
> >> }
> >> #endif
> >>
> >> - if (!opts->addr.port) {
> >> - if (opts->ahmac) {
> >> + if (!addr_info->port) {
> >> + if (!echo) {
> >> put_unaligned_be64(opts->ahmac, ptr);
> >> ptr += 2;
> >> }
> >> } else {
> >> - u16 port = ntohs(opts->addr.port);
> >> + u16 port = ntohs(addr_info->port);
> >>
> >> - if (opts->ahmac) {
> >> + if (!echo) {
> >> u8 *bptr = (u8 *)ptr;
> >>
> >> put_unaligned_be16(port, bptr);
> >> @@ -1275,7 +1304,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >> bptr += 8;
> >> put_unaligned_be16(TCPOPT_NOP << 8 |
> >> TCPOPT_NOP, bptr);
> >> -
> >> ptr += 3;
> >> } else {
> >> put_unaligned_be32(port << 16 |
> >> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> >> index 107a5a2..a62d4a5 100644
> >> --- a/net/mptcp/pm.c
> >> +++ b/net/mptcp/pm.c
> >> @@ -22,7 +22,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
> >>
> >> lockdep_assert_held(&msk->pm.lock);
> >>
> >> - if (add_addr) {
> >> + if (add_addr &
> >> + (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> >> pr_warn("addr_signal error, add_addr=%d", add_addr);
> >> return -EINVAL;
> >> }
> >> @@ -252,32 +253,19 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >>
> >> /* path manager helpers */
> >>
> >> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> - struct mptcp_addr_info *saddr, bool *echo, bool *port)
> >> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> >> + struct mptcp_addr_info *daddr, u8 *add_addr)
> >> {
> >> - u8 add_addr;
> >> - int ret = false;
> >> -
> >> spin_lock_bh(&msk->pm.lock);
> >>
> >> - /* double check after the lock is acquired */
> >> - if (!mptcp_pm_should_add_signal(msk))
> >> - goto out_unlock;
> >> -
> >> - *echo = mptcp_pm_should_add_signal_echo(msk);
> >> - *port = mptcp_pm_should_add_signal_port(msk);
> >> -
> >> - if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> >> - goto out_unlock;
> >> -
> >> *saddr = msk->pm.local;
> >> - add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> >> - WRITE_ONCE(msk->pm.addr_signal, add_addr);
> >> - ret = true;
> >> + *daddr = msk->pm.remote;
> >> + *add_addr = msk->pm.addr_signal;
> >>
> >> -out_unlock:
> >> spin_unlock_bh(&msk->pm.lock);
> >> - return ret;
> >> +
> >> + if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> >> + mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> >> }
> >>
> >> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> >> index a0b0ec0..90fb532 100644
> >> --- a/net/mptcp/protocol.h
> >> +++ b/net/mptcp/protocol.h
> >> @@ -22,10 +22,11 @@
> >> #define OPTION_MPTCP_MPJ_SYNACK BIT(4)
> >> #define OPTION_MPTCP_MPJ_ACK BIT(5)
> >> #define OPTION_MPTCP_ADD_ADDR BIT(6)
> >> -#define OPTION_MPTCP_RM_ADDR BIT(7)
> >> -#define OPTION_MPTCP_FASTCLOSE BIT(8)
> >> -#define OPTION_MPTCP_PRIO BIT(9)
> >> -#define OPTION_MPTCP_RST BIT(10)
> >> +#define OPTION_MPTCP_ADD_ECHO BIT(7)
> >> +#define OPTION_MPTCP_RM_ADDR BIT(8)
> >> +#define OPTION_MPTCP_FASTCLOSE BIT(9)
> >> +#define OPTION_MPTCP_PRIO BIT(10)
> >> +#define OPTION_MPTCP_RST BIT(11)
> >>
> >> /* MPTCP option subtypes */
> >> #define MPTCPOPT_MP_CAPABLE 0
> >> @@ -760,8 +761,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
> >> return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
> >> }
> >>
> >> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> - struct mptcp_addr_info *saddr, bool *echo, bool *port);
> >> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> >> + struct mptcp_addr_info *daddr, u8 *add_addr);
> >> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> struct mptcp_rm_list *rm_list);
> >> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> >> --
> >> 1.8.3.1
> >>
> >
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-21 6:42 ` Geliang Tang
@ 2021-06-21 7:15 ` Yonglong Li
2021-06-21 7:39 ` Geliang Tang
0 siblings, 1 reply; 15+ messages in thread
From: Yonglong Li @ 2021-06-21 7:15 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp, Mat Martineau, qitiepeng
On 2021/6/21 14:42, Geliang Tang wrote:
> Hi Yonglong,
>
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 上午11:52写道：
>>
>>
>> On 2021/6/18 19:20, Geliang Tang wrote:
>>> Hi Yonglong,
>>>
>>> Thanks for v4!
>>>
>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>>>> ADD_ADDR/echo-ADD_ADDR option
>>>>
>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>>>
>>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
>>>> ---
>>>> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
>>>> net/mptcp/pm.c | 30 ++++---------
>>>> net/mptcp/protocol.h | 13 +++---
>>>> 3 files changed, 92 insertions(+), 75 deletions(-)
>>>>
>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>> index 1aec016..43e3241 100644
>>>> --- a/net/mptcp/options.c
>>>> +++ b/net/mptcp/options.c
>>>> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>> bool drop_other_suboptions = false;
>>>> unsigned int opt_size = *size;
>>>> - bool echo;
>>>> - bool port;
>>>> + struct mptcp_addr_info remote;
>>>> + struct mptcp_addr_info local;
>>>> + u8 add_addr, flags = 0xff;
>>>> int len;
>>>>
>>>> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>>>> - mptcp_pm_should_add_signal_port(msk) ||
>>>> - mptcp_pm_should_add_signal_echo(msk)) &&
>>>> - skb && skb_is_tcp_pure_ack(skb)) {
>>>> - pr_debug("drop other suboptions");
>>>> - opts->suboptions = 0;
>>>> - opts->ext_copy.use_ack = 0;
>>>> - opts->ext_copy.use_map = 0;
>>>> - remaining += opt_size;
>>>> - drop_other_suboptions = true;
>>>> - }
>>>> -
>>>> - if (!mptcp_pm_should_add_signal(msk) ||
>>>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>>>> - return false;
>>>> -
>>>> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
>>>> - if (remaining < len)
>>>> + if (!mptcp_pm_should_add_signal(msk))
>>>> return false;
>>>>
>>>> - *size = len;
>>>> - if (drop_other_suboptions)
>>>> - *size -= opt_size;
>>>> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>> - if (!echo) {
>>>> + *size = 0;
>>>> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
>>>> + if (mptcp_pm_should_add_signal_echo(msk)) {
>>>> + if (skb && skb_is_tcp_pure_ack(skb)) {
>>> '''
>>>> + pr_debug("drop other suboptions");
>>>> + opts->suboptions = 0;
>>>> + opts->ext_copy.use_ack = 0;
>>>> + opts->ext_copy.use_map = 0;
>>>> + remaining += opt_size;
>>>> + drop_other_suboptions = true;
>>> '''
>>>
>>>> + }
>>>> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
>>>> + if (remaining < len)
>>>> + return false;
>>>> + remaining -= len;
>>>> + *size += len;
>>>> + opts->remote = remote;
>>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>>>> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
>>>> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
>>>> + opts->remote.id, ntohs(opts->remote.port), add_addr);
>>>> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
>>>> + if ((local.family == AF_INET6 || local.port) && skb &&
>>>> + skb_is_tcp_pure_ack(skb)) {
>>> '''
>>>> + pr_debug("drop other suboptions");
>>>> + opts->suboptions = 0;
>>>> + opts->ext_copy.use_ack = 0;
>>>> + opts->ext_copy.use_map = 0;
>>>> + remaining += opt_size;
>>>> + drop_other_suboptions = true;
>>> '''
>>>
>>> I think this "drop other suboptions" trunk here is still duplicated. Can
>>> we just use one "drop other suboptions" trunk only?
>>>
>>> Thanks.
>>> -Geliang
>>>
>> Hi Geliang, Thanks for you replay.
>>
>> The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
>> carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
>> only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
>> IP version."
>> ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
>> to decide "drop other suboptions" in two trunk.
> Could we change it like this:
>
> '''
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index e77b5d532fb8..8b4cb0581a49 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -673,15 +673,20 @@ static bool
> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>
> *size = 0;
> mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> +
> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> + (mptcp_pm_should_add_signal_addr(msk) &&
> + (local.family == AF_INET6 || local.port))) &&
> + skb && skb_is_tcp_pure_ack(skb)) {
> + pr_debug("drop other suboptions");
> + opts->suboptions = 0;
> + opts->ext_copy.use_ack = 0;
> + opts->ext_copy.use_map = 0;
> + remaining += opt_size;
> + drop_other_suboptions = true;
> + }
> +
> if (mptcp_pm_should_add_signal_echo(msk)) {
> - if (skb && skb_is_tcp_pure_ack(skb)) {
> - pr_debug("drop other suboptions");
> - opts->suboptions = 0;
> - opts->ext_copy.use_ack = 0;
> - opts->ext_copy.use_map = 0;
> - remaining += opt_size;
> - drop_other_suboptions = true;
> - }
> len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> if (remaining < len)
> return false;
> @@ -693,15 +698,6 @@ static bool
> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> opts->remote.id, ntohs(opts->remote.port), add_addr);
> } else if (mptcp_pm_should_add_signal_addr(msk)) {
> - if ((local.family == AF_INET6 || local.port) && skb &&
> - skb_is_tcp_pure_ack(skb)) {
> - pr_debug("drop other suboptions");
> - opts->suboptions = 0;
> - opts->ext_copy.use_ack = 0;
> - opts->ext_copy.use_map = 0;
> - remaining += opt_size;
> - drop_other_suboptions = true;
> - }
> len = mptcp_add_addr_len(local.family, false, !!local.port);
> if (remaining < len)
> return false;
> '''
> WDYT?
Thanks for your advice.
Because MPTCP_ADD_ADDR_ECHO and MPTCP_ADD_ADDR_SIGNAL can be set at the same time. So as your advice we should
change like this(still I think it not clear than before):
mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
+ if ((mptcp_pm_should_add_signal_echo(msk) ||
+ (!mptcp_pm_should_add_signal_echo(msk) &&
+ mptcp_pm_should_add_signal_addr(msk) &&
+ (local.family == AF_INET6 || local.port))) &&
+ skb && skb_is_tcp_pure_ack(skb)) {
+ pr_debug("drop other suboptions");
+ opts->suboptions = 0;
+ opts->ext_copy.use_ack = 0;
+ opts->ext_copy.use_map = 0;
+ remaining += opt_size;
+ drop_other_suboptions = true;
+ }
+
if (mptcp_pm_should_add_signal_echo(msk)) {
- if (skb && skb_is_tcp_pure_ack(skb)) {
>
>>>
>>>> + }
>>>> + len = mptcp_add_addr_len(local.family, false, !!local.port);
>>>> + if (remaining < len)
>>>> + return false;
> And here, I think "remaining -= len;" is missing.
>
> Thanks,
> -Geliang
>
"remaining" is not being used in the flowing code. So "remaining -=len;" is not necessary. But you remindme that the "remaining -= len;" can be removed in the first trunk.
I will send v5 as your advice.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-21 7:15 ` Yonglong Li
@ 2021-06-21 7:39 ` Geliang Tang
2021-06-21 7:49 ` Yonglong Li
0 siblings, 1 reply; 15+ messages in thread
From: Geliang Tang @ 2021-06-21 7:39 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 下午3:16写道：
>
>
>
> On 2021/6/21 14:42, Geliang Tang wrote:
> > Hi Yonglong,
> >
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 上午11:52写道：
> >>
> >>
> >> On 2021/6/18 19:20, Geliang Tang wrote:
> >>> Hi Yonglong,
> >>>
> >>> Thanks for v4!
> >>>
> >>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
> >>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> >>>> ADD_ADDR/echo-ADD_ADDR option
> >>>>
> >>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >>>>
> >>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> >>>> ---
> >>>> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
> >>>> net/mptcp/pm.c | 30 ++++---------
> >>>> net/mptcp/protocol.h | 13 +++---
> >>>> 3 files changed, 92 insertions(+), 75 deletions(-)
> >>>>
> >>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>>> index 1aec016..43e3241 100644
> >>>> --- a/net/mptcp/options.c
> >>>> +++ b/net/mptcp/options.c
> >>>> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >>>> bool drop_other_suboptions = false;
> >>>> unsigned int opt_size = *size;
> >>>> - bool echo;
> >>>> - bool port;
> >>>> + struct mptcp_addr_info remote;
> >>>> + struct mptcp_addr_info local;
> >>>> + u8 add_addr, flags = 0xff;
> >>>> int len;
> >>>>
> >>>> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> >>>> - mptcp_pm_should_add_signal_port(msk) ||
> >>>> - mptcp_pm_should_add_signal_echo(msk)) &&
> >>>> - skb && skb_is_tcp_pure_ack(skb)) {
> >>>> - pr_debug("drop other suboptions");
> >>>> - opts->suboptions = 0;
> >>>> - opts->ext_copy.use_ack = 0;
> >>>> - opts->ext_copy.use_map = 0;
> >>>> - remaining += opt_size;
> >>>> - drop_other_suboptions = true;
> >>>> - }
> >>>> -
> >>>> - if (!mptcp_pm_should_add_signal(msk) ||
> >>>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> >>>> - return false;
> >>>> -
> >>>> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> >>>> - if (remaining < len)
> >>>> + if (!mptcp_pm_should_add_signal(msk))
> >>>> return false;
> >>>>
> >>>> - *size = len;
> >>>> - if (drop_other_suboptions)
> >>>> - *size -= opt_size;
> >>>> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >>>> - if (!echo) {
> >>>> + *size = 0;
> >>>> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> >>>> + if (mptcp_pm_should_add_signal_echo(msk)) {
> >>>> + if (skb && skb_is_tcp_pure_ack(skb)) {
> >>> '''
> >>>> + pr_debug("drop other suboptions");
> >>>> + opts->suboptions = 0;
> >>>> + opts->ext_copy.use_ack = 0;
> >>>> + opts->ext_copy.use_map = 0;
> >>>> + remaining += opt_size;
> >>>> + drop_other_suboptions = true;
> >>> '''
> >>>
> >>>> + }
> >>>> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> >>>> + if (remaining < len)
> >>>> + return false;
> >>>> + remaining -= len;
> >>>> + *size += len;
> >>>> + opts->remote = remote;
> >>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> >>>> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> >>>> + opts->remote.id, ntohs(opts->remote.port), add_addr);
> >>>> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
> >>>> + if ((local.family == AF_INET6 || local.port) && skb &&
> >>>> + skb_is_tcp_pure_ack(skb)) {
> >>> '''
> >>>> + pr_debug("drop other suboptions");
> >>>> + opts->suboptions = 0;
> >>>> + opts->ext_copy.use_ack = 0;
> >>>> + opts->ext_copy.use_map = 0;
> >>>> + remaining += opt_size;
> >>>> + drop_other_suboptions = true;
> >>> '''
> >>>
> >>> I think this "drop other suboptions" trunk here is still duplicated. Can
> >>> we just use one "drop other suboptions" trunk only?
> >>>
> >>> Thanks.
> >>> -Geliang
> >>>
> >> Hi Geliang, Thanks for you replay.
> >>
> >> The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
> >> carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
> >> only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
> >> IP version."
> >> ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
> >> to decide "drop other suboptions" in two trunk.
> > Could we change it like this:
> >
> > '''
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index e77b5d532fb8..8b4cb0581a49 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -673,15 +673,20 @@ static bool
> > mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >
> > *size = 0;
> > mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> > +
> > + if ((mptcp_pm_should_add_signal_echo(msk) ||
> > + (mptcp_pm_should_add_signal_addr(msk) &&
> > + (local.family == AF_INET6 || local.port))) &&
> > + skb && skb_is_tcp_pure_ack(skb)) {
> > + pr_debug("drop other suboptions");
> > + opts->suboptions = 0;
> > + opts->ext_copy.use_ack = 0;
> > + opts->ext_copy.use_map = 0;
> > + remaining += opt_size;
> > + drop_other_suboptions = true;
> > + }
> > +
> > if (mptcp_pm_should_add_signal_echo(msk)) {
> > - if (skb && skb_is_tcp_pure_ack(skb)) {
> > - pr_debug("drop other suboptions");
> > - opts->suboptions = 0;
> > - opts->ext_copy.use_ack = 0;
> > - opts->ext_copy.use_map = 0;
> > - remaining += opt_size;
> > - drop_other_suboptions = true;
> > - }
> > len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> > if (remaining < len)
> > return false;
> > @@ -693,15 +698,6 @@ static bool
> > mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> > pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> > opts->remote.id, ntohs(opts->remote.port), add_addr);
> > } else if (mptcp_pm_should_add_signal_addr(msk)) {
> > - if ((local.family == AF_INET6 || local.port) && skb &&
> > - skb_is_tcp_pure_ack(skb)) {
> > - pr_debug("drop other suboptions");
> > - opts->suboptions = 0;
> > - opts->ext_copy.use_ack = 0;
> > - opts->ext_copy.use_map = 0;
> > - remaining += opt_size;
> > - drop_other_suboptions = true;
> > - }
> > len = mptcp_add_addr_len(local.family, false, !!local.port);
> > if (remaining < len)
> > return false;
> > '''
> > WDYT?
> Thanks for your advice.
>
> Because MPTCP_ADD_ADDR_ECHO and MPTCP_ADD_ADDR_SIGNAL can be set at the same time. So as your advice we should
> change like this(still I think it not clear than before):
>
> mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> + (!mptcp_pm_should_add_signal_echo(msk) &&
> + mptcp_pm_should_add_signal_addr(msk) &&
> + (local.family == AF_INET6 || local.port))) &&
> + skb && skb_is_tcp_pure_ack(skb)) {
> + pr_debug("drop other suboptions");
> + opts->suboptions = 0;
> + opts->ext_copy.use_ack = 0;
> + opts->ext_copy.use_map = 0;
> + remaining += opt_size;
> + drop_other_suboptions = true;
> + }
> +
> if (mptcp_pm_should_add_signal_echo(msk)) {
> - if (skb && skb_is_tcp_pure_ack(skb)) {
>
>
> >
> >>>
> >>>> + }
> >>>> + len = mptcp_add_addr_len(local.family, false, !!local.port);
> >>>> + if (remaining < len)
> >>>> + return false;
> > And here, I think "remaining -= len;" is missing.
> >
> > Thanks,
> > -Geliang
> >
> "remaining" is not being used in the flowing code. So "remaining -=len;" is not necessary. But you remindme that the "remaining -= len;" can be removed in the first trunk.
I think we should keep this 'remaining -= len;', remaining can be used
in tcp_established_options.
>
> I will send v5 as your advice.
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-21 7:39 ` Geliang Tang
@ 2021-06-21 7:49 ` Yonglong Li
2021-06-21 8:06 ` Geliang Tang
0 siblings, 1 reply; 15+ messages in thread
From: Yonglong Li @ 2021-06-21 7:49 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp, Mat Martineau, qitiepeng
On 2021/6/21 15:39, Geliang Tang wrote:
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 下午3:16写道：
>>
>>
>>
>> On 2021/6/21 14:42, Geliang Tang wrote:
>>> Hi Yonglong,
>>>
>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 上午11:52写道：
>>>>
>>>>
>>>> On 2021/6/18 19:20, Geliang Tang wrote:
>>>>> Hi Yonglong,
>>>>>
>>>>> Thanks for v4!
>>>>>
>>>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
>>>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
>>>>>> ADD_ADDR/echo-ADD_ADDR option
>>>>>>
>>>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>>>>>>
>>>>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
>>>>>> ---
>>>>>> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
>>>>>> net/mptcp/pm.c | 30 ++++---------
>>>>>> net/mptcp/protocol.h | 13 +++---
>>>>>> 3 files changed, 92 insertions(+), 75 deletions(-)
>>>>>>
>>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>>>> index 1aec016..43e3241 100644
>>>>>> --- a/net/mptcp/options.c
>>>>>> +++ b/net/mptcp/options.c
>>>>>> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>>>>>> bool drop_other_suboptions = false;
>>>>>> unsigned int opt_size = *size;
>>>>>> - bool echo;
>>>>>> - bool port;
>>>>>> + struct mptcp_addr_info remote;
>>>>>> + struct mptcp_addr_info local;
>>>>>> + u8 add_addr, flags = 0xff;
>>>>>> int len;
>>>>>>
>>>>>> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
>>>>>> - mptcp_pm_should_add_signal_port(msk) ||
>>>>>> - mptcp_pm_should_add_signal_echo(msk)) &&
>>>>>> - skb && skb_is_tcp_pure_ack(skb)) {
>>>>>> - pr_debug("drop other suboptions");
>>>>>> - opts->suboptions = 0;
>>>>>> - opts->ext_copy.use_ack = 0;
>>>>>> - opts->ext_copy.use_map = 0;
>>>>>> - remaining += opt_size;
>>>>>> - drop_other_suboptions = true;
>>>>>> - }
>>>>>> -
>>>>>> - if (!mptcp_pm_should_add_signal(msk) ||
>>>>>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
>>>>>> - return false;
>>>>>> -
>>>>>> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
>>>>>> - if (remaining < len)
>>>>>> + if (!mptcp_pm_should_add_signal(msk))
>>>>>> return false;
>>>>>>
>>>>>> - *size = len;
>>>>>> - if (drop_other_suboptions)
>>>>>> - *size -= opt_size;
>>>>>> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>>>>>> - if (!echo) {
>>>>>> + *size = 0;
>>>>>> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
>>>>>> + if (mptcp_pm_should_add_signal_echo(msk)) {
>>>>>> + if (skb && skb_is_tcp_pure_ack(skb)) {
>>>>> '''
>>>>>> + pr_debug("drop other suboptions");
>>>>>> + opts->suboptions = 0;
>>>>>> + opts->ext_copy.use_ack = 0;
>>>>>> + opts->ext_copy.use_map = 0;
>>>>>> + remaining += opt_size;
>>>>>> + drop_other_suboptions = true;
>>>>> '''
>>>>>
>>>>>> + }
>>>>>> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
>>>>>> + if (remaining < len)
>>>>>> + return false;
>>>>>> + remaining -= len;
>>>>>> + *size += len;
>>>>>> + opts->remote = remote;
>>>>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
>>>>>> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
>>>>>> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
>>>>>> + opts->remote.id, ntohs(opts->remote.port), add_addr);
>>>>>> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
>>>>>> + if ((local.family == AF_INET6 || local.port) && skb &&
>>>>>> + skb_is_tcp_pure_ack(skb)) {
>>>>> '''
>>>>>> + pr_debug("drop other suboptions");
>>>>>> + opts->suboptions = 0;
>>>>>> + opts->ext_copy.use_ack = 0;
>>>>>> + opts->ext_copy.use_map = 0;
>>>>>> + remaining += opt_size;
>>>>>> + drop_other_suboptions = true;
>>>>> '''
>>>>>
>>>>> I think this "drop other suboptions" trunk here is still duplicated. Can
>>>>> we just use one "drop other suboptions" trunk only?
>>>>>
>>>>> Thanks.
>>>>> -Geliang
>>>>>
>>>> Hi Geliang, Thanks for you replay.
>>>>
>>>> The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
>>>> carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
>>>> only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
>>>> IP version."
>>>> ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
>>>> to decide "drop other suboptions" in two trunk.
>>> Could we change it like this:
>>>
>>> '''
>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>> index e77b5d532fb8..8b4cb0581a49 100644
>>> --- a/net/mptcp/options.c
>>> +++ b/net/mptcp/options.c
>>> @@ -673,15 +673,20 @@ static bool
>>> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>>
>>> *size = 0;
>>> mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
>>> +
>>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
>>> + (mptcp_pm_should_add_signal_addr(msk) &&
>>> + (local.family == AF_INET6 || local.port))) &&
>>> + skb && skb_is_tcp_pure_ack(skb)) {
>>> + pr_debug("drop other suboptions");
>>> + opts->suboptions = 0;
>>> + opts->ext_copy.use_ack = 0;
>>> + opts->ext_copy.use_map = 0;
>>> + remaining += opt_size;
>>> + drop_other_suboptions = true;
>>> + }
>>> +
>>> if (mptcp_pm_should_add_signal_echo(msk)) {
>>> - if (skb && skb_is_tcp_pure_ack(skb)) {
>>> - pr_debug("drop other suboptions");
>>> - opts->suboptions = 0;
>>> - opts->ext_copy.use_ack = 0;
>>> - opts->ext_copy.use_map = 0;
>>> - remaining += opt_size;
>>> - drop_other_suboptions = true;
>>> - }
>>> len = mptcp_add_addr_len(remote.family, true, !!remote.port);
>>> if (remaining < len)
>>> return false;
>>> @@ -693,15 +698,6 @@ static bool
>>> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>>> pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
>>> opts->remote.id, ntohs(opts->remote.port), add_addr);
>>> } else if (mptcp_pm_should_add_signal_addr(msk)) {
>>> - if ((local.family == AF_INET6 || local.port) && skb &&
>>> - skb_is_tcp_pure_ack(skb)) {
>>> - pr_debug("drop other suboptions");
>>> - opts->suboptions = 0;
>>> - opts->ext_copy.use_ack = 0;
>>> - opts->ext_copy.use_map = 0;
>>> - remaining += opt_size;
>>> - drop_other_suboptions = true;
>>> - }
>>> len = mptcp_add_addr_len(local.family, false, !!local.port);
>>> if (remaining < len)
>>> return false;
>>> '''
>>> WDYT?
>> Thanks for your advice.
>>
>> Because MPTCP_ADD_ADDR_ECHO and MPTCP_ADD_ADDR_SIGNAL can be set at the same time. So as your advice we should
>> change like this(still I think it not clear than before):
>>
>> mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
>> + (!mptcp_pm_should_add_signal_echo(msk) &&
>> + mptcp_pm_should_add_signal_addr(msk) &&
>> + (local.family == AF_INET6 || local.port))) &&
>> + skb && skb_is_tcp_pure_ack(skb)) {
>> + pr_debug("drop other suboptions");
>> + opts->suboptions = 0;
>> + opts->ext_copy.use_ack = 0;
>> + opts->ext_copy.use_map = 0;
>> + remaining += opt_size;
>> + drop_other_suboptions = true;
>> + }
>> +
>> if (mptcp_pm_should_add_signal_echo(msk)) {
>> - if (skb && skb_is_tcp_pure_ack(skb)) {
>>
>>
>>>
>>>>>
>>>>>> + }
>>>>>> + len = mptcp_add_addr_len(local.family, false, !!local.port);
>>>>>> + if (remaining < len)
>>>>>> + return false;
>>> And here, I think "remaining -= len;" is missing.
>>>
>>> Thanks,
>>> -Geliang
>>>
>> "remaining" is not being used in the flowing code. So "remaining -=len;" is not necessary. But you remindme that the "remaining -= len;" can be removed in the first trunk.
>
> I think we should keep this 'remaining -= len;', remaining can be used
> in tcp_established_options.
>
Thanks for your review.
I think "remaining" will not use in tcp_established_options. "size" is used by tcp_established_options.
>>
>> I will send v5 as your advice.
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-21 7:49 ` Yonglong Li
@ 2021-06-21 8:06 ` Geliang Tang
0 siblings, 0 replies; 15+ messages in thread
From: Geliang Tang @ 2021-06-21 8:06 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 下午3:50写道：
>
>
>
> On 2021/6/21 15:39, Geliang Tang wrote:
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 下午3:16写道：
> >>
> >>
> >>
> >> On 2021/6/21 14:42, Geliang Tang wrote:
> >>> Hi Yonglong,
> >>>
> >>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月21日周一 上午11:52写道：
> >>>>
> >>>>
> >>>> On 2021/6/18 19:20, Geliang Tang wrote:
> >>>>> Hi Yonglong,
> >>>>>
> >>>>> Thanks for v4!
> >>>>>
> >>>>> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
> >>>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> >>>>>> ADD_ADDR/echo-ADD_ADDR option
> >>>>>>
> >>>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
> >>>>>>
> >>>>>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> >>>>>> ---
> >>>>>> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
> >>>>>> net/mptcp/pm.c | 30 ++++---------
> >>>>>> net/mptcp/protocol.h | 13 +++---
> >>>>>> 3 files changed, 92 insertions(+), 75 deletions(-)
> >>>>>>
> >>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>>>>> index 1aec016..43e3241 100644
> >>>>>> --- a/net/mptcp/options.c
> >>>>>> +++ b/net/mptcp/options.c
> >>>>>> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >>>>>> bool drop_other_suboptions = false;
> >>>>>> unsigned int opt_size = *size;
> >>>>>> - bool echo;
> >>>>>> - bool port;
> >>>>>> + struct mptcp_addr_info remote;
> >>>>>> + struct mptcp_addr_info local;
> >>>>>> + u8 add_addr, flags = 0xff;
> >>>>>> int len;
> >>>>>>
> >>>>>> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> >>>>>> - mptcp_pm_should_add_signal_port(msk) ||
> >>>>>> - mptcp_pm_should_add_signal_echo(msk)) &&
> >>>>>> - skb && skb_is_tcp_pure_ack(skb)) {
> >>>>>> - pr_debug("drop other suboptions");
> >>>>>> - opts->suboptions = 0;
> >>>>>> - opts->ext_copy.use_ack = 0;
> >>>>>> - opts->ext_copy.use_map = 0;
> >>>>>> - remaining += opt_size;
> >>>>>> - drop_other_suboptions = true;
> >>>>>> - }
> >>>>>> -
> >>>>>> - if (!mptcp_pm_should_add_signal(msk) ||
> >>>>>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> >>>>>> - return false;
> >>>>>> -
> >>>>>> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> >>>>>> - if (remaining < len)
> >>>>>> + if (!mptcp_pm_should_add_signal(msk))
> >>>>>> return false;
> >>>>>>
> >>>>>> - *size = len;
> >>>>>> - if (drop_other_suboptions)
> >>>>>> - *size -= opt_size;
> >>>>>> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> >>>>>> - if (!echo) {
> >>>>>> + *size = 0;
> >>>>>> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> >>>>>> + if (mptcp_pm_should_add_signal_echo(msk)) {
> >>>>>> + if (skb && skb_is_tcp_pure_ack(skb)) {
> >>>>> '''
> >>>>>> + pr_debug("drop other suboptions");
> >>>>>> + opts->suboptions = 0;
> >>>>>> + opts->ext_copy.use_ack = 0;
> >>>>>> + opts->ext_copy.use_map = 0;
> >>>>>> + remaining += opt_size;
> >>>>>> + drop_other_suboptions = true;
> >>>>> '''
> >>>>>
> >>>>>> + }
> >>>>>> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> >>>>>> + if (remaining < len)
> >>>>>> + return false;
> >>>>>> + remaining -= len;
> >>>>>> + *size += len;
> >>>>>> + opts->remote = remote;
> >>>>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> >>>>>> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> >>>>>> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> >>>>>> + opts->remote.id, ntohs(opts->remote.port), add_addr);
> >>>>>> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
> >>>>>> + if ((local.family == AF_INET6 || local.port) && skb &&
> >>>>>> + skb_is_tcp_pure_ack(skb)) {
> >>>>> '''
> >>>>>> + pr_debug("drop other suboptions");
> >>>>>> + opts->suboptions = 0;
> >>>>>> + opts->ext_copy.use_ack = 0;
> >>>>>> + opts->ext_copy.use_map = 0;
> >>>>>> + remaining += opt_size;
> >>>>>> + drop_other_suboptions = true;
> >>>>> '''
> >>>>>
> >>>>> I think this "drop other suboptions" trunk here is still duplicated. Can
> >>>>> we just use one "drop other suboptions" trunk only?
> >>>>>
> >>>>> Thanks.
> >>>>> -Geliang
> >>>>>
> >>>> Hi Geliang, Thanks for you replay.
> >>>>
> >>>> The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR
> >>>> carried over pure TCP ACKs, so there is no need to add a DSS element that would fit
> >>>> only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the
> >>>> IP version."
> >>>> ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear
> >>>> to decide "drop other suboptions" in two trunk.
> >>> Could we change it like this:
> >>>
> >>> '''
> >>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >>> index e77b5d532fb8..8b4cb0581a49 100644
> >>> --- a/net/mptcp/options.c
> >>> +++ b/net/mptcp/options.c
> >>> @@ -673,15 +673,20 @@ static bool
> >>> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>>
> >>> *size = 0;
> >>> mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> >>> +
> >>> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> >>> + (mptcp_pm_should_add_signal_addr(msk) &&
> >>> + (local.family == AF_INET6 || local.port))) &&
> >>> + skb && skb_is_tcp_pure_ack(skb)) {
> >>> + pr_debug("drop other suboptions");
> >>> + opts->suboptions = 0;
> >>> + opts->ext_copy.use_ack = 0;
> >>> + opts->ext_copy.use_map = 0;
> >>> + remaining += opt_size;
> >>> + drop_other_suboptions = true;
> >>> + }
> >>> +
> >>> if (mptcp_pm_should_add_signal_echo(msk)) {
> >>> - if (skb && skb_is_tcp_pure_ack(skb)) {
> >>> - pr_debug("drop other suboptions");
> >>> - opts->suboptions = 0;
> >>> - opts->ext_copy.use_ack = 0;
> >>> - opts->ext_copy.use_map = 0;
> >>> - remaining += opt_size;
> >>> - drop_other_suboptions = true;
> >>> - }
> >>> len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> >>> if (remaining < len)
> >>> return false;
> >>> @@ -693,15 +698,6 @@ static bool
> >>> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >>> pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> >>> opts->remote.id, ntohs(opts->remote.port), add_addr);
> >>> } else if (mptcp_pm_should_add_signal_addr(msk)) {
> >>> - if ((local.family == AF_INET6 || local.port) && skb &&
> >>> - skb_is_tcp_pure_ack(skb)) {
> >>> - pr_debug("drop other suboptions");
> >>> - opts->suboptions = 0;
> >>> - opts->ext_copy.use_ack = 0;
> >>> - opts->ext_copy.use_map = 0;
> >>> - remaining += opt_size;
> >>> - drop_other_suboptions = true;
> >>> - }
> >>> len = mptcp_add_addr_len(local.family, false, !!local.port);
> >>> if (remaining < len)
> >>> return false;
> >>> '''
> >>> WDYT?
> >> Thanks for your advice.
> >>
> >> Because MPTCP_ADD_ADDR_ECHO and MPTCP_ADD_ADDR_SIGNAL can be set at the same time. So as your advice we should
> >> change like this(still I think it not clear than before):
> >>
> >> mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> >> + if ((mptcp_pm_should_add_signal_echo(msk) ||
> >> + (!mptcp_pm_should_add_signal_echo(msk) &&
> >> + mptcp_pm_should_add_signal_addr(msk) &&
> >> + (local.family == AF_INET6 || local.port))) &&
> >> + skb && skb_is_tcp_pure_ack(skb)) {
> >> + pr_debug("drop other suboptions");
> >> + opts->suboptions = 0;
> >> + opts->ext_copy.use_ack = 0;
> >> + opts->ext_copy.use_map = 0;
> >> + remaining += opt_size;
> >> + drop_other_suboptions = true;
> >> + }
> >> +
> >> if (mptcp_pm_should_add_signal_echo(msk)) {
> >> - if (skb && skb_is_tcp_pure_ack(skb)) {
> >>
> >>
> >>>
> >>>>>
> >>>>>> + }
> >>>>>> + len = mptcp_add_addr_len(local.family, false, !!local.port);
> >>>>>> + if (remaining < len)
> >>>>>> + return false;
> >>> And here, I think "remaining -= len;" is missing.
> >>>
> >>> Thanks,
> >>> -Geliang
> >>>
> >> "remaining" is not being used in the flowing code. So "remaining -=len;" is not necessary. But you remindme that the "remaining -= len;" can be removed in the first trunk.
> >
> > I think we should keep this 'remaining -= len;', remaining can be used
> > in tcp_established_options.
> >
> Thanks for your review.
> I think "remaining" will not use in tcp_established_options. "size" is used by tcp_established_options.
You're right, we should drop this 'remaining -= len;' in this function.
>
> >>
> >> I will send v5 as your advice.
> >>
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-18 8:18 ` [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
2021-06-18 11:20 ` Geliang Tang
@ 2021-06-21 7:42 ` Geliang Tang
2021-06-21 7:51 ` Yonglong Li
2021-06-21 8:29 ` Geliang Tang
2 siblings, 1 reply; 15+ messages in thread
From: Geliang Tang @ 2021-06-21 7:42 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
>
> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> ADD_ADDR/echo-ADD_ADDR option
>
> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>
> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> ---
> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
> net/mptcp/pm.c | 30 ++++---------
> net/mptcp/protocol.h | 13 +++---
> 3 files changed, 92 insertions(+), 75 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..43e3241 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> bool drop_other_suboptions = false;
> unsigned int opt_size = *size;
> - bool echo;
> - bool port;
> + struct mptcp_addr_info remote;
> + struct mptcp_addr_info local;
> + u8 add_addr, flags = 0xff;
> int len;
>
> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> - mptcp_pm_should_add_signal_port(msk) ||
> - mptcp_pm_should_add_signal_echo(msk)) &&
> - skb && skb_is_tcp_pure_ack(skb)) {
> - pr_debug("drop other suboptions");
> - opts->suboptions = 0;
> - opts->ext_copy.use_ack = 0;
> - opts->ext_copy.use_map = 0;
> - remaining += opt_size;
> - drop_other_suboptions = true;
> - }
> -
> - if (!mptcp_pm_should_add_signal(msk) ||
> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> - return false;
> -
> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> - if (remaining < len)
> + if (!mptcp_pm_should_add_signal(msk))
> return false;
>
> - *size = len;
> - if (drop_other_suboptions)
> - *size -= opt_size;
> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> - if (!echo) {
> + *size = 0;
> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> + if (mptcp_pm_should_add_signal_echo(msk)) {
> + if (skb && skb_is_tcp_pure_ack(skb)) {
> + pr_debug("drop other suboptions");
> + opts->suboptions = 0;
> + opts->ext_copy.use_ack = 0;
> + opts->ext_copy.use_map = 0;
> + remaining += opt_size;
> + drop_other_suboptions = true;
> + }
> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> + if (remaining < len)
> + return false;
> + remaining -= len;
> + *size += len;
> + opts->remote = remote;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> + opts->remote.id, ntohs(opts->remote.port), add_addr);
> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
> + if ((local.family == AF_INET6 || local.port) && skb &&
> + skb_is_tcp_pure_ack(skb)) {
> + pr_debug("drop other suboptions");
> + opts->suboptions = 0;
> + opts->ext_copy.use_ack = 0;
> + opts->ext_copy.use_map = 0;
> + remaining += opt_size;
> + drop_other_suboptions = true;
> + }
> + len = mptcp_add_addr_len(local.family, false, !!local.port);
> + if (remaining < len)
> + return false;
> + *size += len;
> + opts->addr = local;
Could we rename this struct member addr in struct mptcp_out_options to
local?
> opts->ahmac = add_addr_generate_hmac(msk->local_key,
> msk->remote_key,
> &opts->addr);
> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> + pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
> + opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
Could we merge these two debug logs into one and move it at the the end
of this function, before 'return true'?
-Geliang
> }
> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> +
> + if (drop_other_suboptions)
> + *size -= opt_size;
> + spin_lock_bh(&msk->pm.lock);
> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> + spin_unlock_bh(&msk->pm.lock);
>
> return true;
> }
> @@ -1228,45 +1251,51 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> }
>
> mp_capable_done:
> - if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> - u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> - u8 echo = MPTCP_ADDR_ECHO;
> + if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
> + struct mptcp_addr_info *addr_info;
> + u8 len = 0;
> + u8 echo = 0;
> +
> + if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> + len += sizeof(opts->ahmac);
> + addr_info = &opts->addr;
> + } else {
> + echo = MPTCP_ADDR_ECHO;
> + addr_info = &opts->remote;
> + }
>
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> - if (opts->addr.family == AF_INET6)
> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + if (addr_info->family == AF_INET6)
> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + else
> #endif
> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>
> - if (opts->addr.port)
> + if (addr_info->port)
> len += TCPOLEN_MPTCP_PORT_LEN;
>
> - if (opts->ahmac) {
> - len += sizeof(opts->ahmac);
> - echo = 0;
> - }
> -
> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> - len, echo, opts->addr.id);
> - if (opts->addr.family == AF_INET) {
> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> + len, echo, addr_info->id);
> + if (addr_info->family == AF_INET) {
> + memcpy((u8 *)ptr, (u8 *)&addr_info->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_info->family == AF_INET6) {
> + memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
> ptr += 4;
> }
> #endif
>
> - if (!opts->addr.port) {
> - if (opts->ahmac) {
> + if (!addr_info->port) {
> + if (!echo) {
> put_unaligned_be64(opts->ahmac, ptr);
> ptr += 2;
> }
> } else {
> - u16 port = ntohs(opts->addr.port);
> + u16 port = ntohs(addr_info->port);
>
> - if (opts->ahmac) {
> + if (!echo) {
> u8 *bptr = (u8 *)ptr;
>
> put_unaligned_be16(port, bptr);
> @@ -1275,7 +1304,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> bptr += 8;
> put_unaligned_be16(TCPOPT_NOP << 8 |
> TCPOPT_NOP, bptr);
> -
> ptr += 3;
> } else {
> put_unaligned_be32(port << 16 |
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 107a5a2..a62d4a5 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -22,7 +22,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
>
> lockdep_assert_held(&msk->pm.lock);
>
> - if (add_addr) {
> + if (add_addr &
> + (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> pr_warn("addr_signal error, add_addr=%d", add_addr);
> return -EINVAL;
> }
> @@ -252,32 +253,19 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>
> /* path manager helpers */
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> - struct mptcp_addr_info *saddr, bool *echo, bool *port)
> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> + struct mptcp_addr_info *daddr, u8 *add_addr)
> {
> - u8 add_addr;
> - int ret = false;
> -
> spin_lock_bh(&msk->pm.lock);
>
> - /* double check after the lock is acquired */
> - if (!mptcp_pm_should_add_signal(msk))
> - goto out_unlock;
> -
> - *echo = mptcp_pm_should_add_signal_echo(msk);
> - *port = mptcp_pm_should_add_signal_port(msk);
> -
> - if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> - goto out_unlock;
> -
> *saddr = msk->pm.local;
> - add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> - WRITE_ONCE(msk->pm.addr_signal, add_addr);
> - ret = true;
> + *daddr = msk->pm.remote;
> + *add_addr = msk->pm.addr_signal;
>
> -out_unlock:
> spin_unlock_bh(&msk->pm.lock);
> - return ret;
> +
> + if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> + mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> }
>
> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index a0b0ec0..90fb532 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -22,10 +22,11 @@
> #define OPTION_MPTCP_MPJ_SYNACK BIT(4)
> #define OPTION_MPTCP_MPJ_ACK BIT(5)
> #define OPTION_MPTCP_ADD_ADDR BIT(6)
> -#define OPTION_MPTCP_RM_ADDR BIT(7)
> -#define OPTION_MPTCP_FASTCLOSE BIT(8)
> -#define OPTION_MPTCP_PRIO BIT(9)
> -#define OPTION_MPTCP_RST BIT(10)
> +#define OPTION_MPTCP_ADD_ECHO BIT(7)
> +#define OPTION_MPTCP_RM_ADDR BIT(8)
> +#define OPTION_MPTCP_FASTCLOSE BIT(9)
> +#define OPTION_MPTCP_PRIO BIT(10)
> +#define OPTION_MPTCP_RST BIT(11)
>
> /* MPTCP option subtypes */
> #define MPTCPOPT_MP_CAPABLE 0
> @@ -760,8 +761,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
> return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
> }
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> - struct mptcp_addr_info *saddr, bool *echo, bool *port);
> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> + struct mptcp_addr_info *daddr, u8 *add_addr);
> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> struct mptcp_rm_list *rm_list);
> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-21 7:42 ` Geliang Tang
@ 2021-06-21 7:51 ` Yonglong Li
0 siblings, 0 replies; 15+ messages in thread
From: Yonglong Li @ 2021-06-21 7:51 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp, Mat Martineau
On 2021/6/21 15:42, Geliang Tang wrote:
>> + }
>> + len = mptcp_add_addr_len(local.family, false, !!local.port);
>> + if (remaining < len)
>> + return false;
>> + *size += len;
>> + opts->addr = local;
> Could we rename this struct member addr in struct mptcp_out_options to
> local?
>
>> opts->ahmac = add_addr_generate_hmac(msk->local_key,
>> msk->remote_key,
>> &opts->addr);
>> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
>> + pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
>> + opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
> Could we merge these two debug logs into one and move it at the the end
> of this function, before 'return true'?
>
> -Geliang
>
Thanks for your review.
I will change them in v5 as your advice.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
2021-06-18 8:18 ` [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
2021-06-18 11:20 ` Geliang Tang
2021-06-21 7:42 ` Geliang Tang
@ 2021-06-21 8:29 ` Geliang Tang
2 siblings, 0 replies; 15+ messages in thread
From: Geliang Tang @ 2021-06-21 8:29 UTC (permalink / raw)
To: Yonglong Li; +Cc: mptcp, Mat Martineau, qitiepeng
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月18日周五 下午4:19写道：
>
> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build
> ADD_ADDR/echo-ADD_ADDR option
>
> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option
>
> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> ---
> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++--------------------
> net/mptcp/pm.c | 30 ++++---------
> net/mptcp/protocol.h | 13 +++---
> 3 files changed, 92 insertions(+), 75 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec016..43e3241 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> bool drop_other_suboptions = false;
> unsigned int opt_size = *size;
> - bool echo;
> - bool port;
> + struct mptcp_addr_info remote;
> + struct mptcp_addr_info local;
> + u8 add_addr, flags = 0xff;
> int len;
>
> - if ((mptcp_pm_should_add_signal_ipv6(msk) ||
> - mptcp_pm_should_add_signal_port(msk) ||
> - mptcp_pm_should_add_signal_echo(msk)) &&
> - skb && skb_is_tcp_pure_ack(skb)) {
> - pr_debug("drop other suboptions");
> - opts->suboptions = 0;
> - opts->ext_copy.use_ack = 0;
> - opts->ext_copy.use_map = 0;
> - remaining += opt_size;
> - drop_other_suboptions = true;
> - }
> -
> - if (!mptcp_pm_should_add_signal(msk) ||
> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port)))
> - return false;
> -
> - len = mptcp_add_addr_len(opts->addr.family, echo, port);
> - if (remaining < len)
> + if (!mptcp_pm_should_add_signal(msk))
> return false;
>
> - *size = len;
> - if (drop_other_suboptions)
> - *size -= opt_size;
> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> - if (!echo) {
> + *size = 0;
> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr);
> + if (mptcp_pm_should_add_signal_echo(msk)) {
> + if (skb && skb_is_tcp_pure_ack(skb)) {
> + pr_debug("drop other suboptions");
> + opts->suboptions = 0;
> + opts->ext_copy.use_ack = 0;
> + opts->ext_copy.use_map = 0;
> + remaining += opt_size;
> + drop_other_suboptions = true;
> + }
> + len = mptcp_add_addr_len(remote.family, true, !!remote.port);
> + if (remaining < len)
> + return false;
> + remaining -= len;
> + *size += len;
Could we drop the above '*size = 0', change this line to "*size = len;",
and move it out of the if... else... trunk, just like the original code:
*size = len;
if (drop_other_suboptions)
*size -= opt_size;
> + opts->remote = remote;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO);
> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO;
> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x",
> + opts->remote.id, ntohs(opts->remote.port), add_addr);
> + } else if (mptcp_pm_should_add_signal_addr(msk)) {
Since we called mptcp_pm_should_add_signal before, could we just use
'else' here?
-Geliang
> + if ((local.family == AF_INET6 || local.port) && skb &&
> + skb_is_tcp_pure_ack(skb)) {
> + pr_debug("drop other suboptions");
> + opts->suboptions = 0;
> + opts->ext_copy.use_ack = 0;
> + opts->ext_copy.use_map = 0;
> + remaining += opt_size;
> + drop_other_suboptions = true;
> + }
> + len = mptcp_add_addr_len(local.family, false, !!local.port);
> + if (remaining < len)
> + return false;
> + *size += len;
> + opts->addr = local;
> opts->ahmac = add_addr_generate_hmac(msk->local_key,
> msk->remote_key,
> &opts->addr);
> + opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> + flags = (u8)~BIT(MPTCP_ADD_ADDR_SIGNAL);
> + pr_debug("addr_id=%d, ahmac=%llu, echo=0, port=%d, addr_signal:%x",
> + opts->addr.id, opts->ahmac, ntohs(opts->addr.port), add_addr);
> }
> - pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
> - opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> +
> + if (drop_other_suboptions)
> + *size -= opt_size;
> + spin_lock_bh(&msk->pm.lock);
> + WRITE_ONCE(msk->pm.addr_signal, flags & msk->pm.addr_signal);
> + spin_unlock_bh(&msk->pm.lock);
>
> return true;
> }
> @@ -1228,45 +1251,51 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> }
>
> mp_capable_done:
> - if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> - u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> - u8 echo = MPTCP_ADDR_ECHO;
> + if ((OPTION_MPTCP_ADD_ADDR | OPTION_MPTCP_ADD_ECHO) & opts->suboptions) {
> + struct mptcp_addr_info *addr_info;
> + u8 len = 0;
> + u8 echo = 0;
> +
> + if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> + len += sizeof(opts->ahmac);
> + addr_info = &opts->addr;
> + } else {
> + echo = MPTCP_ADDR_ECHO;
> + addr_info = &opts->remote;
> + }
>
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> - if (opts->addr.family == AF_INET6)
> - len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + if (addr_info->family == AF_INET6)
> + len += TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + else
> #endif
> + len += TCPOLEN_MPTCP_ADD_ADDR_BASE;
>
> - if (opts->addr.port)
> + if (addr_info->port)
> len += TCPOLEN_MPTCP_PORT_LEN;
>
> - if (opts->ahmac) {
> - len += sizeof(opts->ahmac);
> - echo = 0;
> - }
> -
> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> - len, echo, opts->addr.id);
> - if (opts->addr.family == AF_INET) {
> - memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4);
> + len, echo, addr_info->id);
> + if (addr_info->family == AF_INET) {
> + memcpy((u8 *)ptr, (u8 *)&addr_info->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_info->family == AF_INET6) {
> + memcpy((u8 *)ptr, addr_info->addr6.s6_addr, 16);
> ptr += 4;
> }
> #endif
>
> - if (!opts->addr.port) {
> - if (opts->ahmac) {
> + if (!addr_info->port) {
> + if (!echo) {
> put_unaligned_be64(opts->ahmac, ptr);
> ptr += 2;
> }
> } else {
> - u16 port = ntohs(opts->addr.port);
> + u16 port = ntohs(addr_info->port);
>
> - if (opts->ahmac) {
> + if (!echo) {
> u8 *bptr = (u8 *)ptr;
>
> put_unaligned_be16(port, bptr);
> @@ -1275,7 +1304,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> bptr += 8;
> put_unaligned_be16(TCPOPT_NOP << 8 |
> TCPOPT_NOP, bptr);
> -
> ptr += 3;
> } else {
> put_unaligned_be32(port << 16 |
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 107a5a2..a62d4a5 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -22,7 +22,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
>
> lockdep_assert_held(&msk->pm.lock);
>
> - if (add_addr) {
> + if (add_addr &
> + (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> pr_warn("addr_signal error, add_addr=%d", add_addr);
> return -EINVAL;
> }
> @@ -252,32 +253,19 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
>
> /* path manager helpers */
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> - struct mptcp_addr_info *saddr, bool *echo, bool *port)
> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> + struct mptcp_addr_info *daddr, u8 *add_addr)
> {
> - u8 add_addr;
> - int ret = false;
> -
> spin_lock_bh(&msk->pm.lock);
>
> - /* double check after the lock is acquired */
> - if (!mptcp_pm_should_add_signal(msk))
> - goto out_unlock;
> -
> - *echo = mptcp_pm_should_add_signal_echo(msk);
> - *port = mptcp_pm_should_add_signal_port(msk);
> -
> - if (remaining < mptcp_add_addr_len(msk->pm.local.family, *echo, *port))
> - goto out_unlock;
> -
> *saddr = msk->pm.local;
> - add_addr = msk->pm.addr_signal & ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO));
> - WRITE_ONCE(msk->pm.addr_signal, add_addr);
> - ret = true;
> + *daddr = msk->pm.remote;
> + *add_addr = msk->pm.addr_signal;
>
> -out_unlock:
> spin_unlock_bh(&msk->pm.lock);
> - return ret;
> +
> + if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> + mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> }
>
> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index a0b0ec0..90fb532 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -22,10 +22,11 @@
> #define OPTION_MPTCP_MPJ_SYNACK BIT(4)
> #define OPTION_MPTCP_MPJ_ACK BIT(5)
> #define OPTION_MPTCP_ADD_ADDR BIT(6)
> -#define OPTION_MPTCP_RM_ADDR BIT(7)
> -#define OPTION_MPTCP_FASTCLOSE BIT(8)
> -#define OPTION_MPTCP_PRIO BIT(9)
> -#define OPTION_MPTCP_RST BIT(10)
> +#define OPTION_MPTCP_ADD_ECHO BIT(7)
> +#define OPTION_MPTCP_RM_ADDR BIT(8)
> +#define OPTION_MPTCP_FASTCLOSE BIT(9)
> +#define OPTION_MPTCP_PRIO BIT(10)
> +#define OPTION_MPTCP_RST BIT(11)
>
> /* MPTCP option subtypes */
> #define MPTCPOPT_MP_CAPABLE 0
> @@ -760,8 +761,8 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
> return TCPOLEN_MPTCP_RM_ADDR_BASE + roundup(rm_list->nr - 1, 4) + 1;
> }
>
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> - struct mptcp_addr_info *saddr, bool *echo, bool *port);
> +void mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct mptcp_addr_info *saddr,
> + struct mptcp_addr_info *daddr, u8 *add_addr);
> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> struct mptcp_rm_list *rm_list);
> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread