netdev - 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: <6ec331d6-16b7-4e0d-a32a-0fa92a439d97@gmail.com>
Date: Tue, 19 Nov 2024 03:49:04 +0200
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Antonio Quartulli <antonio@...nvpn.net>, Jakub Kicinski <kuba@...nel.org>
Cc: Eric Dumazet <edumazet@...gle.com>, 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 15.11.2024 11:56, Antonio Quartulli wrote:
> On 06/11/2024 01:31, Sergey Ryazanov wrote:
>>> 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.

Sure! I hope Jakub can literally merge the series as a branch with the 
merge commit containing the text from the cover later. If it's not Ok, 
then keeping the introduction in the first patch is a good idea.

> 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.

The text itself looks a nice introduction into the functionality, keep 
it please.

>>> +/* 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.

715 of 784 network driver modules directly specify the description. I 
see mostly old drivers are using a dedicated macro. But it's up to you 
how to say hello to future code readers.

>>> --- 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.

Ok. I believe it's not a technical problem to introduce it here. The 
module will be buildable. It's more about facilitating reviewers' life. 
A code, introduced just in time, saves from two kinds of wonderings: (a) 
do we need this at all, when you see an unused definition, and (b) what 
it exactly means, when you see a user two patches after. Anyway, it was 
just a nit pick, and it's up to you how to introduce the module.

--
Sergey