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

[Antonio Quartulli]

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 :))

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! :))

> > > 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 agree with Sabrina here.
I also initially thought about having MIN/MAX, but it wouldn't make 
things simpler for the ovpn_mode.

Regards,


--
Antonio Quartulli
OpenVPN Inc.