Re: [PATCH net-next v11 02/23] net: introduce OpenVPN Data Channel Offload (ovpn)

[Antonio Quartulli]

On 06/11/2024 01:31, Sergey Ryazanov wrote:
[...]

> > Both UDP and TCP sockets ae supported.
>
> s/ae/are/

ACK

> > As explained above, in case of P2MP mode, OpenVPN will use the main 
> > system
> > routing table to decide which packet goes to which peer. This implies
> > that no routing table was re-implemented in the `ovpn` kernel module.
> >
> > This kernel module can be enabled by selecting the CONFIG_OVPN entry
> > in the networking drivers section.
>
> Most of the above text has no relation to the patch itself. Should it be 
> moved to the cover letter?

I think this needs to be in the git history.
We are introducing a new kernel module and this is the presentation, so 
I expect this to live in git.

This was the original text when ovpn was a 1/1 patch.
I can better clarify what this patch is doing and what comes in 
following patches, if that can help.

[...]

> > --- a/drivers/net/Kconfig
> > +++ b/drivers/net/Kconfig
> > @@ -115,6 +115,19 @@ config WIREGUARD_DEBUG
> >         Say N here unless you know what you're doing.
> > +config OVPN
> > +    tristate "OpenVPN data channel offload"
> > +    depends on NET && INET
> > +    select NET_UDP_TUNNEL
> > +    select DST_CACHE
> > +    select CRYPTO
> > +    select CRYPTO_AES
> > +    select CRYPTO_GCM
> > +    select CRYPTO_CHACHA20POLY1305
>
> nit: Options from NET_UDP_TUNNEL to CRYPTO_CHACHA20POLY1305 are not 
> required for changes introduced in this patch. Should they be moved to 
> corresponding patches?

Originally I wanted to introduce all deps with patch 1, but then I added 
STREAM_PARSER to the TCP patch.

I will do the same with the others and add deps only when needed.

[...]

> > +/* Driver info */
> > +#define DRV_DESCRIPTION    "OpenVPN data channel offload (ovpn)"
> > +#define DRV_COPYRIGHT    "(C) 2020-2024 OpenVPN, Inc."
>
> nit: these strings are used only once for MODULE_{DESCRIPTION,AUTHOR} 
> below. Can we directly use strings to avoid levels of indirection?

I liked to have these defines at the top as if they were some form of 
greeting :) But I can move them down and drop the constants.

> > +
> > +/**
> > + * ovpn_dev_is_valid - check if the netdevice is of type 'ovpn'
> > + * @dev: the interface to check
> > + *
> > + * Return: whether the netdevice is of type 'ovpn'
> > + */
> > +bool ovpn_dev_is_valid(const struct net_device *dev)
> > +{
> > +    return dev->netdev_ops->ndo_start_xmit == ovpn_net_xmit;
>
> You can directly check for the ops matching saving one dereferencing 
> operation:
>
> return dev->netdev_ops == &ovpn_netdev_ops;

I see all net drivers do what you are suggesting.
Will do the same, thanks

> You can define an empty ovpn_netdev_ops struct for this purpose in this 
> patch and fill ops later with next patches. This way you can even move 
> the ovpn_net_xmit() definition to the interface creation/destruction patch.

It's a device driver, so having a placeholder xmit() in the first patch 
doesn't sound that bad :-)
And xmit is more about packet flow rather than creation/destruction.

I prefer to keep the stub here.

[...]

> > --- a/include/uapi/linux/udp.h
> > +++ b/include/uapi/linux/udp.h
> > @@ -43,5 +43,6 @@ struct udphdr {
> >   #define UDP_ENCAP_GTP1U        5 /* 3GPP TS 29.060 */
> >   #define UDP_ENCAP_RXRPC        6
> >   #define TCP_ENCAP_ESPINTCP    7 /* Yikes, this is really xfrm encap 
> > types. */
> > +#define UDP_ENCAP_OVPNINUDP    8 /* OpenVPN traffic */
>
> nit: this specific change does not belong to this specific patch.

Right. Like for the Kconfig, I wanted to keep "general" changes and 
things that touch the rest of the kernel in this patch.

But since we are moving other things to related patches, I will also 
move this to the UDP patch.

Thanks!

Regards,


--
Antonio Quartulli
OpenVPN Inc.