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

[Sabrina Dubroca <sd@queasysnail.net>]

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.


> > 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?").

-- 
Sabrina