Re: [PATCH net-next v11 04/23] ovpn: add basic interface creation/destruction/management routines

[Sergey Ryazanov <ryazanov.s.a@gmail.com>]

On 12.11.2024 18:47, Sabrina Dubroca wrote:
> 2024-11-09, 03:01:21 +0200, Sergey Ryazanov wrote:
>> On 29.10.2024 12:47, Antonio Quartulli wrote:
>>> +/* When the OpenVPN protocol is ran in AEAD mode, use
>>> + * the OpenVPN packet ID as the AEAD nonce:
>>> + *
>>> + *    00000005 521c3b01 4308c041
>>> + *    [seq # ] [  nonce_tail   ]
>>> + *    [     12-byte full IV    ] -> NONCE_SIZE
>>> + *    [4-bytes                   -> NONCE_WIRE_SIZE
>>> + *    on wire]
>>> + */
>>
>> Nice diagram! Can we go futher and define the OpenVPN packet header as a
>> stucture? Referencing the structure instead of using magic sizes and offsets
>> can greatly improve the code readability. Especially when it comes to header
>> construction/parsing in the encryption/decryption code.
>>
>> E.g. define a structures like this:
>>
>> struct ovpn_pkt_hdr {
>>    __be32 op;
>>    __be32 pktid;
>>    u8 auth[];
>> } __attribute__((packed));
>>
>> struct ovpn_aead_iv {
>>    __be32 pktid;
>>    u8 nonce[OVPN_NONCE_TAIL_SIZE];
>> } __attribute__((packed));
> 
> __attribute__((packed)) should not be needed here as the fields in
> both structs look properly aligned, and IIRC using packed can cause
> the compiler to generate worse code.

True, the fields are pretty good aligned and from code generation 
perspective packed indication is unneeded. I suggested to mark structs 
as packed mostly as a documentation to clearly state that these 
structures represent specific memory layout.

>>> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>>> index 8516c1ccd57a7c7634a538fe3ac16c858f647420..84d294aab20b79b8e9cb9b736a074105c99338f3 100644
>>> --- a/include/uapi/linux/if_link.h
>>> +++ b/include/uapi/linux/if_link.h
>>> @@ -1975,4 +1975,19 @@ enum {
>>>    #define IFLA_DSA_MAX	(__IFLA_DSA_MAX - 1)
>>> +/* OVPN section */
>>> +
>>> +enum ovpn_mode {
>>> +	OVPN_MODE_P2P,
>>> +	OVPN_MODE_MP,
>>> +};
>>
>> Mode min/max values can be defined here and the netlink policy can reference
>> these values:
>>
>> enum ovpn_mode {
>>    OVPN_MODE_P2P,
>>    OVPN_MODE_MP,
>>    __OVPN_MODE_MAX
>> };
>>
>> #define OVPN_MODE_MIN OVPN_MODE_P2P
>> #define OVPN_MODE_MAX (__OVPN_MODE_MAX - 1)
>>
>> ... = NLA_POLICY_RANGE(NLA_U8, OVPN_MODE_MIN, OVPN_MODE_MAX)
> 
> I don't think there's much benefit to that, other than making the diff
> smaller on a (very unlikely) patch that would add a new mode in the
> future. It even looks more inconvenient to me when reading the code
> ("ok what are _MIN and _MAX?  the code is using _P2P and _MP, do they
> match?").

I would answer yes. Just prefer to trust these kind of statements until 
it crashes badly. Honestly, I never thought that referring to a max 
value might raise such a question. Can you give an example why it should 
be meaningful to know exact min/max values of an unordered set?

I suggested to define boundaries indeed for documentation purpose. Diff 
reduction is also desirable, but as you already mentioned, here it is 
not the case. Using specific values in a range declaration assigns them 
with extra semantic. Like, MODE_P2P is also a minimal possible value 
while MODE_MP has this extra meaning of minimal possible value. And we 
can learn this only from the policy which is specified far way from the 
modes declarations. I also see policies declaration as referring to 
already defined information rather than creating new meanings. On 
another hand the NL policy is the only user, so maybe we should left it 
as-is for the sake of simplicity.

--
Sergey