| Message ID | 48a821dc73542f7d4729015eb4c10ad95370dfb1.1615966219.git.geliangtang@gmail.com |
|---|---|
| State | Accepted, archived |
| Commit | adbca536f2e09ca691b349d26e9e462c70df0160 |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | refactor mptcp_addr_info and cleanups | expand |
On Wed, 17 Mar 2021, Geliang Tang wrote: > This patch moved the mptcp_addr_info struct from protocol.h to mptcp.h, > added a new struct mptcp_addr_info member addr in struct mptcp_out_options, > and dropped the original addr, addr6, addr_id and port fields in it. Then > we can use opts->addr to get the adding address from PM directly using > mptcp_pm_add_addr_signal. > > Since the port number became as a big-endian order now, use ntohs to > convert it before printing it out. > > Signed-off-by: Geliang Tang <geliangtang@gmail.com> > --- > include/net/mptcp.h | 21 +++++++++++++-------- > net/mptcp/options.c | 42 ++++++++++++++++++------------------------ > net/mptcp/protocol.h | 12 ------------ > 3 files changed, 31 insertions(+), 44 deletions(-) > > diff --git a/include/net/mptcp.h b/include/net/mptcp.h > index 16fe34d139c3..80d98a7db3c6 100644 > --- a/include/net/mptcp.h > +++ b/include/net/mptcp.h > @@ -41,20 +41,25 @@ struct mptcp_rm_list { > u8 nr; > }; > > +struct mptcp_addr_info { > + u8 id; > + sa_family_t family : 4; It's unusual to use a bitfield size specifier with a special type like sa_family_t. This patch moves the mptcp_addr_info struct from protocol.h to mptcp.h, and the deleted struct below does not have the " : 4" for the family. Was this intentional? While AF_INET and AF_INET6 do fit in 4 bits, and AF_MAX is only 45, I think this code should either use a bitfield to represent IPv4/v6, or use a whole sa_family_t. Maybe this is why MPTCP_ADDR_IPVERSION_? values were defined. Mat > + __be16 port; > + union { > + struct in_addr addr; > +#if IS_ENABLED(CONFIG_MPTCP_IPV6) > + struct in6_addr addr6; > +#endif > + }; > +}; > + > struct mptcp_out_options { > #if IS_ENABLED(CONFIG_MPTCP) > u16 suboptions; > u64 sndr_key; > u64 rcvr_key; > - union { > - struct in_addr addr; > -#if IS_ENABLED(CONFIG_MPTCP_IPV6) > - struct in6_addr addr6; > -#endif > - }; > - u8 addr_id; > - u16 port; > u64 ahmac; > + struct mptcp_addr_info addr; > struct mptcp_rm_list rm_list; > u8 join_id; > u8 backup; > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > index 4b7119eb2c31..7e01f44ed885 100644 > --- a/net/mptcp/options.c > +++ b/net/mptcp/options.c > @@ -626,7 +626,6 @@ 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; > - struct mptcp_addr_info saddr; > bool echo; > bool port; > int len; > @@ -643,45 +642,40 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff * > } > > if (!mptcp_pm_should_add_signal(msk) || > - !(mptcp_pm_add_addr_signal(msk, remaining, &saddr, &echo, &port))) > + !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port))) > return false; > > - len = mptcp_add_addr_len(saddr.family, echo, port); > + len = mptcp_add_addr_len(opts->addr.family, echo, port); > if (remaining < len) > return false; > > *size = len; > if (drop_other_suboptions) > *size -= opt_size; > - opts->addr_id = saddr.id; > - if (port) > - opts->port = ntohs(saddr.port); > - if (saddr.family == AF_INET) { > + if (opts->addr.family == AF_INET) { > opts->suboptions |= OPTION_MPTCP_ADD_ADDR; > - opts->addr = saddr.addr; > if (!echo) { > opts->ahmac = add_addr_generate_hmac(msk->local_key, > msk->remote_key, > - opts->addr_id, > - &opts->addr, > - opts->port); > + opts->addr.id, > + &opts->addr.addr, > + opts->addr.port); > } > } > #if IS_ENABLED(CONFIG_MPTCP_IPV6) > - else if (saddr.family == AF_INET6) { > + else if (opts->addr.family == AF_INET6) { > opts->suboptions |= OPTION_MPTCP_ADD_ADDR6; > - opts->addr6 = saddr.addr6; > if (!echo) { > opts->ahmac = add_addr6_generate_hmac(msk->local_key, > msk->remote_key, > - opts->addr_id, > - &opts->addr6, > - opts->port); > + opts->addr.id, > + &opts->addr.addr6, > + opts->addr.port); > } > } > #endif > pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d", > - opts->addr_id, opts->ahmac, echo, opts->port); > + opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port)); > > return true; > } > @@ -1217,7 +1211,7 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, > len = TCPOLEN_MPTCP_ADD_ADDR6_BASE; > #endif > > - if (opts->port) > + if (opts->addr.port) > len += TCPOLEN_MPTCP_PORT_LEN; > > if (opts->ahmac) { > @@ -1226,19 +1220,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, > } > > *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR, > - len, echo, opts->addr_id); > + len, echo, opts->addr.id); > if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) { > - memcpy((u8 *)ptr, (u8 *)&opts->addr.s_addr, 4); > + memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4); > ptr += 1; > } > #if IS_ENABLED(CONFIG_MPTCP_IPV6) > else if (OPTION_MPTCP_ADD_ADDR6 & opts->suboptions) { > - memcpy((u8 *)ptr, opts->addr6.s6_addr, 16); > + memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16); > ptr += 4; > } > #endif > > - if (!opts->port) { > + if (!opts->addr.port) { > if (opts->ahmac) { > put_unaligned_be64(opts->ahmac, ptr); > ptr += 2; > @@ -1247,7 +1241,7 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, > if (opts->ahmac) { > u8 *bptr = (u8 *)ptr; > > - put_unaligned_be16(opts->port, bptr); > + put_unaligned_be16(opts->addr.port, bptr); > bptr += 2; > put_unaligned_be64(opts->ahmac, bptr); > bptr += 8; > @@ -1256,7 +1250,7 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, > > ptr += 3; > } else { > - put_unaligned_be32(opts->port << 16 | > + put_unaligned_be32(opts->addr.port << 16 | > TCPOPT_NOP << 8 | > TCPOPT_NOP, ptr); > ptr += 1; > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > index 9005ccc2bc7d..b993e372c4ad 100644 > --- a/net/mptcp/protocol.h > +++ b/net/mptcp/protocol.h > @@ -169,18 +169,6 @@ static inline __be32 mptcp_option(u8 subopt, u8 len, u8 nib, u8 field) > ((nib & 0xF) << 8) | field); > } > > -struct mptcp_addr_info { > - sa_family_t family; > - __be16 port; > - u8 id; > - union { > - struct in_addr addr; > -#if IS_ENABLED(CONFIG_MPTCP_IPV6) > - struct in6_addr addr6; > -#endif > - }; > -}; > - > enum mptcp_pm_status { > MPTCP_PM_ADD_ADDR_RECEIVED, > MPTCP_PM_ADD_ADDR_SEND_ACK, > -- > 2.30.2 -- Mat Martineau Intel
Hi Mat, Thanks for your review. Mat Martineau <mathew.j.martineau@linux.intel.com> 于2021年3月19日周五 下午12:13写道： > > On Wed, 17 Mar 2021, Geliang Tang wrote: > > > This patch moved the mptcp_addr_info struct from protocol.h to mptcp.h, > > added a new struct mptcp_addr_info member addr in struct mptcp_out_options, > > and dropped the original addr, addr6, addr_id and port fields in it. Then > > we can use opts->addr to get the adding address from PM directly using > > mptcp_pm_add_addr_signal. > > > > Since the port number became as a big-endian order now, use ntohs to > > convert it before printing it out. > > > > Signed-off-by: Geliang Tang <geliangtang@gmail.com> > > --- > > include/net/mptcp.h | 21 +++++++++++++-------- > > net/mptcp/options.c | 42 ++++++++++++++++++------------------------ > > net/mptcp/protocol.h | 12 ------------ > > 3 files changed, 31 insertions(+), 44 deletions(-) > > > > diff --git a/include/net/mptcp.h b/include/net/mptcp.h > > index 16fe34d139c3..80d98a7db3c6 100644 > > --- a/include/net/mptcp.h > > +++ b/include/net/mptcp.h > > @@ -41,20 +41,25 @@ struct mptcp_rm_list { > > u8 nr; > > }; > > > > +struct mptcp_addr_info { > > + u8 id; > > + sa_family_t family : 4; > > It's unusual to use a bitfield size specifier with a special type like > sa_family_t. This patch moves the mptcp_addr_info struct from protocol.h > to mptcp.h, and the deleted struct below does not have the " : 4" for the > family. Was this intentional? > > While AF_INET and AF_INET6 do fit in 4 bits, and AF_MAX is only 45, I > think this code should either use a bitfield to represent IPv4/v6, or use > a whole sa_family_t. Maybe this is why MPTCP_ADDR_IPVERSION_? values were > defined. I prefer to use a whole sa_family_t, since that can avoid converting the type of the address family. I just sent a squash-to patch (Squash to "mptcp: use mptcp_addr_info in mptcp_out_options") to fix this. Thanks. -Geliang > > Mat > > > > + __be16 port; > > + union { > > + struct in_addr addr; > > +#if IS_ENABLED(CONFIG_MPTCP_IPV6) > > + struct in6_addr addr6; > > +#endif > > + }; > > +}; > > + > > struct mptcp_out_options { > > #if IS_ENABLED(CONFIG_MPTCP) > > u16 suboptions; > > u64 sndr_key; > > u64 rcvr_key; > > - union { > > - struct in_addr addr; > > -#if IS_ENABLED(CONFIG_MPTCP_IPV6) > > - struct in6_addr addr6; > > -#endif > > - }; > > - u8 addr_id; > > - u16 port; > > u64 ahmac; > > + struct mptcp_addr_info addr; > > struct mptcp_rm_list rm_list; > > u8 join_id; > > u8 backup; > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > > index 4b7119eb2c31..7e01f44ed885 100644 > > --- a/net/mptcp/options.c > > +++ b/net/mptcp/options.c > > @@ -626,7 +626,6 @@ 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; > > - struct mptcp_addr_info saddr; > > bool echo; > > bool port; > > int len; > > @@ -643,45 +642,40 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff * > > } > > > > if (!mptcp_pm_should_add_signal(msk) || > > - !(mptcp_pm_add_addr_signal(msk, remaining, &saddr, &echo, &port))) > > + !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port))) > > return false; > > > > - len = mptcp_add_addr_len(saddr.family, echo, port); > > + len = mptcp_add_addr_len(opts->addr.family, echo, port); > > if (remaining < len) > > return false; > > > > *size = len; > > if (drop_other_suboptions) > > *size -= opt_size; > > - opts->addr_id = saddr.id; > > - if (port) > > - opts->port = ntohs(saddr.port); > > - if (saddr.family == AF_INET) { > > + if (opts->addr.family == AF_INET) { > > opts->suboptions |= OPTION_MPTCP_ADD_ADDR; > > - opts->addr = saddr.addr; > > if (!echo) { > > opts->ahmac = add_addr_generate_hmac(msk->local_key, > > msk->remote_key, > > - opts->addr_id, > > - &opts->addr, > > - opts->port); > > + opts->addr.id, > > + &opts->addr.addr, > > + opts->addr.port); > > } > > } > > #if IS_ENABLED(CONFIG_MPTCP_IPV6) > > - else if (saddr.family == AF_INET6) { > > + else if (opts->addr.family == AF_INET6) { > > opts->suboptions |= OPTION_MPTCP_ADD_ADDR6; > > - opts->addr6 = saddr.addr6; > > if (!echo) { > > opts->ahmac = add_addr6_generate_hmac(msk->local_key, > > msk->remote_key, > > - opts->addr_id, > > - &opts->addr6, > > - opts->port); > > + opts->addr.id, > > + &opts->addr.addr6, > > + opts->addr.port); > > } > > } > > #endif > > pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d", > > - opts->addr_id, opts->ahmac, echo, opts->port); > > + opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port)); > > > > return true; > > } > > @@ -1217,7 +1211,7 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, > > len = TCPOLEN_MPTCP_ADD_ADDR6_BASE; > > #endif > > > > - if (opts->port) > > + if (opts->addr.port) > > len += TCPOLEN_MPTCP_PORT_LEN; > > > > if (opts->ahmac) { > > @@ -1226,19 +1220,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, > > } > > > > *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR, > > - len, echo, opts->addr_id); > > + len, echo, opts->addr.id); > > if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) { > > - memcpy((u8 *)ptr, (u8 *)&opts->addr.s_addr, 4); > > + memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4); > > ptr += 1; > > } > > #if IS_ENABLED(CONFIG_MPTCP_IPV6) > > else if (OPTION_MPTCP_ADD_ADDR6 & opts->suboptions) { > > - memcpy((u8 *)ptr, opts->addr6.s6_addr, 16); > > + memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16); > > ptr += 4; > > } > > #endif > > > > - if (!opts->port) { > > + if (!opts->addr.port) { > > if (opts->ahmac) { > > put_unaligned_be64(opts->ahmac, ptr); > > ptr += 2; > > @@ -1247,7 +1241,7 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, > > if (opts->ahmac) { > > u8 *bptr = (u8 *)ptr; > > > > - put_unaligned_be16(opts->port, bptr); > > + put_unaligned_be16(opts->addr.port, bptr); > > bptr += 2; > > put_unaligned_be64(opts->ahmac, bptr); > > bptr += 8; > > @@ -1256,7 +1250,7 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, > > > > ptr += 3; > > } else { > > - put_unaligned_be32(opts->port << 16 | > > + put_unaligned_be32(opts->addr.port << 16 | > > TCPOPT_NOP << 8 | > > TCPOPT_NOP, ptr); > > ptr += 1; > > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > > index 9005ccc2bc7d..b993e372c4ad 100644 > > --- a/net/mptcp/protocol.h > > +++ b/net/mptcp/protocol.h > > @@ -169,18 +169,6 @@ static inline __be32 mptcp_option(u8 subopt, u8 len, u8 nib, u8 field) > > ((nib & 0xF) << 8) | field); > > } > > > > -struct mptcp_addr_info { > > - sa_family_t family; > > - __be16 port; > > - u8 id; > > - union { > > - struct in_addr addr; > > -#if IS_ENABLED(CONFIG_MPTCP_IPV6) > > - struct in6_addr addr6; > > -#endif > > - }; > > -}; > > - > > enum mptcp_pm_status { > > MPTCP_PM_ADD_ADDR_RECEIVED, > > MPTCP_PM_ADD_ADDR_SEND_ACK, > > -- > > 2.30.2 > > -- > Mat Martineau > Intel
diff --git a/include/net/mptcp.h b/include/net/mptcp.h index 16fe34d139c3..80d98a7db3c6 100644 --- a/include/net/mptcp.h +++ b/include/net/mptcp.h @@ -41,20 +41,25 @@ struct mptcp_rm_list { u8 nr; }; +struct mptcp_addr_info { + u8 id; + sa_family_t family : 4; + __be16 port; + union { + struct in_addr addr; +#if IS_ENABLED(CONFIG_MPTCP_IPV6) + struct in6_addr addr6; +#endif + }; +}; + struct mptcp_out_options { #if IS_ENABLED(CONFIG_MPTCP) u16 suboptions; u64 sndr_key; u64 rcvr_key; - union { - struct in_addr addr; -#if IS_ENABLED(CONFIG_MPTCP_IPV6) - struct in6_addr addr6; -#endif - }; - u8 addr_id; - u16 port; u64 ahmac; + struct mptcp_addr_info addr; struct mptcp_rm_list rm_list; u8 join_id; u8 backup; diff --git a/net/mptcp/options.c b/net/mptcp/options.c index 4b7119eb2c31..7e01f44ed885 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -626,7 +626,6 @@ 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; - struct mptcp_addr_info saddr; bool echo; bool port; int len; @@ -643,45 +642,40 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff * } if (!mptcp_pm_should_add_signal(msk) || - !(mptcp_pm_add_addr_signal(msk, remaining, &saddr, &echo, &port))) + !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port))) return false; - len = mptcp_add_addr_len(saddr.family, echo, port); + len = mptcp_add_addr_len(opts->addr.family, echo, port); if (remaining < len) return false; *size = len; if (drop_other_suboptions) *size -= opt_size; - opts->addr_id = saddr.id; - if (port) - opts->port = ntohs(saddr.port); - if (saddr.family == AF_INET) { + if (opts->addr.family == AF_INET) { opts->suboptions |= OPTION_MPTCP_ADD_ADDR; - opts->addr = saddr.addr; if (!echo) { opts->ahmac = add_addr_generate_hmac(msk->local_key, msk->remote_key, - opts->addr_id, - &opts->addr, - opts->port); + opts->addr.id, + &opts->addr.addr, + opts->addr.port); } } #if IS_ENABLED(CONFIG_MPTCP_IPV6) - else if (saddr.family == AF_INET6) { + else if (opts->addr.family == AF_INET6) { opts->suboptions |= OPTION_MPTCP_ADD_ADDR6; - opts->addr6 = saddr.addr6; if (!echo) { opts->ahmac = add_addr6_generate_hmac(msk->local_key, msk->remote_key, - opts->addr_id, - &opts->addr6, - opts->port); + opts->addr.id, + &opts->addr.addr6, + opts->addr.port); } } #endif pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d", - opts->addr_id, opts->ahmac, echo, opts->port); + opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port)); return true; } @@ -1217,7 +1211,7 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, len = TCPOLEN_MPTCP_ADD_ADDR6_BASE; #endif - if (opts->port) + if (opts->addr.port) len += TCPOLEN_MPTCP_PORT_LEN; if (opts->ahmac) { @@ -1226,19 +1220,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, } *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR, - len, echo, opts->addr_id); + len, echo, opts->addr.id); if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) { - memcpy((u8 *)ptr, (u8 *)&opts->addr.s_addr, 4); + memcpy((u8 *)ptr, (u8 *)&opts->addr.addr.s_addr, 4); ptr += 1; } #if IS_ENABLED(CONFIG_MPTCP_IPV6) else if (OPTION_MPTCP_ADD_ADDR6 & opts->suboptions) { - memcpy((u8 *)ptr, opts->addr6.s6_addr, 16); + memcpy((u8 *)ptr, opts->addr.addr6.s6_addr, 16); ptr += 4; } #endif - if (!opts->port) { + if (!opts->addr.port) { if (opts->ahmac) { put_unaligned_be64(opts->ahmac, ptr); ptr += 2; @@ -1247,7 +1241,7 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, if (opts->ahmac) { u8 *bptr = (u8 *)ptr; - put_unaligned_be16(opts->port, bptr); + put_unaligned_be16(opts->addr.port, bptr); bptr += 2; put_unaligned_be64(opts->ahmac, bptr); bptr += 8; @@ -1256,7 +1250,7 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, ptr += 3; } else { - put_unaligned_be32(opts->port << 16 | + put_unaligned_be32(opts->addr.port << 16 | TCPOPT_NOP << 8 | TCPOPT_NOP, ptr); ptr += 1; diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index 9005ccc2bc7d..b993e372c4ad 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -169,18 +169,6 @@ static inline __be32 mptcp_option(u8 subopt, u8 len, u8 nib, u8 field) ((nib & 0xF) << 8) | field); } -struct mptcp_addr_info { - sa_family_t family; - __be16 port; - u8 id; - union { - struct in_addr addr; -#if IS_ENABLED(CONFIG_MPTCP_IPV6) - struct in6_addr addr6; -#endif - }; -}; - enum mptcp_pm_status { MPTCP_PM_ADD_ADDR_RECEIVED, MPTCP_PM_ADD_ADDR_SEND_ACK,
This patch moved the mptcp_addr_info struct from protocol.h to mptcp.h, added a new struct mptcp_addr_info member addr in struct mptcp_out_options, and dropped the original addr, addr6, addr_id and port fields in it. Then we can use opts->addr to get the adding address from PM directly using mptcp_pm_add_addr_signal. Since the port number became as a big-endian order now, use ntohs to convert it before printing it out. Signed-off-by: Geliang Tang <geliangtang@gmail.com> --- include/net/mptcp.h | 21 +++++++++++++-------- net/mptcp/options.c | 42 ++++++++++++++++++------------------------ net/mptcp/protocol.h | 12 ------------ 3 files changed, 31 insertions(+), 44 deletions(-)