[MPTCP] Re: [MPTCP][PATCH mptcp-next 1/8] mptcp: add ADD_ADDR port support for writing options

[Mat Martineau <mathew.j.martineau at linux.intel.com>]

[-- 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