* [MPTCP] Re: [MPTCP][PATCH mptcp-next 1/8] mptcp: add ADD_ADDR port support for writing options
@ 2020-10-30 0:42 Mat Martineau
0 siblings, 0 replies; 2+ messages in thread
From: Mat Martineau @ 2020-10-30 0:42 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 6471 bytes --]
On Thu, 29 Oct 2020, Geliang Tang wrote:
> This patch added ADD_ADDR port support for writing options.
>
> In rfc8684, the length of ADD_ADDR suboption with IPv4 address and port
> is 18 octets:
>
> 1 2 3
> 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
> +---------------+---------------+-------+-------+---------------+
> | Kind | Length |Subtype|(rsv)|E| Address ID |
> +---------------+---------------+-------+-------+---------------+
> | Address (IPv4: 4 octets / IPv6: 16 octets) |
> +-------------------------------+-------------------------------+
> | Port (2 octets, optional) | |
> +-------------------------------+ |
> | Truncated HMAC (8 octets, if E=0) |
> | +-------------------------------+
> | |
> +-------------------------------+
>
> But mptcp_write_options is 32-bit aligned, so we need to pad it to 20
> octets.
>
> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> ---
> include/net/mptcp.h | 1 +
> net/mptcp/options.c | 64 +++++++++++++++++++++++++++++++++++++-------
> net/mptcp/protocol.h | 10 +++----
> 3 files changed, 60 insertions(+), 15 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index b6cf07143a8a..bb8b7b931490 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -46,6 +46,7 @@ struct mptcp_out_options {
> #endif
> };
> u8 addr_id;
> + __be16 port;
This should be a u16 (all the members in this struct use CPU byte order).
The put_unaligned_be16() calls below also assume this is in CPU order.
> u64 ahmac;
> u8 rm_id;
> u8 join_id;
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 248e3930c0cb..7c928286c0da 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -1067,39 +1067,83 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>
> mp_capable_done:
> if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> + int add_len, echo_len;
> +
> + if (!opts->port) {
> + add_len = TCPOLEN_MPTCP_ADD_ADDR;
> + echo_len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> + } else {
> + add_len = TCPOLEN_MPTCP_ADD_ADDR_PORT;
> + echo_len = TCPOLEN_MPTCP_ADD_ADDR_BASE_PORT;
> + }
> +
> if (opts->ahmac)
> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> - TCPOLEN_MPTCP_ADD_ADDR, 0,
> + add_len, 0,
> opts->addr_id);
> else
> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> - TCPOLEN_MPTCP_ADD_ADDR_BASE,
> + echo_len,
> MPTCP_ADDR_ECHO,
> opts->addr_id);
> memcpy((u8 *)ptr, (u8 *)&opts->addr.s_addr, 4);
> ptr += 1;
> - if (opts->ahmac) {
> - put_unaligned_be64(opts->ahmac, ptr);
> - ptr += 2;
> +
> + if (!opts->port) {
> + if (opts->ahmac) {
> + put_unaligned_be64(opts->ahmac, ptr);
> + ptr += 2;
> + }
> + } else {
> + if (opts->ahmac) {
> + put_unaligned_be32(opts->port << 16 | opts->ahmac >> 48, ptr);
It doesn't work to bitwise-or these values before the endianness
conversion. They need to have endianness converted first, then populate
the headers in network byte order.
> + ptr += 1;
> + put_unaligned_be64(opts->ahmac << 16 | TCPOPT_NOP, ptr);
Same issue here - need to make sure the byte order conversion happens
first. For two bytes of padding, will need two bytes of TCPOPT_NOP.
> + ptr += 2;
> + } else {
> + put_unaligned_be16(opts->port, ptr);
> + ptr += 1;
Will need padding here too.
> + }
> }
> }
>
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> if (OPTION_MPTCP_ADD_ADDR6 & opts->suboptions) {
> + int add_len, echo_len;
> +
> + if (!opts->port) {
> + add_len = TCPOLEN_MPTCP_ADD_ADDR6;
> + echo_len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> + } else {
> + add_len = TCPOLEN_MPTCP_ADD_ADDR6_PORT;
> + echo_len = TCPOLEN_MPTCP_ADD_ADDR6_BASE_PORT;
> + }
> if (opts->ahmac)
> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> - TCPOLEN_MPTCP_ADD_ADDR6, 0,
> + add_len, 0,
> opts->addr_id);
> else
> *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
> - TCPOLEN_MPTCP_ADD_ADDR6_BASE,
> + echo_len,
> MPTCP_ADDR_ECHO,
> opts->addr_id);
> memcpy((u8 *)ptr, opts->addr6.s6_addr, 16);
> ptr += 4;
> - if (opts->ahmac) {
> - put_unaligned_be64(opts->ahmac, ptr);
> - ptr += 2;
> + if (!opts->port) {
> + if (opts->ahmac) {
> + put_unaligned_be64(opts->ahmac, ptr);
> + ptr += 2;
> + }
> + } else {
> + if (opts->ahmac) {
> + put_unaligned_be32(opts->port << 16 | opts->ahmac >> 48, ptr);
> + ptr += 1;
> + put_unaligned_be64(opts->ahmac << 16 | TCPOPT_NOP, ptr);
> + ptr += 2;
> + } else {
> + put_unaligned_be16(opts->port, ptr);
> + ptr += 1;
Same issues as above with bitwise operations and byte order conversion.
Maybe a new helper function to populate these bytes? The helper could use
a local u8 pointer to make it easier to align the values using separate
put_unaligned_* calls rather than using the bitwise-or.
> + }
> }
> }
> #endif
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index d29c6a4749eb..f303c46bcc29 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -49,14 +49,14 @@
> #define TCPOLEN_MPTCP_DSS_MAP64 14
> #define TCPOLEN_MPTCP_DSS_CHECKSUM 2
> #define TCPOLEN_MPTCP_ADD_ADDR 16
> -#define TCPOLEN_MPTCP_ADD_ADDR_PORT 18
> +#define TCPOLEN_MPTCP_ADD_ADDR_PORT 20
> #define TCPOLEN_MPTCP_ADD_ADDR_BASE 8
> -#define TCPOLEN_MPTCP_ADD_ADDR_BASE_PORT 10
> +#define TCPOLEN_MPTCP_ADD_ADDR_BASE_PORT 12
> #define TCPOLEN_MPTCP_ADD_ADDR6 28
> -#define TCPOLEN_MPTCP_ADD_ADDR6_PORT 30
> +#define TCPOLEN_MPTCP_ADD_ADDR6_PORT 32
> #define TCPOLEN_MPTCP_ADD_ADDR6_BASE 20
> -#define TCPOLEN_MPTCP_ADD_ADDR6_BASE_PORT 22
> -#define TCPOLEN_MPTCP_PORT_LEN 2
> +#define TCPOLEN_MPTCP_ADD_ADDR6_BASE_PORT 24
> +#define TCPOLEN_MPTCP_PORT_LEN 4
> #define TCPOLEN_MPTCP_RM_ADDR_BASE 4
>
> /* MPTCP MP_JOIN flags */
> --
> 2.26.2
Thanks for the patches.
(Note: I haven't reviewed the rest of the series yet, will continue
tomorrow)
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 2+ messages in thread* [MPTCP] Re: [MPTCP][PATCH mptcp-next 1/8] mptcp: add ADD_ADDR port support for writing options
@ 2020-10-31 0:14 Mat Martineau
0 siblings, 0 replies; 2+ messages in thread
From: Mat Martineau @ 2020-10-31 0:14 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 2439 bytes --]
On Thu, 29 Oct 2020, Mat Martineau wrote:
>
> On Thu, 29 Oct 2020, Geliang Tang wrote:
>
>> This patch added ADD_ADDR port support for writing options.
>>
>> In rfc8684, the length of ADD_ADDR suboption with IPv4 address and port
>> is 18 octets:
>>
>> 1 2 3
>> 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>> +---------------+---------------+-------+-------+---------------+
>> | Kind | Length |Subtype|(rsv)|E| Address ID |
>> +---------------+---------------+-------+-------+---------------+
>> | Address (IPv4: 4 octets / IPv6: 16 octets) |
>> +-------------------------------+-------------------------------+
>> | Port (2 octets, optional) | |
>> +-------------------------------+ |
>> | Truncated HMAC (8 octets, if E=0) |
>> | +-------------------------------+
>> | |
>> +-------------------------------+
>>
>> But mptcp_write_options is 32-bit aligned, so we need to pad it to 20
>> octets.
>>
>> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
>> ---
>> include/net/mptcp.h | 1 +
>> net/mptcp/options.c | 64 +++++++++++++++++++++++++++++++++++++-------
>> net/mptcp/protocol.h | 10 +++----
>> 3 files changed, 60 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
>> index b6cf07143a8a..bb8b7b931490 100644
>> --- a/include/net/mptcp.h
>> +++ b/include/net/mptcp.h
>> @@ -46,6 +46,7 @@ struct mptcp_out_options {
>> #endif
>> };
>> u8 addr_id;
>> + __be16 port;
>
> This should be a u16 (all the members in this struct use CPU byte order). The
> put_unaligned_be16() calls below also assume this is in CPU order.
>
Looking at later patches in the patch set, I see now that __be16 is used
in other places for the port number. __be16 is also used for inet_dport in
various places. We should make sure the port number is handled
consistently within the MPTCP code, and to make sure it is also consistent
with the other IP code. Anyone else have an opinion here?
If you keep using __be16, it does remain an issue that you don't want to
use get_unaligned_be16() and put_unaligned_be16() with that data type.
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-10-31 0:14 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-30 0:42 [MPTCP] Re: [MPTCP][PATCH mptcp-next 1/8] mptcp: add ADD_ADDR port support for writing options Mat Martineau
-- strict thread matches above, loose matches on Subject: below --
2020-10-31 0:14 Mat Martineau
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox