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

[Antonio Quartulli <antonio@openvpn.net>]

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.