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

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

On 14.11.2024 10:07, Antonio Quartulli wrote:
> On 12/11/2024 17: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.
> 
> Agreed. Using packed will make certain architecture read every field 
> byte by byte (I remember David M. biting us on this in batman-adv :))

Still curious to see an example of that strange architecture/compiler 
combination. Anyway, as Sabrina mentioned, the header is already pretty 
aligned. So it's up to you how to document the structure.

> This said, I like the idea of using a struct, but I don't feel confident 
> enough to change the code now that we are hitting v12.
> This kind of change will be better implemented later and tested 
> carefully. (and patches are always welcome! :))

The main reason behind the structure introduction is to improve the code 
readability. To reduce a shadow, where bugs can reside. I wonder how 
many people have invested their time to dig through the encryption 
preparation function?

As for risk of breaking something I should say that it can be addressed 
by connecting the kernel implementation to pure usespace implementation, 
which can be assumed the reference. And, I believe, it worth the benefit 
of merging easy to understand code.

--
Sergey