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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <466ec41a-24b7-43a9-b75f-94556785800a@openvpn.net>
Date: Fri, 15 Nov 2024 10:56:13 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>,
 Shuah Khan <shuah@...nel.org>, sd@...asysnail.net,
 Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, steffen.klassert@...unet.com,
 antony.antony@...unet.com
Subject: Re: [PATCH net-next v11 02/23] net: introduce OpenVPN Data Channel
 Offload (ovpn)

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.