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

[Sergey Ryazanov]

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